[PATCH v6 0/6] sco: SCO socket option for voice_setting

[PATCH v6 0/6] sco: SCO socket option for voice_setting

[Frédéric Dalleau]

Hi,

v6 fixes style issues

v5 changes interface to SOL_BLUETOOTH, BT_VOICE.
Rework fallback mechanism.
Add S3, S2, S1 settings.

This is the patch version 4 of the socket option for enabling transparent SCO.
As requested by Marcel, this is now a 16-bit voice_setting.
0x0000 is the value corresponding to current behavior.
0x0003 is the value to use for enabling transparent data.
It is easy to allow all possible values from Bluetooth core spec, but I guess
results can be unexpected...
Should we filter allowed values in setsockopt ?

Let me know what you think.
Regards,
Fred

Frédéric Dalleau (6):
  Bluetooth: Move and rename hci_conn_accept
  Bluetooth: Add bluetooth socket voice option
  Bluetooth: Use hci_connect_sco directly
  Bluetooth: Use voice setting in incoming SCO connection
  Bluetooth: Parameters for outgoing SCO connections
  Bluetooth: Fallback transparent SCO from T2 to T1

 include/net/bluetooth/bluetooth.h |    8 ++++
 include/net/bluetooth/hci_core.h  |    5 +-
 include/net/bluetooth/sco.h       |    1 +
 net/bluetooth/hci_conn.c          |   45 ++++++++++++++----
 net/bluetooth/hci_event.c         |   39 +---------------
 net/bluetooth/sco.c               |   92 +++++++++++++++++++++++++++++++++++--
 6 files changed, 138 insertions(+), 52 deletions(-)

-- 
1.7.9.5

[PATCH v6 1/6] Bluetooth: Move and rename hci_conn_accept

[Frédéric Dalleau]

Since this function is only used by sco, move it from hci_event.c to
sco.c and rename to sco_conn_defer_accept. Make it static.

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 include/net/bluetooth/hci_core.h |    1 -
 net/bluetooth/hci_event.c        |   36 ------------------------------------
 net/bluetooth/sco.c              |   38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 78ea9c7..33f570c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -584,7 +584,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
 int hci_conn_del(struct hci_conn *conn);
 void hci_conn_hash_flush(struct hci_dev *hdev);
 void hci_conn_check_pending(struct hci_dev *hdev);
-void hci_conn_accept(struct hci_conn *conn, int mask);
 
 struct hci_chan *hci_chan_create(struct hci_conn *conn);
 void hci_chan_del(struct hci_chan *chan);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index f6ea3c7..333646c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1753,42 +1753,6 @@ unlock:
 	hci_conn_check_pending(hdev);
 }
 
-void hci_conn_accept(struct hci_conn *conn, int mask)
-{
-	struct hci_dev *hdev = conn->hdev;
-
-	BT_DBG("conn %p", conn);
-
-	conn->state = BT_CONFIG;
-
-	if (!lmp_esco_capable(hdev)) {
-		struct hci_cp_accept_conn_req cp;
-
-		bacpy(&cp.bdaddr, &conn->dst);
-
-		if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
-			cp.role = 0x00; /* Become master */
-		else
-			cp.role = 0x01; /* Remain slave */
-
-		hci_send_cmd(hdev, HCI_OP_ACCEPT_CONN_REQ, sizeof(cp), &cp);
-	} else /* lmp_esco_capable(hdev)) */ {
-		struct hci_cp_accept_sync_conn_req cp;
-
-		bacpy(&cp.bdaddr, &conn->dst);
-		cp.pkt_type = cpu_to_le16(conn->pkt_type);
-
-		cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
-		cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
-		cp.max_latency    = __constant_cpu_to_le16(0xffff);
-		cp.content_format = cpu_to_le16(hdev->voice_setting);
-		cp.retrans_effort = 0xff;
-
-		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
-			     sizeof(cp), &cp);
-	}
-}
-
 static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_ev_conn_request *ev = (void *) skb->data;
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index d883680..9e62102 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -652,6 +652,42 @@ static int sco_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return err;
 }
 
+static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
+{
+	struct hci_dev *hdev = conn->hdev;
+
+	BT_DBG("conn %p", conn);
+
+	conn->state = BT_CONFIG;
+
+	if (!lmp_esco_capable(hdev)) {
+		struct hci_cp_accept_conn_req cp;
+
+		bacpy(&cp.bdaddr, &conn->dst);
+
+		if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
+			cp.role = 0x00; /* Become master */
+		else
+			cp.role = 0x01; /* Remain slave */
+
+		hci_send_cmd(hdev, HCI_OP_ACCEPT_CONN_REQ, sizeof(cp), &cp);
+	} else {
+		struct hci_cp_accept_sync_conn_req cp;
+
+		bacpy(&cp.bdaddr, &conn->dst);
+		cp.pkt_type = cpu_to_le16(conn->pkt_type);
+
+		cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
+		cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
+		cp.max_latency    = __constant_cpu_to_le16(0xffff);
+		cp.content_format = cpu_to_le16(hdev->voice_setting);
+		cp.retrans_effort = 0xff;
+
+		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
+			     sizeof(cp), &cp);
+	}
+}
+
 static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 			    struct msghdr *msg, size_t len, int flags)
 {
@@ -662,7 +698,7 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 
 	if (sk->sk_state == BT_CONNECT2 &&
 	    test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
-		hci_conn_accept(pi->conn->hcon, 0);
+		sco_conn_defer_accept(pi->conn->hcon, 0);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

Re: [PATCH v6 1/6] Bluetooth: Move and rename hci_conn_accept

[Marcel Holtmann]

Hi Fred,

> Since this function is only used by sco, move it from hci_event.c to
> sco.c and rename to sco_conn_defer_accept. Make it static.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h |    1 -
> net/bluetooth/hci_event.c        |   36 ------------------------------------
> net/bluetooth/sco.c              |   38 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 37 insertions(+), 38 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH v6 1/6] Bluetooth: Move and rename hci_conn_accept

[Gustavo Padovan]

Hi Frédéric,

* Frédéric Dalleau <frederic.dalleau@linux.intel.com> [2013-04-16 17:28:58 +0200]:

> Since this function is only used by sco, move it from hci_event.c to
> sco.c and rename to sco_conn_defer_accept. Make it static.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 -
>  net/bluetooth/hci_event.c        |   36 ------------------------------------
>  net/bluetooth/sco.c              |   38 +++++++++++++++++++++++++++++++++++++-
>  3 files changed, 37 insertions(+), 38 deletions(-)

Patch has been applied to bluetooth-next. Thanks.

	Gustavo

[PATCH v6 2/6] Bluetooth: Add bluetooth socket voice option

[Frédéric Dalleau]

This patch extends the current bluetooth socket option to add BT_VOICE.
This is intended to choose voice data type at runtime. It only applies to SCO
sockets.
Incoming connections shall be setup during defered setup. Outgoing connections
shall be setup before connect(). The desired setting is stored in the sco
socket info.
This patch declares needed members, modifies getsockopt() and setsockopt().

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 include/net/bluetooth/bluetooth.h |    8 ++++++++
 include/net/bluetooth/sco.h       |    1 +
 net/bluetooth/sco.c               |   38 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 591fee7..e66b9b0 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -107,6 +107,14 @@ struct bt_power {
  */
 #define BT_CHANNEL_POLICY_AMP_PREFERRED		2
 
+#define BT_VOICE		11
+struct bt_voice {
+	__u16 setting;
+};
+
+#define BT_VOICE_TRANSPARENT			0x0003
+#define BT_VOICE_CVSD				0x0060
+
 __printf(1, 2)
 int bt_info(const char *fmt, ...);
 __printf(1, 2)
diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index 1e35c43..e252a31 100644
--- a/include/net/bluetooth/sco.h
+++ b/include/net/bluetooth/sco.h
@@ -73,6 +73,7 @@ struct sco_conn {
 struct sco_pinfo {
 	struct bt_sock	bt;
 	__u32		flags;
+	__u16		setting;
 	struct sco_conn	*conn;
 };
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 9e62102..454ab64 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -417,6 +417,8 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock, int pro
 	sk->sk_protocol = proto;
 	sk->sk_state    = BT_OPEN;
 
+	sco_pi(sk)->setting = 0;
+
 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
 
 	bt_sock_link(&sco_sk_list, sk);
@@ -713,7 +715,8 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 static int sco_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
-	int err = 0;
+	int len, err = 0;
+	struct bt_voice voice;
 	u32 opt;
 
 	BT_DBG("sk %p", sk);
@@ -739,6 +742,29 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname, char
 			clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
 		break;
 
+	case BT_VOICE:
+		if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND &&
+		    sk->sk_state != BT_CONNECT2) {
+			err = -EINVAL;
+			break;
+		}
+
+		len = min_t(unsigned int, sizeof(voice), optlen);
+		if (copy_from_user((char *) &voice, optval, len)) {
+			err = -EFAULT;
+			break;
+		}
+
+
+		if (voice.setting != BT_VOICE_TRANSPARENT &&
+		    voice.setting != BT_VOICE_CVSD) {
+			err = -EINVAL;
+			break;
+		}
+
+		sco_pi(sk)->setting = voice.setting;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -808,6 +834,7 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, char
 {
 	struct sock *sk = sock->sk;
 	int len, err = 0;
+	struct bt_voice voice;
 
 	BT_DBG("sk %p", sk);
 
@@ -833,6 +860,15 @@ static int sco_sock_getsockopt(struct socket *sock, int level, int optname, char
 
 		break;
 
+	case BT_VOICE:
+		voice.setting = sco_pi(sk)->setting;
+
+		len = min_t(unsigned int, len, sizeof(voice));
+		if (copy_to_user(optval, (char *)&voice, len))
+			err = -EFAULT;
+
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
-- 
1.7.9.5

Re: [PATCH v6 2/6] Bluetooth: Add bluetooth socket voice option

[Marcel Holtmann]

Hi Fred,

> This patch extends the current bluetooth socket option to add BT_VOICE.
> This is intended to choose voice data type at runtime. It only applies to SCO
> sockets.
> Incoming connections shall be setup during defered setup. Outgoing connections
> shall be setup before connect(). The desired setting is stored in the sco
> socket info.
> This patch declares needed members, modifies getsockopt() and setsockopt().
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/bluetooth.h |    8 ++++++++
> include/net/bluetooth/sco.h       |    1 +
> net/bluetooth/sco.c               |   38 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 591fee7..e66b9b0 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -107,6 +107,14 @@ struct bt_power {
>  */
> #define BT_CHANNEL_POLICY_AMP_PREFERRED		2
> 
> +#define BT_VOICE		11
> +struct bt_voice {
> +	__u16 setting;
> +};
> +
> +#define BT_VOICE_TRANSPARENT			0x0003
> +#define BT_VOICE_CVSD				0x0060
> +
> __printf(1, 2)
> int bt_info(const char *fmt, ...);
> __printf(1, 2)
> diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
> index 1e35c43..e252a31 100644
> --- a/include/net/bluetooth/sco.h
> +++ b/include/net/bluetooth/sco.h
> @@ -73,6 +73,7 @@ struct sco_conn {
> struct sco_pinfo {
> 	struct bt_sock	bt;
> 	__u32		flags;
> +	__u16		setting;
> 	struct sco_conn	*conn;
> };
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 9e62102..454ab64 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -417,6 +417,8 @@ static struct sock *sco_sock_alloc(struct net *net, struct socket *sock, int pro
> 	sk->sk_protocol = proto;
> 	sk->sk_state    = BT_OPEN;
> 
> +	sco_pi(sk)->setting = 0;
> +

wouldn't it be better if this default to the current controller setting. However I realize that the controller might be actually not yet present. So shouldn't we default to 0x0060 for CVSD.

Regards

Marcel

Re: [PATCH v6 2/6] Bluetooth: Add bluetooth socket voice option

[Frédéric DALLEAU]

Hello Marcel,

> wouldn't it be better if this default to the current controller setting. However I realize that the controller might be actually not yet present. So shouldn't we default to 0x0060 for CVSD.

For now, 0 is a placeholder. It maintains behavior for existing
code. BT_VOICE_CVSD is planned for S3, S2, S1 fallback for eSCO. But
IMHO, this needs some heavy interop testing. When interoperability is
ensured,  BT_VOICE_CVSD could be used as default. The patch will be a
one liner.

Regards,
Frédéric

[PATCH v6 3/6] Bluetooth: Use hci_connect_sco directly

[Frédéric Dalleau]

hci_connect is a super function for connecting hci protocols. But the
voice_setting parameter is only needed by SCO and security requirements are not
needed for SCO channels. Thus, it makes sense to have a separate function.

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_conn.c         |    9 +++------
 net/bluetooth/sco.c              |    3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 33f570c..5c6c5b0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -592,6 +592,8 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
 
 struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 			     __u8 dst_type, __u8 sec_level, __u8 auth_type);
+struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
+				 __u16 setting);
 int hci_conn_check_link_mode(struct hci_conn *conn);
 int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
 int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b1a02ce..61f686b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -563,13 +563,13 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 	return acl;
 }
 
-static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
-				bdaddr_t *dst, u8 sec_level, u8 auth_type)
+struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
+				 __u16 setting)
 {
 	struct hci_conn *acl;
 	struct hci_conn *sco;
 
-	acl = hci_connect_acl(hdev, dst, sec_level, auth_type);
+	acl = hci_connect_acl(hdev, dst, BT_SECURITY_LOW, HCI_AT_NO_BONDING);
 	if (IS_ERR(acl))
 		return acl;
 
@@ -615,9 +615,6 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
 	case ACL_LINK:
 		return hci_connect_acl(hdev, dst, sec_level, auth_type);
-	case SCO_LINK:
-	case ESCO_LINK:
-		return hci_connect_sco(hdev, type, dst, sec_level, auth_type);
 	}
 
 	return ERR_PTR(-EINVAL);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 454ab64..4e69923 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -176,8 +176,7 @@ static int sco_connect(struct sock *sk)
 	else
 		type = SCO_LINK;
 
-	hcon = hci_connect(hdev, type, dst, BDADDR_BREDR, BT_SECURITY_LOW,
-			   HCI_AT_NO_BONDING);
+	hcon = hci_connect_sco(hdev, type, dst, sco_pi(sk)->setting);
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
 		goto done;
-- 
1.7.9.5

Re: [PATCH v6 3/6] Bluetooth: Use hci_connect_sco directly

[Marcel Holtmann]

Hi Fred,

> hci_connect is a super function for connecting hci protocols. But the
> voice_setting parameter is only needed by SCO and security requirements are not
> needed for SCO channels. Thus, it makes sense to have a separate function.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h |    2 ++
> net/bluetooth/hci_conn.c         |    9 +++------
> net/bluetooth/sco.c              |    3 +--
> 3 files changed, 6 insertions(+), 8 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[PATCH v6 4/6] Bluetooth: Use voice setting in incoming SCO connection

[Frédéric Dalleau]

When an incoming SCO connection is requested, check the selected voice setting,
and reply appropriately. Voice setting should have been negotiated previously.
For example, in case of HFP, the codec is negotiated using AT commands on the
RFCOMM channel. This patch only changes replies for socket with defered setup
enabled.

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 net/bluetooth/sco.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 4e69923..876f80e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -653,7 +653,7 @@ static int sco_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return err;
 }
 
-static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
+static void sco_conn_defer_accept(struct hci_conn *conn, int mask, int setting)
 {
 	struct hci_dev *hdev = conn->hdev;
 
@@ -680,9 +680,22 @@ static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
 
 		cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
 		cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
-		cp.max_latency    = __constant_cpu_to_le16(0xffff);
-		cp.content_format = cpu_to_le16(hdev->voice_setting);
-		cp.retrans_effort = 0xff;
+
+		switch (setting) {
+		case BT_VOICE_TRANSPARENT:
+			if (conn->pkt_type & ESCO_2EV3)
+				cp.max_latency = __constant_cpu_to_le16(0x0008);
+			else
+				cp.max_latency = __constant_cpu_to_le16(0x000D);
+			cp.content_format = __constant_cpu_to_le16(0x0003);
+			cp.retrans_effort = 0x02;
+			break;
+		default:
+			cp.max_latency    = __constant_cpu_to_le16(0xffff);
+			cp.content_format = cpu_to_le16(hdev->voice_setting);
+			cp.retrans_effort = 0xff;
+			break;
+		}
 
 		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
 			     sizeof(cp), &cp);
@@ -699,7 +712,7 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 
 	if (sk->sk_state == BT_CONNECT2 &&
 	    test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
-		sco_conn_defer_accept(pi->conn->hcon, 0);
+		sco_conn_defer_accept(pi->conn->hcon, 0, pi->setting);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

Re: [PATCH v6 4/6] Bluetooth: Use voice setting in incoming SCO connection

[Marcel Holtmann]

Hi Fred,

> When an incoming SCO connection is requested, check the selected voice setting,
> and reply appropriately. Voice setting should have been negotiated previously.
> For example, in case of HFP, the codec is negotiated using AT commands on the
> RFCOMM channel. This patch only changes replies for socket with defered setup
> enabled.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> net/bluetooth/sco.c |   23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 4e69923..876f80e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -653,7 +653,7 @@ static int sco_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> 	return err;
> }
> 
> -static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
> +static void sco_conn_defer_accept(struct hci_conn *conn, int mask, int setting)
> {
> 	struct hci_dev *hdev = conn->hdev;
> 
> @@ -680,9 +680,22 @@ static void sco_conn_defer_accept(struct hci_conn *conn, int mask)

are we keep using the mask parameter here?

> 
> 		cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
> 		cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
> -		cp.max_latency    = __constant_cpu_to_le16(0xffff);
> -		cp.content_format = cpu_to_le16(hdev->voice_setting);
> -		cp.retrans_effort = 0xff;
> +
> +		switch (setting) {
> +		case BT_VOICE_TRANSPARENT:
> +			if (conn->pkt_type & ESCO_2EV3)
> +				cp.max_latency = __constant_cpu_to_le16(0x0008);
> +			else
> +				cp.max_latency = __constant_cpu_to_le16(0x000D);
> +			cp.content_format = __constant_cpu_to_le16(0x0003);
> +			cp.retrans_effort = 0x02;
> +			break;
> +		default:
> +			cp.max_latency    = __constant_cpu_to_le16(0xffff);
> +			cp.content_format = cpu_to_le16(hdev->voice_setting);
> +			cp.retrans_effort = 0xff;
> +			break;
> +		}
> 
> 		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
> 			     sizeof(cp), &cp);
> @@ -699,7 +712,7 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> 
> 	if (sk->sk_state == BT_CONNECT2 &&
> 	    test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
> -		sco_conn_defer_accept(pi->conn->hcon, 0);
> +		sco_conn_defer_accept(pi->conn->hcon, 0, pi->setting);

If the mask is always 0, then please just send a patch to remove that parameter.

Regards

Marcel

[PATCH v6 5/6] Bluetooth: Parameters for outgoing SCO connections

[Frédéric Dalleau]

In order to establish a transparent SCO connection, the correct settings must
be specified in the Setup Synchronous Connection request. For that,
voice_setting is added to ACL connection flags to set up the desired
parameters. If this value is zero, a legacy SCO connection will be requested.
This patch uses T2 settings.

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |   22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5c6c5b0..98f8c15 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -328,6 +328,7 @@ struct hci_conn {
 	__u32		passkey_notify;
 	__u8		passkey_entered;
 	__u16		disc_timeout;
+	__u16		setting;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 61f686b..cface45 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -185,13 +185,25 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	conn->attempt++;
 
 	cp.handle   = cpu_to_le16(handle);
-	cp.pkt_type = cpu_to_le16(conn->pkt_type);
 
 	cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
 	cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
-	cp.max_latency    = __constant_cpu_to_le16(0xffff);
-	cp.voice_setting  = cpu_to_le16(hdev->voice_setting);
-	cp.retrans_effort = 0xff;
+
+	switch (conn->setting) {
+	case BT_VOICE_TRANSPARENT:
+		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK &
+							   ~ESCO_2EV3);
+		cp.max_latency    = __constant_cpu_to_le16(0x000d);
+		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
+		cp.retrans_effort = 0x02;
+		break;
+	default:
+		cp.pkt_type       = cpu_to_le16(conn->pkt_type);
+		cp.max_latency    = __constant_cpu_to_le16(0xffff);
+		cp.voice_setting  = cpu_to_le16(hdev->voice_setting);
+		cp.retrans_effort = 0xff;
+		break;
+	}
 
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
@@ -587,6 +599,8 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 
 	hci_conn_hold(sco);
 
+	sco->setting = setting;
+
 	if (acl->state == BT_CONNECTED &&
 	    (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
 		set_bit(HCI_CONN_POWER_SAVE, &acl->flags);
-- 
1.7.9.5

Re: [PATCH v6 5/6] Bluetooth: Parameters for outgoing SCO connections

[Marcel Holtmann]

Hi Fred,

> In order to establish a transparent SCO connection, the correct settings must
> be specified in the Setup Synchronous Connection request. For that,
> voice_setting is added to ACL connection flags to set up the desired
> parameters. If this value is zero, a legacy SCO connection will be requested.
> This patch uses T2 settings.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h |    1 +
> net/bluetooth/hci_conn.c         |   22 ++++++++++++++++++----
> 2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5c6c5b0..98f8c15 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -328,6 +328,7 @@ struct hci_conn {
> 	__u32		passkey_notify;
> 	__u8		passkey_entered;
> 	__u16		disc_timeout;
> +	__u16		setting;
> 	unsigned long	flags;
> 
> 	__u8		remote_cap;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 61f686b..cface45 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -185,13 +185,25 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
> 	conn->attempt++;
> 
> 	cp.handle   = cpu_to_le16(handle);
> -	cp.pkt_type = cpu_to_le16(conn->pkt_type);
> 
> 	cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
> 	cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
> -	cp.max_latency    = __constant_cpu_to_le16(0xffff);
> -	cp.voice_setting  = cpu_to_le16(hdev->voice_setting);
> -	cp.retrans_effort = 0xff;
> +
> +	switch (conn->setting) {
> +	case BT_VOICE_TRANSPARENT:

I think we should just check the bits for air codec and see if they match transparent vs cvsd and not use constants here.

> +		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK &
> +							   ~ESCO_2EV3);
> +		cp.max_latency    = __constant_cpu_to_le16(0x000d);
> +		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
> +		cp.retrans_effort = 0x02;
> +		break;
> +	default:
> +		cp.pkt_type       = cpu_to_le16(conn->pkt_type);
> +		cp.max_latency    = __constant_cpu_to_le16(0xffff);
> +		cp.voice_setting  = cpu_to_le16(hdev->voice_setting);

Also if we start setting socket voice_setting options, we need to start using that at some point. The controller default should only be used to initialize the controller or fill in the missing information in case of using the old Add_SCO command.

Regards

Marcel

[PATCH v6 6/6] Bluetooth: Fallback transparent SCO from T2 to T1

[Frédéric Dalleau]

When initiating a transparent eSCO connection, make use of T2 settings at
first try. T2 is the recommended settings from HFP 1.6 WideBand Speech. Upon
connection failure, try T1 settings.
T2 failure is detected if Synchronous Connection Complete event fails with
error 0x0d. This error code has been found experimentally by sending a T2
request to a T1 only SCO listener. It means "Connection Rejected due to
Limited resource".
To know which of T2 or T1 should be used, conn->fallback is used.  Bluez only
attempt to reconnect twice. In the future, we will need more than 2 fallback
possibilities. Thus, test conn->fallback as an alternative measure. We want to
fallback only if conn->fallback is positive. Calling hci_setup_sync with
conn->fallback == 0 is an initial connection attempt.

Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |   20 +++++++++++++++++---
 net/bluetooth/hci_event.c        |    3 ++-
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 98f8c15..605715e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -329,6 +329,7 @@ struct hci_conn {
 	__u8		passkey_entered;
 	__u16		disc_timeout;
 	__u16		setting;
+	__u8		fallback;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cface45..bfed66a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -31,6 +31,17 @@
 #include <net/bluetooth/a2mp.h>
 #include <net/bluetooth/smp.h>
 
+struct sco_param {
+	u16 pkt_type;
+	u16 max_latency;
+	u8 next;
+};
+
+static const struct sco_param sco_param_wideband[] = {
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d, 1 },
+	{ EDR_ESCO_MASK & ESCO_EV3,   0x0007, 0 },
+};
+
 static void hci_le_create_connection(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -176,6 +187,7 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_setup_sync_conn cp;
+	const struct sco_param *param;
 
 	BT_DBG("hcon %p", conn);
 
@@ -191,11 +203,13 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 
 	switch (conn->setting) {
 	case BT_VOICE_TRANSPARENT:
-		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK &
-							   ~ESCO_2EV3);
-		cp.max_latency    = __constant_cpu_to_le16(0x000d);
+		param = &sco_param_wideband[conn->fallback];
+		cp.pkt_type       = __constant_cpu_to_le16(param->pkt_type);
+		cp.max_latency    = __constant_cpu_to_le16(param->max_latency);
 		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
 		cp.retrans_effort = 0x02;
+
+		conn->fallback    = param->next;
 		break;
 	default:
 		cp.pkt_type       = cpu_to_le16(conn->pkt_type);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 333646c..610c7a8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2955,11 +2955,12 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
 		hci_conn_add_sysfs(conn);
 		break;
 
+	case 0x0d:	/* No resource available */
 	case 0x11:	/* Unsupported Feature or Parameter Value */
 	case 0x1c:	/* SCO interval rejected */
 	case 0x1a:	/* Unsupported Remote Feature */
 	case 0x1f:	/* Unspecified error */
-		if (conn->out && conn->attempt < 2) {
+		if (conn->out && (conn->attempt < 2 || conn->fallback > 0)) {
 			conn->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
 					(hdev->esco_type & EDR_ESCO_MASK);
 			hci_setup_sync(conn, conn->link->handle);
-- 
1.7.9.5