[PATCH 1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk

[PATCH 1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk

[Ismael Ferreras Morezuelas]

A patch series by a Qualcomm engineer essentially removed my
quirk/workaround because they thought it was unnecessary.

It wasn't, and it broke everything again:

https://patchwork.kernel.org/project/netdevbpf/list/?series=661703&archive=both&state=*

He argues that the quirk is not necessary because the code should check if the dongle
says if it's supported or not. The problem is that for these Chinese CSR
clones they say that it would work, but it's a lie. Take a look:

= New Index: 00:00:00:00:00:00 (Primary,USB,hci0)                              [hci0] 11.272194
= Open Index: 00:00:00:00:00:00                                                [hci0] 11.272384
< HCI Command: Read Local Version Information (0x04|0x0001) plen 0          #1 [hci0] 11.272400
> HCI Event: Command Complete (0x0e) plen 12                                #2
> [hci0] 11.276039
      Read Local Version Information (0x04|0x0001) ncmd 1
        Status: Success (0x00)
        HCI version: Bluetooth 5.0 (0x09) - Revision 2064 (0x0810)
        LMP version: Bluetooth 5.0 (0x09) - Subversion 8978 (0x2312)
        Manufacturer: Cambridge Silicon Radio (10)
...
< HCI Command: Read Local Supported Features (0x04|0x0003) plen 0           #5 [hci0] 11.648370
> HCI Event: Command Complete (0x0e) plen 68                               #12
> [hci0] 11.668030
      Read Local Supported Commands (0x04|0x0002) ncmd 1
        Status: Success (0x00)
        Commands: 163 entries
          ...
          Read Default Erroneous Data Reporting (Octet 18 - Bit 2)
          Write Default Erroneous Data Reporting (Octet 18 - Bit 3)
          ...
...
< HCI Command: Read Default Erroneous Data Reporting (0x03|0x005a) plen 0  #47 [hci0] 11.748352
= Close Index: 00:1A:7D:DA:71:XX                                               [hci0] 13.776824

So bring it back wholesale.

Fixes: 63b1a7dd3 (Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING)
Fixes: e168f69008 (Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR)
Fixes: 766ae2422b (Bluetooth: hci_sync: Check LMP feature bit instead of quirk)

Cc: stable@vger.kernel.org
Cc: Zijun Hu <quic_zijuhu@quicinc.com>
Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
---
 drivers/bluetooth/btusb.c   |  1 +
 include/net/bluetooth/hci.h | 11 +++++++++++
 net/bluetooth/hci_sync.c    |  9 +++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3b269060e91f..1360b2163ec5 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2174,6 +2174,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		 * without these the controller will lock up.
 		 */
 		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);
 
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e004ba04a9ae..0fe789f6a653 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -228,6 +228,17 @@ enum {
 	 */
 	HCI_QUIRK_VALID_LE_STATES,
 
+	/* When this quirk is set, then erroneous data reporting
+	 * is ignored. This is mainly due to the fact that the HCI
+	 * Read Default Erroneous Data Reporting command is advertised,
+	 * but not supported; these controllers often reply with unknown
+	 * command and tend to lock up randomly. Needing a hard reset.
+	 *
+	 * This quirk can be set before hci_register_dev is called or
+	 * during the hdev->setup vendor callback.
+	 */
+	HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
+
 	/*
 	 * When this quirk is set, then the hci_suspend_notifier is not
 	 * registered. This is intended for devices which drop completely
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index bd9eb713b26b..0a7abc817f10 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3798,7 +3798,8 @@ static int hci_read_page_scan_activity_sync(struct hci_dev *hdev)
 static int hci_read_def_err_data_reporting_sync(struct hci_dev *hdev)
 {
 	if (!(hdev->commands[18] & 0x04) ||
-	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
+	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
+	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
 		return 0;
 
 	return __hci_cmd_sync_status(hdev, HCI_OP_READ_DEF_ERR_DATA_REPORTING,
@@ -4316,7 +4317,8 @@ static int hci_set_err_data_report_sync(struct hci_dev *hdev)
 	bool enabled = hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED);
 
 	if (!(hdev->commands[18] & 0x08) ||
-	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
+	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
+	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
 		return 0;
 
 	if (enabled == hdev->err_data_reporting)
@@ -4475,6 +4477,9 @@ static const struct {
 	HCI_QUIRK_BROKEN(STORED_LINK_KEY,
 			 "HCI Delete Stored Link Key command is advertised, "
 			 "but not supported."),
+	HCI_QUIRK_BROKEN(ERR_DATA_REPORTING,
+			 "HCI Read Default Erroneous Data Reporting command is "
+			 "advertised, but not supported."),
 	HCI_QUIRK_BROKEN(READ_TRANSMIT_POWER,
 			 "HCI Read Transmit Power Level command is advertised, "
 			 "but not supported."),
-- 
2.38.1

[PATCH 2/3] Bluetooth: btusb: Add a setup message for CSR dongles showing the Read Local Information values

[Ismael Ferreras Morezuelas]

The rationale of showing this is that it's potentially critical information to diagnose
and find more CSR compatibility bugs in the future and it will save a lot of headaches.

We can't ask normal people to run btmon, but infinitely more users already post their dmesg.
Because in many cases the device doesn't go up, most of the tools won't show these either.

Given that clones come from a wide array of vendors (some are actually Barrot,
some are something else) and these numbers are what let us find differences between
actual and fake ones, it will be immensely helpful to scour the Internet looking
for this pattern and building an actual database to find correlations and
improve the checks. I can't buy a sack of clones and do it myself.

Cc: stable@vger.kernel.org
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
---
 drivers/bluetooth/btusb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1360b2163ec5..8f34bf195bae 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2112,6 +2112,11 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 
 	rp = (struct hci_rp_read_local_version *)skb->data;
 
+	bt_dev_info(hdev, "CSR: Setting up dongle with HCI ver=%u rev=%04x; LMP ver=%u subver=%04x; manufacturer=%u",
+		le16_to_cpu(rp->hci_ver), le16_to_cpu(rp->hci_rev),
+		le16_to_cpu(rp->lmp_ver), le16_to_cpu(rp->lmp_subver),
+		le16_to_cpu(rp->manufacturer));
+
 	/* Detect a wide host of Chinese controllers that aren't CSR.
 	 *
 	 * Known fake bcdDevices: 0x0100, 0x0134, 0x1915, 0x2520, 0x7558, 0x8891
-- 
2.38.1

[PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Ismael Ferreras Morezuelas]

A few users have reported that their cloned Chinese dongle doesn't
work well with the hack Hans de Goede added, that tries this
off-on mechanism as a way to unfreeze them.

It's still more than worthwhile to have it, as in the vast majority
of cases it either completely brings dongles to life or just resets
them harmlessly as it already happens during normal USB operation.

This is nothing new and the controllers are expected to behave
correctly. But yeah, go figure. :)

For that unhappy minority we can easily handle this edge case by letting
users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.

I believe this is the most generic way of doing it, given the constraints
and by still having a good out-of-the-box experience.

No clone left behind.

Cc: stable@vger.kernel.org
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
---
 drivers/bluetooth/btusb.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 8f34bf195bae..d31d4f925463 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -34,6 +34,7 @@ static bool force_scofix;
 static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
 static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
 static bool reset = true;
+static bool disable_fake_csr_forcesuspend_hack;
 
 static struct usb_driver btusb_driver;
 
@@ -2171,7 +2172,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 and force-suspending once...");
+		bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
 
 		/* Generally these clones have big discrepancies between
 		 * advertised features and what's actually supported.
@@ -2215,21 +2216,24 @@ static int btusb_setup_csr(struct hci_dev *hdev)
 		 * apply this initialization quirk to every controller that gets here,
 		 * it should be harmless. The alternative is to not work at all.
 		 */
-		pm_runtime_allow(&data->udev->dev);
+		if (!disable_fake_csr_forcesuspend_hack) {
+			bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; force-suspending once...");
+			pm_runtime_allow(&data->udev->dev);
 
-		ret = pm_runtime_suspend(&data->udev->dev);
-		if (ret >= 0)
-			msleep(200);
-		else
-			bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
+			ret = pm_runtime_suspend(&data->udev->dev);
+			if (ret >= 0)
+				msleep(200);
+			else
+				bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
 
-		pm_runtime_forbid(&data->udev->dev);
+			pm_runtime_forbid(&data->udev->dev);
 
-		device_set_wakeup_capable(&data->udev->dev, false);
+			device_set_wakeup_capable(&data->udev->dev, false);
 
-		/* Re-enable autosuspend if this was requested */
-		if (enable_autosuspend)
-			usb_enable_autosuspend(data->udev);
+			/* Re-enable autosuspend if this was requested */
+			if (enable_autosuspend)
+				usb_enable_autosuspend(data->udev);
+		}
 	}
 
 	kfree_skb(skb);
@@ -4312,6 +4316,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
 module_param(reset, bool, 0644);
 MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
 
+module_param(disable_fake_csr_forcesuspend_hack, bool, 0644);
+MODULE_PARM_DESC(disable_fake_csr_forcesuspend_hack, "Don't indiscriminately force-suspend Chinese-cloned CSR dongles trying to unfreeze them");
+
 MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
 MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION);
 MODULE_VERSION(VERSION);
