Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220808110315.1.I5a39052e33f6f3c7406f53b0304a32ccf9f340fa@changeid>
In-Reply-To: <20220808110315.1.I5a39052e33f6f3c7406f53b0304a32ccf9f340fa@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 8 Aug 2022 13:31:41 -0700
Message-ID: <CABBYNZJf_6SmRD0tUUUfxfpZOksL-=5jLN8+5c4cZcQB6J-xbg@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: Ignore cmd_timeout with HCI_USER_CHANNEL
To:     Abhishek Pandit-Subedi <abhishekpandit@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Abhishek Pandit-Subedi <abhishekpandit@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Marcel Holtmann <marcel@holtmann.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Abhishek,

On Mon, Aug 8, 2022 at 11:04 AM Abhishek Pandit-Subedi
<abhishekpandit@google.com> wrote:
>
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.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@chromium.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