[PATCH v7 0/8] sco: SCO socket option for voice_setting

[PATCH v7 0/8] sco: SCO socket option for voice_setting

[Frédéric Dalleau]

Hi,

v7 changes defaults to BT_VOICE_CVSD
Remove mask parameter to sco_conn_defer_accept, it was always 0
check the bits for air codec instead of use constants.
Add S3, S2, S1, D1, D0 settings.
The controller default is now only used to initialize the controller or fill
in the missing information in case of using the old Add_SCO command

v6 fixes style issues

v5 changes interface to SOL_BLUETOOTH, BT_VOICE.
Rework fallback mechanism.

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 (8):
  Bluetooth: Use hci_connect_sco directly
  Bluetooth: Remove unused mask parameter in sco_conn_defer_accept
  Bluetooth: Add bluetooth socket voice option
  Bluetooth: Constants and macro declaration for SCO airmode
  Bluetooth: Use voice setting in incoming SCO connection
  Bluetooth: Parameters for outgoing SCO connections
  Bluetooth: Fallback transparent SCO from T2 to T1
  Bluetooth: Add support for fallback between CVSD recommended settings

 include/net/bluetooth/bluetooth.h |    8 +++++
 include/net/bluetooth/hci_core.h  |    9 +++++
 include/net/bluetooth/sco.h       |    1 +
 net/bluetooth/hci_conn.c          |   56 +++++++++++++++++++++++------
 net/bluetooth/hci_event.c         |    3 +-
 net/bluetooth/sco.c               |   70 ++++++++++++++++++++++++++++++-------
 6 files changed, 123 insertions(+), 24 deletions(-)

-- 
1.7.9.5

[PATCH v7 1/8] 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>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 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 606615c..e9cf9fa 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -584,6 +584,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 6c7f363..d1d9919 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -560,13 +560,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;
 
@@ -612,9 +612,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 9e62102..a85182f 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

[PATCH v7 2/8] Bluetooth: Remove unused mask parameter in sco_conn_defer_accept

[Frédéric Dalleau]

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

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index a85182f..26c9d51 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -651,7 +651,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)
 {
 	struct hci_dev *hdev = conn->hdev;
 
@@ -663,11 +663,7 @@ static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
 		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 */
+		cp.role = 0x01; /* Remain slave */
 
 		hci_send_cmd(hdev, HCI_OP_ACCEPT_CONN_REQ, sizeof(cp), &cp);
 	} else {
@@ -697,7 +693,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);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

Re: [PATCH v7 2/8] Bluetooth: Remove unused mask parameter in sco_conn_defer_accept

[Marcel Holtmann]

Hi Fred,

> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>

please add a commit message on why it is unused and okay to remove.

Regards

Marcel

[PATCH v7 3/8] 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 6912ef9..0239a49 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 26c9d51..7b4c3df 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -416,6 +416,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 = BT_VOICE_CVSD;
+
 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
 
 	bt_sock_link(&sco_sk_list, sk);
@@ -708,7 +710,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);
@@ -734,6 +737,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;
@@ -803,6 +829,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);
 
@@ -828,6 +855,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 v7 3/8] 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 6912ef9..0239a49 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

call this BT_VOICE_CVSD_16BIT. You could run this with CVSD_8BIT as well if we ever care to get that fixed.

> +
> __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 26c9d51..7b4c3df 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -416,6 +416,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 = BT_VOICE_CVSD;
> +
> 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
> 
> 	bt_sock_link(&sco_sk_list, sk);
> @@ -708,7 +710,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);
> @@ -734,6 +737,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;
> +		}
> +

Actually you need to set defaults here first. Otherwise updated userspaces with more options might not work properly.

		voice.setting = sco_pi(sk)->setting.

> +		len = min_t(unsigned int, sizeof(voice), optlen);
> +		if (copy_from_user((char *) &voice, optval, len)) {
> +			err = -EFAULT;
> +			break;
> +		}
> +
> +

Double empty line here. I assume you wanted to add a comment that for now we are only accepting two settings at the moment.

> +		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;
> @@ -803,6 +829,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);
> 
> @@ -828,6 +855,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;

Regards

Marcel

