| Message ID | 906e95ce-b0e5-239e-f544-f34d8424c8da@gmail.com |
|---|---|
| State | Accepted |
| Commit | f4292e2faf522f899b642d2040a2edbcbd455b9f |
| Headers | show |
| Series | Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic | expand |
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=516901
---Test result---
Test Summary:
CheckPatch FAIL 0.57 seconds
GitLint FAIL 0.13 seconds
BuildKernel PASS 606.18 seconds
TestRunner: Setup PASS 400.30 seconds
TestRunner: l2cap-tester PASS 2.80 seconds
TestRunner: bnep-tester PASS 2.11 seconds
TestRunner: mgmt-tester PASS 30.91 seconds
TestRunner: rfcomm-tester PASS 2.34 seconds
TestRunner: sco-tester PASS 2.35 seconds
TestRunner: smp-tester FAIL 2.32 seconds
TestRunner: userchan-tester PASS 2.15 seconds
Details
##############################
Test: CheckPatch - FAIL - 0.57 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic
WARNING: Unknown commit id '81cac64ba258a', maybe rebased or not pulled?
#21:
Fixes: 81cac64ba258a ("Bluetooth: Deal with USB devices that are faking CSR vendor")
WARNING: Unknown commit id 'cde1a8a992875', maybe rebased or not pulled?
#22:
Fixes: cde1a8a992875 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")
WARNING: Unknown commit id 'd74e0ae7e0303', maybe rebased or not pulled?
#23:
Fixes: d74e0ae7e0303 ("Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134")
WARNING: Unknown commit id '0671c0662383e', maybe rebased or not pulled?
#24:
Fixes: 0671c0662383e ("Bluetooth: btusb: Add workaround for remote-wakeup issues with Barrot 8041a02 fake CSR controllers")
total: 0 errors, 4 warnings, 78 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] Bluetooth: btusb: Make the CSR clone chip force-suspend" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 0.13 seconds
Run gitlint with rule in .gitlint
Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic
1: T1 Title exceeds max length (79>72): "Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic"
17: B1 Line exceeds max length (84>80): "Fixes: 81cac64ba258a ("Bluetooth: Deal with USB devices that are faking CSR vendor")"
18: B1 Line exceeds max length (99>80): "Fixes: cde1a8a992875 ("Bluetooth: btusb: Fix and detect most of the Chinese Bluetooth controllers")"
19: B1 Line exceeds max length (116>80): "Fixes: d74e0ae7e0303 ("Bluetooth: btusb: Fix detection of some fake CSR controllers with a bcdDevice val of 0x0134")"
20: B1 Line exceeds max length (123>80): "Fixes: 0671c0662383e ("Bluetooth: btusb: Add workaround for remote-wakeup issues with Barrot 8041a02 fake CSR controllers")"
##############################
Test: BuildKernel - PASS - 606.18 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 400.30 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.80 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 2.11 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 30.91 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 443 (99.3%), Failed: 0, Not Run: 3
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.34 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.35 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.32 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
Failed Test Cases
SMP Client - SC Request 2 Failed 0.024 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 2.15 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
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); >
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
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. :)
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);
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(-)