[PATCH v1] Bluetooth: btintel: read ROM debug registers on FW download failure

[PATCH v1] Bluetooth: btintel: read ROM debug registers on FW download failure

[Ravindra]

The bootloader records init sequence progress and debug information in
four core debug registers, which helps debugging FW download failures.

Add btintel_get_rom_debug_info() to issue the vendor-specific HCI
command BTINTEL_GET_ROM_DEBUG_INFO (0xfcde) and log these registers, and
invoke it from the error path in btintel_prepare_fw_download_tlv()
before resetting to the bootloader.

Signed-off-by: Ravindra <ravindra@intel.com>
---
 drivers/bluetooth/btintel.c | 40 +++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btintel.h |  9 +++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 964d2de30e65..15366a6b54c6 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -963,6 +963,43 @@ int btintel_send_intel_reset(struct hci_dev *hdev, u32 boot_param)
 }
 EXPORT_SYMBOL_GPL(btintel_send_intel_reset);
 
+static void btintel_get_rom_debug_info(struct hci_dev *hdev)
+{
+	struct btintel_rp_get_rom_debug_info *rom_debug_info;
+	struct sk_buff *skb;
+
+	skb = __hci_cmd_sync(hdev, BTINTEL_GET_ROM_DEBUG_INFO, 0, NULL,
+			     HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Failed to send intel get rom debug info command (%ld)",
+			   PTR_ERR(skb));
+		return;
+	}
+
+	if (skb->len != sizeof(*rom_debug_info)) {
+		bt_dev_err(hdev, "Intel get rom debug info command parameters size mismatch");
+		kfree_skb(skb);
+		return;
+	}
+
+	rom_debug_info = (struct btintel_rp_get_rom_debug_info *)skb->data;
+
+	if (rom_debug_info->status) {
+		bt_dev_err(hdev, "Intel get rom debug info command failed (%02x)",
+			   rom_debug_info->status);
+		kfree_skb(skb);
+		return;
+	}
+
+	bt_dev_info(hdev, "Intel get rom debug info: dr0:0x%08x dr1:0x%08x dr2:0x%08x dr3:0x%08x",
+		    le32_to_cpu(rom_debug_info->debug_reg0),
+		    le32_to_cpu(rom_debug_info->debug_reg1),
+		    le32_to_cpu(rom_debug_info->debug_reg2),
+		    le32_to_cpu(rom_debug_info->debug_reg3));
+
+	kfree_skb(skb);
+}
+
 int btintel_read_boot_params(struct hci_dev *hdev,
 			     struct intel_boot_params *params)
 {
@@ -2465,6 +2502,9 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
 			goto done;
 		}
 
+		/* FW download failed, hence collecting rom debug info */
+		btintel_get_rom_debug_info(hdev);
+
 		/* When FW download fails, send Intel Reset to retry
 		 * FW download.
 		 */
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index c00f793d9dc8..dedda3e77487 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -178,6 +178,15 @@ struct hci_ppag_enable_cmd {
 	__le32	ppag_enable_flags;
 } __packed;
 
+#define BTINTEL_GET_ROM_DEBUG_INFO	0xFCDE
+struct btintel_rp_get_rom_debug_info {
+	__u8	status;
+	__le32	debug_reg0;
+	__le32	debug_reg1;
+	__le32	debug_reg2;
+	__le32	debug_reg3;
+} __packed;
+
 #define INTEL_TLV_TYPE_ID		0x01
 
 #define INTEL_TLV_SYSTEM_EXCEPTION	0x00
-- 
2.43.0

Re: [PATCH v1] Bluetooth: btintel: read ROM debug registers on FW download failure

[Paul Menzel]

Dear Ravindra,


Thank you for your patch.

Am 17.09.26 um 06:08 schrieb Ravindra:
> The bootloader records init sequence progress and debug information in
> four core debug registers, which helps debugging FW download failures.
> 
> Add btintel_get_rom_debug_info() to issue the vendor-specific HCI
> command BTINTEL_GET_ROM_DEBUG_INFO (0xfcde) and log these registers, and
> invoke it from the error path in btintel_prepare_fw_download_tlv()
> before resetting to the bootloader.

Please paste an example to the commit message.

