[PATCH 0/3] Bluetooth: btusb: Expose hw reset to debugfs

[PATCH 0/3] Bluetooth: btusb: Expose hw reset to debugfs

[Abhishek Pandit-Subedi]

Hi Marcel,

This series updates the reset gpio based recovery from command
timeouts with the following changes:
  * Refactor duplicate code to use a single btusb_gpio_cmd_timeout
  * Reduce the number of command timeouts needed for reset to 3
  * Expose the gpio based hardware reset to debugfs for testing

The last one is important to us so that we can confirm new chips support
hardware based reset (which is part of our requirements for BT chips).

We will probably also add the 'toggle_hw_reset' debugfs entry to other
drivers that support hardware based reset (i.e. hci_qca, hci_h5, etc) in
subsequent changes.

Thanks
Abhishek



Abhishek Pandit-Subedi (3):
  Bluetooth: btusb: Refactor gpio reset
  Bluetooth: btusb: Trigger gpio reset quicker
  Bluetooth: btusb: Expose reset gpio to debugfs

 drivers/bluetooth/btusb.c | 107 +++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 41 deletions(-)

-- 
2.30.0.284.gd98b1dd5eaa7-goog

[PATCH 1/3] Bluetooth: btusb: Refactor gpio reset

[Abhishek Pandit-Subedi]

Refactor gpio reset to use a common gpio reset function.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 drivers/bluetooth/btusb.c | 59 +++++++++++++--------------------------
 1 file changed, 19 insertions(+), 40 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b14102fba6018..03341e6cbf3ed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -547,6 +547,7 @@ struct btusb_data {
 	struct usb_endpoint_descriptor *diag_rx_ep;
 
 	struct gpio_desc *reset_gpio;
+	unsigned int reset_duration_ms;
 
 	__u8 cmdreq_type;
 	__u8 cmdreq;
@@ -566,15 +567,21 @@ struct btusb_data {
 	unsigned cmd_timeout_cnt;
 };
 
-static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
+static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
+{
+	gpiod_set_value_cansleep(desc, 1);
+	msleep(duration);
+	gpiod_set_value_cansleep(desc, 0);
+}
+
+static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
-	struct gpio_desc *reset_gpio = data->reset_gpio;
 
 	if (++data->cmd_timeout_cnt < 5)
 		return;
 
-	if (!reset_gpio) {
+	if (!data->reset_gpio) {
 		bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
 		return;
 	}
@@ -592,39 +599,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
 	}
 
 	bt_dev_err(hdev, "Initiating HW reset via gpio");
-	gpiod_set_value_cansleep(reset_gpio, 1);
-	msleep(100);
-	gpiod_set_value_cansleep(reset_gpio, 0);
-}
-
-static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
-{
-	struct btusb_data *data = hci_get_drvdata(hdev);
-	struct gpio_desc *reset_gpio = data->reset_gpio;
-
-	if (++data->cmd_timeout_cnt < 5)
-		return;
-
-	if (!reset_gpio) {
-		bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring");
-		return;
-	}
-
-	/* Toggle the hard reset line. The Realtek device is going to
-	 * yank itself off the USB and then replug. The cleanup is handled
-	 * correctly on the way out (standard USB disconnect), and the new
-	 * device is detected cleanly and bound to the driver again like
-	 * it should be.
-	 */
-	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
-		bt_dev_err(hdev, "last reset failed? Not resetting again");
-		return;
-	}
-
-	bt_dev_err(hdev, "Reset Realtek device via gpio");
-	gpiod_set_value_cansleep(reset_gpio, 1);
-	msleep(200);
-	gpiod_set_value_cansleep(reset_gpio, 0);
+	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
 }
 
 static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
@@ -4462,7 +4437,8 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->shutdown = btusb_shutdown_intel;
 		hdev->set_diag = btintel_set_diag_mfg;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4476,7 +4452,8 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->hw_error = btintel_hw_error;
 		hdev->set_diag = btintel_set_diag;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4490,7 +4467,8 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->hw_error = btintel_hw_error;
 		hdev->set_diag = btintel_set_diag;
 		hdev->set_bdaddr = btintel_set_bdaddr;
