Bluetooth: Allow reset via sysfs

Allow sysfs to trigger reset via the cmd_timeout function in hci device.
This is required to recover devices that are not responsive from
userspace.

Also remove the cmd timeout count in btusb since we only ever allow one
command in flight at a time. We should always reset after a single
command times out.

Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
---
This commit has been tested on a Chromebook by running
`echo 1 > /sys/class/bluetooth/hci0/reset`

 drivers/bluetooth/btusb.c | 10 ----------
 net/bluetooth/hci_sysfs.c | 19 +++++++++++++++++++
 2 files changed, 19 insertions(+), 10 deletions(-)

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=921922

---Test result---

Test Summary:
CheckPatch                    PENDING   0.26 seconds
GitLint                       PENDING   0.21 seconds
SubjectPrefix                 PASS      0.70 seconds
BuildKernel                   PASS      24.84 seconds
CheckAllWarning               PASS      27.05 seconds
CheckSparse                   PASS      30.62 seconds
BuildKernel32                 PASS      24.57 seconds
TestRunnerSetup               PASS      443.19 seconds
TestRunner_l2cap-tester       PASS      22.93 seconds
TestRunner_iso-tester         PASS      32.42 seconds
TestRunner_bnep-tester        PASS      4.97 seconds
TestRunner_mgmt-tester        PASS      118.65 seconds
TestRunner_rfcomm-tester      PASS      8.41 seconds
TestRunner_sco-tester         PASS      9.47 seconds
TestRunner_ioctl-tester       PASS      8.23 seconds
TestRunner_mesh-tester        FAIL      8.27 seconds
TestRunner_smp-tester         PASS      7.13 seconds
TestRunner_userchan-tester    PASS      5.10 seconds
IncrementalBuild              PENDING   0.61 seconds

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

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

##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
BUG: KASAN: slab-use-after-free in run_timer_softirq+0x76c/0x7d0
WARNING: CPU: 0 PID: 64 at kernel/workqueue.c:2257 __queue_work+0x687/0xb40
Total: 10, Passed: 9 (90.0%), Failed: 1, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Failed       0.110 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Hi Hsin-chen,

On Thu, Jan 2, 2025 at 10:21 PM Hsin-chen Chuang <chharry@chromium.org> wrote:
>
> Allow sysfs to trigger reset via the cmd_timeout function in hci device.
> This is required to recover devices that are not responsive from
> userspace.

Don't we have a similar control over USB to reset the device? I think
that would be better than introducing something btusb specific.

