From: "João Paulo Rechi Vita" <jprvita@gmail.com>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: "Johan Hedberg" <johan.hedberg@gmail.com>,
"Balakrishna Godavarthi" <bgodavar@codeaurora.org>,
ytkim@qca.qualcomm.com, "David S. Miller" <davem@davemloft.net>,
linux-bluetooth@vger.kernel.org,
"Network Development" <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
"Linux Upstreaming Team" <linux@endlessm.com>,
"João Paulo Rechi Vita" <jprvita@endlessm.com>
Subject: Re: [PATCH] Bluetooth: Ignore CC events not matching the last HCI command
Date: Thu, 25 Apr 2019 14:37:25 +0800 [thread overview]
Message-ID: <CA+A7VXVa9pFizfQ8C8QUS=3=YDjojmScOKPS=Yhcx-w8G=wsVw@mail.gmail.com> (raw)
In-Reply-To: <547E8A62-51BF-4545-931E-40734134B27E@holtmann.org>
On Wed, Apr 24, 2019 at 2:45 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Joao Paulo,
>
> > This commit makes the kernel not send the next queued HCI command until
> > a command complete arrives for the last HCI command sent to the
> > controller. This change avoids a problem with some buggy controllers
> > (seen on two SKUs of QCA9377) that send an extra command complete event
> > for the previous command after the kernel had already sent a new HCI
> > command to the controller.
> >
> > The problem was reproduced when starting an active scanning procedure,
> > where an extra command complete event arrives for the LE_SET_RANDOM_ADDR
> > command. When this happends the kernel ends up not processing the
> > command complete for the following commmand, LE_SET_SCAN_PARAM, and
> > ultimately behaving as if a passive scanning procedure was being
> > performed, when in fact controller is performing an active scanning
> > procedure. This makes it impossible to discover BLE devices as no device
> > found events are sent to userspace.
> >
> > This problem is reproducible on 100% of the attempts on the affected
> > controllers. The extra command complete event can be seen at timestamp
> > 27.420131 on the btmon logs bellow.
> >
> > Bluetooth monitor ver 5.50
> > = Note: Linux version 5.0.0+ (x86_64) 0.352340
> > = Note: Bluetooth subsystem version 2.22 0.352343
> > = New Index: 80:C5:F2:8F:87:84 (Primary,USB,hci0) [hci0] 0.352344
> > = Open Index: 80:C5:F2:8F:87:84 [hci0] 0.352345
> > = Index Info: 80:C5:F2:8F:87:84 (Qualcomm) [hci0] 0.352346
> > @ MGMT Open: bluetoothd (privileged) version 1.14 {0x0001} 0.352347
> > @ MGMT Open: btmon (privileged) version 1.14 {0x0002} 0.352366
> > @ MGMT Open: btmgmt (privileged) version 1.14 {0x0003} 27.302164
> > @ MGMT Command: Start Discovery (0x0023) plen 1 {0x0003} [hci0] 27.302310
> > Address type: 0x06
> > LE Public
> > LE Random
> > < HCI Command: LE Set Random Address (0x08|0x0005) plen 6 #1 [hci0] 27.302496
> > Address: 15:60:F2:91:B2:24 (Non-Resolvable)
> >> HCI Event: Command Complete (0x0e) plen 4 #2 [hci0] 27.419117
> > LE Set Random Address (0x08|0x0005) ncmd 1
> > Status: Success (0x00)
> > < HCI Command: LE Set Scan Parameters (0x08|0x000b) plen 7 #3 [hci0] 27.419244
> > Type: Active (0x01)
> > Interval: 11.250 msec (0x0012)
> > Window: 11.250 msec (0x0012)
> > Own address type: Random (0x01)
> > Filter policy: Accept all advertisement (0x00)
> >> HCI Event: Command Complete (0x0e) plen 4 #4 [hci0] 27.420131
> > LE Set Random Address (0x08|0x0005) ncmd 1
> > Status: Success (0x00)
> > < HCI Command: LE Set Scan Enable (0x08|0x000c) plen 2 #5 [hci0] 27.420259
> > Scanning: Enabled (0x01)
> > Filter duplicates: Enabled (0x01)
> >> HCI Event: Command Complete (0x0e) plen 4 #6 [hci0] 27.420969
> > LE Set Scan Parameters (0x08|0x000b) ncmd 1
> > Status: Success (0x00)
> >> HCI Event: Command Complete (0x0e) plen 4 #7 [hci0] 27.421983
> > LE Set Scan Enable (0x08|0x000c) ncmd 1
> > Status: Success (0x00)
> > @ MGMT Event: Command Complete (0x0001) plen 4 {0x0003} [hci0] 27.422059
> > Start Discovery (0x0023) plen 1
> > Status: Success (0x00)
> > Address type: 0x06
> > LE Public
> > LE Random
> > @ MGMT Event: Discovering (0x0013) plen 2 {0x0003} [hci0] 27.422067
> > Address type: 0x06
> > LE Public
> > LE Random
> > Discovery: Enabled (0x01)
> > @ MGMT Event: Discovering (0x0013) plen 2 {0x0002} [hci0] 27.422067
> > Address type: 0x06
> > LE Public
> > LE Random
> > Discovery: Enabled (0x01)
> > @ MGMT Event: Discovering (0x0013) plen 2 {0x0001} [hci0] 27.422067
> > Address type: 0x06
> > LE Public
> > LE Random
> > Discovery: Enabled (0x01)
> >
> > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com>
> > ---
> > net/bluetooth/hci_event.c | 13 +++++++++++++
> > 1 file changed, 13 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 609fd6871c5a..b349fc2b4123 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3071,6 +3071,19 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> > *opcode = __le16_to_cpu(ev->opcode);
> > *status = skb->data[sizeof(*ev)];
> >
> > + if (*opcode != HCI_OP_RESET && !hci_sent_cmd_data(hdev, *opcode)) {
> > + /* Some buggy controllers generate an extra command complete
> > + * event for the previous command after a new commend was
> > + * already sent, so if the opcode does not match the last sent
> > + * command we should ignore this event.
> > + *
> > + * HCI_OP_RESET is a special case handled in
> > + * hci_req_cmd_complete.
> > + */
> > + bt_dev_err(hdev, "unexpected CC event opcode 0x%4.4x", *opcode);
> > + return;
> > + }
> > +
>
> the return; here is not going to fly. You need to go through the switch statement since it actually does bookkeeping for a lot of features even if the command is not executed by hci_req and instead comes from user space or someone else.
>
Ok, so you want the same approach I had before but not bound to a
quirk. Sorry, when you previously said "ignore this command complete
and not start ahead with the next command" I thought you meant to also
avoid going through that switch statement.
> What is the problem with handling this in hci_req_cmd_complete. We have hci_sent_cmd_data check there already. Lets extend this to ensure that command complete for random commands don’t trigger us to continue with the request.
>
I'll send an updated revision soon.
--
João Paulo Rechi Vita
http://about.me/jprvita
next prev parent reply other threads:[~2019-04-25 6:38 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-23 7:22 [PATCH 0/2] Quirk to enable QCA9377 to discover BLE devices João Paulo Rechi Vita
2019-04-23 7:22 ` [PATCH 1/2] Bluetooth: Create new HCI_QUIRK_WAIT_FOR_MATCHING_CC João Paulo Rechi Vita
2019-04-23 7:22 ` [PATCH 2/2] Bluetooth: Set HCI_QUIRK_WAIT_FOR_MATCHING_CC for QCA9377 João Paulo Rechi Vita
2019-04-23 16:17 ` [PATCH 0/2] Quirk to enable QCA9377 to discover BLE devices Marcel Holtmann
2019-04-24 5:42 ` João Paulo Rechi Vita
2019-04-24 5:43 ` [PATCH] Bluetooth: Ignore CC events not matching the last HCI command João Paulo Rechi Vita
2019-04-24 6:07 ` João Paulo Rechi Vita
2019-04-24 6:45 ` Marcel Holtmann
2019-04-25 6:37 ` João Paulo Rechi Vita [this message]
2019-04-25 6:38 ` João Paulo Rechi Vita
2019-04-25 11:07 ` Marcel Holtmann
2019-04-26 8:55 ` João Paulo Rechi Vita
2019-04-26 8:56 ` [PATCH v3] " João Paulo Rechi Vita
2019-04-26 17:01 ` Marcel Holtmann
2019-04-29 3:30 ` João Paulo Rechi Vita
2019-04-29 3:31 ` [PATCH v4] " João Paulo Rechi Vita
2019-04-29 11:10 ` Marcel Holtmann
2019-05-02 2:01 ` [PATCH v5] " João Paulo Rechi Vita
2019-05-05 17:33 ` Marcel Holtmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CA+A7VXVa9pFizfQ8C8QUS=3=YDjojmScOKPS=Yhcx-w8G=wsVw@mail.gmail.com' \
--to=jprvita@gmail.com \
--cc=bgodavar@codeaurora.org \
--cc=davem@davemloft.net \
--cc=johan.hedberg@gmail.com \
--cc=jprvita@endlessm.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@endlessm.com \
--cc=marcel@holtmann.org \
--cc=netdev@vger.kernel.org \
--cc=ytkim@qca.qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).