[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 8 Aug 2022 13:31:41 -0700
From: Luiz Augusto von Dentz <luiz.dentz@...il.com>
To: Abhishek Pandit-Subedi <abhishekpandit@...gle.com>
Cc: "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Marcel Holtmann <marcel@...tmann.org>,
Paolo Abeni <pabeni@...hat.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH] Bluetooth: Ignore cmd_timeout with HCI_USER_CHANNEL
Hi Abhishek,
On Mon, Aug 8, 2022 at 11:04 AM Abhishek Pandit-Subedi
<abhishekpandit@...gle.com> wrote:
>
> From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
>
> When using HCI_USER_CHANNEL, hci traffic is expected to be handled by
> userspace entirely. However, it is still possible (and sometimes
> desirable) to be able to send commands to the controller directly. In
> such cases, the kernel command timeout shouldn't do any error handling.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> ---
> This was tested by running a userchannel stack and sending commands via
> hcitool cmd on an Intel AX200 controller. Without this change, each
> command sent via hcitool would result in hci_cmd_timeout being called
> and after 5 commands, the controller would reset.
>
> Hcitool continues working here because it marks the socket as
> promiscuous and gets a copy of all traffic while the socket is open (and
> does filtering in userspace).
There is something not quite right here, if you have a controller
using user_channel (addr.hci_channel = HCI_CHANNEL_USER) it probably
shouldn't even accept to be opened again by the likes of hcitool which
uses HCI_CHANNEL_RAW as it can cause conflicts. If you really need a
test tool that does send the command while in HCI_CHANNEL_USER then it
must be send on that mode but I wouldn't do it with hcitool anyway as
that is deprecated and this exercise seem to revolve to a entire stack
on top of HCI_USER_CHANNEL then you shall use tools of that stack and
mix with BlueZ userspace tools.
> Tested on Chromebook with 5.4 kernel with patch (and applied cleanly on
> bluetooth-next).
>
> net/bluetooth/hci_core.c | 26 +++++++++++++++++---------
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b3a5a3cc9372..c9a15f6633f7 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1481,17 +1481,25 @@ static void hci_cmd_timeout(struct work_struct *work)
> struct hci_dev *hdev = container_of(work, struct hci_dev,
> cmd_timer.work);
>
> - if (hdev->sent_cmd) {
> - struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
> - u16 opcode = __le16_to_cpu(sent->opcode);
> + /* Don't trigger the timeout behavior if it happens while we're in
> + * userchannel mode. Userspace is responsible for handling any command
> + * timeouts.
> + */
> + if (!(hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> + test_bit(HCI_UP, &hdev->flags))) {
> + 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);
> - } else {
> - bt_dev_err(hdev, "command tx timeout");
> - }
> + bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
> + } else {
> + bt_dev_err(hdev, "command tx timeout");
> + }
>
> - if (hdev->cmd_timeout)
> - hdev->cmd_timeout(hdev);
> + if (hdev->cmd_timeout)
> + hdev->cmd_timeout(hdev);
> + }
I wonder why hci_cmd_timeout is even active if the controller is in
HCI_USER_CHANNEL mode, that sounds like a bug already.
> atomic_set(&hdev->cmd_cnt, 1);
> queue_work(hdev->workqueue, &hdev->cmd_work);
> --
> 2.37.1.559.g78731f0fdb-goog
>
--
Luiz Augusto von Dentz
Powered by blists - more mailing lists