Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Pauli Virtanen <pav@iki.fi>
To:     linux-bluetooth@vger.kernel.org
Cc:     Pauli Virtanen <pav@iki.fi>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        Kiran K <kiran.k@intel.com>
Subject: [RFC] Bluetooth: don't use ESCO setup for BT_VOICE
Date:   Sun, 27 Feb 2022 18:34:29 +0200
Message-Id: <20220227163430.24694-1-pav@iki.fi>
In-Reply-To: <20220222212227.24515-1-luiz.dentz@gmail.com>
References: <20220222212227.24515-1-luiz.dentz@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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 <pav@iki.fi>
---

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(-)

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;
 
-- 
2.35.1