-- 
2.38.1

RE: [1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 7611 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=690177

---Test result---

Test Summary:
CheckPatch                    FAIL      5.31 seconds
GitLint                       FAIL      3.04 seconds
SubjectPrefix                 PASS      2.64 seconds
BuildKernel                   PASS      36.58 seconds
BuildKernel32                 PASS      33.32 seconds
Incremental Build with patchesPASS      62.81 seconds
TestRunner: Setup             PASS      549.33 seconds
TestRunner: l2cap-tester      PASS      18.34 seconds
TestRunner: iso-tester        PASS      18.03 seconds
TestRunner: bnep-tester       PASS      6.91 seconds
TestRunner: mgmt-tester       PASS      111.13 seconds
TestRunner: rfcomm-tester     PASS      10.85 seconds
TestRunner: sco-tester        PASS      10.29 seconds
TestRunner: ioctl-tester      PASS      11.57 seconds
TestRunner: mesh-tester       PASS      8.46 seconds
TestRunner: smp-tester        PASS      10.19 seconds
TestRunner: userchan-tester   PASS      7.11 seconds

Details
##############################
Test: CheckPatch - FAIL - 5.31 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk\Use of uninitialized value $cid in concatenation (.) or string at /usr/bin/checkpatch.pl line 3185.
Use of uninitialized value $cid in concatenation (.) or string at /usr/bin/checkpatch.pl line 3185.
Use of uninitialized value $cid in concatenation (.) or string at /usr/bin/checkpatch.pl line 3185.
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#86: 
https://patchwork.kernel.org/project/netdevbpf/list/?series=661703&archive=both&state=*

WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes:  ("Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING")'
#119: 
Fixes: 63b1a7dd3 (Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING)

WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes:  ("Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR")'
#120: 
Fixes: e168f69008 (Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR)

WARNING:BAD_FIXES_TAG: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes:  ("Bluetooth: hci_sync: Check LMP feature bit instead of quirk")'
#121: 
Fixes: 766ae2422b (Bluetooth: hci_sync: Check LMP feature bit instead of quirk)

WARNING:SPLIT_STRING: quoted string split across lines
#199: FILE: net/bluetooth/hci_sync.c:4482:
+			 "HCI Read Default Erroneous Data Reporting command is "
+			 "advertised, but not supported."),

