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

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

[Frédéric Dalleau]

Hi,

v8 declares BT_VOICE_CVSD_16BIT
Merge T*, S* and D* patches in one
The last patch returns -ECONNABORTED when trying to setup a transparent data
connection if eSCO is not supported.

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 declaration for SCO airmode
  Bluetooth: Use voice setting in defered SCO connection request
  Bluetooth: Parameters for outgoing SCO connections
  Bluetooth: SCO connection fallback
  Bluetooth: Prevent transparent SCO on older devices

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

-- 
1.7.9.5

[PATCH v8 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 f77885e..679b07f 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 e7bd4ee..7114984 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 v8 2/8] Bluetooth: Remove unused mask parameter in sco_conn_defer_accept

[Frédéric Dalleau]

Bluetooth specification states that when accepting a bluetooth synchronous
connection request, the role parameter is unused and will be ignored by the
controller. Moreover, the function is called only once with a fixed value.

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 7114984..0c15a0c 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 = 0x00; /* Ignored */
 
 		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;
 		msg->msg_namelen = 0;
 
-- 
1.7.9.5

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

[Marcel Holtmann]

Hi Fred,

> Bluetooth specification states that when accepting a bluetooth synchronous
> connection request, the role parameter is unused and will be ignored by the
> controller. Moreover, the function is called only once with a fixed value.

can you give the exact reference where this is stated. Since I fail to find it.

> 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 7114984..0c15a0c 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 = 0x00; /* Ignored */

Is it really 0x00 and not 0x01 to stay 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;
> 		msg->msg_namelen = 0;

Regards

Marcel

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

[Frédéric DALLEAU]

Hi Marcel,

Le 08/07/2013 21:10, Marcel Holtmann a écrit :
> can you give the exact reference where this is stated. Since I fail to find it.

Core spec v4, 7.1.8 Accept Connection Request Command, p 472, just 
before command paremeters

"Note: When accepting synchronous connection request, the Role parameter 
is not used and will be ignored by the BR/EDR Controller."

>> -		if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
>> -			cp.role = 0x00; /* Become master */
>> -		else
>> -			cp.role = 0x01; /* Remain slave */
>> +		cp.role = 0x00; /* Ignored */
>
> Is it really 0x00 and not 0x01 to stay slave.

Since it is ignored, there is no need to care.

Fred

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

[Marcel Holtmann]

Hi Fred,

> Bluetooth specification states that when accepting a bluetooth synchronous
> connection request, the role parameter is unused and will be ignored by the
> controller. Moreover, the function is called only once with a fixed value.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> net/bluetooth/sco.c |   10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)

I am fine with this patch, but please quote the spec section in the commit message when you send the update patch series.

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

Regards

Marcel

[PATCH v8 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               |   40 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10eb9b3..10d43d8 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_16BIT			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 0c15a0c..ce8690f 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_16BIT;
+
 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
 
 	bt_sock_link(&sco_sk_list, sk);
@@ -709,7 +711,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);
@@ -735,6 +738,31 @@ 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;
+		}
+
+		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;
+		}
+
+		/* Explicitly check for these values */
+		if (voice.setting != BT_VOICE_TRANSPARENT &&
+		    voice.setting != BT_VOICE_CVSD_16BIT) {
+			err = -EINVAL;
+			break;
+		}
+
+		sco_pi(sk)->setting = voice.setting;
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -804,6 +832,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);
 
@@ -829,6 +858,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 v8 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               |   40 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 48 insertions(+), 1 deletion(-)

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

Regards

Marcel

[PATCH v8 4/8] Bluetooth: Constants declaration for SCO airmode

[Frédéric Dalleau]

This patchs define constants for 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 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 679b07f..69bdb67 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1215,4 +1215,8 @@ 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
+
 #endif /* __HCI_CORE_H */
-- 
1.7.9.5

Re: [PATCH v8 4/8] Bluetooth: Constants declaration for SCO airmode

