| Message ID | 20240213160152.2836131-1-kiran.k@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] Bluetooth: btintel: Print Firmware Sequencer information | expand |
Dear Kiran, Thank you for your patch. Am 13.02.24 um 17:01 schrieb Kiran K: > Firmware sequencer(FSEQ) is a common code shared across Bluetooth Please add a space before (. > and Wifi. Printing FSEQ will help to debug if there is any mismatch > between Bluetooth and Wifi FSEQ. Please give an example output, and document the system, you tested this on. > Signed-off-by: Kiran K <kiran.k@intel.com> > --- > drivers/bluetooth/btintel.c | 106 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 106 insertions(+) > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > index e5b043d96207..0d067ee39408 100644 > --- a/drivers/bluetooth/btintel.c > +++ b/drivers/bluetooth/btintel.c > @@ -2670,6 +2670,111 @@ static void btintel_set_msft_opcode(struct hci_dev *hdev, u8 hw_variant) > } > } > > +static void btintel_print_fseq_info(struct hci_dev *hdev) > +{ > + struct sk_buff *skb; > + u8 *p; > + const char *str; > + > + skb = __hci_cmd_sync(hdev, 0xfcb3, 0, NULL, HCI_CMD_TIMEOUT); > + if (IS_ERR(skb)) { > + bt_dev_dbg(hdev, "Reading fseq status command failed (%ld)", > + PTR_ERR(skb)); > + return; > + } > + > + if (skb->len < (sizeof(u32) * 16 + 2)) { > + bt_dev_dbg(hdev, "Malformed packet"); Please print out the length values. > + kfree_skb(skb); > + return; > + } > + > + if (skb->data[0]) { > + bt_dev_dbg(hdev, "Failed to get fseq status (0x%2.2x)", > + skb->data[0]); > + kfree_skb(skb); > + return; > + } > + > + p = skb->data; > + /* skip status */ > + p = p + 1; > + > + switch (*p) { > + case 0: > + str = "Success"; > + break; > + case 1: > + str = "Fatal error"; > + break; > + case 2: > + str = "Sem acq error"; Maybe elaborate here? > + break; > + default: > + str = "Unknown error"; > + break; > + } > + > + bt_dev_info(hdev, "Fseq status: %s (0x%2.2x)", str, *p); > + if (*p) > + return; Should non-success levels have a different log level? > + p = p + 1; > + bt_dev_dbg(hdev, "Reason: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Global version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Installed version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_info(hdev, "Fseq executed: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1], > + p[2], p[3]); > + > + p = p + 4; > + bt_dev_info(hdev, "Fseq BT Top: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1], > + p[2], p[3]); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq Top init version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq Cnvio init version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq MBX Wifi file version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq BT version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq Top reset address: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq MBX timeout: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq MBX ack: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq CNVi id: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq CNVr id: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq Error handle: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq Magic noalive indication: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq OTP version: 0x%8.8x", get_unaligned_le32(p)); > + > + p = p + 4; > + bt_dev_dbg(hdev, "Fseq MBX otp version: 0x%8.8x", get_unaligned_le32(p)); > +} > + > static int btintel_setup_combined(struct hci_dev *hdev) > { > const u8 param[1] = { 0xFF }; > @@ -2902,6 +3007,7 @@ static int btintel_setup_combined(struct hci_dev *hdev) > > err = btintel_bootloader_setup_tlv(hdev, &ver_tlv); > btintel_register_devcoredump_support(hdev); > + btintel_print_fseq_info(hdev); > break; > default: > bt_dev_err(hdev, "Unsupported Intel hw variant (%u)", Kind regards, Paul
Hi Paul, Thanks for your comments. > -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Tuesday, February 13, 2024 9:35 PM > To: K, Kiran <kiran.k@intel.com> > Cc: Srivatsa, Ravishankar <ravishankar.srivatsa@intel.com>; Tumkur Narayan, > Chethan <chethan.tumkur.narayan@intel.com>; linux- > bluetooth@vger.kernel.org > Subject: Re: [PATCH v1] Bluetooth: btintel: Print Firmware Sequencer > information > > Dear Kiran, > > > Thank you for your patch. > > Am 13.02.24 um 17:01 schrieb Kiran K: > > Firmware sequencer(FSEQ) is a common code shared across Bluetooth > > Please add a space before (. Ack > > and Wifi. Printing FSEQ will help to debug if there is any mismatch > > between Bluetooth and Wifi FSEQ. > > Please give an example output, and document the system, you tested this on. This patch was tested with Typhoon Peak2 controller. > > > Signed-off-by: Kiran K <kiran.k@intel.com> > > --- > > drivers/bluetooth/btintel.c | 106 > ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > > index e5b043d96207..0d067ee39408 100644 > > --- a/drivers/bluetooth/btintel.c > > +++ b/drivers/bluetooth/btintel.c > > @@ -2670,6 +2670,111 @@ static void btintel_set_msft_opcode(struct > hci_dev *hdev, u8 hw_variant) > > } > > } > > > > +static void btintel_print_fseq_info(struct hci_dev *hdev) { > > + struct sk_buff *skb; > > + u8 *p; > > + const char *str; > > + > > + skb = __hci_cmd_sync(hdev, 0xfcb3, 0, NULL, HCI_CMD_TIMEOUT); > > + if (IS_ERR(skb)) { > > + bt_dev_dbg(hdev, "Reading fseq status command failed > (%ld)", > > + PTR_ERR(skb)); > > + return; > > + } > > + > > + if (skb->len < (sizeof(u32) * 16 + 2)) { > > + bt_dev_dbg(hdev, "Malformed packet"); > > Please print out the length values. Sure. > > > + kfree_skb(skb); > > + return; > > + } > > + > > + if (skb->data[0]) { > > + bt_dev_dbg(hdev, "Failed to get fseq status (0x%2.2x)", > > + skb->data[0]); > > + kfree_skb(skb); > > + return; > > + } > > + > > + p = skb->data; > > + /* skip status */ > > + p = p + 1; > > + > > + switch (*p) { > > + case 0: > > + str = "Success"; > > + break; > > + case 1: > > + str = "Fatal error"; > > + break; > > + case 2: > > + str = "Sem acq error"; > > Maybe elaborate here? FSEQ code execution is mutually exclusive between Wifi and Bluetooth. If Bluetooth not able to acquire semaphore, then error code 2 will be reported. > > > + break; > > + default: > > + str = "Unknown error"; > > + break; > > + } > > + > > + bt_dev_info(hdev, "Fseq status: %s (0x%2.2x)", str, *p); > > + if (*p) > > + return; > > Should non-success levels have a different log level? I will add bt_dev_err for non-success case. > > > + p = p + 1; > > + bt_dev_dbg(hdev, "Reason: 0x%8.8x", get_unaligned_le32(p)); Thanks, Kiran
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Tuesday, February 13, 2024 9:40 PM > To: K, Kiran <kiran.k@intel.com> > Cc: linux-bluetooth@vger.kernel.org; Srivatsa, Ravishankar > <ravishankar.srivatsa@intel.com>; Tumkur Narayan, Chethan > <chethan.tumkur.narayan@intel.com> > Subject: Re: [PATCH v1] Bluetooth: btintel: Print Firmware Sequencer > information > > Hi Kiran, > > On Tue, Feb 13, 2024 at 10:51 AM Kiran K <kiran.k@intel.com> wrote: > > > > Firmware sequencer(FSEQ) is a common code shared across Bluetooth and > > Wifi. Printing FSEQ will help to debug if there is any mismatch > > between Bluetooth and Wifi FSEQ. > > > > Signed-off-by: Kiran K <kiran.k@intel.com> > > --- > > drivers/bluetooth/btintel.c | 106 > > ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 106 insertions(+) > > > > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c > > index e5b043d96207..0d067ee39408 100644 > > --- a/drivers/bluetooth/btintel.c > > +++ b/drivers/bluetooth/btintel.c > > @@ -2670,6 +2670,111 @@ static void btintel_set_msft_opcode(struct > hci_dev *hdev, u8 hw_variant) > > } > > } > > > > +static void btintel_print_fseq_info(struct hci_dev *hdev) { > > + struct sk_buff *skb; > > + u8 *p; > > + const char *str; > > + > > + skb = __hci_cmd_sync(hdev, 0xfcb3, 0, NULL, HCI_CMD_TIMEOUT); > > + if (IS_ERR(skb)) { > > + bt_dev_dbg(hdev, "Reading fseq status command failed (%ld)", > > + PTR_ERR(skb)); > > + return; > > + } > > + > > + if (skb->len < (sizeof(u32) * 16 + 2)) { > > + bt_dev_dbg(hdev, "Malformed packet"); > > + kfree_skb(skb); > > + return; > > + } > > + > > + if (skb->data[0]) { > > + bt_dev_dbg(hdev, "Failed to get fseq status (0x%2.2x)", > > + skb->data[0]); > > + kfree_skb(skb); > > + return; > > + } > > + > > + p = skb->data; > > + /* skip status */ > > + p = p + 1; > > How about we use skb_pull_data instead of accessing these fields with a > pointer cursor? I will fix it in v2 version of patch. > > > + switch (*p) { Thanks, Kiran
diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c index e5b043d96207..0d067ee39408 100644 --- a/drivers/bluetooth/btintel.c +++ b/drivers/bluetooth/btintel.c @@ -2670,6 +2670,111 @@ static void btintel_set_msft_opcode(struct hci_dev *hdev, u8 hw_variant) } } +static void btintel_print_fseq_info(struct hci_dev *hdev) +{ + struct sk_buff *skb; + u8 *p; + const char *str; + + skb = __hci_cmd_sync(hdev, 0xfcb3, 0, NULL, HCI_CMD_TIMEOUT); + if (IS_ERR(skb)) { + bt_dev_dbg(hdev, "Reading fseq status command failed (%ld)", + PTR_ERR(skb)); + return; + } + + if (skb->len < (sizeof(u32) * 16 + 2)) { + bt_dev_dbg(hdev, "Malformed packet"); + kfree_skb(skb); + return; + } + + if (skb->data[0]) { + bt_dev_dbg(hdev, "Failed to get fseq status (0x%2.2x)", + skb->data[0]); + kfree_skb(skb); + return; + } + + p = skb->data; + /* skip status */ + p = p + 1; + + switch (*p) { + case 0: + str = "Success"; + break; + case 1: + str = "Fatal error"; + break; + case 2: + str = "Sem acq error"; + break; + default: + str = "Unknown error"; + break; + } + + bt_dev_info(hdev, "Fseq status: %s (0x%2.2x)", str, *p); + if (*p) + return; + p = p + 1; + bt_dev_dbg(hdev, "Reason: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Global version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Installed version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_info(hdev, "Fseq executed: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1], + p[2], p[3]); + + p = p + 4; + bt_dev_info(hdev, "Fseq BT Top: %2.2u.%2.2u.%2.2u.%2.2u", p[0], p[1], + p[2], p[3]); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq Top init version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq Cnvio init version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq MBX Wifi file version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq BT version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq Top reset address: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq MBX timeout: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq MBX ack: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq CNVi id: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq CNVr id: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq Error handle: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq Magic noalive indication: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq OTP version: 0x%8.8x", get_unaligned_le32(p)); + + p = p + 4; + bt_dev_dbg(hdev, "Fseq MBX otp version: 0x%8.8x", get_unaligned_le32(p)); +} + static int btintel_setup_combined(struct hci_dev *hdev) { const u8 param[1] = { 0xFF }; @@ -2902,6 +3007,7 @@ static int btintel_setup_combined(struct hci_dev *hdev) err = btintel_bootloader_setup_tlv(hdev, &ver_tlv); btintel_register_devcoredump_support(hdev); + btintel_print_fseq_info(hdev); break; default: bt_dev_err(hdev, "Unsupported Intel hw variant (%u)",
Firmware sequencer(FSEQ) is a common code shared across Bluetooth and Wifi. Printing FSEQ will help to debug if there is any mismatch between Bluetooth and Wifi FSEQ. Signed-off-by: Kiran K <kiran.k@intel.com> --- drivers/bluetooth/btintel.c | 106 ++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+)