-		hdev->cmd_timeout = btusb_intel_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 100;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
@@ -4557,7 +4535,8 @@ static int btusb_probe(struct usb_interface *intf,
 	    (id->driver_info & BTUSB_REALTEK)) {
 		hdev->setup = btrtl_setup_realtek;
 		hdev->shutdown = btrtl_shutdown_realtek;
-		hdev->cmd_timeout = btusb_rtl_cmd_timeout;
+		hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+		data->reset_duration_ms = 200;
 
 		/* Realtek devices lose their updated firmware over global
 		 * suspend that means host doesn't send SET_FEATURE
-- 
2.30.0.284.gd98b1dd5eaa7-goog

RE: Bluetooth: btusb: Expose hw reset to debugfs

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2428 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=417689

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - FAIL
    workflow: Add workflow files for ci
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Refactor gpio reset
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Trigger gpio reset quicker
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing

Bluetooth: btusb: Expose reset gpio to debugfs
1: T1 Title exceeds max length (92>72): "Merge a4ba03ae3144a0297db94b0f7e05f7d1317f9c79 into db399c15154d49dd44bfb3b97a77b377819d8566"
3: B6 Body message is missing


    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - PASS
    Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43340 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3530 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 546678 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11651 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9886 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11797 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5428 bytes --]

Re: [PATCH 1/3] Bluetooth: btusb: Refactor gpio reset

[Marcel Holtmann]

Hi Abhishek,

> Refactor gpio reset to use a common gpio reset function.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> drivers/bluetooth/btusb.c | 59 +++++++++++++--------------------------
> 1 file changed, 19 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b14102fba6018..03341e6cbf3ed 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -547,6 +547,7 @@ struct btusb_data {
> 	struct usb_endpoint_descriptor *diag_rx_ep;
> 
> 	struct gpio_desc *reset_gpio;
> +	unsigned int reset_duration_ms;
> 
> 	__u8 cmdreq_type;
> 	__u8 cmdreq;
> @@ -566,15 +567,21 @@ struct btusb_data {
> 	unsigned cmd_timeout_cnt;
> };
> 
> -static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> +static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
> +{
> +	gpiod_set_value_cansleep(desc, 1);
> +	msleep(duration);
> +	gpiod_set_value_cansleep(desc, 0);
> +}
> +
> +static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
> {
> 	struct btusb_data *data = hci_get_drvdata(hdev);
> -	struct gpio_desc *reset_gpio = data->reset_gpio;
> 
> 	if (++data->cmd_timeout_cnt < 5)
> 		return;
> 
> -	if (!reset_gpio) {
> +	if (!data->reset_gpio) {
> 		bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
> 		return;
> 	}
> @@ -592,39 +599,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> 	}
> 
> 	bt_dev_err(hdev, "Initiating HW reset via gpio");
> -	gpiod_set_value_cansleep(reset_gpio, 1);
> -	msleep(100);
> -	gpiod_set_value_cansleep(reset_gpio, 0);
> -}
> -
> -static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
> -{
> -	struct btusb_data *data = hci_get_drvdata(hdev);
> -	struct gpio_desc *reset_gpio = data->reset_gpio;
> -
> -	if (++data->cmd_timeout_cnt < 5)
> -		return;
> -
> -	if (!reset_gpio) {
> -		bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring");
> -		return;
> -	}
> -
> -	/* Toggle the hard reset line. The Realtek device is going to
> -	 * yank itself off the USB and then replug. The cleanup is handled
> -	 * correctly on the way out (standard USB disconnect), and the new
> -	 * device is detected cleanly and bound to the driver again like
> -	 * it should be.
> -	 */
> -	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> -		bt_dev_err(hdev, "last reset failed? Not resetting again");
> -		return;
> -	}
> -
> -	bt_dev_err(hdev, "Reset Realtek device via gpio");
> -	gpiod_set_value_cansleep(reset_gpio, 1);
> -	msleep(200);
> -	gpiod_set_value_cansleep(reset_gpio, 0);
> +	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
> }

You need to explain why this patch is correct. You are removing more code here. And there is an extra check in case of Realtek and a large comment.

Regards

Marcel

[PATCH 2/3] Bluetooth: btusb: Trigger gpio reset quicker

[Abhishek Pandit-Subedi]

Currently, btusb will only trigger gpio reset during cmd_timeout after
5 commands fail. This number is arbitrarily large and can result in
resets taking longer to occur than necessary.

Reduce this number to 3, which was chosen as a recommended value by
Intel (their firmware allow two commands in flight so they recommend
resetting on the third failed command).

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Daniel Winkler <danielwinkler@google.com>
---

 drivers/bluetooth/btusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 03341e6cbf3ed..880e9cd4ee713 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -578,7 +578,7 @@ static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
 