[Marcel Holtmann]

Hi Fred,

> This patchs define constants for 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 |    4 ++++
> 1 file changed, 4 insertions(+)

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

Regards

Marcel

[PATCH v8 5/8] Bluetooth: Use voice setting in defered SCO connection request

[Frédéric Dalleau]

When an incoming eSCO 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 |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index ce8690f..546b7f6 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,23 @@ 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 (setting & SCO_AIRMODE_MASK) {
+		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 +709,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;
 		msg->msg_namelen = 0;
 
-- 
1.7.9.5

Re: [PATCH v8 5/8] Bluetooth: Use voice setting in defered SCO connection request

[Marcel Holtmann]

Hi Fred,

> When an incoming eSCO 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 |   24 +++++++++++++++++++-----
> 1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index ce8690f..546b7f6 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,23 @@ 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 (setting & SCO_AIRMODE_MASK) {
> +		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);

why do we hardcode here …

> +			cp.retrans_effort = 0x02;
> +			break;
> +		case SCO_AIRMODE_CVSD:
> +			cp.max_latency    = __constant_cpu_to_le16(0xffff);
> +			cp.content_format = cpu_to_le16(setting);

… and use setting here.

I would prefer that we use setting in the common section.

> +			cp.retrans_effort = 0xff;
> +			break;
> +		}
> 
> 		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
> 			     sizeof(cp), &cp);
> @@ -695,7 +709,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;
> 		msg->msg_namelen = 0;

Regards

Marcel

Re: [PATCH v8 5/8] Bluetooth: Use voice setting in defered SCO connection request

[Frédéric DALLEAU]

Marcel,

Le 08/07/2013 21:12, Marcel Holtmann a écrit :
> Hi Fred,

+				__constant_cpu_to_le16(SCO_AIRMODE_TRANSP);
> why do we hardcode here …
>
>> +			cp.retrans_effort = 0x02;
>> +			break;
>> +		case SCO_AIRMODE_CVSD:
>> +			cp.max_latency    = __constant_cpu_to_le16(0xffff);
>> +			cp.content_format = cpu_to_le16(setting);
>
> … and use setting here.
>

SCO_AIRMODE_CVSD needs some additional bits describing Input coding and 
Data format. There is no additional bits for SCO_AIRMODE_TRANSP.

But having it common will fix this ugly line break :)

Fred

[PATCH v8 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,
a setting field is added to ACL connection data to set up the desired
parameters.
Remove usage of hdev->voice_setting in CVSD connection.
Make use of T2 parameters for transparent data.

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 69bdb67..61ca2ce 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..6282c26 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 & SCO_AIRMODE_MASK) {
+	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(conn->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 v8 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,
> a setting field is added to ACL connection data to set up the desired
> parameters.
> Remove usage of hdev->voice_setting in CVSD connection.
> Make use of T2 parameters for transparent data.
> 
> 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 69bdb67..61ca2ce 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..6282c26 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 & SCO_AIRMODE_MASK) {
> +	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(conn->setting);
> +		cp.retrans_effort = 0xff;
> +		break;
> +	}

Same are as in the other patch. Lets assign cp.voice_setting globally and not different in the two cases.

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

Regards

Marcel

[PATCH v8 7/8] Bluetooth: SCO connection fallback

[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.

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.

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 setting should be used, conn->fallback is used.  Bluez only
attempt to reconnect twice. Since, more than 2 fallback are required,
conn->fallback is tested as an alternative measure. We want to fallback only if
conn->fallback is positive. Calling hci_setup_sync with conn->fallback == 0
triggers initial connection attempt.

These setting and the fallback order are described in Bluetooth HFP 1.6
specification p. 101.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 61ca2ce..c4509e2 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 6282c26..3c684c9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -31,6 +31,25 @@
 #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_cvsd[] = {
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a, 1 }, /* S3 */
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007, 2 }, /* S2 */
+	{ EDR_ESCO_MASK | ESCO_EV3,   0x0007, 3 }, /* S1 */
+	{ EDR_ESCO_MASK | ESCO_HV3,   0xffff, 4 }, /* D1 */
+	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff, 0 }, /* D0 */
+};
+
+static const struct sco_param sco_param_wideband[] = {
+	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d, 1 }, /* T2 */
+	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008, 0 }, /* T1 */
+};
+
 static void hci_le_create_connection(struct hci_conn *conn)
 {
 	struct hci_dev *hdev = conn->hdev;
@@ -176,6 +195,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,17 +211,22 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 
 	switch (conn->setting & SCO_AIRMODE_MASK) {
 	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);
-		cp.max_latency    = __constant_cpu_to_le16(0xffff);
+		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  = cpu_to_le16(conn->setting);
-		cp.retrans_effort = 0xff;
+		cp.retrans_effort = 0x01;
+
+		conn->fallback    = param->next;
 		break;
 	}
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0437200..6659af8 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 v8 7/8] Bluetooth: SCO connection fallback

