Re: [PATCH v5 4/4] Bluetooth: btusb: Use the cmd_timeout method to reset the Intel BT chip

[Rajat Jain]

On Thu, Jan 24, 2019 at 1:46 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Rajat,
>
> > If the platform provides it, use the reset gpio to reset the Intel BT
> > chip, as part of cmd_timeout handling. This has been found helpful on
> > Intel bluetooth controllers where the firmware gets stuck and the only
> > way out is a hard reset pin provided by the platform.
> >
> > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> > ---
> > v5: Rename the hook to cmd_timeout, and wait for 5 timeouts before
> >    resetting the device.
> > v4: Use data->flags instead of clearing the quirk in btusb_hw_reset()
> > v3: Better error handling for gpiod_get_optional()
> > v2: Handle the EPROBE_DEFER case.
> >
> > drivers/bluetooth/btusb.c | 54 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 54 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 4761499db9ee..8949ea30a1bc 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -29,6 +29,7 @@
> > #include <linux/of_device.h>
> > #include <linux/of_irq.h>
> > #include <linux/suspend.h>
> > +#include <linux/gpio/consumer.h>
> > #include <asm/unaligned.h>
> >
> > #include <net/bluetooth/bluetooth.h>
> > @@ -439,6 +440,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
> > #define BTUSB_BOOTING         9
> > #define BTUSB_DIAG_RUNNING    10
> > #define BTUSB_OOB_WAKE_ENABLED        11
> > +#define BTUSB_HW_RESET_ACTIVE        12
> >
> > struct btusb_data {
> >       struct hci_dev       *hdev;
> > @@ -476,6 +478,8 @@ struct btusb_data {
> >       struct usb_endpoint_descriptor *diag_tx_ep;
> >       struct usb_endpoint_descriptor *diag_rx_ep;
> >
> > +     struct gpio_desc *reset_gpio;
> > +
> >       __u8 cmdreq_type;
> >       __u8 cmdreq;
> >
> > @@ -489,8 +493,39 @@ struct btusb_data {
> >       int (*setup_on_usb)(struct hci_dev *hdev);
> >
> >       int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
> > +     unsigned num_cmd_timeout;
>
> Make this cmd_timeout_count or cmd_timeout_cnt.

Sure, will do.

>
> > };
> >
> > +
> > +static void btusb_intel_cmd_timeout(struct hci_dev *hdev,
> > +                                 struct hci_command_hdr *cmd)
> > +{
> > +     struct btusb_data *data = hci_get_drvdata(hdev);
> > +     struct gpio_desc *reset_gpio = data->reset_gpio;
> > +
> > +     bt_dev_err(hdev, "btusb num_cmd_timeout = %u", ++data->num_cmd_timeout);
>
> I would prefer if you donât do the increase in the bt_dev_err(). And you can also scrap the âbtusb â prefix here since that is all present via bt_dev_err if really needed at some point. Actually I question the whole bt_dev_err here in the first place since the core will put our the error. You are just repeating it here.

will do.

>
> > +
> > +     if (data->num_cmd_timeout < 5)
> > +             return;
>
> So I would propose to do just this:
>
>         if (++data->cmd_timeout_count < 5)
>                 return;

will do.


>
> > +
> > +     /*
> > +      * Toggle the hard reset line if the platform provides one. The reset
> > +      * is going to yank the device off the USB and then replug. So doing
> > +      * once is enough. 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, "Initiating HW reset via gpio");
> > +     gpiod_set_value(reset_gpio, 1);
> > +     mdelay(100);
> > +     gpiod_set_value(reset_gpio, 0);
> > +}
> > +
> > static inline void btusb_free_frags(struct btusb_data *data)
> > {
> >       unsigned long flags;
> > @@ -2915,6 +2950,7 @@ static int btusb_probe(struct usb_interface *intf,
> >                      const struct usb_device_id *id)
> > {
> >       struct usb_endpoint_descriptor *ep_desc;
> > +     struct gpio_desc *reset_gpio;
> >       struct btusb_data *data;
> >       struct hci_dev *hdev;
> >       unsigned ifnum_base;
> > @@ -3028,6 +3064,15 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >       SET_HCIDEV_DEV(hdev, &intf->dev);
> >
> > +     reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
> > +                                     GPIOD_OUT_LOW);
>
> Any reason to not use the devm_gpiod_get_optional() version here?

Yes, we're using the &data->udev->dev device (parent of the USB
interface) to fetch the gpio, so if we use the devm_* variant, the
gpio resource will not be freed immediately if the btusb_probe fails.
I'll add a comment in the code to make this more clear.

>
> > +     if (IS_ERR(reset_gpio)) {
> > +             err = PTR_ERR(reset_gpio);
> > +             goto out_free_dev;
> > +     } else if (reset_gpio) {
> > +             data->reset_gpio = reset_gpio;
> > +     }
> > +
> >       hdev->open   = btusb_open;
> >       hdev->close  = btusb_close;
> >       hdev->flush  = btusb_flush;
> > @@ -3085,6 +3130,8 @@ static int btusb_probe(struct usb_interface *intf,
> >               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);
> > +             if (data->reset_gpio)
> > +                     hdev->cmd_timeout = btusb_intel_cmd_timeout;
> >       }
>
> Always assign hdev->cmd_timeout = btusb_intel_cmd_timeout and put it after btintel_set_bdaddr and before the quirks.

will do.

>
> Move the if (data->reset_gpio) check into btusb_intel_cmd_timeout() function and print a warning that no reset GPIO is present.

There'll be Intel platforms already out there which will obviously not
have the reset pin described in the ACPI as of today. So I'm not sure
if it is right for all of them to start complaining about a missing
"reset gpio" in case of timeout post this change. Thus I would prefer
to assign hdev->cmd_timeout only if ACPI does provide a gpio i.e.
newer platforms that want to support this feature. Thoughts?

Thanks,

Rajat

>
> >
> >       if (id->driver_info & BTUSB_INTEL_NEW) {
> > @@ -3097,6 +3144,8 @@ static int btusb_probe(struct usb_interface *intf,
> >               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);
> > +             if (data->reset_gpio)
> > +                     hdev->cmd_timeout = btusb_intel_cmd_timeout;
> >       }
> >
> >       if (id->driver_info & BTUSB_MARVELL)
> > @@ -3226,6 +3275,8 @@ static int btusb_probe(struct usb_interface *intf,
> >       return 0;
> >
> > out_free_dev:
> > +     if (data->reset_gpio)
> > +             gpiod_put(data->reset_gpio);
> >       hci_free_dev(hdev);
> >       return err;
> > }
> > @@ -3269,6 +3320,9 @@ static void btusb_disconnect(struct usb_interface *intf)
> >       if (data->oob_wake_irq)
> >               device_init_wakeup(&data->udev->dev, false);
> >
> > +     if (data->reset_gpio)
> > +             gpiod_put(data->reset_gpio);
> > +
> >       hci_free_dev(hdev);
> > }
>
> Regards
>
> Marcel
>