total: 0 errors, 5 warnings, 0 checks, 51 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.

/github/workspace/src/13024775.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

[2/3] Bluetooth: btusb: Add a setup message for CSR dongles showing the Read Local Information values\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#85: 
The rationale of showing this is that it's potentially critical information to diagnose

total: 0 errors, 1 warnings, 11 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.

/github/workspace/src/13024776.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

[3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#97: 
users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.

total: 0 errors, 1 warnings, 59 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.

/github/workspace/src/13024777.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 3.04 seconds
Run gitlint with rule in .gitlint
[1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk
1: T1 Title exceeds max length (91>80): "[1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk"
8: B1 Line exceeds max length (87>80): "https://patchwork.kernel.org/project/netdevbpf/list/?series=661703&archive=both&state=*"
10: B1 Line exceeds max length (85>80): "He argues that the quirk is not necessary because the code should check if the dongle"
14: B1 Line exceeds max length (95>80): "= New Index: 00:00:00:00:00:00 (Primary,USB,hci0)                              [hci0] 11.272194"
15: B1 Line exceeds max length (95>80): "= Open Index: 00:00:00:00:00:00                                                [hci0] 11.272384"
16: B1 Line exceeds max length (95>80): "< HCI Command: Read Local Version Information (0x04|0x0001) plen 0          #1 [hci0] 11.272400"
25: B1 Line exceeds max length (95>80): "< HCI Command: Read Local Supported Features (0x04|0x0003) plen 0           #5 [hci0] 11.648370"
36: B1 Line exceeds max length (95>80): "< HCI Command: Read Default Erroneous Data Reporting (0x03|0x005a) plen 0  #47 [hci0] 11.748352"
37: B1 Line exceeds max length (95>80): "= Close Index: 00:1A:7D:DA:71:XX                                               [hci0] 13.776824"

[2/3] Bluetooth: btusb: Add a setup message for CSR dongles showing the Read Local Information values
1: T1 Title exceeds max length (101>80): "[2/3] Bluetooth: btusb: Add a setup message for CSR dongles showing the Read Local Information values"
3: B1 Line exceeds max length (87>80): "The rationale of showing this is that it's potentially critical information to diagnose"
4: B1 Line exceeds max length (87>80): "and find more CSR compatibility bugs in the future and it will save a lot of headaches."
6: B1 Line exceeds max length (92>80): "We can't ask normal people to run btmon, but infinitely more users already post their dmesg."
7: B1 Line exceeds max length (90>80): "Because in many cases the device doesn't go up, most of the tools won't show these either."
10: B1 Line exceeds max length (83>80): "some are something else) and these numbers are what let us find differences between"

[3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack
1: T1 Title exceeds max length (92>80): "[3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack"
15: B1 Line exceeds max length (84>80): "users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option."




---
Regards,
Linux Bluetooth

Re: [PATCH 1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk

[Hans de Goede]

Hi Ismael,

On 10/29/22 22:24, Ismael Ferreras Morezuelas wrote:
> A patch series by a Qualcomm engineer essentially removed my
> quirk/workaround because they thought it was unnecessary.
> 
> It wasn't, and it broke everything again:
> 
> https://patchwork.kernel.org/project/netdevbpf/list/?series=661703&archive=both&state=*
> 
> He argues that the quirk is not necessary because the code should check if the dongle
> says if it's supported or not. The problem is that for these Chinese CSR
> clones they say that it would work, but it's a lie. Take a look:
> 
> = New Index: 00:00:00:00:00:00 (Primary,USB,hci0)                              [hci0] 11.272194
> = Open Index: 00:00:00:00:00:00                                                [hci0] 11.272384
> < HCI Command: Read Local Version Information (0x04|0x0001) plen 0          #1 [hci0] 11.272400
>> HCI Event: Command Complete (0x0e) plen 12                                #2
>> [hci0] 11.276039
>       Read Local Version Information (0x04|0x0001) ncmd 1
>         Status: Success (0x00)
>         HCI version: Bluetooth 5.0 (0x09) - Revision 2064 (0x0810)
>         LMP version: Bluetooth 5.0 (0x09) - Subversion 8978 (0x2312)
>         Manufacturer: Cambridge Silicon Radio (10)
> ...
> < HCI Command: Read Local Supported Features (0x04|0x0003) plen 0           #5 [hci0] 11.648370
>> HCI Event: Command Complete (0x0e) plen 68                               #12
>> [hci0] 11.668030
>       Read Local Supported Commands (0x04|0x0002) ncmd 1
>         Status: Success (0x00)
>         Commands: 163 entries
>           ...
>           Read Default Erroneous Data Reporting (Octet 18 - Bit 2)
>           Write Default Erroneous Data Reporting (Octet 18 - Bit 3)
>           ...
> ...
> < HCI Command: Read Default Erroneous Data Reporting (0x03|0x005a) plen 0  #47 [hci0] 11.748352
> = Close Index: 00:1A:7D:DA:71:XX                                               [hci0] 13.776824
> 
> So bring it back wholesale.
> 
> Fixes: 63b1a7dd3 (Bluetooth: hci_sync: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING)
> Fixes: e168f69008 (Bluetooth: btusb: Remove HCI_QUIRK_BROKEN_ERR_DATA_REPORTING for fake CSR)
> Fixes: 766ae2422b (Bluetooth: hci_sync: Check LMP feature bit instead of quirk)
> 
> Cc: stable@vger.kernel.org
> Cc: Zijun Hu <quic_zijuhu@quicinc.com>
> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>

Thank you very much for your continued work on making these
clones work with Linux!

The entire series looks good to me:

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

for the series.

Regards,

Hans


> ---
>  drivers/bluetooth/btusb.c   |  1 +
>  include/net/bluetooth/hci.h | 11 +++++++++++
>  net/bluetooth/hci_sync.c    |  9 +++++++--
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3b269060e91f..1360b2163ec5 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2174,6 +2174,7 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>  		 * without these the controller will lock up.
>  		 */
>  		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);
>  
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index e004ba04a9ae..0fe789f6a653 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -228,6 +228,17 @@ enum {
>  	 */
>  	HCI_QUIRK_VALID_LE_STATES,
>  
> +	/* When this quirk is set, then erroneous data reporting
> +	 * is ignored. This is mainly due to the fact that the HCI
> +	 * Read Default Erroneous Data Reporting command is advertised,
> +	 * but not supported; these controllers often reply with unknown
> +	 * command and tend to lock up randomly. Needing a hard reset.
> +	 *
> +	 * This quirk can be set before hci_register_dev is called or
> +	 * during the hdev->setup vendor callback.
> +	 */
> +	HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
> +
>  	/*
>  	 * When this quirk is set, then the hci_suspend_notifier is not
>  	 * registered. This is intended for devices which drop completely
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index bd9eb713b26b..0a7abc817f10 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3798,7 +3798,8 @@ static int hci_read_page_scan_activity_sync(struct hci_dev *hdev)
>  static int hci_read_def_err_data_reporting_sync(struct hci_dev *hdev)
>  {
>  	if (!(hdev->commands[18] & 0x04) ||
> -	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
> +	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
> +	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
>  		return 0;
>  
>  	return __hci_cmd_sync_status(hdev, HCI_OP_READ_DEF_ERR_DATA_REPORTING,
> @@ -4316,7 +4317,8 @@ static int hci_set_err_data_report_sync(struct hci_dev *hdev)
>  	bool enabled = hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED);
>  
>  	if (!(hdev->commands[18] & 0x08) ||
> -	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING))
> +	    !(hdev->features[0][6] & LMP_ERR_DATA_REPORTING) ||
> +	    test_bit(HCI_QUIRK_BROKEN_ERR_DATA_REPORTING, &hdev->quirks))
>  		return 0;
>  
>  	if (enabled == hdev->err_data_reporting)
> @@ -4475,6 +4477,9 @@ static const struct {
>  	HCI_QUIRK_BROKEN(STORED_LINK_KEY,
>  			 "HCI Delete Stored Link Key command is advertised, "
>  			 "but not supported."),
> +	HCI_QUIRK_BROKEN(ERR_DATA_REPORTING,
> +			 "HCI Read Default Erroneous Data Reporting command is "
> +			 "advertised, but not supported."),
>  	HCI_QUIRK_BROKEN(READ_TRANSMIT_POWER,
>  			 "HCI Read Transmit Power Level command is advertised, "
>  			 "but not supported."),

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Luiz Augusto von Dentz]

Hi Ismael,

On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
<swyterzone@gmail.com> wrote:
>
> A few users have reported that their cloned Chinese dongle doesn't
> work well with the hack Hans de Goede added, that tries this
> off-on mechanism as a way to unfreeze them.
>
> It's still more than worthwhile to have it, as in the vast majority
> of cases it either completely brings dongles to life or just resets
> them harmlessly as it already happens during normal USB operation.
>
> This is nothing new and the controllers are expected to behave
> correctly. But yeah, go figure. :)
>
> For that unhappy minority we can easily handle this edge case by letting
> users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.

Don't really like the idea of adding module parameter for device
specific problem.

> I believe this is the most generic way of doing it, given the constraints
> and by still having a good out-of-the-box experience.
>
> No clone left behind.
>
> Cc: stable@vger.kernel.org
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Ismael Ferreras Morezuelas <swyterzone@gmail.com>
> ---
>  drivers/bluetooth/btusb.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 8f34bf195bae..d31d4f925463 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -34,6 +34,7 @@ static bool force_scofix;
>  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
>  static bool enable_poll_sync = IS_ENABLED(CONFIG_BT_HCIBTUSB_POLL_SYNC);
>  static bool reset = true;
> +static bool disable_fake_csr_forcesuspend_hack;
>
>  static struct usb_driver btusb_driver;
>
> @@ -2171,7 +2172,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 and force-suspending once...");
> +               bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; adding workarounds...");
>
>                 /* Generally these clones have big discrepancies between
>                  * advertised features and what's actually supported.
> @@ -2215,21 +2216,24 @@ static int btusb_setup_csr(struct hci_dev *hdev)
>                  * apply this initialization quirk to every controller that gets here,
>                  * it should be harmless. The alternative is to not work at all.
>                  */
> -               pm_runtime_allow(&data->udev->dev);
> +               if (!disable_fake_csr_forcesuspend_hack) {
> +                       bt_dev_warn(hdev, "CSR: Unbranded CSR clone detected; force-suspending once...");
> +                       pm_runtime_allow(&data->udev->dev);
>
> -               ret = pm_runtime_suspend(&data->udev->dev);
> -               if (ret >= 0)
> -                       msleep(200);
> -               else
> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
> +                       ret = pm_runtime_suspend(&data->udev->dev);
> +                       if (ret >= 0)
> +                               msleep(200);
> +                       else
> +                               bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");

Is this specific to Barrot 8041a02? Why don't we add a quirk then?

> -               pm_runtime_forbid(&data->udev->dev);
> +                       pm_runtime_forbid(&data->udev->dev);
>
> -               device_set_wakeup_capable(&data->udev->dev, false);
> +                       device_set_wakeup_capable(&data->udev->dev, false);
>
> -               /* Re-enable autosuspend if this was requested */
> -               if (enable_autosuspend)
> -                       usb_enable_autosuspend(data->udev);
> +                       /* Re-enable autosuspend if this was requested */
> +                       if (enable_autosuspend)
> +                               usb_enable_autosuspend(data->udev);
> +               }
>         }
>
>         kfree_skb(skb);
> @@ -4312,6 +4316,9 @@ MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
>  module_param(reset, bool, 0644);
>  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> +module_param(disable_fake_csr_forcesuspend_hack, bool, 0644);
> +MODULE_PARM_DESC(disable_fake_csr_forcesuspend_hack, "Don't indiscriminately force-suspend Chinese-cloned CSR dongles trying to unfreeze them");
> +
>  MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.org>");
>  MODULE_DESCRIPTION("Generic Bluetooth USB driver ver " VERSION);
>  MODULE_VERSION(VERSION);
> --
> 2.38.1
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Swyter]

On 09/11/2022 21:49, Luiz Augusto von Dentz wrote:
> Hi Ismael,
> 
> On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
> <swyterzone@gmail.com> wrote:
>>
>> A few users have reported that their cloned Chinese dongle doesn't
>> work well with the hack Hans de Goede added, that tries this
>> off-on mechanism as a way to unfreeze them.
>>
>> It's still more than worthwhile to have it, as in the vast majority
>> of cases it either completely brings dongles to life or just resets
>> them harmlessly as it already happens during normal USB operation.
>>
>> This is nothing new and the controllers are expected to behave
>> correctly. But yeah, go figure. :)
>>
>> For that unhappy minority we can easily handle this edge case by letting
>> users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.
> 
> Don't really like the idea of adding module parameter for device
> specific problem.

It's not for a single device, it's for a whole class of unnamed devices
that are currently screwed even after all this.


>> -               ret = pm_runtime_suspend(&data->udev->dev);
>> -               if (ret >= 0)
>> -                       msleep(200);
>> -               else
>> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
>> +                       ret = pm_runtime_suspend(&data->udev->dev);
>> +                       if (ret >= 0)
>> +                               msleep(200);
>> +                       else
>> +                               bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
> 
> Is this specific to Barrot 8041a02? Why don't we add a quirk then?
> 

We don't know how specific it is, we suspect the getting stuck thing happens with Barrot controllers,
but in this world of lasered-out counterfeit chip IDs you can never be sure. Unless someone decaps them.

Hans added that name because it's the closest thing we have, but this applies to a lot of chips.
So much that now we do the hack by default, for very good reasons.

So please reconsider, this closes the gap.

With this last patch we go from ~+90% to almost ~100%, as the rest of generic quirks we added
don't really hurt; even if a particular dongle only needs a few of the zoo of quirks we set,
it's alright if we vaccinate them against all of these, except some are "allergic"
against this particular "vaccine". Let people skip this one. :-)

