[PATCH v1 1/2] Bluetooth: btnxpuart: Move vendor specific initialization to .post_init

[PATCH v1 1/2] Bluetooth: btnxpuart: Move vendor specific initialization to .post_init

[Neeraj Sanjay Kale]

This moves change baudrate and power save vendor commands from
nxp_setup() to nxp_post_init().

This also moves the baudrate restore logic from nxp_serdev_remove() to
nxp_shutdown() which ensure baudrate is restored even when HCI dev is
down, preventing baudrate mismatch between host and controller when
device is probed again next time.

In case of removal when the hdev is up and running, we have to call the
shutdown procedure explicitly before unregistering the hdev.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Co-developed-by: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
---
 drivers/bluetooth/btnxpuart.c | 45 +++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 4f2f429c9800..5f07a57532cb 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -630,11 +630,6 @@ static void ps_init(struct hci_dev *hdev)
 
 	psdata->cur_psmode = PS_MODE_DISABLE;
 	psdata->target_ps_mode = DEFAULT_PS_MODE;
-
-	if (psdata->cur_h2c_wakeupmode != psdata->h2c_wakeupmode)
-		hci_cmd_sync_queue(hdev, send_wakeup_method_cmd, NULL, NULL);
-	if (psdata->cur_psmode != psdata->target_ps_mode)
-		hci_cmd_sync_queue(hdev, send_ps_cmd, NULL, NULL);
 }
 
 /* NXP Firmware Download Feature */
@@ -1228,11 +1223,6 @@ static int nxp_setup(struct hci_dev *hdev)
 	serdev_device_set_baudrate(nxpdev->serdev, nxpdev->fw_init_baudrate);
 	nxpdev->current_baudrate = nxpdev->fw_init_baudrate;
 
-	if (nxpdev->current_baudrate != HCI_NXP_SEC_BAUDRATE) {
-		nxpdev->new_baudrate = HCI_NXP_SEC_BAUDRATE;
-		hci_cmd_sync_queue(hdev, nxp_set_baudrate_cmd, NULL, NULL);
-	}
-
 	ps_init(hdev);
 
 	if (test_and_clear_bit(BTNXPUART_IR_IN_PROGRESS, &nxpdev->tx_state))
@@ -1241,6 +1231,22 @@ static int nxp_setup(struct hci_dev *hdev)
 	return 0;
 }
 
+static int nxp_post_init(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct ps_data *psdata = &nxpdev->psdata;
+
+	if (nxpdev->current_baudrate != HCI_NXP_SEC_BAUDRATE) {
+		nxpdev->new_baudrate = HCI_NXP_SEC_BAUDRATE;
+		nxp_set_baudrate_cmd(hdev, NULL);
+	}
+	if (psdata->cur_h2c_wakeupmode != psdata->h2c_wakeupmode)
+		send_wakeup_method_cmd(hdev, NULL);
+	if (psdata->cur_psmode != psdata->target_ps_mode)
+		send_ps_cmd(hdev, NULL);
+	return 0;
+}
+
 static void nxp_hw_err(struct hci_dev *hdev, u8 code)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
@@ -1273,6 +1279,9 @@ static int nxp_shutdown(struct hci_dev *hdev)
 			set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
 		}
 		kfree_skb(skb);
+	} else if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
+		nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
+		nxp_set_baudrate_cmd(hdev, NULL);
 	}
 
 	return 0;
@@ -1566,6 +1575,7 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
 	hdev->close = btnxpuart_close;
 	hdev->flush = btnxpuart_flush;
 	hdev->setup = nxp_setup;
+	hdev->post_init = nxp_post_init;
 	hdev->send  = nxp_enqueue;
 	hdev->hw_error = nxp_hw_err;
 	hdev->shutdown = nxp_shutdown;
@@ -1597,16 +1607,15 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
 		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 (test_bit(HCI_RUNNING, &hdev->flags)) {
+		/* Ensure shutdown callback is executed before unregistering, so
+		 * that baudrate is reset to initial value.
 		 */
-		if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
-			nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
-			nxp_set_baudrate_cmd(hdev, NULL);
-		}
+		nxp_shutdown(hdev);
 	}
+
 	ps_cleanup(nxpdev);
 	hci_unregister_dev(hdev);
 	hci_free_dev(hdev);
-- 
2.25.1

[PATCH v1 2/2] Bluetooth: btnxpuart: Add support for HCI coredump feature

[Neeraj Sanjay Kale]

This adds support for Bluetooth Coredump feature to BTNXPUART driver to
collect FW dumps on demand, or in case FW goes in a bad state.

