[PATCH v2 0/2] Bluetooth: btusb: 2 fixes for handling of fake CSR USB dongles

[PATCH v2 0/2] Bluetooth: btusb: 2 fixes for handling of fake CSR USB dongles

[Hans de Goede]

Hi All,

I just realized that after going through the trouble to open the fake
dongle (for which patch 1 restores identifiying it as fake) to read the
chip number, I forgot to actually add the chip number to the commit-msg.

So here is a v2 with the chip number added to the commit-msg for 1/2,
no other changes.

Regards,

Hans

[PATCH v2 1/2] Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134

[Hans de Goede]

Commit cde1a8a99287 ("Bluetooth: btusb: Fix and detect most of the
Chinese Bluetooth controllers") made the detection of fake controllers
more generic fixing it for much of the newer fakes / clones.

But this does not work for a fake CSR controller with a bcdDevice
value of 0x0134, which was correctly identified as fake before
this change.

Add an extra check for this special case, checking for a combination
of a bcdDevice value of 0x0134, together with a lmp_subver of 0x0c5c
and a hci_ver of BLUETOOTH_VER_2_0.

The chip inside this fake dongle is marked as with "clockwise cw6629d".

Fixes: cde1a8a99287 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Add description of chip inside the fake dongle to the commit message
---
 drivers/bluetooth/btusb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1005b6e8ff74..ac7fede4f951 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1763,6 +1763,8 @@ static int btusb_setup_bcm92035(struct hci_dev *hdev)
 
 static int btusb_setup_csr(struct hci_dev *hdev)
 {
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	u16 bcdDevice = le16_to_cpu(data->udev->descriptor.bcdDevice);
 	struct hci_rp_read_local_version *rp;
 	struct sk_buff *skb;
 	bool is_fake = false;
@@ -1832,6 +1834,12 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		 le16_to_cpu(rp->hci_ver) > BLUETOOTH_VER_4_0)
 		is_fake = true;
 
+	/* Other clones which beat all the above checks */
+	else if (bcdDevice == 0x0134 &&
+		 le16_to_cpu(rp->lmp_subver) == 0x0c5c &&
+		 le16_to_cpu(rp->hci_ver) == BLUETOOTH_VER_2_0)
+		is_fake = true;
+
 	if (is_fake) {
 		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
 
-- 
2.28.0

[PATCH v2 2/2] Bluetooth: btusb: Add workaround for remote-wakeup issues with some fake CSR controllers

[Hans de Goede]

With the recent btusb change to detect and deal with more fake CSR
controllers, I decided to see if one of the fakes which I have
lying around would now work.

After much experimentation I came to the conclusion that it works, if I
have autosuspend enabled initially and then disable it after the device
has suspended at least once. Yes this is very weird, but I've tried many
things, like manually clearing the remote-wakeup feature. Doing a
runtime-resume + runtime suspend is the only way to get the receiver
to actually report received data (and/or pairing info) through its
bulk rx endpoint.

But the funkyness of the bulk-endpoint does not stop there, I mainly
found out about this problem, because with autosuspend enabled
(which usually ensures the suspend at least once condition is met),
the receiver stops reporting received data through its bulk rx endpoint
as soon as autosuspend kicks in. So I initially just disabled
autosuspend, but then the receiver does not work at all.

This was with a fake CSR receiver with a bcdDevice value of 0x8891,
a lmp_subver of 0x0x1012, a hci_rev of 0x0810 and a hci_ver of
BLUETOOTH_VER_4_0.

Summarizing this specific fake CSR receiver has the following 2 issues:

1. The bulk rx endpoint will never report any data unless
the device was suspended at least once.

2. They will not wakeup when autosuspended and receiving data on their
bulk rx endpoint from e.g. a keyboard or mouse (IOW remote-wakeup support
is broken for the bulk endpoint).

Add a workaround for 1. which enables runtime-suspend, force-suspends
the hci and then wakes-it up by disabling runtime-suspend again.

Add a workaround for 2. which clears the hci's can_wake flag, this way
the hci will still be autosuspended when it is not open.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btusb.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ac7fede4f951..48e404dfa246 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1768,6 +1768,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 	struct hci_rp_read_local_version *rp;
 	struct sk_buff *skb;
 	bool is_fake = false;
+	int ret;
 
 	BT_DBG("%s", hdev->name);
 
@@ -1856,6 +1857,37 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		 */
 		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
 		clear_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
+
+		/*
+		 * Some of these clones are really messed-up:
+		 * 1. Their bulk rx endpoint will never report any data unless
+		 * the device was suspended at least once (yes really).
+		 * 2. They will not wakeup when autosuspended and receiving data
+		 * on their bulk rx endpoint from e.g. a keyboard or mouse
+		 * (IOW remote-wakeup support is broken for the bulk endpoint).
+		 *
+		 * To fix 1. enable runtime-suspend, force-suspend the
+		 * hci and then wake-it up by disabling runtime-suspend.
+		 *
+		 * To fix 2. clear the hci's can_wake flag, this way the hci
+		 * will still be autosuspended when it is not open.
+		 */
+		if (device_can_wakeup(&data->udev->dev)) {
+			pm_runtime_allow(&data->udev->dev);
+
+			ret = pm_runtime_suspend(&data->udev->dev);
+			if (ret >= 0)
+				msleep(200);
+			else
+				bt_dev_warn(hdev, "Failed to suspend the device for CSR clone receive-issue workaround\n");
+
+			pm_runtime_forbid(&data->udev->dev);
+
+			device_set_wakeup_capable(&data->udev->dev, false);
+			/* Re-enable autosuspend if this was requested */
+			if (enable_autosuspend)
+				usb_enable_autosuspend(data->udev);
+		}
 	}
 
 	kfree_skb(skb);
-- 
2.28.0

Re: [PATCH v2 2/2] Bluetooth: btusb: Add workaround for remote-wakeup issues with some fake CSR controllers

[Hans de Goede]

Hi,

On 11/23/20 11:13 AM, Hans de Goede wrote:
> With the recent btusb change to detect and deal with more fake CSR
> controllers, I decided to see if one of the fakes which I have
> lying around would now work.
> 
> After much experimentation I came to the conclusion that it works, if I
> have autosuspend enabled initially and then disable it after the device
> has suspended at least once. Yes this is very weird, but I've tried many
> things, like manually clearing the remote-wakeup feature. Doing a
> runtime-resume + runtime suspend is the only way to get the receiver
> to actually report received data (and/or pairing info) through its
> bulk rx endpoint.
> 
> But the funkyness of the bulk-endpoint does not stop there, I mainly
> found out about this problem, because with autosuspend enabled
> (which usually ensures the suspend at least once condition is met),
> the receiver stops reporting received data through its bulk rx endpoint
> as soon as autosuspend kicks in. So I initially just disabled
> autosuspend, but then the receiver does not work at all.
> 
> This was with a fake CSR receiver with a bcdDevice value of 0x8891,
> a lmp_subver of 0x0x1012, a hci_rev of 0x0810 and a hci_ver of
> BLUETOOTH_VER_4_0.

I just realized that I should have opened this one and add the
chipmarkings to the commit msg here too. So I've just opened it
and took a look.

This one has a Barrot 8041a02 chip and searching for 8041a92 shows that
the internet is full of reports of how crappy it is.

I guess this patch probably will get some review remarks anyways,
but let me know if you want a v3 with just the chip added to the
commit msg.

Regards,

Hans


> 
> Summarizing this specific fake CSR receiver has the following 2 issues:
> 
> 1. The bulk rx endpoint will never report any data unless
> the device was suspended at least once.
> 
> 2. They will not wakeup when autosuspended and receiving data on their
> bulk rx endpoint from e.g. a keyboard or mouse (IOW remote-wakeup support
> is broken for the bulk endpoint).
> 
> Add a workaround for 1. which enables runtime-suspend, force-suspends
> the hci and then wakes-it up by disabling runtime-suspend again.
> 
> Add a workaround for 2. which clears the hci's can_wake flag, this way
> the hci will still be autosuspended when it is not open.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/bluetooth/btusb.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index ac7fede4f951..48e404dfa246 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1768,6 +1768,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  	struct hci_rp_read_local_version *rp;
>  	struct sk_buff *skb;
>  	bool is_fake = false;
> +	int ret;
>  
>  	BT_DBG("%s", hdev->name);
>  
> @@ -1856,6 +1857,37 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  		 */
>  		clear_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
>  		clear_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> +
> +		/*
> +		 * Some of these clones are really messed-up:
> +		 * 1. Their bulk rx endpoint will never report any data unless
> +		 * the device was suspended at least once (yes really).
> +		 * 2. They will not wakeup when autosuspended and receiving data
> +		 * on their bulk rx endpoint from e.g. a keyboard or mouse
> +		 * (IOW remote-wakeup support is broken for the bulk endpoint).
> +		 *
> +		 * To fix 1. enable runtime-suspend, force-suspend the
> +		 * hci and then wake-it up by disabling runtime-suspend.
> +		 *
> +		 * To fix 2. clear the hci's can_wake flag, this way the hci
> +		 * will still be autosuspended when it is not open.
> +		 */
> +		if (device_can_wakeup(&data->udev->dev)) {
> +			pm_runtime_allow(&data->udev->dev);
> +
> +			ret = pm_runtime_suspend(&data->udev->dev);
> +			if (ret >= 0)
> +				msleep(200);
> +			else
> +				bt_dev_warn(hdev, "Failed to suspend the device for CSR clone receive-issue workaround\n");
> +
> +			pm_runtime_forbid(&data->udev->dev);
> +
> +			device_set_wakeup_capable(&data->udev->dev, false);
> +			/* Re-enable autosuspend if this was requested */
> +			if (enable_autosuspend)
> +				usb_enable_autosuspend(data->udev);
> +		}
>  	}
>  
>  	kfree_skb(skb);
> 