[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.
> 
> 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.
> 
> 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 setting should be used, conn->fallback is used.  Bluez only
> attempt to reconnect twice. Since, more than 2 fallback are required,
> conn->fallback is tested as an alternative measure. We want to fallback only if
> conn->fallback is positive. Calling hci_setup_sync with conn->fallback == 0
> triggers initial connection attempt.
> 
> These setting and the fallback order are described in Bluetooth HFP 1.6
> specification p. 101.

before we do the fallback handling, lets get the basic CVSD vs transparent work done properly. The one thing that I am missing is the handling of SCO/eSCO setup error handling. Like say when using connect() and we try to establish transparent link where not available. Or accept() with defer setup and the transparent link does not work out.

The fallback handling can be a separate patch once the others are all in a mergable state.

Regards

Marcel

Re: [PATCH v8 7/8] Bluetooth: SCO connection fallback

[Frédéric DALLEAU]

Marcel,

Le 08/07/2013 21:23, Marcel Holtmann a écrit :
> Hi Fred,
> before we do the fallback handling, lets get the basic CVSD vs transparent work done properly. The one thing that I am missing is the handling of SCO/eSCO setup error handling. Like say when using connect() and we try to establish transparent link where not available. Or accept() with defer setup and the transparent link does not work out.
>
> The fallback handling can be a separate patch once the others are all in a mergable state.

AFAIR if Accept Synchronous Connection does not specify a compatible
voice setting, Synchronous Connection Complete will return error
2.13 INVALID HCI COMMAND PARAMETERS (0x12).
Setup Synchronous Connection also returns error in Synchronous 
Connection Complete.

This is the reason why it is negociated before in profile.

Regards,
Fred

Re: [PATCH v8 7/8] Bluetooth: SCO connection fallback

[Marcel Holtmann]

Hi Fred,

>> before we do the fallback handling, lets get the basic CVSD vs transparent work done properly. The one thing that I am missing is the handling of SCO/eSCO setup error handling. Like say when using connect() and we try to establish transparent link where not available. Or accept() with defer setup and the transparent link does not work out.
>> 
>> The fallback handling can be a separate patch once the others are all in a mergable state.
> 
> AFAIR if Accept Synchronous Connection does not specify a compatible
> voice setting, Synchronous Connection Complete will return error
> 2.13 INVALID HCI COMMAND PARAMETERS (0x12).
> Setup Synchronous Connection also returns error in Synchronous Connection Complete.
> 
> This is the reason why it is negociated before in profile.

I am not sure I like that. What we want is to return proper error codes when a connect() or accept() + read() fails due to an invalid combination.

Regards

Marcel

[PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Frédéric Dalleau]

Older Bluetooth devices may not support Setup Synchronous Connection. This is
indicated by the eSCO support feature bit. It is not possible to know if the
adapter support eSCO before setting BT_VOICE option since the socket is not
bound to an adapter. An adapter can also be added after the socket is created.
The socket can be bound to an address before adapter is plugged in.

Thus, on a such adapters, if user requested something else than
BT_VOICE_CVSD_16BIT, outgoing connections fail on connect() and returns
-ECONNABORTED. Incoming connections do not fail. However, they should only be
allowed depending on what was specified in Write_Voice_Settings command.

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

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 546b7f6..02a602e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -176,6 +176,11 @@ static int sco_connect(struct sock *sk)
 	else
 		type = SCO_LINK;
 
+	if (type == SCO_LINK && sco_pi(sk)->setting != BT_VOICE_CVSD_16BIT) {
+		err = -ECONNABORTED;
+		goto done;
+	}
+
 	hcon = hci_connect_sco(hdev, type, dst, sco_pi(sk)->setting);
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
-- 
1.7.9.5

Re: [PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Vinicius Costa Gomes]

Hi Frédéric,

On 17:01 Fri 05 Jul, Frédéric Dalleau wrote:
> Older Bluetooth devices may not support Setup Synchronous Connection. This is
> indicated by the eSCO support feature bit. It is not possible to know if the
> adapter support eSCO before setting BT_VOICE option since the socket is not
> bound to an adapter. An adapter can also be added after the socket is created.
> The socket can be bound to an address before adapter is plugged in.

I remember seeing a couple of 2.0 controllers that have Transparent SCO
feature bit set but don't have support for eSCO. 

I wonder if it would be better to use Setup Synchronous Connection if
transparent SCO is needed (and the controller supports it).

Another problem is in the accepting side, we are using Add SCO if the
controller doesn't support eSCO, this fails (IIRC Invalid LMP Parameters 
or somehing) if the other side has requested a SCO link expecting
transparent SCO data.