To trigger manual FW dump, following command can be used:
echo 1 > /sys/class/bluetooth/hci0/device/coredump

Once FW dump is complete, it can be written to a file:
cat /sys/class/bluetooth/hci0/devcoredump/data > fw_dump

While FW dump is in progress, any HCI command will return -EBUSY.

After FW dump is complete, driver will give HCI_NXP_IND_RESET command
which soft-resets the chip, allowing FW re-download.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 147 ++++++++++++++++++++++++++++++----
 1 file changed, 132 insertions(+), 15 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 5f07a57532cb..200ed95ff588 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -31,6 +31,7 @@
 #define BTNXPUART_SERDEV_OPEN		4
 #define BTNXPUART_IR_IN_PROGRESS	5
 #define BTNXPUART_FW_DOWNLOAD_ABORT	6
+#define BTNXPUART_FW_DUMP_IN_PROGRESS	7
 
 /* NXP HW err codes */
 #define BTNXPUART_IR_HW_ERR		0xb0
@@ -106,6 +107,8 @@
 #define HCI_NXP_SET_OPER_SPEED	0xfc09
 /* Bluetooth vendor command: Independent Reset */
 #define HCI_NXP_IND_RESET	0xfcfc
+/* Bluetooth vendor command: Trigger FW dump */
+#define HCI_NXP_TRIGGER_DUMP	0xfe91
 
 /* Bluetooth Power State : Vendor cmd params */
 #define BT_PS_ENABLE			0x02
@@ -310,6 +313,16 @@ union nxp_v3_rx_timeout_nak_u {
 	u8 buf[6];
 };
 
+/* FW dump */
+#define NXP_FW_DUMP_SIZE	(1024 * 1000)
+
+struct nxp_fw_dump_hdr {
+	__le16 seq_num;
+	__le16 reserved;
+	__le16 buf_type;
+	__le16 buf_len;
+};
+
 static u8 crc8_table[CRC8_TABLE_SIZE];
 
 /* Default configurations */
@@ -774,6 +787,16 @@ static bool is_fw_downloading(struct btnxpuart_dev *nxpdev)
 	return test_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
 }
 
