| Message ID | 20190118223407.64818-3-rajatja@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/5] usb: split code locating ACPI companion into port and device | expand |
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@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. 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. Regards Marcel
On Sat, Jan 19, 2019 at 11:51 AM Marcel Holtmann <marcel@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 <rajatja@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 >
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; } atomic_set(&hdev->cmd_cnt, 1);
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@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(-)