This enables prints for firmware download which can help automation
tests to verify firmware download functionality.
A new flag BTNXPUART_FW_DOWNLOAD_ABORT is added which handles the
situation where driver is removed while firmware download is in
progress.
This also adds a check before freeing the rx->skb in flush and close
functions to handle the kernel crash seen in case of firmware download
timeout.
Signed-off-by: Neeraj Sanjay Kale <[email protected]>
Tested-by: Guillaume Legoupil <[email protected]>
---
v2: Handle firmware download abort scenario. (Guillaume Legoupil)
v3: Minor corrections. Add details to commit message.
---
drivers/bluetooth/btnxpuart.c | 67 +++++++++++++++++++++++------------
1 file changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 0b93c2ff29e4..7aed5298ee6f 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -29,6 +29,7 @@
#define BTNXPUART_CHECK_BOOT_SIGNATURE 3
#define BTNXPUART_SERDEV_OPEN 4
#define BTNXPUART_IR_IN_PROGRESS 5
+#define BTNXPUART_FW_DOWNLOAD_ABORT 6
/* NXP HW err codes */
#define BTNXPUART_IR_HW_ERR 0xb0
@@ -159,6 +160,7 @@ struct btnxpuart_dev {
u8 fw_name[MAX_FW_FILE_NAME_LEN];
u32 fw_dnld_v1_offset;
u32 fw_v1_sent_bytes;
+ u32 fw_dnld_v3_offset;
u32 fw_v3_offset_correction;
u32 fw_v1_expected_len;
u32 boot_reg_offset;
@@ -550,6 +552,7 @@ static int nxp_download_firmware(struct hci_dev *hdev)
nxpdev->fw_v1_sent_bytes = 0;
nxpdev->fw_v1_expected_len = HDR_LEN;
nxpdev->boot_reg_offset = 0;
+ nxpdev->fw_dnld_v3_offset = 0;
nxpdev->fw_v3_offset_correction = 0;
nxpdev->baudrate_changed = false;
nxpdev->timeout_changed = false;
@@ -564,14 +567,23 @@ static int nxp_download_firmware(struct hci_dev *hdev)
!test_bit(BTNXPUART_FW_DOWNLOADING,
&nxpdev->tx_state),
msecs_to_jiffies(60000));
+
+ release_firmware(nxpdev->fw);
+ memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
+
if (err == 0) {
- bt_dev_err(hdev, "FW Download Timeout.");
+ bt_dev_err(hdev, "FW Download Timeout. offset: %d",
+ nxpdev->fw_dnld_v1_offset ?
+ nxpdev->fw_dnld_v1_offset :
+ nxpdev->fw_dnld_v3_offset);
return -ETIMEDOUT;
}
+ if (test_bit(BTNXPUART_FW_DOWNLOAD_ABORT, &nxpdev->tx_state)) {
+ bt_dev_err(hdev, "FW Download Aborted");
+ return -EINTR;
+ }
serdev_device_set_flow_control(nxpdev->serdev, true);
- release_firmware(nxpdev->fw);
- memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
/* Allow the downloaded FW to initialize */
msleep(1200);
@@ -693,7 +705,7 @@ static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name)
if (!strlen(nxpdev->fw_name)) {
snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s", fw_name);
- bt_dev_dbg(hdev, "Request Firmware: %s", nxpdev->fw_name);
+ bt_dev_info(hdev, "Request Firmware: %s", nxpdev->fw_name);
err = request_firmware(&nxpdev->fw, nxpdev->fw_name, &hdev->dev);
if (err < 0) {
bt_dev_err(hdev, "Firmware file %s not found", nxpdev->fw_name);
@@ -781,7 +793,7 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
}
if (!len) {
- bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes",
+ bt_dev_info(hdev, "FW Download Complete: %zu bytes",
nxpdev->fw->size);
if (nxp_data->helper_fw_name && !nxpdev->helper_downloaded) {
nxpdev->helper_downloaded = true;
@@ -934,7 +946,7 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
}
if (req->len == 0) {
- bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes",
+ bt_dev_info(hdev, "FW Download Complete: %zu bytes",
nxpdev->fw->size);
clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
@@ -954,8 +966,9 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
goto free_skb;
}
- serdev_device_write_buf(nxpdev->serdev, nxpdev->fw->data + offset -
- nxpdev->fw_v3_offset_correction, len);
+ nxpdev->fw_dnld_v3_offset = offset - nxpdev->fw_v3_offset_correction;
+ serdev_device_write_buf(nxpdev->serdev, nxpdev->fw->data +
+ nxpdev->fw_dnld_v3_offset, len);
free_skb:
kfree_skb(skb);
@@ -1037,7 +1050,7 @@ static int nxp_setup(struct hci_dev *hdev)
if (err < 0)
return err;
} else {
- bt_dev_dbg(hdev, "FW already running.");
+ bt_dev_info(hdev, "FW already running.");
clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
}
@@ -1253,8 +1266,10 @@ static int btnxpuart_close(struct hci_dev *hdev)
ps_wakeup(nxpdev);
serdev_device_close(nxpdev->serdev);
skb_queue_purge(&nxpdev->txq);
- kfree_skb(nxpdev->rx_skb);
- nxpdev->rx_skb = NULL;
+ if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
+ kfree_skb(nxpdev->rx_skb);
+ nxpdev->rx_skb = NULL;
+ }
clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state);
return 0;
}
@@ -1269,8 +1284,10 @@ static int btnxpuart_flush(struct hci_dev *hdev)
cancel_work_sync(&nxpdev->tx_work);
- kfree_skb(nxpdev->rx_skb);
- nxpdev->rx_skb = NULL;
+ if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
+ kfree_skb(nxpdev->rx_skb);
+ nxpdev->rx_skb = NULL;
+ }
return 0;
}
@@ -1385,16 +1402,22 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
struct hci_dev *hdev = nxpdev->hdev;
- /* Restore FW baudrate to fw_init_baudrate if changed.
- * This will ensure FW baudrate is in sync with
- * driver baudrate in case this driver is re-inserted.
- */
- if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
- nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
- nxp_set_baudrate_cmd(hdev, NULL);
+ if (is_fw_downloading(nxpdev)) {
+ set_bit(BTNXPUART_FW_DOWNLOAD_ABORT, &nxpdev->tx_state);
+ clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
+ wake_up_interruptible(&nxpdev->check_boot_sign_wait_q);
+ wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
+ } else {
+ /* Restore FW baudrate to fw_init_baudrate if changed.
+ * This will ensure FW baudrate is in sync with
+ * driver baudrate in case this driver is re-inserted.
+ */
+ if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
+ nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
+ nxp_set_baudrate_cmd(hdev, NULL);
+ }
+ ps_cancel_timer(nxpdev);
}
-
- ps_cancel_timer(nxpdev);
hci_unregister_dev(hdev);
hci_free_dev(hdev);
}
--
2.34.1
Dear Neeraj,
Thank you for the patch.
Am 14.05.24 um 11:14 schrieb Neeraj Sanjay Kale:
> This enables prints for firmware download which can help automation
> tests to verify firmware download functionality.
Please paste the old and new log messages to the commit message.
> A new flag BTNXPUART_FW_DOWNLOAD_ABORT is added which handles the
> situation where driver is removed while firmware download is in
> progress.
Could this be a separate patch?
> This also adds a check before freeing the rx->skb in flush and close
> functions to handle the kernel crash seen in case of firmware download
> timeout.
Please make this a separate commit with a Fixes: tag.
Kind regrads,
Paul
> Signed-off-by: Neeraj Sanjay Kale <[email protected]>
> Tested-by: Guillaume Legoupil <[email protected]>
> ---
> v2: Handle firmware download abort scenario. (Guillaume Legoupil)
> v3: Minor corrections. Add details to commit message.
> ---
> drivers/bluetooth/btnxpuart.c | 67 +++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 0b93c2ff29e4..7aed5298ee6f 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -29,6 +29,7 @@
> #define BTNXPUART_CHECK_BOOT_SIGNATURE 3
> #define BTNXPUART_SERDEV_OPEN 4
> #define BTNXPUART_IR_IN_PROGRESS 5
> +#define BTNXPUART_FW_DOWNLOAD_ABORT 6
>
> /* NXP HW err codes */
> #define BTNXPUART_IR_HW_ERR 0xb0
> @@ -159,6 +160,7 @@ struct btnxpuart_dev {
> u8 fw_name[MAX_FW_FILE_NAME_LEN];
> u32 fw_dnld_v1_offset;
> u32 fw_v1_sent_bytes;
> + u32 fw_dnld_v3_offset;
> u32 fw_v3_offset_correction;
> u32 fw_v1_expected_len;
> u32 boot_reg_offset;
> @@ -550,6 +552,7 @@ static int nxp_download_firmware(struct hci_dev *hdev)
> nxpdev->fw_v1_sent_bytes = 0;
> nxpdev->fw_v1_expected_len = HDR_LEN;
> nxpdev->boot_reg_offset = 0;
> + nxpdev->fw_dnld_v3_offset = 0;
> nxpdev->fw_v3_offset_correction = 0;
> nxpdev->baudrate_changed = false;
> nxpdev->timeout_changed = false;
> @@ -564,14 +567,23 @@ static int nxp_download_firmware(struct hci_dev *hdev)
> !test_bit(BTNXPUART_FW_DOWNLOADING,
> &nxpdev->tx_state),
> msecs_to_jiffies(60000));
> +
> + release_firmware(nxpdev->fw);
> + memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
> +
> if (err == 0) {
> - bt_dev_err(hdev, "FW Download Timeout.");
> + bt_dev_err(hdev, "FW Download Timeout. offset: %d",
> + nxpdev->fw_dnld_v1_offset ?
> + nxpdev->fw_dnld_v1_offset :
> + nxpdev->fw_dnld_v3_offset);
> return -ETIMEDOUT;
> }
> + if (test_bit(BTNXPUART_FW_DOWNLOAD_ABORT, &nxpdev->tx_state)) {
> + bt_dev_err(hdev, "FW Download Aborted");
> + return -EINTR;
> + }
>
> serdev_device_set_flow_control(nxpdev->serdev, true);
> - release_firmware(nxpdev->fw);
> - memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
>
> /* Allow the downloaded FW to initialize */
> msleep(1200);
> @@ -693,7 +705,7 @@ static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name)
> if (!strlen(nxpdev->fw_name)) {
> snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s", fw_name);
>
> - bt_dev_dbg(hdev, "Request Firmware: %s", nxpdev->fw_name);
> + bt_dev_info(hdev, "Request Firmware: %s", nxpdev->fw_name);
> err = request_firmware(&nxpdev->fw, nxpdev->fw_name, &hdev->dev);
> if (err < 0) {
> bt_dev_err(hdev, "Firmware file %s not found", nxpdev->fw_name);
> @@ -781,7 +793,7 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
> }
>
> if (!len) {
> - bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes",
> + bt_dev_info(hdev, "FW Download Complete: %zu bytes",
> nxpdev->fw->size);
> if (nxp_data->helper_fw_name && !nxpdev->helper_downloaded) {
> nxpdev->helper_downloaded = true;
> @@ -934,7 +946,7 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
> }
>
> if (req->len == 0) {
> - bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes",
> + bt_dev_info(hdev, "FW Download Complete: %zu bytes",
> nxpdev->fw->size);
> clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
> @@ -954,8 +966,9 @@ static int nxp_recv_fw_req_v3(struct hci_dev *hdev, struct sk_buff *skb)
> goto free_skb;
> }
>
> - serdev_device_write_buf(nxpdev->serdev, nxpdev->fw->data + offset -
> - nxpdev->fw_v3_offset_correction, len);
> + nxpdev->fw_dnld_v3_offset = offset - nxpdev->fw_v3_offset_correction;
> + serdev_device_write_buf(nxpdev->serdev, nxpdev->fw->data +
> + nxpdev->fw_dnld_v3_offset, len);
>
> free_skb:
> kfree_skb(skb);
> @@ -1037,7 +1050,7 @@ static int nxp_setup(struct hci_dev *hdev)
> if (err < 0)
> return err;
> } else {
> - bt_dev_dbg(hdev, "FW already running.");
> + bt_dev_info(hdev, "FW already running.");
> clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> }
>
> @@ -1253,8 +1266,10 @@ static int btnxpuart_close(struct hci_dev *hdev)
> ps_wakeup(nxpdev);
> serdev_device_close(nxpdev->serdev);
> skb_queue_purge(&nxpdev->txq);
> - kfree_skb(nxpdev->rx_skb);
> - nxpdev->rx_skb = NULL;
> + if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
> + kfree_skb(nxpdev->rx_skb);
> + nxpdev->rx_skb = NULL;
> + }
> clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state);
> return 0;
> }
> @@ -1269,8 +1284,10 @@ static int btnxpuart_flush(struct hci_dev *hdev)
>
> cancel_work_sync(&nxpdev->tx_work);
>
> - kfree_skb(nxpdev->rx_skb);
> - nxpdev->rx_skb = NULL;
> + if (!IS_ERR_OR_NULL(nxpdev->rx_skb)) {
> + kfree_skb(nxpdev->rx_skb);
> + nxpdev->rx_skb = NULL;
> + }
>
> return 0;
> }
> @@ -1385,16 +1402,22 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
> struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
> struct hci_dev *hdev = nxpdev->hdev;
>
> - /* Restore FW baudrate to fw_init_baudrate if changed.
> - * This will ensure FW baudrate is in sync with
> - * driver baudrate in case this driver is re-inserted.
> - */
> - if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
> - nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
> - nxp_set_baudrate_cmd(hdev, NULL);
> + if (is_fw_downloading(nxpdev)) {
> + set_bit(BTNXPUART_FW_DOWNLOAD_ABORT, &nxpdev->tx_state);
> + clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> + wake_up_interruptible(&nxpdev->check_boot_sign_wait_q);
> + wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
> + } else {
> + /* Restore FW baudrate to fw_init_baudrate if changed.
> + * This will ensure FW baudrate is in sync with
> + * driver baudrate in case this driver is re-inserted.
> + */
> + if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
> + nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
> + nxp_set_baudrate_cmd(hdev, NULL);
> + }
> + ps_cancel_timer(nxpdev);
> }
> -
> - ps_cancel_timer(nxpdev);
> hci_unregister_dev(hdev);
> hci_free_dev(hdev);
> }