diff mbox series

[v1] Bluetooth: btintel: Print Firmware Sequencer information

Message ID 20240213160152.2836131-1-kiran.k@intel.com
State New
Headers show
Series [v1] Bluetooth: btintel: Print Firmware Sequencer information | expand

Commit Message

K, Kiran Feb. 13, 2024, 4:01 p.m. UTC
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(+)

Comments

Paul Menzel Feb. 13, 2024, 4:05 p.m. UTC | #1
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
K, Kiran Feb. 21, 2024, 1:11 p.m. UTC | #2
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
K, Kiran Feb. 21, 2024, 1:13 p.m. UTC | #3
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 mbox series

Patch

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)",