+static bool ind_reset_in_progress(struct btnxpuart_dev *nxpdev)
+{
+	return test_bit(BTNXPUART_IR_IN_PROGRESS, &nxpdev->tx_state);
+}
+
+static bool fw_dump_in_progress(struct btnxpuart_dev *nxpdev)
+{
+	return test_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state);
+}
+
 static bool process_boot_signature(struct btnxpuart_dev *nxpdev)
 {
 	if (test_bit(BTNXPUART_CHECK_BOOT_SIGNATURE, &nxpdev->tx_state)) {
@@ -1175,7 +1198,7 @@ static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data)
 static int nxp_check_boot_sign(struct btnxpuart_dev *nxpdev)
 {
 	serdev_device_set_baudrate(nxpdev->serdev, HCI_NXP_PRI_BAUDRATE);
-	if (test_bit(BTNXPUART_IR_IN_PROGRESS, &nxpdev->tx_state))
+	if (ind_reset_in_progress(nxpdev))
 		serdev_device_set_flow_control(nxpdev->serdev, false);
 	else
 		serdev_device_set_flow_control(nxpdev->serdev, true);
@@ -1204,6 +1227,73 @@ static int nxp_set_ind_reset(struct hci_dev *hdev, void *data)
 	return hci_recv_frame(hdev, skb);
 }
 
+/* Firmware dump */
+static void nxp_coredump(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	u8 pcmd = 2;
+
+	skb = nxp_drv_send_cmd(hdev, HCI_NXP_TRIGGER_DUMP, 1, &pcmd);
+	if (!IS_ERR(skb))
+		kfree_skb(skb);
+}
+
+static void nxp_coredump_hdr(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* Nothing to be added in FW dump header */
+}
+
+static int nxp_process_fw_dump(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_acl_hdr *acl_hdr = (struct hci_acl_hdr *)skb_pull_data(skb,
+									  sizeof(*acl_hdr));
+	struct nxp_fw_dump_hdr *fw_dump_hdr = (struct nxp_fw_dump_hdr *)skb->data;
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	__u16 seq_num = __le16_to_cpu(fw_dump_hdr->seq_num);
+	__u16 buf_len = __le16_to_cpu(fw_dump_hdr->buf_len);
+	int err;
+
+	if (seq_num == 0x0001) {
+		if (test_and_set_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state)) {
+			bt_dev_err(hdev, "FW dump already in progress");
+			goto free_skb;
+		}
+		bt_dev_warn(hdev, "==== Start FW dump ===");
+		err = hci_devcd_init(hdev, NXP_FW_DUMP_SIZE);
+		if (err < 0)
+			goto free_skb;
+
+		schedule_delayed_work(&hdev->dump.dump_timeout,
+				      msecs_to_jiffies(20000));
+	}
+
+	err = hci_devcd_append(hdev, skb_clone(skb, GFP_ATOMIC));
+	if (err < 0)
+		goto free_skb;
+
+	if (buf_len == 0) {
+		bt_dev_warn(hdev, "==== FW dump complete ===");
+		clear_bit(BTNXPUART_FW_DUMP_IN_PROGRESS, &nxpdev->tx_state);
+		hci_devcd_complete(hdev);
+		nxp_set_ind_reset(hdev, NULL);
+	}
+
+free_skb:
+	kfree_skb(skb);
+	return 0;
+}
+
+static int nxp_recv_acl_pkt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	__u16 handle = __le16_to_cpu(hci_acl_hdr(skb)->handle);
+
+	/* FW dump chunks are ACL packets with conn handle 0xfff */
+	if ((handle & 0x0FFF) == 0xFFF)
+		return nxp_process_fw_dump(hdev, skb);
+	else
+		return hci_recv_frame(hdev, skb);
+}
+
 /* NXP protocol */
 static int nxp_setup(struct hci_dev *hdev)
 {
@@ -1265,20 +1355,15 @@ static int nxp_shutdown(struct hci_dev *hdev)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 	struct sk_buff *skb;
-	u8 *status;
 	u8 pcmd = 0;
 
-	if (test_bit(BTNXPUART_IR_IN_PROGRESS, &nxpdev->tx_state)) {
+	if (ind_reset_in_progress(nxpdev)) {
 		skb = nxp_drv_send_cmd(hdev, HCI_NXP_IND_RESET, 1, &pcmd);
-		if (IS_ERR(skb))
-			return PTR_ERR(skb);
-
-		status = skb_pull_data(skb, 1);
-		if (status) {
-			serdev_device_set_flow_control(nxpdev->serdev, false);
-			set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
-		}
-		kfree_skb(skb);
+		serdev_device_set_flow_control(nxpdev->serdev, false);
+		set_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
+		/* HCI_NXP_IND_RESET command may not returns any response */
+		if (!IS_ERR(skb))
+			kfree_skb(skb);
 	} else if (nxpdev->current_baudrate != nxpdev->fw_init_baudrate) {
 		nxpdev->new_baudrate = nxpdev->fw_init_baudrate;
 		nxp_set_baudrate_cmd(hdev, NULL);
@@ -1298,6 +1383,16 @@ static bool nxp_wakeup(struct hci_dev *hdev)
 	return false;
 }
 
+static void nxp_cmd_timeout(struct hci_dev *hdev)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+
+	if (!ind_reset_in_progress(nxpdev) && !fw_dump_in_progress(nxpdev)) {
+		bt_dev_dbg(hdev, "CMD Timeout detected. Resetting.");
+		nxp_set_ind_reset(hdev, NULL);
+	}
+}
+
 static int btnxpuart_queue_skb(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
@@ -1318,6 +1413,9 @@ static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff *skb)
 	struct wakeup_cmd_payload wakeup_parm;
 	__le32 baudrate_parm;
 
+	if (fw_dump_in_progress(nxpdev))
+		return -EBUSY;
+
 	/* if vendor commands are received from user space (e.g. hcitool), update
 	 * driver flags accordingly and ask driver to re-send the command to FW.
 	 * In case the payload for any command does not match expected payload
@@ -1486,7 +1584,7 @@ static int btnxpuart_flush(struct hci_dev *hdev)
 }
 
 static const struct h4_recv_pkt nxp_recv_pkts[] = {
-	{ H4_RECV_ACL,          .recv = hci_recv_frame },
+	{ H4_RECV_ACL,          .recv = nxp_recv_acl_pkt },
 	{ H4_RECV_SCO,          .recv = hci_recv_frame },
 	{ H4_RECV_EVENT,        .recv = hci_recv_frame },
 	{ H4_RECV_ISO,		.recv = hci_recv_frame },
@@ -1508,11 +1606,13 @@ static size_t btnxpuart_receive_buf(struct serdev_device *serdev,
 	if (IS_ERR(nxpdev->rx_skb)) {
 		int err = PTR_ERR(nxpdev->rx_skb);
 		/* Safe to ignore out-of-sync bootloader signatures */
-		if (!is_fw_downloading(nxpdev))
+		if (!is_fw_downloading(nxpdev) &&
+		    !ind_reset_in_progress(nxpdev))
 			bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
 		return count;
 	}
