Message ID | 4957ed07-36b8-58a0-2307-d2e6e2940527@gmail.com (mailing list archive) |
---|---|
State | Queued, archived |
Headers | show |
Series | [v2] Bluetooth: btusb: Add a new quirk to skip HCI_FLT_CLEAR_ALL on fake CSR controllers | expand |
Context | Check | Description |
---|---|---|
tedd_an/pre-ci_am | fail | Unable to apply to tree |
Dear Ismael, Thank you for working on fixing these controllers. Just a nit, to make the commit message summary a little shorter, you can just write: Add quirk to skip HCI_FLT_CLEAR_ALL on fake CSR controllers Am 18.01.22 um 08:14 schrieb Ismael Ferreras Morezuelas: > Another subset of the more recent batch of Chinese clones aren't > specs-compliant and seem to lock up whenever they receive a > HCI_OP_SET_EVENT_FLT with flt_type set to zero/HCI_FLT_CLEAR_ALL, > which on Linux happens right at BR/EDR setup. > > So, while all the previous efforts to wrangle the herd of fake CSRs > seem to be paying off (and these also get detected as such) we > still need to take care of this quirk; testers seem to agree > that these dongles tend to work well enough afterwards. > > From some cursory USB packet capture on Windows it seems like > that driver doesn't appear to use this clear-all functionality at all. > > This patch was tested on some really popular AliExpress-style > dongles, in my case marked as "V5.0". Chip markings: UG8413, > the backside of the PCB says "USB Dangel" (sic). > > Here is the `hciconfig -a` output; for completeness: > > hci0: Type: Primary Bus: USB > BD Address: 00:1A:7D:DA:7X:XX ACL MTU: 679:8 SCO MTU: 48:16 > UP RUNNING PSCAN ISCAN > Features: 0xbf 0x3e 0x4d 0xfa 0xdb 0x3d 0x7b 0xc7 > Packet type: DM1 DM3 DM5 DH1 DH3 DH5 HV1 HV2 HV3 > Link policy: RSWITCH SNIFF > Link mode: PERIPHERAL ACCEPT > Name: 'CSR8510 A10.' > Class: 0x7c0104 > Service Classes: Rendering, Capturing, Object Transfer, Audio, Telephony > Device Class: Computer, Desktop workstation > HCI Version: 4.0 (0x6) Revision: 0x3120 > LMP Version: 4.0 (0x6) Subversion: 0x22bb > Manufacturer: Cambridge Silicon Radio (10) > > As well as the `lsusb -vv -d 0a12:0001`: > > ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode) > Device Descriptor: > bLength 18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 224 Wireless > bDeviceSubClass 1 Radio Frequency > bDeviceProtocol 1 Bluetooth > bMaxPacketSize0 64 > idVendor 0x0a12 Cambridge Silicon Radio, Ltd > idProduct 0x0001 Bluetooth Dongle (HCI mode) > bcdDevice 88.91 > iManufacturer 0 > iProduct 2 BT DONGLE10 > iSerial 0 > bNumConfigurations 1 > > Also, changed the benign dmesg print that shows up whenever the > generic force-suspend fails from bt_dev_err to bt_dev_warn; > it's okay and done on a best-effort basis, not a problem > if that does not work. If there are more iterations, you might want to split that one into a separate commit. > Also, swapped the HCI subver and LMP subver numbers for the Barrot > in the comment, which I copied wrong the last time around. > > 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") > Fixes: f4292e2faf522 ("Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic") > > https://bugzilla.kernel.org/show_bug.cgi?id=60824 The tags Link and BugLink exist (in `scripts/checkpatch.pl`), and you might want to use these tags. The bug report is from 2013 and talks about a regression upgrading from Linux 3.9 to 3.11. Was the bug report repurposed, or is the original issue also fixed by your change? > https://gist.github.com/nevack/6b36b82d715dc025163d9e9124840a07 > > Cc: stable@vger.kernel.org > Cc: Hans de Goede <hdegoede@redhat.com> > Tested-by: Gonzalo Tornaría <tornaria@cmat.edu.uy> > Tested-by: Mateus Lemos <lemonsmateus@gmail.com> > Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com> > Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com> > --- > > Changes for v2: > * Added HCI_QUIRK_NO_SUSPEND_NOTIFIER to btusb_setup_csr(). > In theory we already disable runtime suspend in btusb, button > better safe than sorry. Doesn't hurt to explicitly avoid the > hci_suspend_dev() codepath, it also works as example. > * Improved the quirk description in hci.h, as suggested by Hans > de Goede. Explain the need to disable runtime suspend. > * Also check for the quirk in hci_req_set_event_filter(); > mirror its hci_req_clear_event_filter() counterpart. > * The struct hci_dev *hdev variables always go first. > * Removed some misc formatting changes. > > The dongles still seem all peachy after the v2 changes. :) > > drivers/bluetooth/btusb.c | 6 ++++-- > include/net/bluetooth/hci.h | 10 ++++++++++ > net/bluetooth/hci_core.c | 11 ++++++++--- > net/bluetooth/hci_request.c | 13 ++++++++++--- > 4 files changed, 32 insertions(+), 8 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 75c83768c..c3bfb9bbe 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -1932,6 +1932,8 @@ static int btusb_setup_csr(struct hci_dev *hdev) > */ > set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks); > set_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks); > + set_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks); > + set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks); > > /* Clear the reset quirk since this is not an actual > * early Bluetooth 1.1 device from CSR. > @@ -1942,7 +1944,7 @@ static int btusb_setup_csr(struct hci_dev *hdev) > /* > * Special workaround for these BT 4.0 chip clones, and potentially more: > * > - * - 0x0134: a Barrot 8041a02 (HCI rev: 0x1012 sub: 0x0810) > + * - 0x0134: a Barrot 8041a02 (HCI rev: 0x0810 sub: 0x1012) > * - 0x7558: IC markings FR3191AHAL 749H15143 (HCI rev/sub-version: 0x0709) > * > * These controllers are really messed-up. > @@ -1971,7 +1973,7 @@ static int btusb_setup_csr(struct hci_dev *hdev) > if (ret >= 0) > msleep(200); > else > - bt_dev_err(hdev, "CSR: Failed to suspend the device for our Barrot 8041a02 receive-issue workaround"); > + bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround"); > > pm_runtime_forbid(&data->udev->dev); > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 63065bc01..a17b8453a 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -246,6 +246,16 @@ enum { > * HCI after resume. > */ > HCI_QUIRK_NO_SUSPEND_NOTIFIER, > + > + /* When this quirk is set, HCI_OP_SET_EVENT_FLT requests with > + * HCI_FLT_CLEAR_ALL are ignored and event filtering is > + * completely avoided. A subset of the CSR controller > + * clones struggle with this and instantly lock up. > + * > + * Note that devices using this must (separately) disable > + * runtime suspend, because event filtering takes place there. > + */ > + HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, > }; > > /* HCI device flags */ > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 8d33aa648..51d53c476 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -148,6 +148,7 @@ static int hci_init1_req(struct hci_request *req, unsigned long opt) > > static void bredr_setup(struct hci_request *req) > { > + struct hci_dev *hdev = req->hdev; > __le16 param; > __u8 flt_type; > > @@ -169,9 +170,13 @@ static void bredr_setup(struct hci_request *req) > /* Read Current IAC LAP */ > hci_req_add(req, HCI_OP_READ_CURRENT_IAC_LAP, 0, NULL); > > - /* Clear Event Filters */ > - flt_type = HCI_FLT_CLEAR_ALL; > - hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &flt_type); > + /* Clear Event Filters; some fake CSR controllers lock up after setting > + * this type of filter, so avoid sending the request altogether. > + */ > + if (!test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) { > + flt_type = HCI_FLT_CLEAR_ALL; > + hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &flt_type); > + } > > /* Connection accept timeout ~20 secs */ > param = cpu_to_le16(0x7d00); > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index 92611bfc0..c417325fe 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -979,12 +979,16 @@ void hci_req_add_le_passive_scan(struct hci_request *req) > > static void hci_req_clear_event_filter(struct hci_request *req) > { > + struct hci_dev *hdev = req->hdev; > struct hci_cp_set_event_filter f; > > - if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED)) > + if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) > + return; > + > + if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) > return; > > - if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) { > + if (hci_dev_test_flag(hdev, HCI_EVENT_FILTER_CONFIGURED)) { > memset(&f, 0, sizeof(f)); > f.flt_type = HCI_FLT_CLEAR_ALL; > hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f); > @@ -993,15 +997,18 @@ static void hci_req_clear_event_filter(struct hci_request *req) > > static void hci_req_set_event_filter(struct hci_request *req) > { > + struct hci_dev *hdev = req->hdev; > struct bdaddr_list_with_flags *b; > struct hci_cp_set_event_filter f; > - struct hci_dev *hdev = req->hdev; > u8 scan = SCAN_DISABLED; > bool scanning = test_bit(HCI_PSCAN, &hdev->flags); > > if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) > return; > > + if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) > + return; > + > /* Always clear event filter when starting */ > hci_req_clear_event_filter(req); > Acked-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
Hi Ismael, > Another subset of the more recent batch of Chinese clones aren't > specs-compliant and seem to lock up whenever they receive a > HCI_OP_SET_EVENT_FLT with flt_type set to zero/HCI_FLT_CLEAR_ALL, > which on Linux happens right at BR/EDR setup. > > So, while all the previous efforts to wrangle the herd of fake CSRs > seem to be paying off (and these also get detected as such) we > still need to take care of this quirk; testers seem to agree > that these dongles tend to work well enough afterwards. > > From some cursory USB packet capture on Windows it seems like > that driver doesn't appear to use this clear-all functionality at all. > > This patch was tested on some really popular AliExpress-style > dongles, in my case marked as "V5.0". Chip markings: UG8413, > the backside of the PCB says "USB Dangel" (sic). > > Here is the `hciconfig -a` output; for completeness: > > hci0: Type: Primary Bus: USB > BD Address: 00:1A:7D:DA:7X:XX ACL MTU: 679:8 SCO MTU: 48:16 > UP RUNNING PSCAN ISCAN > Features: 0xbf 0x3e 0x4d 0xfa 0xdb 0x3d 0x7b 0xc7 > Packet type: DM1 DM3 DM5 DH1 DH3 DH5 HV1 HV2 HV3 > Link policy: RSWITCH SNIFF > Link mode: PERIPHERAL ACCEPT > Name: 'CSR8510 A10.' > Class: 0x7c0104 > Service Classes: Rendering, Capturing, Object Transfer, Audio, Telephony > Device Class: Computer, Desktop workstation > HCI Version: 4.0 (0x6) Revision: 0x3120 > LMP Version: 4.0 (0x6) Subversion: 0x22bb > Manufacturer: Cambridge Silicon Radio (10) > > As well as the `lsusb -vv -d 0a12:0001`: > > ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode) > Device Descriptor: > bLength 18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass 224 Wireless > bDeviceSubClass 1 Radio Frequency > bDeviceProtocol 1 Bluetooth > bMaxPacketSize0 64 > idVendor 0x0a12 Cambridge Silicon Radio, Ltd > idProduct 0x0001 Bluetooth Dongle (HCI mode) > bcdDevice 88.91 > iManufacturer 0 > iProduct 2 BT DONGLE10 > iSerial 0 > bNumConfigurations 1 > > Also, changed the benign dmesg print that shows up whenever the > generic force-suspend fails from bt_dev_err to bt_dev_warn; > it's okay and done on a best-effort basis, not a problem > if that does not work. > > Also, swapped the HCI subver and LMP subver numbers for the Barrot > in the comment, which I copied wrong the last time around. > > 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") > Fixes: f4292e2faf522 ("Bluetooth: btusb: Make the CSR clone chip force-suspend workaround more generic") > > https://bugzilla.kernel.org/show_bug.cgi?id=60824 > https://gist.github.com/nevack/6b36b82d715dc025163d9e9124840a07 > > Cc: stable@vger.kernel.org > Cc: Hans de Goede <hdegoede@redhat.com> > Tested-by: Gonzalo Tornaría <tornaria@cmat.edu.uy> > Tested-by: Mateus Lemos <lemonsmateus@gmail.com> > Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com> > Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com> > --- > > Changes for v2: > * Added HCI_QUIRK_NO_SUSPEND_NOTIFIER to btusb_setup_csr(). > In theory we already disable runtime suspend in btusb, button > better safe than sorry. Doesn't hurt to explicitly avoid the > hci_suspend_dev() codepath, it also works as example. > * Improved the quirk description in hci.h, as suggested by Hans > de Goede. Explain the need to disable runtime suspend. > * Also check for the quirk in hci_req_set_event_filter(); > mirror its hci_req_clear_event_filter() counterpart. > * The struct hci_dev *hdev variables always go first. > * Removed some misc formatting changes. > > The dongles still seem all peachy after the v2 changes. :) > > drivers/bluetooth/btusb.c | 6 ++++-- > include/net/bluetooth/hci.h | 10 ++++++++++ > net/bluetooth/hci_core.c | 11 ++++++++--- > net/bluetooth/hci_request.c | 13 ++++++++++--- > 4 files changed, 32 insertions(+), 8 deletions(-) the patch no longer applies cleanly against bluetooth-next tree. Please send an updated version. Regards Marcel
El 07/02/2022 a las 16:39, Marcel Holtmann escribió: > Hi Ismael, > > the patch no longer applies cleanly against bluetooth-next tree. Please send an updated version. > > Regards > > Marcel Fair enough, I didn't expect the sheer amount of changes that the new HCI implementation would bring right in January. This must have been brewing for a while. But the result is cleaner. Lucky me. :) Finally had some time for this today, done. Sent v3.
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 75c83768c..c3bfb9bbe 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -1932,6 +1932,8 @@ static int btusb_setup_csr(struct hci_dev *hdev) */ set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks); set_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks); + set_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks); + set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks); /* Clear the reset quirk since this is not an actual * early Bluetooth 1.1 device from CSR. @@ -1942,7 +1944,7 @@ static int btusb_setup_csr(struct hci_dev *hdev) /* * Special workaround for these BT 4.0 chip clones, and potentially more: * - * - 0x0134: a Barrot 8041a02 (HCI rev: 0x1012 sub: 0x0810) + * - 0x0134: a Barrot 8041a02 (HCI rev: 0x0810 sub: 0x1012) * - 0x7558: IC markings FR3191AHAL 749H15143 (HCI rev/sub-version: 0x0709) * * These controllers are really messed-up. @@ -1971,7 +1973,7 @@ static int btusb_setup_csr(struct hci_dev *hdev) if (ret >= 0) msleep(200); else - bt_dev_err(hdev, "CSR: Failed to suspend the device for our Barrot 8041a02 receive-issue workaround"); + bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround"); pm_runtime_forbid(&data->udev->dev); diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 63065bc01..a17b8453a 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -246,6 +246,16 @@ enum { * HCI after resume. */ HCI_QUIRK_NO_SUSPEND_NOTIFIER, + + /* When this quirk is set, HCI_OP_SET_EVENT_FLT requests with + * HCI_FLT_CLEAR_ALL are ignored and event filtering is + * completely avoided. A subset of the CSR controller + * clones struggle with this and instantly lock up. + * + * Note that devices using this must (separately) disable + * runtime suspend, because event filtering takes place there. + */ + HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, }; /* HCI device flags */ diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 8d33aa648..51d53c476 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -148,6 +148,7 @@ static int hci_init1_req(struct hci_request *req, unsigned long opt) static void bredr_setup(struct hci_request *req) { + struct hci_dev *hdev = req->hdev; __le16 param; __u8 flt_type; @@ -169,9 +170,13 @@ static void bredr_setup(struct hci_request *req) /* Read Current IAC LAP */ hci_req_add(req, HCI_OP_READ_CURRENT_IAC_LAP, 0, NULL); - /* Clear Event Filters */ - flt_type = HCI_FLT_CLEAR_ALL; - hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &flt_type); + /* Clear Event Filters; some fake CSR controllers lock up after setting + * this type of filter, so avoid sending the request altogether. + */ + if (!test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) { + flt_type = HCI_FLT_CLEAR_ALL; + hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &flt_type); + } /* Connection accept timeout ~20 secs */ param = cpu_to_le16(0x7d00); diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index 92611bfc0..c417325fe 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -979,12 +979,16 @@ void hci_req_add_le_passive_scan(struct hci_request *req) static void hci_req_clear_event_filter(struct hci_request *req) { + struct hci_dev *hdev = req->hdev; struct hci_cp_set_event_filter f; - if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED)) + if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) + return; + + if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) return; - if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) { + if (hci_dev_test_flag(hdev, HCI_EVENT_FILTER_CONFIGURED)) { memset(&f, 0, sizeof(f)); f.flt_type = HCI_FLT_CLEAR_ALL; hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f); @@ -993,15 +997,18 @@ static void hci_req_clear_event_filter(struct hci_request *req) static void hci_req_set_event_filter(struct hci_request *req) { + struct hci_dev *hdev = req->hdev; struct bdaddr_list_with_flags *b; struct hci_cp_set_event_filter f; - struct hci_dev *hdev = req->hdev; u8 scan = SCAN_DISABLED; bool scanning = test_bit(HCI_PSCAN, &hdev->flags); if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) return; + if (test_bit(HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL, &hdev->quirks)) + return; + /* Always clear event filter when starting */ hci_req_clear_event_filter(req);