Re: [PATCH] Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers

[Swyter <swyterzone@gmail.com>]

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