Hi Hsin-chen,
On Tue, Jan 7, 2025 at 2:21 AM Hsin-chen Chuang <chharry@xxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
> >
> > Hi Hsin-chen,
> >
> > On Thu, Jan 2, 2025 at 10:21 PM Hsin-chen Chuang <chharry@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > > 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
--
Luiz Augusto von Dentz
