Bluetooth: HCI: Add HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk
Subsystems (2)
Luiz Augusto von Dentz
maintainerPATCH v4 1/4 Bluetooth: HCI: Add HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk
From: Luiz Augusto von Dentz <[email protected]>
This adds HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk which can be used to mark HCI_Enhanced_Setup_Synchronous_Connection as broken even if its support command bit are set since some controller report it as supported but the command don't work properly with some configurations (e.g. BT_VOICE_TRANSPARENT/mSBC).
---
v4: Address latest comments, add patch 4/4 spliting hci_dev_open_sync so each stage can be handled by its own function.
v3: Make HCI_QUIRK_BROKEN macro expand quirk name and address other comments from v2.
v2: Change hci_broken/hci_quirk_broken and HCI_BROKEN/HCI_QUIRK_BROKEN and add patch 3/3 setting HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA controllers.
include/net/bluetooth/hci.h | 9 +++++++++ include/net/bluetooth/hci_core.h | 8 ++++++-- net/bluetooth/hci_conn.c | 2 +- net/bluetooth/sco.c | 2 +- 4 files changed, 17 insertions(+), 4 deletions(-)
include/net/bluetooth/hci.h +9 -0
@@ -265,6 +265,15 @@ enum {* runtime suspend, because event filtering takes place there. */ HCI_QUIRK_BROKEN_FILTER_CLEAR_ALL , + +/* +* When this quirk is set, disables the use of +* HCI_OP_ENHANCED_SETUP_SYNC_CONN command to setup SCO connections. +* +* This quirk can be set before hci_register_dev is called or +* during the hdev->setup vendor callback. +*/ + HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN, };/* HCI device flags */
include/net/bluetooth/hci_core.h +6 -2
@@ -1492,8 +1492,12 @@ void hci_conn_del_sysfs(struct hci_conn *conn);#define privacy_mode_capable (dev) (use_ll_privacy(dev) && \ (hdev->commands[39] & 0x04)) -/* Use enhanced synchronous connection if command is supported */ -#define enhanced_sco_capable (dev) ((dev)->commands[29] & 0x08) +/* Use enhanced synchronous connection if command is supported and its quirk +* has not been set. +*/ +#define enhanced_sync_conn_capable (dev) \ + (((dev)->commands[29] & 0x08) && \ + !test_bit(HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN, &(dev)->quirks))/* Use ext scanning if set ext scan param and ext scan enable is supported */ #define use_ext_scan (dev) (((dev)->commands[37] & 0x20) && \
net/bluetooth/hci_conn.c +1 -1
@@ -481,7 +481,7 @@ static bool hci_setup_sync_conn(struct hci_conn *conn, __u16 handle)bool hci_setup_sync (struct hci_conn *conn,__u16 handle) { -if (enhanced_sco_capable (conn->hdev)) +if (enhanced_sync_conn_capable (conn->hdev))return hci_enhanced_setup_sync_conn (conn, handle);return hci_setup_sync_conn (conn, handle);
net/bluetooth/sco.c +1 -1
@@ -885,7 +885,7 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname, err = -EBADFD ;break ; } -if (enhanced_sco_capable (hdev) && +if (enhanced_sync_conn_capable (hdev) && voice.setting ==BT_VOICE_TRANSPARENT )sco_pi (sk)->codec.id =BT_CODEC_TRANSPARENT ;hci_dev_put (hdev);
Signature · 1 line
2.35.1
Luiz Augusto von Dentz
maintainerPATCH v4 2/4 Bluetooth: Print broken quirks
From: Luiz Augusto von Dentz <[email protected]>
This prints warnings for controllers setting broken quirks to increase their visibility and warn about broken controllers firmware that probably needs updates to behave properly.
--- net/bluetooth/hci_sync.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
net/bluetooth/hci_sync.c +31 -0
@@ -3825,6 +3825,30 @@ static int hci_init_sync(struct hci_dev *hdev)return 0 ; } +#define HCI_QUIRK_BROKEN (_quirk, _desc) { HCI_QUIRK_BROKEN_##_quirk, _desc } + +static const struct { +unsigned long quirk; +const char *desc; +} hci_broken_table[] = { +HCI_QUIRK_BROKEN (LOCAL_COMMANDS , +"HCI Read Local Supported Commands not supported" ), +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." ), +HCI_QUIRK_BROKEN (FILTER_CLEAR_ALL , +"HCI Set Event Filter command not supported." ), +HCI_QUIRK_BROKEN (ENHANCED_SETUP_SYNC_CONN , +"HCI Enhanced Setup Synchronous Connection command is " +"advertised, but not supported." ) +}; +int hci_dev_open_sync (struct hci_dev *hdev) {int ret =0 ;
@@ -3886,12 +3910,19 @@ int hci_dev_open_sync(struct hci_dev *hdev)if (hci_dev_test_flag (hdev,HCI_SETUP ) ||test_bit (HCI_QUIRK_NON_PERSISTENT_SETUP , &hdev->quirks)) {bool invalid_bdaddr; +size_t i;hci_sock_dev_event (hdev,HCI_DEV_SETUP );if (hdev->setup) ret = hdev->setup (hdev); +for (i =0 ; i <ARRAY_SIZE (hci_broken_table); i++) { +if (test_bit (hci_broken_table[i].quirk, &hdev->quirks)) +bt_dev_warn (hdev,"%s" , + hci_broken_table[i].desc); + } +/* The transport driver can set the quirk to mark the * BD_ADDR invalid before creating the HCI device or in * its setup callback.
Signature · 1 line
2.35.1
Luiz Augusto von Dentz
maintainerPATCH v4 3/4 Bluetooth: btusb: Set HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA
In reply to Luiz Augusto von DentzFrom: Luiz Augusto von Dentz <[email protected]>
This sets HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA controllers since SCO appear to not work when using HCI_OP_ENHANCED_SETUP_SYNC_CONN.
--- drivers/bluetooth/btusb.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/bluetooth/btusb.c +5 -0
@@ -3335,6 +3335,11 @@ static int btusb_setup_qca(struct hci_dev *hdev)msleep (QCA_BT_RESET_WAIT_MS ); } +/* Mark HCI_OP_ENHANCED_SETUP_SYNC_CONN as broken as it doesn't seem to +* work with the likes of HSP/HFP mSBC. +*/ +set_bit (HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN , &hdev->quirks); +return 0 ; }
Signature · 1 line
2.35.1
Luiz Augusto von Dentz
maintainerPATCH v4 4/4 Bluetooth: hci_sync: Split hci_dev_open_sync
In reply to Luiz Augusto von DentzFrom: Luiz Augusto von Dentz <[email protected]>
This splits hci_dev_open_sync so each stage is handle by its own function so it is easier to identify each stage.
--- net/bluetooth/hci_sync.c | 225 ++++++++++++++++++++++----------------- 1 file changed, 126 insertions(+), 99 deletions(-)
net/bluetooth/hci_sync.c +126 -99
@@ -3849,90 +3849,40 @@ static const struct { "advertised, but not supported.") }; -int hci_dev_open_sync (struct hci_dev *hdev) +/* This function handles hdev setup stage: +* +* Calls hdev->setup +* Setup address if HCI_QUIRK_USE_BDADDR_PROPERTY is set. +*/ +static int hci_dev_setup_sync (struct hci_dev *hdev) {int ret =0 ; - -bt_dev_dbg (hdev,"" ); - -if (hci_dev_test_flag (hdev,HCI_UNREGISTER )) { - ret = -ENODEV ; - goto done; - } +bool invalid_bdaddr; +size_t i;if (!hci_dev_test_flag (hdev,HCI_SETUP ) && - !hci_dev_test_flag (hdev,HCI_CONFIG )) { -/* Check for rfkill but allow the HCI setup stage to -* proceed (which in itself doesn't cause any RF activity). -*/ -if (hci_dev_test_flag (hdev,HCI_RFKILLED )) { - ret = -ERFKILL ; - goto done; - } - -/* Check for valid public address or a configured static -* random address, but let the HCI setup proceed to -* be able to determine if there is a public address -* or not. -* -* In case of user channel usage, it is not important -* if a public address or static random address is -* available. -* -* This check is only valid for BR/EDR controllers -* since AMP controllers do not have an address. -*/ -if (!hci_dev_test_flag (hdev,HCI_USER_CHANNEL ) && - hdev->dev_type ==HCI_PRIMARY && - !bacmp (&hdev->bdaddr,BDADDR_ANY ) && - !bacmp (&hdev->static_addr,BDADDR_ANY )) { - ret = -EADDRNOTAVAIL ; - goto done; - } - } - -if (test_bit (HCI_UP , &hdev->flags)) { - ret = -EALREADY ; - goto done; - } - -if (hdev->open (hdev)) { - ret = -EIO ; - goto done; - } - -set_bit (HCI_RUNNING , &hdev->flags); -hci_sock_dev_event (hdev,HCI_DEV_OPEN ); - -atomic_set (&hdev->cmd_cnt,1 ); -set_bit (HCI_INIT , &hdev->flags); - -if (hci_dev_test_flag (hdev,HCI_SETUP ) || -test_bit (HCI_QUIRK_NON_PERSISTENT_SETUP , &hdev->quirks)) { -bool invalid_bdaddr; -size_t i; + !test_bit (HCI_QUIRK_NON_PERSISTENT_SETUP , &hdev->quirks)) +return 0 ; -hci_sock_dev_event (hdev,HCI_DEV_SETUP ); +bt_dev_dbg (hdev,"" ); -if (hdev->setup) - ret = hdev->setup (hdev); +hci_sock_dev_event (hdev,HCI_DEV_SETUP ); -for (i =0 ; i <ARRAY_SIZE (hci_broken_table); i++) { -if (test_bit (hci_broken_table[i].quirk, &hdev->quirks)) -bt_dev_warn (hdev,"%s" , - hci_broken_table[i].desc); - } +if (hdev->setup) + ret = hdev->setup (hdev); -/* The transport driver can set the quirk to mark the -* BD_ADDR invalid before creating the HCI device or in -* its setup callback. -*/ - invalid_bdaddr =test_bit (HCI_QUIRK_INVALID_BDADDR , - &hdev->quirks); +for (i =0 ; i <ARRAY_SIZE (hci_broken_table); i++) { +if (test_bit (hci_broken_table[i].quirk, &hdev->quirks)) +bt_dev_warn (hdev,"%s" , hci_broken_table[i].desc); + } -if (ret) - goto setup_failed; +/* The transport driver can set the quirk to mark the +* BD_ADDR invalid before creating the HCI device or in +* its setup callback. +*/ + invalid_bdaddr =test_bit (HCI_QUIRK_INVALID_BDADDR , &hdev->quirks); +if (!ret) {if (test_bit (HCI_QUIRK_USE_BDADDR_PROPERTY , &hdev->quirks)) {if (!bacmp (&hdev->public_addr,BDADDR_ANY ))hci_dev_get_bd_addr_from_property (hdev);
@@ -3951,33 +3901,51 @@ int hci_dev_open_sync(struct hci_dev *hdev) invalid_bdaddr = false; } } + } -setup_failed: -/* The transport driver can set these quirks before -* creating the HCI device or in its setup callback. -* -* For the invalid BD_ADDR quirk it is possible that -* it becomes a valid address if the bootloader does -* provide it (see above). -* -* In case any of them is set, the controller has to -* start up as unconfigured. -*/ -if (test_bit (HCI_QUIRK_EXTERNAL_CONFIG , &hdev->quirks) || - invalid_bdaddr) -hci_dev_set_flag (hdev,HCI_UNCONFIGURED ); +/* The transport driver can set these quirks before +* creating the HCI device or in its setup callback. +* +* For the invalid BD_ADDR quirk it is possible that +* it becomes a valid address if the bootloader does +* provide it (see above). +* +* In case any of them is set, the controller has to +* start up as unconfigured. +*/ +if (test_bit (HCI_QUIRK_EXTERNAL_CONFIG , &hdev->quirks) || + invalid_bdaddr) +hci_dev_set_flag (hdev,HCI_UNCONFIGURED ); -/* For an unconfigured controller it is required to -* read at least the version information provided by -* the Read Local Version Information command. -* -* If the set_bdaddr driver callback is provided, then -* also the original Bluetooth public device address -* will be read using the Read BD Address command. -*/ -if (hci_dev_test_flag (hdev,HCI_UNCONFIGURED )) - ret =hci_unconf_init_sync (hdev); - } +/* For an unconfigured controller it is required to +* read at least the version information provided by +* the Read Local Version Information command. +* +* If the set_bdaddr driver callback is provided, then +* also the original Bluetooth public device address +* will be read using the Read BD Address command. +*/ +if (hci_dev_test_flag (hdev,HCI_UNCONFIGURED )) +return hci_unconf_init_sync (hdev); + +return ret; +} + +/* This function handles hdev init stage: +* +* Calls hci_dev_setup_sync to perform setup stage +* Calls hci_init_sync to perform HCI command init sequence +*/ +static int hci_dev_init_sync (struct hci_dev *hdev) +{ +int ret; + +bt_dev_dbg (hdev,"" ); + +atomic_set (&hdev->cmd_cnt,1 ); +set_bit (HCI_INIT , &hdev->flags); + + ret =hci_dev_setup_sync (hdev);if (hci_dev_test_flag (hdev,HCI_CONFIG )) {/* If public address change is configured, ensure that
@@ -4017,6 +3985,65 @@ int hci_dev_open_sync(struct hci_dev *hdev)clear_bit (HCI_INIT , &hdev->flags); +return ret; +} + +int hci_dev_open_sync (struct hci_dev *hdev) +{ +int ret; + +bt_dev_dbg (hdev,"" ); + +if (hci_dev_test_flag (hdev,HCI_UNREGISTER )) { + ret = -ENODEV ; + goto done; + } + +if (!hci_dev_test_flag (hdev,HCI_SETUP ) && + !hci_dev_test_flag (hdev,HCI_CONFIG )) { +/* Check for rfkill but allow the HCI setup stage to +* proceed (which in itself doesn't cause any RF activity). +*/ +if (hci_dev_test_flag (hdev,HCI_RFKILLED )) { + ret = -ERFKILL ; + goto done; + } + +/* Check for valid public address or a configured static +* random address, but let the HCI setup proceed to +* be able to determine if there is a public address +* or not. +* +* In case of user channel usage, it is not important +* if a public address or static random address is +* available. +* +* This check is only valid for BR/EDR controllers +* since AMP controllers do not have an address. +*/ +if (!hci_dev_test_flag (hdev,HCI_USER_CHANNEL ) && + hdev->dev_type ==HCI_PRIMARY && + !bacmp (&hdev->bdaddr,BDADDR_ANY ) && + !bacmp (&hdev->static_addr,BDADDR_ANY )) { + ret = -EADDRNOTAVAIL ; + goto done; + } + } + +if (test_bit (HCI_UP , &hdev->flags)) { + ret = -EALREADY ; + goto done; + } + +if (hdev->open (hdev)) { + ret = -EIO ; + goto done; + } + +set_bit (HCI_RUNNING , &hdev->flags); +hci_sock_dev_event (hdev,HCI_DEV_OPEN ); + + ret =hci_dev_init_sync (hdev);if (!ret) {hci_dev_hold (hdev);hci_dev_set_flag (hdev,HCI_RPA_EXPIRED );
Signature · 1 line
2.35.1
Marcel Holtmann
maintainerRe: [PATCH v4 4/4] Bluetooth: hci_sync: Split hci_dev_open_sync
Hi Luiz,
Quoted 7 lines
This splits
hci_dev_open_syncso each stage is handle by its own function so it is easier to identify each stage.Signed-off-by Luiz Augusto von Dentz <[email protected]>---
net/bluetooth/hci_sync.c| 225 ++++++++++++++++++++++-----------------1 file changed, 126 insertions(+), 99 deletions(-)
can you try to create the diff with --patience or --histogram so that patch can be reviewed.
Regards
Marcel
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=628357
---Test result---
Test Summary:
CheckPatch FAIL 3.82 seconds GitLint PASS 4.00 seconds SubjectPrefix PASS 3.54 seconds BuildKernel PASS 30.56 seconds BuildKernel32 PASS 27.22 seconds Incremental Build with patchesPASS 57.55 seconds TestRunner: Setup PASS 453.98 seconds TestRunner: l2cap-tester PASS 14.95 seconds TestRunner: bnep-tester PASS 5.76 seconds TestRunner: mgmt-tester PASS 95.91 seconds TestRunner: rfcomm-tester PASS 7.36 seconds TestRunner: sco-tester PASS 7.14 seconds TestRunner: smp-tester PASS 7.14 seconds TestRunner: userchan-tester PASS 5.90 seconds
Details
##############################
Test: CheckPatch - FAIL - 3.82 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v4,2/4] Bluetooth: Print broken quirks\WARNING:SPLIT_STRING: quoted string split across lines #109: FILE: net/bluetooth/hci_sync.c:3838:
+ "HCI Delete Stored Link Key command is advertised, "
+ "but not supported."),
WARNING:SPLIT_STRING: quoted string split across lines #112: FILE: net/bluetooth/hci_sync.c:3841:
+ "HCI Read Default Erroneous Data Reporting command is "
+ "advertised, but not supported."),
WARNING:SPLIT_STRING: quoted string split across lines #115: FILE: net/bluetooth/hci_sync.c:3844:
+ "HCI Read Transmit Power Level command is advertised, "
+ "but not supported."),
WARNING:SPLIT_STRING: quoted string split across lines #120: FILE: net/bluetooth/hci_sync.c:3849:
+ "HCI Enhanced Setup Synchronous Connection command is "
+ "advertised, but not supported.")
total: 0 errors, 4 warnings, 0 checks, 49 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/12798857.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.
---
Regards,
Linux Bluetooth
This series was applied to bluetooth/bluetooth-next.git (master) by Marcel Holtmann <[email protected]>:
Hello:
This series was applied to bluetooth/bluetooth-next.git (master) by Marcel Holtmann <[email protected]>:
Quoted 9 lines · On Fri, 1 Apr 2022 16:38:23 -0700 you wrote:
From: Luiz Augusto von Dentz <[email protected]>
This adds
HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONNquirk which can be used to markHCI_Enhanced_Setup_Synchronous_Connectionas broken even if its support command bit are set since some controller report it as supported but the command don't work properly with some configurations (e.g. BT_VOICE_TRANSPARENT/mSBC).[...]
Here is the summary with links:
- [v4,1/4] Bluetooth: HCI: Add HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN quirk
https://git.kernel.org/bluetooth/bluetooth-next/c/c625b467f47c - [v4,2/4] Bluetooth: Print broken quirks https://git.kernel.org/bluetooth/bluetooth-next/c/fa5cd0fd5bdf - [v4,3/4] Bluetooth: btusb: Set HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA https://git.kernel.org/bluetooth/bluetooth-next/c/38a1944deda4
- [v4,4/4] Bluetooth: hci_sync: Split hci_dev_open_sync (no matching commit)
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Hi, this is your Linux kernel regression tracker.
Quoted 6 lines · On 05.04.22 19:30, [email protected] wrote:
This series was applied to bluetooth/bluetooth-next.git (master) by Marcel Holtmann <[email protected]>:
Quoted 1 line · On Fri, 1 Apr 2022 16:38:23 -0700 you wrote:
From: Luiz Augusto von Dentz <[email protected]>
BTW: Thx for taking care of this, Luiz!
Quoted 15 lines
Quoted 7 lines
This adds
HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONNquirk which can be used to markHCI_Enhanced_Setup_Synchronous_Connectionas broken even if its support command bit are set since some controller report it as supported but the command don't work properly with some configurations (e.g. BT_VOICE_TRANSPARENT/mSBC).[...]
Here is the summary with links:
- [v4,1/4] Bluetooth: HCI: AddHCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONNquirkhttps://git.kernel.org/bluetooth/bluetooth-next/c/c625b467f47c - [v4,2/4] Bluetooth: Print broken quirks https://git.kernel.org/bluetooth/bluetooth-next/c/fa5cd0fd5bdf - [v4,3/4] Bluetooth: btusb: Set HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN for QCA https://git.kernel.org/bluetooth/bluetooth-next/c/38a1944deda4
Quoted 4 lines
- [v4,4/4] Bluetooth:
hci_sync: Splithci_dev_open_sync(no matching commit)You are awesome, thank you!
Hey, this patchset is fixing a regression from 5.16 (see patch 3/4), but apparently is only scheduled to go upstream in the next cycle, as it was applied to bluetooth-next.git. The patches also seem to miss tags to get them backported to stable. Is that intentional? At least patches 1, 2, and 3 don't look that complex, why can't they get in this cycle and get backported to stable soon to finally provide a solution for users that are plagued by this for weeks already?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them and lack knowledge about most of the areas they concern. I thus unfortunately will sometimes get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight.