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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D1490A23-9076-4BAA-A932-CDE1C073475B@holtmann.org>
Date:   Sat, 19 Jan 2019 20:51:06 +0100
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Rajat Jain <rajatja@...gle.com>
Cc:     Johan Hedberg <johan.hedberg@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Dmitry Torokhov <dtor@...omium.org>,
        Alex Hung <alex.hung@...onical.com>,
        linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org, netdev@...r.kernel.org,
        rajatxjain@...il.com, dtor@...gle.com, raghuram.hegde@...el.com,
        chethan.tumkur.narayan@...el.com, sukumar.ghorai@...el.com
Subject: Re: [PATCH v4 3/5] Bluetooth: Reset Bluetooth chip after multiple
 command timeouts

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