> Signed-off-by: Ravindra <ravindra@intel.com>
> ---
>   drivers/bluetooth/btintel.c | 40 +++++++++++++++++++++++++++++++++++++
>   drivers/bluetooth/btintel.h |  9 +++++++++
>   2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 964d2de30e65..15366a6b54c6 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -963,6 +963,43 @@ int btintel_send_intel_reset(struct hci_dev *hdev, u32 boot_param)
>   }
>   EXPORT_SYMBOL_GPL(btintel_send_intel_reset);
>   
> +static void btintel_get_rom_debug_info(struct hci_dev *hdev)
> +{
> +	struct btintel_rp_get_rom_debug_info *rom_debug_info;
> +	struct sk_buff *skb;
> +
> +	skb = __hci_cmd_sync(hdev, BTINTEL_GET_ROM_DEBUG_INFO, 0, NULL,
> +			     HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Failed to send intel get rom debug info command (%ld)",
> +			   PTR_ERR(skb));
> +		return;
> +	}
> +
> +	if (skb->len != sizeof(*rom_debug_info)) {
> +		bt_dev_err(hdev, "Intel get rom debug info command parameters size mismatch");

Please add both sizes to the log message.

> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	rom_debug_info = (struct btintel_rp_get_rom_debug_info *)skb->data;
> +
> +	if (rom_debug_info->status) {
> +		bt_dev_err(hdev, "Intel get rom debug info command failed (%02x)",
> +			   rom_debug_info->status);
> +		kfree_skb(skb);
> +		return;
> +	}
> +
> +	bt_dev_info(hdev, "Intel get rom debug info: dr0:0x%08x dr1:0x%08x dr2:0x%08x dr3:0x%08x",
> +		    le32_to_cpu(rom_debug_info->debug_reg0),
> +		    le32_to_cpu(rom_debug_info->debug_reg1),
> +		    le32_to_cpu(rom_debug_info->debug_reg2),
> +		    le32_to_cpu(rom_debug_info->debug_reg3));
> +
> +	kfree_skb(skb);
> +}
> +
>   int btintel_read_boot_params(struct hci_dev *hdev,
>   			     struct intel_boot_params *params)
>   {
> @@ -2465,6 +2502,9 @@ static int btintel_prepare_fw_download_tlv(struct hci_dev *hdev,
>   			goto done;
>   		}
>   
> +		/* FW download failed, hence collecting rom debug info */

Use imperative mood: collecting → collect

> +		btintel_get_rom_debug_info(hdev);
> +
>   		/* When FW download fails, send Intel Reset to retry
>   		 * FW download.
>   		 */
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index c00f793d9dc8..dedda3e77487 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -178,6 +178,15 @@ struct hci_ppag_enable_cmd {
>   	__le32	ppag_enable_flags;
>   } __packed;
>   
> +#define BTINTEL_GET_ROM_DEBUG_INFO	0xFCDE
> +struct btintel_rp_get_rom_debug_info {
> +	__u8	status;
> +	__le32	debug_reg0;
> +	__le32	debug_reg1;
> +	__le32	debug_reg2;
> +	__le32	debug_reg3;
> +} __packed;
> +
>   #define INTEL_TLV_TYPE_ID		0x01
>   
>   #define INTEL_TLV_SYSTEM_EXCEPTION	0x00

With my comments addressed, feel free to add:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

RE: [v1] Bluetooth: btintel: read ROM debug registers on FW download failure

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1019 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1167062/

---Test result---

Test Summary:
CheckPatch                    PASS      0.92 seconds
VerifyFixes                   PASS      0.10 seconds
VerifySignedoff               PASS      0.10 seconds
GitLint                       PASS      0.30 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      34.82 seconds
CheckAllWarning               PASS      31.19 seconds
CheckSparse                   PASS      34.75 seconds
BuildKernel32                 PASS      27.69 seconds
CheckKernelLLVM               PASS      31.12 seconds
TestRunnerSetup               PASS      723.08 seconds
IncrementalBuild              PASS      25.64 seconds



https://github.com/bluez/bluetooth-next/pull/784

---
Regards,
Linux Bluetooth

RE: [PATCH v1] Bluetooth: btintel: read ROM debug registers on FW download failure

[Ravindra]

Hi Paul,

Thank you for the review.

I have addressed all the comments in the next revision:
- Added an example of the ROM debug info output to the commit message.
- Included both the expected and actual sizes in the size mismatch
  log message.
- Updated the comment to use imperative mood.
- Incorporated the remaining review feedback as suggested.

These changes are included in patch 2.

Thanks again for the review.

Regards,
Ravindra
>
> Dear Ravindra,
> 
> 
> Thank you for your patch.
> 
> Am 17.09.26 um 06:08 schrieb Ravindra:
> > The bootloader records init sequence progress and debug information in
> > four core debug registers, which helps debugging FW download failures.
> >
> > Add btintel_get_rom_debug_info() to issue the vendor-specific HCI
> > command BTINTEL_GET_ROM_DEBUG_INFO (0xfcde) and log these
> registers,
> > and invoke it from the error path in btintel_prepare_fw_download_tlv()
> > before resetting to the bootloader.
> 
> Please paste an example to the commit message.
> 
> > Signed-off-by: Ravindra <ravindra@intel.com>
> > ---
> >   drivers/bluetooth/btintel.c | 40
> +++++++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/btintel.h |  9 +++++++++
> >   2 files changed, 49 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 964d2de30e65..15366a6b54c6 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -963,6 +963,43 @@ int btintel_send_intel_reset(struct hci_dev *hdev,
> u32 boot_param)
> >   }
> >   EXPORT_SYMBOL_GPL(btintel_send_intel_reset);
> >
> > +static void btintel_get_rom_debug_info(struct hci_dev *hdev) {
> > +	struct btintel_rp_get_rom_debug_info *rom_debug_info;
> > +	struct sk_buff *skb;
> > +
> > +	skb = __hci_cmd_sync(hdev, BTINTEL_GET_ROM_DEBUG_INFO, 0,
> NULL,
> > +			     HCI_CMD_TIMEOUT);
> > +	if (IS_ERR(skb)) {
> > +		bt_dev_err(hdev, "Failed to send intel get rom debug info
> command (%ld)",
> > +			   PTR_ERR(skb));
> > +		return;
> > +	}
> > +
> > +	if (skb->len != sizeof(*rom_debug_info)) {
> > +		bt_dev_err(hdev, "Intel get rom debug info command
> parameters size
> > +mismatch");
> 
> Please add both sizes to the log message.
> 
> > +		kfree_skb(skb);
> > +		return;
> > +	}
> > +
> > +	rom_debug_info = (struct btintel_rp_get_rom_debug_info *)skb-
> >data;
> > +
> > +	if (rom_debug_info->status) {
> > +		bt_dev_err(hdev, "Intel get rom debug info command failed
> (%02x)",
> > +			   rom_debug_info->status);
> > +		kfree_skb(skb);
> > +		return;
> > +	}
> > +
> > +	bt_dev_info(hdev, "Intel get rom debug info: dr0:0x%08x dr1:0x%08x
> dr2:0x%08x dr3:0x%08x",
> > +		    le32_to_cpu(rom_debug_info->debug_reg0),
> > +		    le32_to_cpu(rom_debug_info->debug_reg1),
> > +		    le32_to_cpu(rom_debug_info->debug_reg2),
> > +		    le32_to_cpu(rom_debug_info->debug_reg3));
> > +
> > +	kfree_skb(skb);
> > +}
> > +
> >   int btintel_read_boot_params(struct hci_dev *hdev,
> >   			     struct intel_boot_params *params)
> >   {
> > @@ -2465,6 +2502,9 @@ static int btintel_prepare_fw_download_tlv(struct
> hci_dev *hdev,
> >   			goto done;
> >   		}
> >
> > +		/* FW download failed, hence collecting rom debug info */
> 
> Use imperative mood: collecting → collect
> 
> > +		btintel_get_rom_debug_info(hdev);
> > +
> >   		/* When FW download fails, send Intel Reset to retry
> >   		 * FW download.
> >   		 */
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index c00f793d9dc8..dedda3e77487 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -178,6 +178,15 @@ struct hci_ppag_enable_cmd {
> >   	__le32	ppag_enable_flags;
> >   } __packed;
> >
> > +#define BTINTEL_GET_ROM_DEBUG_INFO	0xFCDE
> > +struct btintel_rp_get_rom_debug_info {
> > +	__u8	status;
> > +	__le32	debug_reg0;
> > +	__le32	debug_reg1;
> > +	__le32	debug_reg2;
> > +	__le32	debug_reg3;
> > +} __packed;
> > +
> >   #define INTEL_TLV_TYPE_ID		0x01
> >
> >   #define INTEL_TLV_SYSTEM_EXCEPTION	0x00
> 
> With my comments addressed, feel free to add:
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul