[v1,1/1] Bluetooth: btnxpuart: Add handling for boot-signature timeout errors

Commit Message

Neeraj Sanjay Kale June 3, 2024, 6:37 a.m. UTC

This handles the timeout errors seen in the bootloader signatures during
FW download.

When the bootloader does not receive a response packet from the host
within a specific time, it adds an error code to the bootloader
signature while requesting for the FW chunk from the same offset.

The host is expected to clear this error code with a NAK, and reply to
only those bootloader signatures which have error code 0.

This error handling is valid for data_req bootloader signatures for V3
and future bootloader versions.

Signed-off-by: Neeraj Sanjay Kale <[e-mail]>
---
 drivers/bluetooth/btnxpuart.c | 46 ++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 4 deletions(-)

Comments

Paul Menzel June 3, 2024, 7:08 a.m. UTC | #1

Dear Neeraj,


Am 03.06.24 um 08:37 schrieb Neeraj Sanjay Kale:
> This handles the timeout errors seen in the bootloader signatures during
> FW download.

Please add the error to the commit message.

> When the bootloader does not receive a response packet from the host
> within a specific time, it adds an error code to the bootloader
> signature while requesting for the FW chunk from the same offset.
> 
> The host is expected to clear this error code with a NAK, and reply to
> only those bootloader signatures which have error code 0.
> 
> This error handling is valid for data_req bootloader signatures for V3
> and future bootloader versions.
> 
> Signed-off-by: Neeraj Sanjay Kale <[e-mail]>

Add a Fixes: tag?

> ---
>   drivers/bluetooth/btnxpuart.c | 46 ++++++++++++++++++++++++++++++++---
>   1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 0b93c2ff29e4..2018513fb961 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -187,6 +187,10 @@ struct btnxpuart_dev {
>   #define NXP_NAK_V3		0x7b
>   #define NXP_CRC_ERROR_V3	0x7c
>   
> +#define NXP_ACK_RX_TIMEOUT	0x0002
> +#define NXP_HDR_RX_TIMEOUT	0x0003
> +#define NXP_DATA_RX_TIMEOUT	0x0004
> +

Please mention the datasheet name, revision and section in the commit 
message for review.

>   #define HDR_LEN			16
>   
>   #define NXP_RECV_CHIP_VER_V1 \
> @@ -277,6 +281,12 @@ struct nxp_bootloader_cmd {
>   	__be32 crc;
>   } __packed;
>   
> +struct nxp_v3_rx_timeout_nak {
> +	u8 nak;
> +	__le32 offset;
> +	u8 crc;
> +} __packed;
> +
>   static u8 crc8_table[CRC8_TABLE_SIZE];
>   
>   /* Default configurations */
> @@ -899,6 +909,32 @@ static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb)
>   	return 0;
>   }
>   
> +static void nxp_handle_fw_dnld_error(struct hci_dev *hdev, struct v3_data_req *req)

I would not use abbreviations, and use download.

> +{
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	__u32 offset = __le32_to_cpu(req->offset);
> +	__u16 err = __le16_to_cpu(req->error);
> +	struct nxp_v3_rx_timeout_nak nak_tx_buf;
> +
> +	switch (err) {
> +	case NXP_ACK_RX_TIMEOUT:
> +	case NXP_HDR_RX_TIMEOUT:
> +	case NXP_DATA_RX_TIMEOUT:
> +		nak_tx_buf.nak = NXP_NAK_V3;
> +		nak_tx_buf.offset = __cpu_to_le32(offset);
> +		nak_tx_buf.crc = crc8(crc8_table, (u8 *)&nak_tx_buf,

Can the cast be avoided and written differently?

> +				      sizeof(nak_tx_buf) - 1, 0xff);
> +		serdev_device_write_buf(nxpdev->serdev, (u8 *)&nak_tx_buf,
> +					sizeof(nak_tx_buf));
> +		break;
> +	default:
> +		bt_dev_dbg(hdev, "Unknown bootloader error: %d", err);
> +		break;
> +
> +	}
> +
> +}
> +
>   static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
>   {
>   	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> @@ -913,7 +949,12 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
>   	if (!req || !nxpdev->fw)
>   		goto free_skb;
>   
> -	nxp_send_ack(NXP_ACK_V3, hdev);
> +	if (!req->error) {
> +		nxp_send_ack(NXP_ACK_V3, hdev);
> +	} else {
> +		nxp_handle_fw_dnld_error(hdev, req);
> +		goto free_skb;
> +	}
>   
>   	len = __le16_to_cpu(req->len);
>   
> @@ -940,9 +981,6 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
>   		wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
>   		goto free_skb;
>   	}
> -	if (req->error)
> -		bt_dev_dbg(hdev, "FW Download received err 0x%02x from chip",
> -			   req->error);
>   
>   	offset = __le32_to_cpu(req->offset);
>   	if (offset < nxpdev->fw_v3_offset_correction) {


Kind regards,

Paul

[e-mail] June 3, 2024, 7:32 a.m. UTC | #2

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/project/bluetooth/list/?series=858161

---Test result---

Test Summary:
CheckPatch                    PASS      3.18 seconds
GitLint                       PASS      1.59 seconds
SubjectPrefix                 PASS      0.16 seconds
BuildKernel                   PASS      29.90 seconds
CheckAllWarning               PASS      32.62 seconds
CheckSparse                   PASS      38.59 seconds
CheckSmatch                   FAIL      34.86 seconds
BuildKernel32                 PASS      28.91 seconds
TestRunnerSetup               PASS      524.43 seconds
TestRunner_l2cap-tester       PASS      20.40 seconds
TestRunner_iso-tester         PASS      28.57 seconds
TestRunner_bnep-tester        PASS      4.83 seconds
TestRunner_mgmt-tester        PASS      116.55 seconds
TestRunner_rfcomm-tester      PASS      8.72 seconds
TestRunner_sco-tester         PASS      15.00 seconds
TestRunner_ioctl-tester       PASS      7.90 seconds
TestRunner_mesh-tester        FAIL      6.14 seconds
TestRunner_smp-tester         PASS      6.89 seconds
TestRunner_userchan-tester    PASS      5.09 seconds
IncrementalBuild              PASS      28.05 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 2                               Failed       0.094 seconds


---
Regards,
Linux Bluetooth

Neeraj Sanjay Kale June 14, 2024, 8:42 a.m. UTC | #3

Hi Paul,

Thank you for reviewing the patch and sorry for the delay in response.
I have addressed most of you comments and sent out patch V2 for review.

> 
> Am 03.06.24 um 08:37 schrieb Neeraj Sanjay Kale:
> > This handles the timeout errors seen in the bootloader signatures
> > during FW download.
> 
> Please add the error to the commit message.
Well there is no error printed in dmesg. The bootloader signature contains an error field which has certain bits set when the chip does not receive the expected response from the host.
There was a debug print "FW Download received err 0x04 from chip" when driver debug was enabled, but this error was not handled in the initial development phase of this driver.
As long as bootloader signature contains an error, the chip keeps requesting for the same FW chunk.
This error clears itself after few iterations, which is indeterministic and FW download proceeds ahead.
The bootloader expects the host to acknowledge the error and clear it by replying with a NAK.
I have tried to explain this by rephrasing the commit message in V2. Let me know if that's fine.

> > @@ -187,6 +187,10 @@ struct btnxpuart_dev {
> >   #define NXP_NAK_V3          0x7b
> >   #define NXP_CRC_ERROR_V3    0x7c
> >
> > +#define NXP_ACK_RX_TIMEOUT   0x0002
> > +#define NXP_HDR_RX_TIMEOUT   0x0003
> > +#define NXP_DATA_RX_TIMEOUT  0x0004
> > +
> 
> Please mention the datasheet name, revision and section in the commit
> message for review.
As mentioned earlier, these error codes are part of bootloader signatures. Unfortunately I could not find any document/datasheet in public domain that mentioned these error codes.
I have added few comments instead which explain the meaning of these error codes in V2 patch.

Thanks,
Neeraj

Patch

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 0b93c2ff29e4..2018513fb961 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -187,6 +187,10 @@  struct btnxpuart_dev {
 #define NXP_NAK_V3		0x7b
 #define NXP_CRC_ERROR_V3	0x7c
 
+#define NXP_ACK_RX_TIMEOUT	0x0002
+#define NXP_HDR_RX_TIMEOUT	0x0003
+#define NXP_DATA_RX_TIMEOUT	0x0004
+
 #define HDR_LEN			16
 
 #define NXP_RECV_CHIP_VER_V1 \
@@ -277,6 +281,12 @@  struct nxp_bootloader_cmd {
 	__be32 crc;
 } __packed;
 
+struct nxp_v3_rx_timeout_nak {
+	u8 nak;
+	__le32 offset;
+	u8 crc;
+} __packed;
+
 static u8 crc8_table[CRC8_TABLE_SIZE];
 
 /* Default configurations */
@@ -899,6 +909,32 @@  static int nxp_recv_chip_ver_v3(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+static void nxp_handle_fw_dnld_error(struct hci_dev *hdev, struct v3_data_req *req)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	__u32 offset = __le32_to_cpu(req->offset);
+	__u16 err = __le16_to_cpu(req->error);
+	struct nxp_v3_rx_timeout_nak nak_tx_buf;
+
+	switch (err) {
+	case NXP_ACK_RX_TIMEOUT:
+	case NXP_HDR_RX_TIMEOUT:
+	case NXP_DATA_RX_TIMEOUT:
+		nak_tx_buf.nak = NXP_NAK_V3;
+		nak_tx_buf.offset = __cpu_to_le32(offset);
+		nak_tx_buf.crc = crc8(crc8_table, (u8 *)&nak_tx_buf,
+				      sizeof(nak_tx_buf) - 1, 0xff);
+		serdev_device_write_buf(nxpdev->serdev, (u8 *)&nak_tx_buf,
+					sizeof(nak_tx_buf));
+		break;
+	default:
+		bt_dev_dbg(hdev, "Unknown bootloader error: %d", err);
+		break;
+
+	}
+
+}
+
 static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
@@ -913,7 +949,12 @@  static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
 	if (!req || !nxpdev->fw)
 		goto free_skb;
 
-	nxp_send_ack(NXP_ACK_V3, hdev);
+	if (!req->error) {
+		nxp_send_ack(NXP_ACK_V3, hdev);
+	} else {
+		nxp_handle_fw_dnld_error(hdev, req);
+		goto free_skb;
+	}
 
 	len = __le16_to_cpu(req->len);
 
@@ -940,9 +981,6 @@  static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
 		wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
 		goto free_skb;
 	}
-	if (req->error)
-		bt_dev_dbg(hdev, "FW Download received err 0x%02x from chip",
-			   req->error);
 
 	offset = __le32_to_cpu(req->offset);
 	if (offset < nxpdev->fw_v3_offset_correction) {