[PATCH v7 4/8] Bluetooth: Constants and macro declaration for SCO airmode

[Frédéric Dalleau]

This patchs define constants and macro for extracting SCO airmode from SCO
voice setting. It refers to Bluetooth Core V4.0 specification, Part E, Chap
6.12 which describe SCO voice setting format.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e9cf9fa..a6fd544 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1214,4 +1214,9 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 
 u8 bdaddr_to_le(u8 bdaddr_type);
 
+#define SCO_AIRMODE_MASK       0x0003
+#define SCO_AIRMODE_CVSD       0x0000
+#define SCO_AIRMODE_TRANSP     0x0003
+#define hci_sco_airmode(setting) ((setting) & SCO_AIRMODE_MASK)
+
 #endif /* __HCI_CORE_H */
-- 
1.7.9.5

Re: [PATCH v7 4/8] Bluetooth: Constants and macro declaration for SCO airmode

[Marcel Holtmann]

On May 16, 2013, at 7:34 PM, Frédéric Dalleau <frederic.dalleau@linux.intel.com> wrote:

> This patchs define constants and macro for extracting SCO airmode from SCO
> voice setting. It refers to Bluetooth Core V4.0 specification, Part E, Chap
> 6.12 which describe SCO voice setting format.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h |    5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e9cf9fa..a6fd544 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1214,4 +1214,9 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 
> u8 bdaddr_to_le(u8 bdaddr_type);
> 
> +#define SCO_AIRMODE_MASK       0x0003
> +#define SCO_AIRMODE_CVSD       0x0000
> +#define SCO_AIRMODE_TRANSP     0x0003
> +#define hci_sco_airmode(setting) ((setting) & SCO_AIRMODE_MASK)

the constants are fine, but I would not bother with the macro. Just use SCO_AIRMODE_MASK directly.

Regards

Marcel

[PATCH v7 5/8] 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 7b4c3df..7881208 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)
+static void sco_conn_defer_accept(struct hci_conn *conn, int setting)
 {
 	struct hci_dev *hdev = conn->hdev;
 
@@ -676,9 +676,22 @@ static void sco_conn_defer_accept(struct hci_conn *conn)
 
 		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 (hci_sco_airmode(setting)) {
+		case SCO_AIRMODE_TRANSP:
+			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(SCO_AIRMODE_TRANSP);
+			cp.retrans_effort = 0x02;
+			break;
+		case SCO_AIRMODE_CVSD:
+			cp.max_latency    = __constant_cpu_to_le16(0xffff);
+			cp.content_format = cpu_to_le16(setting);
+			cp.retrans_effort = 0xff;
+			break;
+		}
 
 		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
 			     sizeof(cp), &cp);
@@ -695,7 +708,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);
+		sco_conn_defer_accept(pi->conn->hcon, pi->setting);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

Re: [PATCH v7 5/8] 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 7b4c3df..7881208 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)
> +static void sco_conn_defer_accept(struct hci_conn *conn, int setting)
> {
> 	struct hci_dev *hdev = conn->hdev;
> 
> @@ -676,9 +676,22 @@ static void sco_conn_defer_accept(struct hci_conn *conn)
> 
> 		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;

so we are now ignoring the hdev->voice_setting. This is fine for eSCO, but what are we doing in case we have a 1.1 controller that only supports the Add_SCO command?

Regards

Marcel

[PATCH v7 6/8] 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 a6fd544..b5996b3 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -320,6 +320,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 d1d9919..cd7452e 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 (hci_sco_airmode(conn->setting)) {
+	case SCO_AIRMODE_TRANSP:
+		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(SCO_AIRMODE_TRANSP);
+		cp.retrans_effort = 0x02;
+		break;
+	case SCO_AIRMODE_CVSD:
+		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);
 }
@@ -584,6 +596,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 v7 6/8] 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 a6fd544..b5996b3 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -320,6 +320,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 d1d9919..cd7452e 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;

same here. What are we doing with Add_SCO fallback on older controllers.

We could just reject setting transparent air mode for controllers that do not support eSCO command procedures. And the only valid option there would be CVSD then. Which is default.

