2022-02-23 02:34:07

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: [RFC] Bluetooth: SCO: Fix codec when using HCI_Enhanced_Setup_Synchronous_Connection

From: Luiz Augusto von Dentz <[email protected]>

This makes sure BT_CODEC_MSBC is used by default if socket user attempt
to use BT_VOICE_TRANSPARENT.

Fixes: b2af264ad3af ("Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command")
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/bluetooth/sco.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 8eabf41b2993..b35c772efc7e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -879,15 +879,9 @@ 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;
+ if (voice.setting == BT_VOICE_TRANSPARENT)
+ sco_pi(sk)->codec.id = BT_CODEC_MSBC;
+
hci_dev_put(hdev);
break;

--
2.35.1


2022-02-24 15:34:55

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [RFC] Bluetooth: SCO: Fix codec when using HCI_Enhanced_Setup_Synchronous_Connection

Hi Luiz,

> This makes sure BT_CODEC_MSBC is used by default if socket user attempt
> to use BT_VOICE_TRANSPARENT.
>
> Fixes: b2af264ad3af ("Bluetooth: Add support for HCI_Enhanced_Setup_Synchronous_Connection command")
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/bluetooth/sco.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 8eabf41b2993..b35c772efc7e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -879,15 +879,9 @@ 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;
> + if (voice.setting == BT_VOICE_TRANSPARENT)
> + sco_pi(sk)->codec.id = BT_CODEC_MSBC;
> +
> hci_dev_put(hdev);
> break;

why are you removing the rest and especially the eSCO check?

Regards

Marcel

2022-02-27 23:22:51

by Pauli Virtanen

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

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 <[email protected]>
---

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

2022-02-28 02:08:12

by bluez.test.bot

[permalink] [raw]
Subject: RE: [RFC] Bluetooth: don't use ESCO setup for BT_VOICE

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