You know how normal BT controllers are utterly and inconsistently broken, now imagine you have a whole host
of vendors reusing a VID/PID/version/subversion, masking as a CSR for bizarre reasons to avoid paying
any USB-IF fees, or whatever. That's what we are fighting against here.

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Luiz Augusto von Dentz]

Hi Swyter,

On Wed, Nov 9, 2022 at 1:30 PM Swyter <swyterzone@gmail.com> wrote:
>
> On 09/11/2022 21:49, Luiz Augusto von Dentz wrote:
> > Hi Ismael,
> >
> > On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
> > <swyterzone@gmail.com> wrote:
> >>
> >> A few users have reported that their cloned Chinese dongle doesn't
> >> work well with the hack Hans de Goede added, that tries this
> >> off-on mechanism as a way to unfreeze them.
> >>
> >> It's still more than worthwhile to have it, as in the vast majority
> >> of cases it either completely brings dongles to life or just resets
> >> them harmlessly as it already happens during normal USB operation.
> >>
> >> This is nothing new and the controllers are expected to behave
> >> correctly. But yeah, go figure. :)
> >>
> >> For that unhappy minority we can easily handle this edge case by letting
> >> users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.
> >
> > Don't really like the idea of adding module parameter for device
> > specific problem.
>
> It's not for a single device, it's for a whole class of unnamed devices
> that are currently screwed even after all this.
>
>
> >> -               ret = pm_runtime_suspend(&data->udev->dev);
> >> -               if (ret >= 0)
> >> -                       msleep(200);
> >> -               else
> >> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
> >> +                       ret = pm_runtime_suspend(&data->udev->dev);
> >> +                       if (ret >= 0)
> >> +                               msleep(200);
> >> +                       else
> >> +                               bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
> >
> > Is this specific to Barrot 8041a02? Why don't we add a quirk then?
> >
>
> We don't know how specific it is, we suspect the getting stuck thing happens with Barrot controllers,
> but in this world of lasered-out counterfeit chip IDs you can never be sure. Unless someone decaps them.
>
> Hans added that name because it's the closest thing we have, but this applies to a lot of chips.
> So much that now we do the hack by default, for very good reasons.
>
> So please reconsider, this closes the gap.
>
> With this last patch we go from ~+90% to almost ~100%, as the rest of generic quirks we added
> don't really hurt; even if a particular dongle only needs a few of the zoo of quirks we set,
> it's alright if we vaccinate them against all of these, except some are "allergic"
> against this particular "vaccine". Let people skip this one. :-)
>
> You know how normal BT controllers are utterly and inconsistently broken, now imagine you have a whole host
> of vendors reusing a VID/PID/version/subversion, masking as a CSR for bizarre reasons to avoid paying
> any USB-IF fees, or whatever. That's what we are fighting against here.