> 
> Thus, on a such adapters, if user requested something else than
> BT_VOICE_CVSD_16BIT, outgoing connections fail on connect() and returns
> -ECONNABORTED. Incoming connections do not fail. However, they should only be
> allowed depending on what was specified in Write_Voice_Settings command.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
>  net/bluetooth/sco.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 546b7f6..02a602e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -176,6 +176,11 @@ static int sco_connect(struct sock *sk)
>  	else
>  		type = SCO_LINK;
>  
> +	if (type == SCO_LINK && sco_pi(sk)->setting != BT_VOICE_CVSD_16BIT) {
> +		err = -ECONNABORTED;
> +		goto done;
> +	}
> +
>  	hcon = hci_connect_sco(hdev, type, dst, sco_pi(sk)->setting);
>  	if (IS_ERR(hcon)) {
>  		err = PTR_ERR(hcon);
> -- 
> 1.7.9.5
> 
> --
> 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

Re: [PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Frédéric DALLEAU]

Hi Vinicius,

Le 08/07/2013 04:33, Vinicius Costa Gomes a écrit :
> I remember seeing a couple of 2.0 controllers that have Transparent SCO
> feature bit set but don't have support for eSCO.
>
> I wonder if it would be better to use Setup Synchronous Connection if
> transparent SCO is needed (and the controller supports it).

I saw your previous mail about this. There are several other places in
the code (hci_sco_setup, hci_conn_request_evt maybe others) where Bluez
decides whether to use Add_Sco or Setup_Synchronous_Connection based on
lmp_esco_capable() macro. Supporting transparent data on these adapters 
would need these places changed as well.

> Another problem is in the accepting side, we are using Add SCO if the
> controller doesn't support eSCO, this fails (IIRC Invalid LMP Parameters
> or somehing) if the other side has requested a SCO link expecting
> transparent SCO data.

This patch only applies to connecting side. Accepting side should
not be impacted. If voice settings on both side are matching, I don't
see why it would fail.

Regards,
Fred

Re: [PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Marcel Holtmann]

Hi Fred,