In case we use Add_SCO is is important that the voice setting programmed into the controller is actually 16-bit CVSD at that time. That needs to be taken care of as well.

Regards

Marcel

Re: [PATCH v7 6/8] Bluetooth: Parameters for outgoing SCO connections

[Vinicius Gomes]

Hi,

On Fri, May 17, 2013 at 5:55 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> 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 reque=
sted.
>> This patch uses T2 settings.
>>
>> Signed-off-by: Fr=C3=A9d=C3=A9ric 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/hc=
i_core.h
>> index a6fd544..b5996b3 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -320,6 +320,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 d1d9919..cd7452e 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 h=
andle)
>>       conn->attempt++;
>>
>>       cp.handle   =3D cpu_to_le16(handle);
>> -     cp.pkt_type =3D cpu_to_le16(conn->pkt_type);
>>
>>       cp.tx_bandwidth   =3D __constant_cpu_to_le32(0x00001f40);
>>       cp.rx_bandwidth   =3D __constant_cpu_to_le32(0x00001f40);
>> -     cp.max_latency    =3D __constant_cpu_to_le16(0xffff);
>> -     cp.voice_setting  =3D cpu_to_le16(hdev->voice_setting);
>> -     cp.retrans_effort =3D 0xff;
>
> same here. What are we doing with Add_SCO fallback on older controllers.
>
> We could just reject setting transparent air mode for controllers that do=
 not support eSCO command procedures. And the only valid option there would=
 be CVSD then. Which is default.

I have seem some controllers (IIRC CSR 2.0) that don't have eSCO but
have support for transparent air mode, in these cases we want to use
the Setup SCO/Accept SCO procedures.

>
> In case we use Add_SCO is is important that the voice setting programmed =
into the controller is actually 16-bit CVSD at that time. That needs to be =
taken care of as well.
>
> Regards
>
> Marcel
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth=
" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
--
Vinicius

[PATCH v7 7/8] 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 b5996b3..c8b91d2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -321,6 +321,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 cd7452e..8d71d73 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,   0x0008, 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 (hci_sco_airmode(conn->setting)) {
 	case SCO_AIRMODE_TRANSP:
-		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(SCO_AIRMODE_TRANSP);
 		cp.retrans_effort = 0x02;
+
+		conn->fallback    = param->next;
 		break;
 	case SCO_AIRMODE_CVSD:
 		cp.pkt_type       = cpu_to_le16(conn->pkt_type);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index db58e72..9be10be 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2904,11 +2904,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

Re: [PATCH v7 7/8] Bluetooth: Fallback transparent SCO from T2 to T1

[Marcel Holtmann]

Hi Fred,

> 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 b5996b3..c8b91d2 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -321,6 +321,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 cd7452e..8d71d73 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,   0x0008, 0 },
> +};
> +

so I would combine patch 7 and 8 into one and have a detailed commit explaining both section.

Also comment the entries in the table properly to list which is which setting. So others have an idea on what is going on.

Regards

Marcel

[PATCH v7 8/8] Bluetooth: Add support for fallback between CVSD recommended settings

[Frédéric Dalleau]

When CVSD is requested and eSCO is supported, try to establish eSCO connection
using S3 settings. If it fails, fallback in sequence to S2, S1, D1, D0 settings.
These settings are described in Bluetooth HFP 1.6 specification p. 101.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 8d71d73..f4aa8b2 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -42,6 +42,14 @@ static const struct sco_param sco_param_wideband[] = {
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008, 0 },
 };
 
+static const struct sco_param sco_param_cvsd[] = {
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a, 1 },
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007, 2 },
+	{ EDR_ESCO_MASK | ESCO_EV3,   0x0007, 3 },
+	{ EDR_ESCO_MASK | ESCO_HV3,   0xffff, 4 },
+	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff, 0 },
+};
+
 static void hci_le_create_connection(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -212,10 +220,13 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		conn->fallback    = param->next;
 		break;
 	case SCO_AIRMODE_CVSD:
-		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;
+		param = &sco_param_cvsd[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(conn->setting);
+		cp.retrans_effort = 0x01;
+
+		conn->fallback    = param->next;
 		break;
 	}
 
-- 
1.7.9.5