I see, but for suspend in particular, can't we actually handle it
somehow? I mean if we can detect the controller is getting stuck and
print some information and flip the quirk? Otherwise Im afraid this
parameter will end up always being set by distros to avoid suspend
problems.

-- 
Luiz Augusto von Dentz

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Swyter]

On 09/11/2022 23:39, Luiz Augusto von Dentz wrote:
>>> Is this specific to Barrot 8041a02? Why don't we add a quirk then?
>>>
>>
>> We don't know how specific it is, we suspect the getting stuck thing happens with Barrot controllers,
>> but in this world of lasered-out counterfeit chip IDs you can never be sure. Unless someone decaps them.
>>
>> Hans added that name because it's the closest thing we have, but this applies to a lot of chips.
>> So much that now we do the hack by default, for very good reasons.
>>
>> So please reconsider, this closes the gap.
>>
>> With this last patch we go from ~+90% to almost ~100%, as the rest of generic quirks we added
>> don't really hurt; even if a particular dongle only needs a few of the zoo of quirks we set,
>> it's alright if we vaccinate them against all of these, except some are "allergic"
>> against this particular "vaccine". Let people skip this one. :-)
>>
>> You know how normal BT controllers are utterly and inconsistently broken, now imagine you have a whole host
>> of vendors reusing a VID/PID/version/subversion, masking as a CSR for bizarre reasons to avoid paying
>> any USB-IF fees, or whatever. That's what we are fighting against here.
> 
> I see, but for suspend in particular, can't we actually handle it
> somehow? I mean if we can detect the controller is getting stuck and
> print some information and flip the quirk? Otherwise Im afraid this
> parameter will end up always being set by distros to avoid suspend
> problems.

