[PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic

[PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic

[Ismael Ferreras Morezuelas]

Turns out Hans de Goede completed the work I started last year trying to
improve Chinese-clone detection of CSR controller chips. Quirk after quirk
these Bluetooth dongles are more usable now.

Even after a few BlueZ regressions; these clones are so fickle that some
days they stop working altogether. Except on Windows, they work fine.


But this force-suspend initialization quirk seems to mostly do the trick,
after a lot of testing Bluetooth now seems to work *all* the time.

The only problem is that the solution ended up being masked under a very
stringent check; when there are probably hundreds of fake dongle
models out there that benefit from a good reset. Make it so.


Fixes: 81cac64ba258a ("Bluetooth: Deal with USB devices that are faking CSR vendor")
Fixes: cde1a8a992875 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")
Fixes: d74e0ae7e0303 ("Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134")
Fixes: 0671c0662383e ("Bluetooth: btusb: Add workaround for remote-wakeup issues with Barrot 8041a02 fake CSR controllers")

Cc: stable@vger.kernel.org
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
---

I've changed the warning line to make it easy to grep and detect if this updated
workaround is part of the driver. Should make it much more obvious to users in
case their dongle doesn't work for other reasons. There's a clear then-now.

Easy to narrow other future issues down. Let me know what you think.

 drivers/bluetooth/btusb.c | 61 +++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9855a2dd..197cafe75 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1890,7 +1890,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		is_fake = true;
 
 	if (is_fake) {
-		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
+		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds and force-suspending once...");
 
 		/* Generally these clones have big discrepancies between
 		 * advertised features and what's actually supported.
@@ -1907,41 +1907,46 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		clear_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
 		/*
-		 * Special workaround for clones with a Barrot 8041a02 chip,
-		 * 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).
+		 * Special workaround for these BT 4.0 chip clones, and potentially more:
+		 *
+		 * - 0x0134: a Barrot 8041a02                 (HCI rev: 0x1012 sub: 0x0810)
+		 * - 0x7558: IC markings FR3191AHAL 749H15143 (HCI rev/sub-version: 0x0709)
+		 *
+		 * These controllers 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
+		 * 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.
+		 * HCI and then wake-it up by disabling runtime-suspend.
 		 *
-		 * To fix 2. clear the hci's can_wake flag, this way the hci
+		 * To fix 2. clear the HCI's can_wake flag, this way the HCI
 		 * will still be autosuspended when it is not open.
+		 *
+		 * --
+		 *
+		 * Because these are widespread problems we prefer generic solutions; so
+		 * apply this initialization quirk to every controller that gets here,
+		 * it should be harmless. The alternative is to not work at all.
 		 */
-		if (bcdDevice == 0x8891 &&
-		    le16_to_cpu(rp->lmp_subver) == 0x1012 &&
-		    le16_to_cpu(rp->hci_rev) == 0x0810 &&
-		    le16_to_cpu(rp->hci_ver) == BLUETOOTH_VER_4_0) {
-			bt_dev_warn(hdev, "CSR: detected a fake CSR dongle using a Barrot 8041a02 chip, this chip is very buggy and may have issues");
-
-			pm_runtime_allow(&data->udev->dev);
+		pm_runtime_allow(&data->udev->dev);
 
-			ret = pm_runtime_suspend(&data->udev->dev);
-			if (ret >= 0)
-				msleep(200);
-			else
-				bt_dev_err(hdev, "Failed to suspend the device for Barrot 8041a02 receive-issue workaround");
-
-			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);
-		}
+		ret = pm_runtime_suspend(&data->udev->dev);
+		if (ret >= 0)
+			msleep(200);
+		else
+			bt_dev_err(hdev, "CSR: Failed to suspend the device for our Barrot 8041a02 receive-issue workaround");
+
+		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.32.0

Re: [PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic

[Hans de Goede]

Hi,

On 7/17/21 1:21 AM, Ismael Ferreras Morezuelas wrote:
> Turns out Hans de Goede completed the work I started last year trying to
> improve Chinese-clone detection of CSR controller chips. Quirk after quirk
> these Bluetooth dongles are more usable now.
> 
> Even after a few BlueZ regressions; these clones are so fickle that some
> days they stop working altogether. Except on Windows, they work fine.
> 
> 
> But this force-suspend initialization quirk seems to mostly do the trick,
> after a lot of testing Bluetooth now seems to work *all* the time.
> 
> The only problem is that the solution ended up being masked under a very
> stringent check; when there are probably hundreds of fake dongle
> models out there that benefit from a good reset. Make it so.
> 
> 
> Fixes: 81cac64ba258a ("Bluetooth: Deal with USB devices that are faking CSR vendor")
> Fixes: cde1a8a992875 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")
> Fixes: d74e0ae7e0303 ("Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134")
> Fixes: 0671c0662383e ("Bluetooth: btusb: Add workaround for remote-wakeup issues with Barrot 8041a02 fake CSR controllers")
> 
> Cc: stable@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>

FWIW I'm fine with making the force-suspend once quirk which
I added generic to all clones.

The new code looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





> ---
> 
> I've changed the warning line to make it easy to grep and detect if this updated
> workaround is part of the driver. Should make it much more obvious to users in
> case their dongle doesn't work for other reasons. There's a clear then-now.
> 
> Easy to narrow other future issues down. Let me know what you think.
> 
>  drivers/bluetooth/btusb.c | 61 +++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9855a2dd..197cafe75 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -1890,7 +1890,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  		is_fake = true;
>  
>  	if (is_fake) {
> -		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
> +		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds and force-suspending once...");
>  
>  		/* Generally these clones have big discrepancies between
>  		 * advertised features and what's actually supported.
> @@ -1907,41 +1907,46 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  		clear_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>  
>  		/*
> -		 * Special workaround for clones with a Barrot 8041a02 chip,
> -		 * 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).
> +		 * Special workaround for these BT 4.0 chip clones, and potentially more:
> +		 *
> +		 * - 0x0134: a Barrot 8041a02                 (HCI rev: 0x1012 sub: 0x0810)
> +		 * - 0x7558: IC markings FR3191AHAL 749H15143 (HCI rev/sub-version: 0x0709)
> +		 *
> +		 * These controllers 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
> +		 * 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.
> +		 * HCI and then wake-it up by disabling runtime-suspend.
>  		 *
> -		 * To fix 2. clear the hci's can_wake flag, this way the hci
> +		 * To fix 2. clear the HCI's can_wake flag, this way the HCI
>  		 * will still be autosuspended when it is not open.
> +		 *
> +		 * --
> +		 *
> +		 * Because these are widespread problems we prefer generic solutions; so
> +		 * apply this initialization quirk to every controller that gets here,
> +		 * it should be harmless. The alternative is to not work at all.
>  		 */
> -		if (bcdDevice == 0x8891 &&
> -		    le16_to_cpu(rp->lmp_subver) == 0x1012 &&
> -		    le16_to_cpu(rp->hci_rev) == 0x0810 &&
> -		    le16_to_cpu(rp->hci_ver) == BLUETOOTH_VER_4_0) {
> -			bt_dev_warn(hdev, "CSR: detected a fake CSR dongle using a Barrot 8041a02 chip, this chip is very buggy and may have issues");
> -
> -			pm_runtime_allow(&data->udev->dev);
> +		pm_runtime_allow(&data->udev->dev);
>  
> -			ret = pm_runtime_suspend(&data->udev->dev);
> -			if (ret >= 0)
> -				msleep(200);
> -			else
> -				bt_dev_err(hdev, "Failed to suspend the device for Barrot 8041a02 receive-issue workaround");
> -
> -			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);
> -		}
> +		ret = pm_runtime_suspend(&data->udev->dev);
> +		if (ret >= 0)
> +			msleep(200);
> +		else
> +			bt_dev_err(hdev, "CSR: Failed to suspend the device for our Barrot 8041a02 receive-issue workaround");
> +
> +		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: [PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic

[Marcel Holtmann]

Hi Ismael,

> Turns out Hans de Goede completed the work I started last year trying to
> improve Chinese-clone detection of CSR controller chips. Quirk after quirk
> these Bluetooth dongles are more usable now.
> 
> Even after a few BlueZ regressions; these clones are so fickle that some
> days they stop working altogether. Except on Windows, they work fine.
> 
> 
> But this force-suspend initialization quirk seems to mostly do the trick,
> after a lot of testing Bluetooth now seems to work *all* the time.
> 
> The only problem is that the solution ended up being masked under a very
> stringent check; when there are probably hundreds of fake dongle
> models out there that benefit from a good reset. Make it so.
> 
> 
> Fixes: 81cac64ba258a ("Bluetooth: Deal with USB devices that are faking CSR vendor")
> Fixes: cde1a8a992875 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")
> Fixes: d74e0ae7e0303 ("Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134")
> Fixes: 0671c0662383e ("Bluetooth: btusb: Add workaround for remote-wakeup issues with Barrot 8041a02 fake CSR controllers")
> 
> Cc: stable@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> ---
> 
> I've changed the warning line to make it easy to grep and detect if this updated
> workaround is part of the driver. Should make it much more obvious to users in
> case their dongle doesn't work for other reasons. There's a clear then-now.
> 
> Easy to narrow other future issues down. Let me know what you think.
> 
> drivers/bluetooth/btusb.c | 61 +++++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 28 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic

[Ismael Ferreras Morezuelas]

On 29/07/2021 21:55, Marcel Holtmann wrote:
> Hi Ismael,
> 
> patch has been applied to bluetooth-next tree.
> 
> Regards
> 
> Marcel
> 

Thanks a lot, Marcel! Upstreaming these changes has been very fun,
looking forward to improve the compatibility more in the future.

Every bit helps. The next step will probably entail
adding a HCI_FLT_CLEAR_ALL quirk in there. :)