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
