Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1826-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240213160152.2836131-1-kiran.k@intel.com>
In-Reply-To: <20240213160152.2836131-1-kiran.k@intel.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Tue, 13 Feb 2024 11:10:17 -0500
Message-ID: <CABBYNZJL9FKuWU_4c1KrskzjAtBVxtW9PDqP==MAYGOc+NZB5Q@mail.gmail.com>
Subject: Re: [PATCH v1] Bluetooth: btintel: Print Firmware Sequencer information
To: Kiran K <kiran.k@intel.com>
Cc: linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com, 
	chethan.tumkur.narayan@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Kiran,

On Tue, Feb 13, 2024 at 10:51=E2=80=AFAM 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_de=
v *hdev, u8 hw_variant)
>         }
>  }
>
> +static void btintel_print_fseq_info(struct hci_dev *hdev)
> +{
> +       struct sk_buff *skb;
> +       u8 *p;
> +       const char *str;
> +
> +       skb =3D __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 =3D skb->data;
> +       /* skip status */
> +       p =3D p + 1;

How about we use skb_pull_data instead of accessing these fields with
a pointer cursor?

> +       switch (*p) {
> +       case 0:
> +               str =3D "Success";
> +               break;
> +       case 1:
> +               str =3D "Fatal error";
> +               break;
> +       case 2:
> +               str =3D "Sem acq error";
> +               break;
> +       default:
> +               str =3D "Unknown error";
> +               break;
> +       }
> +
> +       bt_dev_info(hdev, "Fseq status: %s (0x%2.2x)", str, *p);
> +       if (*p)
> +               return;
> +       p =3D p + 1;
> +       bt_dev_dbg(hdev, "Reason: 0x%8.8x", get_unaligned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Global version: 0x%8.8x", get_unaligned_le32(p)=
);
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Installed version: 0x%8.8x", get_unaligned_le32=
(p));
> +
> +       p =3D 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 =3D 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 =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq Top init version: 0x%8.8x", get_unaligned_=
le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq Cnvio init version: 0x%8.8x", get_unaligne=
d_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq MBX Wifi file version: 0x%8.8x", get_unali=
gned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq BT version: 0x%8.8x", get_unaligned_le32(p=
));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq Top reset address: 0x%8.8x", get_unaligned=
_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq MBX timeout: 0x%8.8x", get_unaligned_le32(=
p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq MBX ack: 0x%8.8x", get_unaligned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq CNVi id: 0x%8.8x", get_unaligned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq CNVr id: 0x%8.8x", get_unaligned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq Error handle: 0x%8.8x", get_unaligned_le32=
(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq Magic noalive indication: 0x%8.8x", get_un=
aligned_le32(p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq OTP version: 0x%8.8x", get_unaligned_le32(=
p));
> +
> +       p =3D p + 4;
> +       bt_dev_dbg(hdev, "Fseq MBX otp version: 0x%8.8x", get_unaligned_l=
e32(p));
> +}
> +
>  static int btintel_setup_combined(struct hci_dev *hdev)
>  {
>         const u8 param[1] =3D { 0xFF };
> @@ -2902,6 +3007,7 @@ static int btintel_setup_combined(struct hci_dev *h=
dev)
>
>                 err =3D 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)",
> --
> 2.34.1
>
>


--=20
Luiz Augusto von Dentz