Maybe, auto-detection is certainly a better way and a potential improvement,
assuming we cover all the edge cases. Which I'm not too sure about.

The controllers don't get totally stuck, they just act weird and give funky
HCI responses at certain points if we don't do this, if I remember correctly.

Unfortunately I can't really justify spending that much time *right now* on
this hobby project. Distros should *definitely* keep doing the hack by default
if they want the widest compatibility. This is comparatively a niche issue.


But yeah, to sum things up; I'm not sure going back to a whitelist of a
whitelist is a good idea without a foolproof method. We want the widest
possible reach by doing it in the most generic way with the smallest
possible side effects. If we can find a way to blacklist this quirk
when we are super sure it's going to cause issues I'm all for it.

Right now I think this is an acceptable solution, as long as
people in charge of distros don't flip these toggles.

Nobody has cared until now, barely any of these devices worked.
Now that most of them work it would be very funny to see
them break the majority to fix a few.

With the amount of effort it takes an outsider like me to
get stuff into the kernel and fight to avoid random reverts
I don't know if I'd be able to get something as involved as
that to work in a satisfying, automatic and simple way.

Perfect is the enemy of good, diminishing returns, and all that. ¯\_(ツ)_/¯

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Hans de Goede]

Hi Luiz,

On 11/9/22 23:39, Luiz Augusto von Dentz wrote:
> Hi Swyter,
> 
> On Wed, Nov 9, 2022 at 1:30 PM Swyter <swyterzone@gmail.com> wrote:
>>
>> On 09/11/2022 21:49, Luiz Augusto von Dentz wrote:
>>> Hi Ismael,
>>>
>>> On Sat, Oct 29, 2022 at 1:25 PM Ismael Ferreras Morezuelas
>>> <swyterzone@gmail.com> wrote:
>>>>
>>>> A few users have reported that their cloned Chinese dongle doesn't
>>>> work well with the hack Hans de Goede added, that tries this
>>>> off-on mechanism as a way to unfreeze them.
>>>>
>>>> It's still more than worthwhile to have it, as in the vast majority
>>>> of cases it either completely brings dongles to life or just resets
>>>> them harmlessly as it already happens during normal USB operation.
>>>>
>>>> This is nothing new and the controllers are expected to behave
>>>> correctly. But yeah, go figure. :)
>>>>
>>>> For that unhappy minority we can easily handle this edge case by letting
>>>> users disable it via our «btusb.disable_fake_csr_forcesuspend_hack=1» kernel option.
>>>
>>> Don't really like the idea of adding module parameter for device
>>> specific problem.
>>
>> It's not for a single device, it's for a whole class of unnamed devices
>> that are currently screwed even after all this.
>>
>>
>>>> -               ret = pm_runtime_suspend(&data->udev->dev);
>>>> -               if (ret >= 0)
>>>> -                       msleep(200);
>>>> -               else
>>>> -                       bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
>>>> +                       ret = pm_runtime_suspend(&data->udev->dev);
>>>> +                       if (ret >= 0)
>>>> +                               msleep(200);
>>>> +                       else
>>>> +                               bt_dev_warn(hdev, "CSR: Couldn't suspend the device for our Barrot 8041a02 receive-issue workaround");
>>>
>>> Is this specific to Barrot 8041a02? Why don't we add a quirk then?
>>>
>>
>> We don't know how specific it is, we suspect the getting stuck thing happens with Barrot controllers,
>> but in this world of lasered-out counterfeit chip IDs you can never be sure. Unless someone decaps them.
>>
>> Hans added that name because it's the closest thing we have, but this applies to a lot of chips.
>> So much that now we do the hack by default, for very good reasons.
>>
>> So please reconsider, this closes the gap.
>>
>> With this last patch we go from ~+90% to almost ~100%, as the rest of generic quirks we added
>> don't really hurt; even if a particular dongle only needs a few of the zoo of quirks we set,
>> it's alright if we vaccinate them against all of these, except some are "allergic"
>> against this particular "vaccine". Let people skip this one. :-)
>>
>> You know how normal BT controllers are utterly and inconsistently broken, now imagine you have a whole host
>> of vendors reusing a VID/PID/version/subversion, masking as a CSR for bizarre reasons to avoid paying
>> any USB-IF fees, or whatever. That's what we are fighting against here.
> 
> I see, but for suspend in particular, can't we actually handle it
> somehow?

Note this is not about suspend. This is about a workaround where
we runtime-suspend the HCI once, before initializing it and
then actually disable runtime suspend (unless BT is turned off)
because the USB remote wakeup functionality is also broken
on these.

IIRC the problem was that without the runtime suspend the HCI
seems to work, but any events from the HCI to the host which
are not direct responses to a request from the host, like a known
previously paired BT device trying to connect would not get
registered by the HCI.

At least not until the host actually requested something from
the HCI, e.g. starting a scan for new devices would all of
a sudden cause the known paired device to show up.

My theory here is that during init the Windows drivers at
least runtime suspend the HCI once (or leave it idle
long enough for Windows to do this) and somehow these clones
rely on that for setting up some of their host notification
functionality.

> I mean if we can detect the controller is getting stuck and
> print some information and flip the quirk?

Detection of devices which need the "runtime-suspend once"
workaround is almost impossible since the problem is the
lack of unsolicited messages from the HCI, which can be
normal if no BT "clients" are in use during probe.

> Otherwise Im afraid this
> parameter will end up always being set by distros to avoid suspend
> problems.

The flag is Swyter is suggesting is actually to disable
the workaround, which is currently enabled for all
USB BT dongles identified as being a CSR clone.

Most clones work fine with it, with many needing it, but some clones
seem to not like the workaround and behave undesirable if we runtime
suspend them once. So the flag is to disable the workaround to make
these last 5% of these (really cheap, bottom of the barrel quality)
clones work.

Since 95% of the clones do work with the workaround distro's enabling
the flag by default would be a bad idea and I don't expect them to
do this.

Regards,

Hans

Re: [PATCH 3/3] Bluetooth: btusb: Add a parameter to let users disable the fake CSR force-suspend hack

[Ismael Ferreras Morezuelas]

On 09/11/2022 23:39, Luiz Augusto von Dentz wrote:
> I see, but for suspend in particular, can't we actually handle it
> somehow? I mean if we can detect the controller is getting stuck and
> print some information and flip the quirk? Otherwise Im afraid this
> parameter will end up always being set by distros to avoid suspend
> problems.

Hi, Luiz. I haven't seen any movement about the [3/3] patch since a few weeks ago.

Given what Hans clarified in his reply, I wondered if you or any of the other
Bluetooth maintainers have changed opinions about including this in some form.

I'm a kernel development newbie, so I'm not good at this. I don't know if I should
do anything else, wait a bit more, or just drop this. Thanks in advance. :)