diff mbox series

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

Message ID 20221029202454.25651-1-swyterzone@gmail.com
State Accepted
Commit 14026a4ed2758d228e63bbab44fb8decf3956057
Headers show
Series [1/3] Bluetooth: btusb: Fix Chinese CSR dongles again by re-adding ERR_DATA_REPORTING quirk | expand

Commit Message

Ismael Ferreras Morezuelas Oct. 29, 2022, 8:24 p.m. UTC 
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(-)

Comments

bluez.test.bot@gmail.com Oct. 29, 2022, 9:09 p.m. UTC | #1 
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
Hans de Goede Oct. 30, 2022, 9:59 a.m. UTC | #2 
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."),
diff mbox series

Patch

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."),