> Also remove the cmd timeout count in btusb since we only ever allow one
> command in flight at a time. We should always reset after a single
> command times out.
>
> Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> ---
> This commit has been tested on a Chromebook by running
> `echo 1 > /sys/class/bluetooth/hci0/reset`
>
>  drivers/bluetooth/btusb.c | 10 ----------
>  net/bluetooth/hci_sysfs.c | 19 +++++++++++++++++++
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 279fe6c115fac..a4810c77fa0da 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -879,7 +879,6 @@ struct btusb_data {
>         int (*disconnect)(struct hci_dev *hdev);
>
>         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> -       unsigned cmd_timeout_cnt;
>
>         struct qca_dump_info qca_dump;
>  };
> @@ -912,9 +911,6 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
>         struct gpio_desc *reset_gpio = data->reset_gpio;
>         struct btintel_data *intel_data = hci_get_priv(hdev);
>
> -       if (++data->cmd_timeout_cnt < 5)
> -               return;
> -
>         if (intel_data->acpi_reset_method) {
>                 if (test_and_set_bit(INTEL_ACPI_RESET_ACTIVE, intel_data->flags)) {
>                         bt_dev_err(hdev, "acpi: last reset failed ? Not resetting again");
> @@ -997,9 +993,6 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
>
>         btusb_rtl_alloc_devcoredump(hdev, &hdr, NULL, 0);
>
> -       if (++data->cmd_timeout_cnt < 5)
> -               return;
> -
>         if (!reset_gpio) {
>                 btusb_reset(hdev);
>                 return;
> @@ -1044,9 +1037,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
>                 return;
>         }
>
> -       if (++data->cmd_timeout_cnt < 5)
> -               return;
> -
>         if (reset_gpio) {
>                 bt_dev_err(hdev, "Reset qca device via bt_en gpio");
>
> diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> index 4b54dbbf0729a..7bf2b10b0a7cf 100644
> --- a/net/bluetooth/hci_sysfs.c
> +++ b/net/bluetooth/hci_sysfs.c
> @@ -90,9 +90,28 @@ static void bt_host_release(struct device *dev)
>         module_put(THIS_MODULE);
>  }
>
> +static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> +                          const char *buf, size_t count)
> +{
> +       struct hci_dev *hdev = to_hci_dev(dev);
> +
> +       if (hdev->cmd_timeout)
> +               hdev->cmd_timeout(hdev);
> +
> +       return count;
> +}
> +static DEVICE_ATTR_WO(reset);
> +
> +static struct attribute *bt_host_attrs[] = {
> +       &dev_attr_reset.attr,
> +       NULL,
> +};
> +ATTRIBUTE_GROUPS(bt_host);
> +
>  static const struct device_type bt_host = {
>         .name    = "host",
>         .release = bt_host_release,
> +       .groups = bt_host_groups,
>  };
>
>  void hci_init_sysfs(struct hci_dev *hdev)
> --
> 2.47.1.613.gc27f4b7a9f-goog
>

Hi Luiz,

This is not btusb specific, and is not limited to USB reset.

The cmd_timeout handler can also be found in the QCA uart and the MTK
sdio controller drivers. It's up to the vendors to implement a reliable
reset mechanism, and we welcome - or prefer - an OOB solution such as
reset over GPIO.

This patch involves some btusb changes because the handlers in btusb
can only do something after being called 5 times, while the other
handlers don't.



On Tue, Jan 7, 2025 at 12:29 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Hsin-chen,
>
> On Thu, Jan 2, 2025 at 10:21 PM Hsin-chen Chuang <chharry@chromium.org> wrote:
> >
> > Allow sysfs to trigger reset via the cmd_timeout function in hci device.
> > This is required to recover devices that are not responsive from
> > userspace.
>
> Don't we have a similar control over USB to reset the device? I think
> that would be better than introducing something btusb specific.
>
> > Also remove the cmd timeout count in btusb since we only ever allow one
> > command in flight at a time. We should always reset after a single
> > command times out.
> >
> > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > ---
> > This commit has been tested on a Chromebook by running
> > `echo 1 > /sys/class/bluetooth/hci0/reset`
> >
> >  drivers/bluetooth/btusb.c | 10 ----------
> >  net/bluetooth/hci_sysfs.c | 19 +++++++++++++++++++
> >  2 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 279fe6c115fac..a4810c77fa0da 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -879,7 +879,6 @@ struct btusb_data {
> >         int (*disconnect)(struct hci_dev *hdev);
> >
> >         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> > -       unsigned cmd_timeout_cnt;
> >
> >         struct qca_dump_info qca_dump;
> >  };
> > @@ -912,9 +911,6 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> >         struct gpio_desc *reset_gpio = data->reset_gpio;
> >         struct btintel_data *intel_data = hci_get_priv(hdev);
> >
> > -       if (++data->cmd_timeout_cnt < 5)
> > -               return;
> > -
> >         if (intel_data->acpi_reset_method) {
> >                 if (test_and_set_bit(INTEL_ACPI_RESET_ACTIVE, intel_data->flags)) {
> >                         bt_dev_err(hdev, "acpi: last reset failed ? Not resetting again");
> > @@ -997,9 +993,6 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
> >
> >         btusb_rtl_alloc_devcoredump(hdev, &hdr, NULL, 0);
> >
> > -       if (++data->cmd_timeout_cnt < 5)
> > -               return;
> > -
> >         if (!reset_gpio) {
> >                 btusb_reset(hdev);
> >                 return;
> > @@ -1044,9 +1037,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
> >                 return;
> >         }
> >
> > -       if (++data->cmd_timeout_cnt < 5)
> > -               return;
> > -
> >         if (reset_gpio) {
> >                 bt_dev_err(hdev, "Reset qca device via bt_en gpio");
> >
> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > index 4b54dbbf0729a..7bf2b10b0a7cf 100644
> > --- a/net/bluetooth/hci_sysfs.c
> > +++ b/net/bluetooth/hci_sysfs.c
> > @@ -90,9 +90,28 @@ static void bt_host_release(struct device *dev)
> >         module_put(THIS_MODULE);
> >  }
> >
> > +static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > +                          const char *buf, size_t count)
> > +{
> > +       struct hci_dev *hdev = to_hci_dev(dev);
> > +
> > +       if (hdev->cmd_timeout)
> > +               hdev->cmd_timeout(hdev);
> > +
> > +       return count;
> > +}
> > +static DEVICE_ATTR_WO(reset);
> > +
> > +static struct attribute *bt_host_attrs[] = {
> > +       &dev_attr_reset.attr,
> > +       NULL,
> > +};
> > +ATTRIBUTE_GROUPS(bt_host);
> > +
> >  static const struct device_type bt_host = {
> >         .name    = "host",
> >         .release = bt_host_release,
> > +       .groups = bt_host_groups,
> >  };
> >
> >  void hci_init_sysfs(struct hci_dev *hdev)
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Hi Hsin-chen,

On Tue, Jan 7, 2025 at 2:21 AM Hsin-chen Chuang <chharry@chromium.org> wrote:
>
> Hi Luiz,
>
> This is not btusb specific, and is not limited to USB reset.
>
> The cmd_timeout handler can also be found in the QCA uart and the MTK
> sdio controller drivers. It's up to the vendors to implement a reliable
> reset mechanism, and we welcome - or prefer - an OOB solution such as
> reset over GPIO.

My bad, I didn't realize it was under hci_sysfs, which makes sense,
that said I do wonder if it wouldn't be better to name the callback as
reset rather than cmd_timeout and we should probably move them from
btusb if there are not usb specific, in fact there is already a reset
callback but it doesn't seem to be used which I think it is a mistake
and we should actually make the vendor reset the callback o
hdev->reset not have hdev->cmd_timeout -> vendor_cmd_timeout ->
btusb_reset -> hdev->reset but rather btusb_reset -> hdev->reset ->
vendor_reset then we can get rid of cmd_timeout.

> This patch involves some btusb changes because the handlers in btusb
> can only do something after being called 5 times, while the other
> handlers don't.

Ok, then please have these changes splitted since they have different purposes.

>
>
> On Tue, Jan 7, 2025 at 12:29 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Thu, Jan 2, 2025 at 10:21 PM Hsin-chen Chuang <chharry@chromium.org> wrote:
> > >
> > > Allow sysfs to trigger reset via the cmd_timeout function in hci device.
> > > This is required to recover devices that are not responsive from
> > > userspace.
> >
> > Don't we have a similar control over USB to reset the device? I think
> > that would be better than introducing something btusb specific.
> >
> > > Also remove the cmd timeout count in btusb since we only ever allow one
> > > command in flight at a time. We should always reset after a single
> > > command times out.
> > >
> > > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
> > > ---
> > > This commit has been tested on a Chromebook by running
> > > `echo 1 > /sys/class/bluetooth/hci0/reset`
> > >
> > >  drivers/bluetooth/btusb.c | 10 ----------
> > >  net/bluetooth/hci_sysfs.c | 19 +++++++++++++++++++
> > >  2 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 279fe6c115fac..a4810c77fa0da 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -879,7 +879,6 @@ struct btusb_data {
> > >         int (*disconnect)(struct hci_dev *hdev);
> > >
> > >         int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> > > -       unsigned cmd_timeout_cnt;
> > >
> > >         struct qca_dump_info qca_dump;
> > >  };
> > > @@ -912,9 +911,6 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
> > >         struct gpio_desc *reset_gpio = data->reset_gpio;
> > >         struct btintel_data *intel_data = hci_get_priv(hdev);
> > >
> > > -       if (++data->cmd_timeout_cnt < 5)
> > > -               return;
> > > -
> > >         if (intel_data->acpi_reset_method) {
> > >                 if (test_and_set_bit(INTEL_ACPI_RESET_ACTIVE, intel_data->flags)) {
> > >                         bt_dev_err(hdev, "acpi: last reset failed ? Not resetting again");
> > > @@ -997,9 +993,6 @@ static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
> > >
> > >         btusb_rtl_alloc_devcoredump(hdev, &hdr, NULL, 0);
> > >
> > > -       if (++data->cmd_timeout_cnt < 5)
> > > -               return;
> > > -
> > >         if (!reset_gpio) {
> > >                 btusb_reset(hdev);
> > >                 return;
> > > @@ -1044,9 +1037,6 @@ static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
> > >                 return;
> > >         }
> > >
> > > -       if (++data->cmd_timeout_cnt < 5)
> > > -               return;
> > > -
> > >         if (reset_gpio) {
> > >                 bt_dev_err(hdev, "Reset qca device via bt_en gpio");
> > >
> > > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
> > > index 4b54dbbf0729a..7bf2b10b0a7cf 100644
> > > --- a/net/bluetooth/hci_sysfs.c
> > > +++ b/net/bluetooth/hci_sysfs.c
> > > @@ -90,9 +90,28 @@ static void bt_host_release(struct device *dev)
> > >         module_put(THIS_MODULE);
> > >  }
> > >
> > > +static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > > +                          const char *buf, size_t count)
> > > +{
> > > +       struct hci_dev *hdev = to_hci_dev(dev);
> > > +
> > > +       if (hdev->cmd_timeout)
> > > +               hdev->cmd_timeout(hdev);
> > > +
> > > +       return count;
> > > +}
> > > +static DEVICE_ATTR_WO(reset);
> > > +
> > > +static struct attribute *bt_host_attrs[] = {
> > > +       &dev_attr_reset.attr,
> > > +       NULL,
> > > +};
> > > +ATTRIBUTE_GROUPS(bt_host);
> > > +
> > >  static const struct device_type bt_host = {
> > >         .name    = "host",
> > >         .release = bt_host_release,
> > > +       .groups = bt_host_groups,
> > >  };
> > >
> > >  void hci_init_sysfs(struct hci_dev *hdev)
> > > --
> > > 2.47.1.613.gc27f4b7a9f-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> --
> Best Regards,
> Hsin-chen