[RFC] Bluetooth: don't use ESCO setup for BT_VOICE

Commit Message

Pauli Virtanen Feb. 27, 2022, 4:34 p.m. UTC

According to user reports, how HCI_Enhanced_Setup_Synchronous_Connection
is currently used to establish MSBC connections results to broken audio
on some adapters (QCA6174, mt7921e).

Revert to previous behavior of using HCI_Setup_Synchronous_Connection,
unless the user has explicitly set BT_CODEC sockopt. Since bt_codec
contents come from Core specification, use a separate flag for the
setting.

Fixes: b2af264ad3af ("Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215576
Signed-off-by: Pauli Virtanen <[e-mail]>
---

Notes:
    Maybe we want to use the ESCO connect setup only when userspace has
    requested the codec offload support.  I don't have any of the broken
    hardware myself, so this is not tested on them.
    
    Alternatively, there should be some driver quirk to indicate the
    enhanced sco connection setup is not broken.

 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_conn.c         | 10 ++++++++--
 net/bluetooth/sco.c              | 22 +++++++---------------
 3 files changed, 16 insertions(+), 17 deletions(-)

Comments

[e-mail] Feb. 27, 2022, 5:16 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=618448

---Test result---

Test Summary:
CheckPatch                    PASS      2.01 seconds
GitLint                       FAIL      1.07 seconds
SubjectPrefix                 PASS      0.86 seconds
BuildKernel                   PASS      34.67 seconds
BuildKernel32                 PASS      30.46 seconds
Incremental Build with patchesPASS      41.73 seconds
TestRunner: Setup             PASS      544.79 seconds
TestRunner: l2cap-tester      PASS      15.36 seconds
TestRunner: bnep-tester       PASS      6.82 seconds
TestRunner: mgmt-tester       PASS      119.15 seconds
TestRunner: rfcomm-tester     PASS      9.02 seconds
TestRunner: sco-tester        PASS      8.56 seconds
TestRunner: smp-tester        PASS      8.42 seconds
TestRunner: userchan-tester   PASS      7.03 seconds

Details
##############################
Test: GitLint - FAIL - 1.07 seconds
Run gitlint with rule in .gitlint
[RFC] Bluetooth: don't use ESCO setup for BT_VOICE
20: B2 Line has trailing whitespace: "    "




---
Regards,
Linux Bluetooth

Marcel Holtmann Feb. 27, 2022, 5:36 p.m. UTC | #2

Hi Pauli,

> According to user reports, how HCI_Enhanced_Setup_Synchronous_Connection
> is currently used to establish MSBC connections results to broken audio
> on some adapters (QCA6174, mt7921e).
> 
> Revert to previous behavior of using HCI_Setup_Synchronous_Connection,
> unless the user has explicitly set BT_CODEC sockopt. Since bt_codec
> contents come from Core specification, use a separate flag for the
> setting.
> 
> Fixes: b2af264ad3af ("Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215576
> Signed-off-by: Pauli Virtanen <[e-mail]>
> ---
> 
> Notes:
>    Maybe we want to use the ESCO connect setup only when userspace has
>    requested the codec offload support.  I don't have any of the broken
>    hardware myself, so this is not tested on them.
> 
>    Alternatively, there should be some driver quirk to indicate the
>    enhanced sco connection setup is not broken.

yes, these needs to be marked as my hardware is broken.

From a specification point of view, the Enhanced Setup Sync Conn is a super set of the Setup Sync Conn (which is a super set of Add SCO). If implementers screwed that up, we clear spell that out.

Maybe we messed up the usage of the Enhanced Setup Sync Conn and that should be of course checked. I hope the hardware manufactures can chime in here. Or provide firmware updates.

Regards

Marcel

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index dd8840e70e25..3a6a3b80368c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -717,6 +717,7 @@  struct hci_conn {
 
 	struct hci_conn	*link;
 	struct bt_codec codec;
+	bool		esco_setup;
 
 	void (*connect_cfm_cb)	(struct hci_conn *conn, u8 status);
 	void (*security_cfm_cb)	(struct hci_conn *conn, u8 status);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index bd669c95b9a7..0aa7f822de32 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -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_sco_capable(conn->hdev) && conn->esco_setup)
 		return hci_enhanced_setup_sync_conn(conn, handle);
 
 	return hci_setup_sync_conn(conn, handle);
@@ -1477,7 +1477,13 @@  struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	hci_conn_hold(sco);
 
 	sco->setting = setting;
-	sco->codec = *codec;
+	if (codec) {
+		sco->codec = *codec;
+		sco->esco_setup = true;
+	} else {
+		memset(&sco->codec, 0, sizeof(sco->codec));
+		sco->esco_setup = false;
+	}
 
 	if (acl->state == BT_CONNECTED &&
 	    (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 8eabf41b2993..e78063d65c1a 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -70,6 +70,7 @@  struct sco_pinfo {
 	__u16		setting;
 	__u8		cmsg_mask;
 	struct bt_codec codec;
+	bool		esco_setup;
 	struct sco_conn	*conn;
 };
 
@@ -239,6 +240,7 @@  static int sco_connect(struct hci_dev *hdev, struct sock *sk)
 {
 	struct sco_conn *conn;
 	struct hci_conn *hcon;
+	struct bt_codec *codec;
 	int err, type;
 
 	BT_DBG("%pMR -> %pMR", &sco_pi(sk)->src, &sco_pi(sk)->dst);
@@ -252,8 +254,9 @@  static int sco_connect(struct hci_dev *hdev, struct sock *sk)
 	    (!lmp_transp_capable(hdev) || !lmp_esco_capable(hdev)))
 		return -EOPNOTSUPP;
 
+	codec = sco_pi(sk)->esco_setup ? &sco_pi(sk)->codec : NULL;
 	hcon = hci_connect_sco(hdev, type, &sco_pi(sk)->dst,
-			       sco_pi(sk)->setting, &sco_pi(sk)->codec);
+			       sco_pi(sk)->setting, codec);
 	if (IS_ERR(hcon))
 		return PTR_ERR(hcon);
 
@@ -496,10 +499,7 @@  static struct sock *sco_sock_alloc(struct net *net, struct socket *sock,
 	sk->sk_state    = BT_OPEN;
 
 	sco_pi(sk)->setting = BT_VOICE_CVSD_16BIT;
-	sco_pi(sk)->codec.id = BT_CODEC_CVSD;
-	sco_pi(sk)->codec.cid = 0xffff;
-	sco_pi(sk)->codec.vid = 0xffff;
-	sco_pi(sk)->codec.data_path = 0x00;
+	sco_pi(sk)->esco_setup = false;
 
 	bt_sock_link(&sco_sk_list, sk);
 	return sk;
@@ -879,16 +879,7 @@  static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 		}
 
 		sco_pi(sk)->setting = voice.setting;
-		hdev = hci_get_route(&sco_pi(sk)->dst, &sco_pi(sk)->src,
-				     BDADDR_BREDR);
-		if (!hdev) {
-			err = -EBADFD;
-			break;
-		}
-		if (enhanced_sco_capable(hdev) &&
-		    voice.setting == BT_VOICE_TRANSPARENT)
-			sco_pi(sk)->codec.id = BT_CODEC_TRANSPARENT;
-		hci_dev_put(hdev);
+		sco_pi(sk)->esco_setup = false;
 		break;
 
 	case BT_PKT_STATUS:
@@ -951,6 +942,7 @@  static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
 		}
 
 		sco_pi(sk)->codec = codecs->codecs[0];
+		sco_pi(sk)->esco_setup = true;
 		hci_dev_put(hdev);
 		break;