Re: [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts

[Rajat Jain]

On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Rajat,
>
> > Add a quirk and a hook to allow the HCI core to reset the BT chip
> > if needed (after a number of timed out commands). Use that new hook to
> > initiate BT chip reset if the controller fails to respond to certain
> > number of commands (currently 5) including the HCI reset commands.
> > This is done based on a newly introduced quirk. This is done based
> > on some initial work by Intel.
> >
> > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx>
> > ---
> > v4: same as v1
> > v3: same as v1
> > v2: same as v1
> >
> > include/net/bluetooth/hci.h      |  8 ++++++++
> > include/net/bluetooth/hci_core.h |  2 ++
> > net/bluetooth/hci_core.c         | 15 +++++++++++++--
> > 3 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index c36dc1e20556..af02fa5ffe54 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -192,6 +192,14 @@ enum {
> >        *
> >        */
> >       HCI_QUIRK_NON_PERSISTENT_SETUP,
> > +
> > +     /* When this quirk is set, hw_reset() would be run to reset the
> > +      * hardware, after a certain number of commands (currently 5)
> > +      * time out because the device fails to respond.
> > +      *
> > +      * This quirk should be set before hci_register_dev is called.
> > +      */
> > +     HCI_QUIRK_HW_RESET_ON_TIMEOUT,
> > };
> >
> > /* HCI device flags */
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index e5ea633ea368..b86218304b80 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -313,6 +313,7 @@ struct hci_dev {
> >       unsigned int    acl_cnt;
> >       unsigned int    sco_cnt;
> >       unsigned int    le_cnt;
> > +     unsigned int    timeout_cnt;
> >
> >       unsigned int    acl_mtu;
> >       unsigned int    sco_mtu;
> > @@ -437,6 +438,7 @@ struct hci_dev {
> >       int (*post_init)(struct hci_dev *hdev);
> >       int (*set_diag)(struct hci_dev *hdev, bool enable);
> >       int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> > +     void (*hw_reset)(struct hci_dev *hdev);
> > };
> >
> > #define HCI_PHY_HANDLE(handle)        (handle & 0xff)
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 7352fe85674b..ab3a6a8b7ba6 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
> >       struct hci_dev *hdev = container_of(work, struct hci_dev,
> >                                           cmd_timer.work);
> >
> > +     hdev->timeout_cnt++;
> >       if (hdev->sent_cmd) {
> >               struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> >               u16 opcode = __le16_to_cpu(sent->opcode);
> >
> > -             bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> > +             bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
> > +                        opcode, hdev->timeout_cnt);
> >       } else {
> > -             bt_dev_err(hdev, "command tx timeout");
> > +             bt_dev_err(hdev, "command tx timeout (cnt = %u)",
> > +                        hdev->timeout_cnt);
> > +     }
> > +
> > +     if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
> > +         hdev->timeout_cnt >= 5) {
> > +             hdev->timeout_cnt = 0;
> > +             if (hdev->hw_reset)
> > +                     hdev->hw_reset(hdev);
> > +             return;
> >       }
>
> so I really do not see the need for the quirk here. Either hdev->hw_reset is provided, then execute it, if it is not provided then donât. The quirk is just duplicate information.

Sure, will do.

>
> I also donât like hdev->hw_reset since that implies that the only way of handling a command timeout is a hardware reset. I prefer you call this hdev->cmd_timeout and also scrap the timeout_cnt. Let the driver decide what number of timeouts it wants to react on. The number 5 is just an arbitrary number you picked based on one hardware manufacturer.

Sure, I can move the timeout_cnt from hdev to btusb_data so that the
btusb can track the number of the timeouts..

Thanks,

Rajat

>
> Regards
>
> Marcel
>