On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <mar...@holtmann.org> 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 <raja...@google.com>
> > ---
> > 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
>
