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.
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