-	if (++data->cmd_timeout_cnt < 5)
+	if (++data->cmd_timeout_cnt < 3)
 		return;
 
 	if (!data->reset_gpio) {
-- 
2.30.0.284.gd98b1dd5eaa7-goog

[PATCH 3/3] Bluetooth: btusb: Expose reset gpio to debugfs

[Abhishek Pandit-Subedi]

If btusb has a reset gpio, expose it to userspace so we can toggle the
reset gpio directly. This is useful for testing and error recovery.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---

 drivers/bluetooth/btusb.c | 46 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 880e9cd4ee713..702be1871ed88 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -6,6 +6,7 @@
  *  Copyright (C) 2005-2008  Marcel Holtmann <marcel@holtmann.org>
  */
 
+#include <linux/debugfs.h>
 #include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/usb.h>
@@ -574,6 +575,46 @@ static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
 	gpiod_set_value_cansleep(desc, 0);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static ssize_t btusb_debugfs_has_reset_gpio(struct file *file,
+					    char __user *user_buf,
+					    size_t count, loff_t *ppos)
+{
+	struct hci_dev *hdev = file->private_data;
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	char buf[3];
+
+	buf[0] = data->reset_gpio ? 'Y' : 'N';
+	buf[1] = '\n';
+	buf[2] = '\0';
+
+	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t btusb_debugfs_reset_gpio(struct file *file,
+					const char __user *user_buf,
+					size_t count, loff_t *ppos)
+{
+	struct hci_dev *hdev = file->private_data;
+	struct btusb_data *data = hci_get_drvdata(hdev);
+
+	if (!data->reset_gpio)
+		return -EOPNOTSUPP;
+
+	bt_dev_warn(hdev, "Debugfs triggering HW reset via gpio");
+	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
+
+	return count;
+}
+
+static const struct file_operations reset_gpio_fops = {
+	.open		= simple_open,
+	.read		= btusb_debugfs_has_reset_gpio,
+	.write		= btusb_debugfs_reset_gpio,
+	.llseek		= default_llseek,
+};
+#endif
+
 static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
@@ -4625,6 +4666,11 @@ static int btusb_probe(struct usb_interface *intf,
 	if (err < 0)
 		goto out_free_dev;
 
+#ifdef CONFIG_DEBUG_FS
+	debugfs_create_file("toggle_hw_reset", 0644, hdev->debugfs, hdev,
+			    &reset_gpio_fops);
+#endif
+
 	usb_set_intfdata(intf, data);
 
 	return 0;
-- 
2.30.0.284.gd98b1dd5eaa7-goog

Re: [PATCH 3/3] Bluetooth: btusb: Expose reset gpio to debugfs

[Marcel Holtmann]

Hi Abhishek,

> If btusb has a reset gpio, expose it to userspace so we can toggle the
> reset gpio directly. This is useful for testing and error recovery.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> drivers/bluetooth/btusb.c | 46 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 880e9cd4ee713..702be1871ed88 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -6,6 +6,7 @@
>  *  Copyright (C) 2005-2008  Marcel Holtmann <marcel@holtmann.org>
>  */
> 
> +#include <linux/debugfs.h>
> #include <linux/dmi.h>
> #include <linux/module.h>
> #include <linux/usb.h>
> @@ -574,6 +575,46 @@ static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
> 	gpiod_set_value_cansleep(desc, 0);
> }
> 
> +#ifdef CONFIG_DEBUG_FS
> +static ssize_t btusb_debugfs_has_reset_gpio(struct file *file,
> +					    char __user *user_buf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct hci_dev *hdev = file->private_data;
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	char buf[3];
> +
> +	buf[0] = data->reset_gpio ? 'Y' : 'N';
> +	buf[1] = '\n';
> +	buf[2] = '\0';
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> +}
> +
> +static ssize_t btusb_debugfs_reset_gpio(struct file *file,
> +					const char __user *user_buf,
> +					size_t count, loff_t *ppos)
> +{
> +	struct hci_dev *hdev = file->private_data;
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +
> +	if (!data->reset_gpio)
> +		return -EOPNOTSUPP;
> +
> +	bt_dev_warn(hdev, "Debugfs triggering HW reset via gpio");
> +	btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
> +
> +	return count;
> +}
> +
> +static const struct file_operations reset_gpio_fops = {
> +	.open		= simple_open,
> +	.read		= btusb_debugfs_has_reset_gpio,
> +	.write		= btusb_debugfs_reset_gpio,
> +	.llseek		= default_llseek,
> +};
> +#endif

while I am not convinced that this is the right way, I am fine doing it temporarily. However, please lets create the debugfs file only when a reset GPIO is available and skip the read callback. Only the write one should be supported since there is no point to read the status. The pure existence of the debugfs should indicate support for a HW reset.

Regards

Marcel