From: Swyter <swyterzone@gmail.com>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: BlueZ <linux-bluetooth@vger.kernel.org>,
Johan Hedberg <johan.hedberg@gmail.com>
Subject: Re: [PATCH] Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers
Date: Tue, 23 Jun 2020 21:59:58 +0200 [thread overview]
Message-ID: <90ead351-ef10-1fe4-21cf-b5913384786a@gmail.com> (raw)
In-Reply-To: <AFE45DF6-8803-4613-9F7C-F9EB9894C4AD@holtmann.org>
On 23/06/2020 8:08, Marcel Holtmann wrote:
> Hi Ismael,
Hi, Marcel. Thanks for reviewing my (first ever) patch.
>
>> PS: I'm wondering how flexible the new 100-column limit really is,
>> I tried to trim the comment a bit but it looked ugly. :)
>
> it might be in general for Linus, but here lets try to keep it at 80.
>
Okay, understood. I'll keep in in mind for the second version. I was curious.
>>
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 5f022e9cf..880fe46aa 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -1739,9 +1739,22 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>>
>> rp = (struct hci_rp_read_local_version *)skb->data;
>>
>> - /* Detect controllers which aren't real CSR ones. */
>> + /* Detect a wide host of Chinese controllers that aren't CSR. Some of these clones even
>> + * respond with the correct HCI manufacturer, and their bcdDevice tags are all over the place,
>> + * which may be another good angle to look into if we really want to have really long quirk lists.
>> + *
>> + * Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
>> + * IC markings on 0x7558: FR3191AHAL 749H15143 (???)
>> + *
>> + * But the main thing they have in common is that these are really popular low-cost
>> + * options that support newer Bluetooth versions but rely on heavy VID/PID
>> + * squatting of this poor old Bluetooth 1.1 device. Even sold as such.
>> + */
>> if (le16_to_cpu(rp->manufacturer) != 10 ||
>> - le16_to_cpu(rp->lmp_subver) == 0x0c5c) {
>> + le16_to_cpu(rp->lmp_subver) == 0x0c5c ||
>> + le16_to_cpu(rp->hci_ver) >= BLUETOOTH_VER_1_2) {
>
> This check will also catch actually Bluetooth 2.0 and later made devices from CSR.
Yeah, you are right. I have been comparing HCI and lsusb dumps for this VID/PID
pair and I've found a much better way of distinguishing which is which.
Without breaking actual CSR chips. Turns out my dongle is legit.
>> + bt_dev_info(hdev, "CSR: Unbranded CSR clone detected; adding workaround");
>> +
>> /* Clear the reset quirk since this is not an actual
>> * early Bluetooth 1.1 device from CSR.
>> */
>> @@ -1751,6 +1764,9 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>> * stored link key handling and so just disable it.
>> */
>> set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
>> + } else {
>> + /* Only apply these quirks to the actual, old CSR devices */
>> + set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> }
>>
>> kfree_skb(skb);
>> @@ -3995,17 +4011,13 @@ static int btusb_probe(struct usb_interface *intf,
>>
>> if (id->driver_info & BTUSB_CSR) {
>> struct usb_device *udev = data->udev;
>> - u16 bcdDevice = le16_to_cpu(udev->descriptor.bcdDevice);
>>
>> /* Old firmware would otherwise execute USB reset */
>> - if (bcdDevice < 0x117)
>> + if (le16_to_cpu(udev->descriptor.bcdDevice) < 0x117)
>> set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>
> Keep this as is.
Okay, makes sense. Keeps things tidy.
For the next version I think I'll keep all the quirk logic in the btusb_setup_csr()
function instead. As it will require to know bcdDevice, hci_rev, lmp_subver
and manufacturer in advance.
>
>>
>> /* Fake CSR devices with broken commands */
>> - if (bcdDevice <= 0x100 || bcdDevice == 0x134)
>> - hdev->setup = btusb_setup_csr;
>> -
>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> + hdev->setup = btusb_setup_csr;
>
> Frankly, I rather add a switch statement and list all the known broken bcdDevice instead of trying to penalize real CSR devices.
>
> Regards
>
> Marcel
>
Yeah, and I have also found a better list of bcdDevice elements. I looked a bit
more in-depth and I have found out that there are actually three classes of
controllers reusing the same 0A12:0001 VID/PID:
* Old CSR Bluetooth 1.1 devices (BlueCore?):
= bcdDevice < 0x117
HCI_QUIRK_SIMULTANEOUS_DISCOVERY
HCI_QUIRK_RESET_ON_CLOSE
* New CSR Bluetooth devices CSR8510 A10 (BlueSoleil?):
= bcdDevice with 0134 1915 1958 3164 4839 5276 7558 8891
HCI_QUIRK_BROKEN_STORED_LINK_KEY
* Unbranded CSR clone:
= Their HCI chip uses a different manufacturer number;
real CSR chips use manufacturer 10 and the HCIRevision
and LMP Subversion always matches.
No quirks, varies depending on the real manufacturer.
I'll post a more throughout explanation in the work-in-progress patch.
Thanks again for your time, I'll submit a second version in a jiffy.
Hopefully we can get this old issue sorted out without breaking anything.
--swyter
prev parent reply other threads:[~2020-06-23 19:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-22 21:07 [PATCH] Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers Ismael Ferreras Morezuelas
2020-06-23 6:08 ` Marcel Holtmann
2020-06-23 19:59 ` Swyter [this message]
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=90ead351-ef10-1fe4-21cf-b5913384786a@gmail.com \
--to=swyterzone@gmail.com \
--cc=johan.hedberg@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=marcel@holtmann.org \
/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).