Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-1923-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240214012328.BlueZ.1.I10eda6f375bc8dfedf4eef3a8cf6572c65803afc@changeid>
 <CABBYNZ+MJRSAWHjgNTNwcWCdq5y0zBU5f8XJvr1tv3LCqvdKJw@mail.gmail.com> <CADg1FFfgNHF9q+dSn=K9qjVMKx1zaBpG=NoeM_3Gr942Zj7_bA@mail.gmail.com>
In-Reply-To: <CADg1FFfgNHF9q+dSn=K9qjVMKx1zaBpG=NoeM_3Gr942Zj7_bA@mail.gmail.com>
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: Fri, 16 Feb 2024 10:54:08 -0500
Message-ID: <CABBYNZJXxYJKR3zxSwO8xuWTTgkSJjdGgZbJYqgJzmpPfwUcWA@mail.gmail.com>
Subject: Re: [BlueZ PATCH] Bluetooth: hci_core: Skip hci_cmd_work if
 hci_request is pending
To: Hsin-chen Chuang <chharry@google.com>
Cc: Hsin-chen Chuang <chharry@chromium.org>, linux-bluetooth@vger.kernel.org, 
	chromeos-bluetooth-upstreaming@chromium.org, 
	Johan Hedberg <johan.hedberg@gmail.com>, Marcel Holtmann <marcel@holtmann.org>, 
	linux-kernel@vger.kernel.org, Ying Hsu <yinghsu@google.com>, 
	Joe Antonetti <joeantonetti@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Hsin-chen,

On Thu, Feb 15, 2024 at 11:21=E2=80=AFPM Hsin-chen Chuang <chharry@google.c=
om> wrote:
>
> +Some Googlers who would be interested in
>
> Hi Luiz,
>
> How about moving the hci_req-related data out from sent_cmd? This allows =
sending HCI commands while hci_req data would not be overwritten.

I have something like the following in the works:

https://gist.github.com/Vudentz/251275bb688fac32585f90ac0076c407

It is not stable yet, but I think we can get away with it since it
just means we can keep the pending request stored in the req_skb, that
said we might need to overhaul this design since it is not very clean
in my opinion.

> On Fri, Feb 16, 2024 at 5:37=E2=80=AFAM Luiz Augusto von Dentz <luiz.dent=
z@gmail.com> wrote:
>>
>> Hi Hsin-chen,
>>
>> On Tue, Feb 13, 2024 at 12:24=E2=80=AFPM Hsin-chen Chuang <chharry@chrom=
ium.org> wrote:
>> >
>> > hci_cmd_work overwrites the hdev->sent_cmd which contains the required
>> > info for a hci_request to work. In the real world, it's observed that
>> > a request from hci_le_ext_create_conn_sync could be interrupted by
>> > the authentication (hci_conn_auth) caused by rfcomm_sock_connect. When
>> > it happends, hci_le_ext_create_conn_sync hangs until timeout; If the
>> > LE connection is triggered by MGMT, it freezes the whole MGMT interfac=
e.
>> >
>> > Signed-off-by: Hsin-chen Chuang <chharry@chromium.org>
>> > ---
>> >
>> >  net/bluetooth/hci_core.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> > index 34c8dca2069f..e3706889976d 100644
>> > --- a/net/bluetooth/hci_core.c
>> > +++ b/net/bluetooth/hci_core.c
>> > @@ -4213,8 +4213,11 @@ static void hci_cmd_work(struct work_struct *wo=
rk)
>> >         BT_DBG("%s cmd_cnt %d cmd queued %d", hdev->name,
>> >                atomic_read(&hdev->cmd_cnt), skb_queue_len(&hdev->cmd_q=
));
>> >
>> > -       /* Send queued commands */
>> > -       if (atomic_read(&hdev->cmd_cnt)) {
>> > +       /* Send queued commands. Don't send the command when there is =
a pending
>> > +        * hci_request because the request callbacks would be overwrit=
ten.
>> > +        */
>> > +       if (atomic_read(&hdev->cmd_cnt) &&
>> > +           !hci_dev_test_flag(hdev, HCI_CMD_PENDING)) {
>> >                 skb =3D skb_dequeue(&hdev->cmd_q);
>> >                 if (!skb)
>> >                         return;
>> > --
>> > 2.43.0.687.g38aa6559b0-goog
>>
>>
>> This seems to be causing some mgmt-tester failures:
>>
>> Pair Device - Sec Mode 3 Success 1                   Timed out   22.753 =
seconds
>> Pair Device - Sec Mode 3 Reject 1                    Timed out   22.533 =
seconds
>> Pair Device - Sec Mode 3 Reject 2                    Timed out   22.526 =
seconds
>>
>> I think this is because we need to respond to an event with a command li=
ke:
>>
>> < HCI Command: Create Conn.. (0x01|0x0005) plen 13  #241 [hci0] 16:25:38=
.699066
>>         Address: 00:AA:01:01:00:00 (Intel Corporation)
>>         Packet type: 0x0018
>>           DM1 may be used
>>           DH1 may be used
>>         Page scan repetition mode: R2 (0x02)
>>         Page scan mode: Mandatory (0x00)
>>         Clock offset: 0x0000
>>         Role switch: Allow peripheral (0x01)
>> > HCI Event: Command Status (0x0f) plen 4           #242 [hci0] 16:25:38=
.701881
>>       Create Connection (0x01|0x0005) ncmd 1
>>         Status: Success (0x00)
>> > HCI Event: Link Key Request (0x17) plen 6         #243 [hci0] 16:25:38=
.702375
>>         Address: 00:AA:01:01:00:00 (Intel Corporation)
>>
>> But because Create Connection is pending we cannot respond to Link Key
>> Request, so it is actually a design problem if we cannot send commands
>> because something is pending so perhaps we need to redesign how we
>> store cmd_sent so we can have multiple outstanding commands rather
>> than just one.
>>
>> --
>> Luiz Augusto von Dentz
>
>
>
> --
> Best Regards,
> Hsin-chen


--=20
Luiz Augusto von Dentz