-	if (!is_fw_downloading(nxpdev))
+	if (!is_fw_downloading(nxpdev) &&
+	    !ind_reset_in_progress(nxpdev))
 		nxpdev->hdev->stat.byte_rx += count;
 	return count;
 }
@@ -1580,6 +1680,7 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
 	hdev->hw_error = nxp_hw_err;
 	hdev->shutdown = nxp_shutdown;
 	hdev->wakeup = nxp_wakeup;
+	hdev->cmd_timeout = nxp_cmd_timeout;
 	SET_HCIDEV_DEV(hdev, &serdev->dev);
 
 	if (hci_register_dev(hdev) < 0) {
@@ -1590,6 +1691,8 @@ static int nxp_serdev_probe(struct serdev_device *serdev)
 	if (ps_setup(hdev))
 		goto probe_fail;
 
+	hci_devcd_register(hdev, nxp_coredump, nxp_coredump_hdr, NULL);
+
 	return 0;
 
 probe_fail:
@@ -1641,6 +1744,17 @@ static int nxp_serdev_resume(struct device *dev)
 }
 #endif
 
+#ifdef CONFIG_DEV_COREDUMP
+static void nxp_serdev_coredump(struct device *dev)
+{
+	struct btnxpuart_dev *nxpdev = dev_get_drvdata(dev);
+	struct hci_dev  *hdev = nxpdev->hdev;
+
+	if (hdev->dump.coredump)
+		hdev->dump.coredump(hdev);
+}
+#endif
+
 static struct btnxpuart_data w8987_data __maybe_unused = {
 	.helper_fw_name = NULL,
 	.fw_name = FIRMWARE_W8987,
@@ -1671,6 +1785,9 @@ static struct serdev_device_driver nxp_serdev_driver = {
 		.name = "btnxpuart",
 		.of_match_table = of_match_ptr(nxpuart_of_match_table),
 		.pm = &nxp_pm_ops,
+#ifdef CONFIG_DEV_COREDUMP
+		.coredump = nxp_serdev_coredump,
+#endif
 	},
 };
 
-- 
2.25.1

RE: [v1,1/2] Bluetooth: btnxpuart: Move vendor specific initialization to .post_init

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1948 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/project/bluetooth/list/?series=938640

---Test result---

Test Summary:
CheckPatch                    PENDING   0.22 seconds
GitLint                       PENDING   0.27 seconds
SubjectPrefix                 PASS      0.21 seconds
BuildKernel                   PASS      23.95 seconds
CheckAllWarning               PASS      26.71 seconds
CheckSparse                   PASS      29.81 seconds
BuildKernel32                 PASS      23.77 seconds
TestRunnerSetup               PASS      427.85 seconds
TestRunner_l2cap-tester       PASS      21.02 seconds
TestRunner_iso-tester         PASS      38.03 seconds
TestRunner_bnep-tester        PASS      4.85 seconds
TestRunner_mgmt-tester        FAIL      120.37 seconds
TestRunner_rfcomm-tester      PASS      7.85 seconds
TestRunner_sco-tester         PASS      9.53 seconds
TestRunner_ioctl-tester       PASS      8.47 seconds
TestRunner_mesh-tester        PASS      5.94 seconds
TestRunner_smp-tester         PASS      7.16 seconds
TestRunner_userchan-tester    PASS      5.01 seconds
IncrementalBuild              PENDING   0.56 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
LL Privacy - Set Flags 3 (2 Devices to RL)           Failed       0.194 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Move vendor specific initialization to .post_init

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 27 Feb 2025 23:56:19 +0530 you wrote:
> This moves change baudrate and power save vendor commands from
> nxp_setup() to nxp_post_init().
> 
> This also moves the baudrate restore logic from nxp_serdev_remove() to
> nxp_shutdown() which ensure baudrate is restored even when HCI dev is
> down, preventing baudrate mismatch between host and controller when
> device is probed again next time.
> 
> [...]

Here is the summary with links:
  - [v1,1/2] Bluetooth: btnxpuart: Move vendor specific initialization to .post_init
    https://git.kernel.org/bluetooth/bluetooth-next/c/6391ddf8e285
  - [v1,2/2] Bluetooth: btnxpuart: Add support for HCI coredump feature
    https://git.kernel.org/bluetooth/bluetooth-next/c/b21ae0e8b437

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html