RE: Bluetooth: btusb: 2 fixes for handling of fake CSR USB dongles

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=389341

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - FAIL
Output:
Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134
1: T1 Title exceeds max length (91>72): "Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134"
17: B1 Line exceeds max length (98>80): "Fixes: cde1a8a99287 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")"

Bluetooth: btusb: Add workaround for remote-wakeup issues with some fake CSR controllers
1: T1 Title exceeds max length (88>72): "Bluetooth: btusb: Add workaround for remote-wakeup issues with some fake CSR controllers"


##############################
Test: CheckBuildK - PASS



---
Regards,
Linux Bluetooth

Re: [PATCH v2 2/2] Bluetooth: btusb: Add workaround for remote-wakeup issues with some fake CSR controllers

[Marcel Holtmann]

Hi Hans,

>> With the recent btusb change to detect and deal with more fake CSR
>> controllers, I decided to see if one of the fakes which I have
>> lying around would now work.
>> 
>> After much experimentation I came to the conclusion that it works, if I
>> have autosuspend enabled initially and then disable it after the device
>> has suspended at least once. Yes this is very weird, but I've tried many
>> things, like manually clearing the remote-wakeup feature. Doing a
>> runtime-resume + runtime suspend is the only way to get the receiver
>> to actually report received data (and/or pairing info) through its
>> bulk rx endpoint.
>> 
>> But the funkyness of the bulk-endpoint does not stop there, I mainly
>> found out about this problem, because with autosuspend enabled
>> (which usually ensures the suspend at least once condition is met),
>> the receiver stops reporting received data through its bulk rx endpoint
>> as soon as autosuspend kicks in. So I initially just disabled
>> autosuspend, but then the receiver does not work at all.
>> 
>> This was with a fake CSR receiver with a bcdDevice value of 0x8891,
>> a lmp_subver of 0x0x1012, a hci_rev of 0x0810 and a hci_ver of
>> BLUETOOTH_VER_4_0.
> 
> I just realized that I should have opened this one and add the
> chipmarkings to the commit msg here too. So I've just opened it
> and took a look.
> 
> This one has a Barrot 8041a02 chip and searching for 8041a92 shows that
> the internet is full of reports of how crappy it is.
> 
> I guess this patch probably will get some review remarks anyways,
> but let me know if you want a v3 with just the chip added to the
> commit msg.

yes please. The more info we add the better.

Regards

Marcel