> Older Bluetooth devices may not support Setup Synchronous Connection. This is
> indicated by the eSCO support feature bit. It is not possible to know if the
> adapter support eSCO before setting BT_VOICE option since the socket is not
> bound to an adapter. An adapter can also be added after the socket is created.
> The socket can be bound to an address before adapter is plugged in.
> 
> Thus, on a such adapters, if user requested something else than
> BT_VOICE_CVSD_16BIT, outgoing connections fail on connect() and returns
> -ECONNABORTED. Incoming connections do not fail. However, they should only be
> allowed depending on what was specified in Write_Voice_Settings command.
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> net/bluetooth/sco.c |    5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 546b7f6..02a602e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -176,6 +176,11 @@ static int sco_connect(struct sock *sk)
> 	else
> 		type = SCO_LINK;
> 
> +	if (type == SCO_LINK && sco_pi(sk)->setting != BT_VOICE_CVSD_16BIT) {
> +		err = -ECONNABORTED;
> +		goto done;
> +	}
> +

we should check for eSCO and transparent SCO support. We do need both. And we also need to figure out on how to handle the incoming situation correctly,

Is the error ECONNABORTED a correct one.

Regards

Marcel

Re: [PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Frédéric DALLEAU]

Marcel, Vinicius,

Le 08/07/2013 21:25, Marcel Holtmann a écrit :
> Hi Fred,
>
> we should check for eSCO and transparent SCO support. We do need both. And we also need to figure out on how to handle the incoming situation correctly,
>
> Is the error ECONNABORTED a correct one.
>
> Regards
>
> Marcel
>

My understanding of Vinicius comment is that some adapters may not
support esco but still have transparent data support thanks to Setup
Sync Conn. Transparent data on these adapters can be supported for
cheap since the connection establishment would be the same.

If we choose to go for eSCO AND transparent SCO support, then these
adapters get unused features.

One possibility is to check for Setup Synchronous Connection command bit
instead of Esco feature bit. But that makes a ugly bunch of tests :
if cvsd is requested
    check esco feature bit.
if transparent data is requested
    check setup sync conn command bit and transparent data feature bit.

The only problem so far is that I have yet to see these adapters :)

About the error ECONNABORTED, it is what the patch does : abort a 
connection. It is not used for other purposes. If you really want to
change, I'm ok with EOPNOTSUPP, otherwise please suggest.

Regards,
Fred

Re: [PATCH v8 8/8] Bluetooth: Prevent transparent SCO on older devices

[Marcel Holtmann]

Hi Fred,

>> we should check for eSCO and transparent SCO support. We do need both. And we also need to figure out on how to handle the incoming situation correctly,
>> 
>> Is the error ECONNABORTED a correct one.
> 
> My understanding of Vinicius comment is that some adapters may not
> support esco but still have transparent data support thanks to Setup
> Sync Conn. Transparent data on these adapters can be supported for
> cheap since the connection establishment would be the same.
> 
> If we choose to go for eSCO AND transparent SCO support, then these
> adapters get unused features.
> 
> One possibility is to check for Setup Synchronous Connection command bit
> instead of Esco feature bit. But that makes a ugly bunch of tests :
> if cvsd is requested
>   check esco feature bit.
> if transparent data is requested
>   check setup sync conn command bit and transparent data feature bit.
> 
> The only problem so far is that I have yet to see these adapters :)

doing transparent SCO on an adapter without eSCO support is useless. I would not bother supporting this at all. Especially for HFP 1.6 with mSBC we have the fact it mandates eSCO.

> About the error ECONNABORTED, it is what the patch does : abort a connection. It is not used for other purposes. If you really want to
> change, I'm ok with EOPNOTSUPP, otherwise please suggest.

I am not sure which error to use. This is more like an open discussion on getting some valid consensus here. Since as a matter of fact once connect() or accept() + read() fails, we need to re-negotiate the codec via HFP. So that must be possible from the API. Hence we need a clear error code.

Regards

Marcel