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

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

[Frédéric Dalleau]

Hi,

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 SCO socket voice_setting 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/hci_core.h |    5 +-
 include/net/bluetooth/sco.h      |    2 +
 net/bluetooth/hci_conn.c         |   36 +++++++++----
 net/bluetooth/hci_event.c        |   39 +-------------
 net/bluetooth/sco.c              |  106 ++++++++++++++++++++++++++++++++++++--
 5 files changed, 137 insertions(+), 51 deletions(-)

-- 
1.7.9.5

[PATCH v4 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.
---
 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 358a698..e6aa0f1 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 1385807..2fe3ccd 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1752,42 +1752,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 183b852..2738014 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -653,6 +653,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)
 {
@@ -663,7 +699,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

[PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Frédéric Dalleau]

This patch extends the current SCO socket option to add a 'voice_setting'
member. This member is intended to choose data type at runtime.
Incoming connections will be setup during defered setup. Outgoing connections
have to be setup before connect(). The desired setting is stored in the sco
socket info.
This patch declares needed members, modifies getsockopt() and implements
setsockopt(). Setting the mtu is not supported.
---
 include/net/bluetooth/sco.h |    2 ++
 net/bluetooth/sco.c         |   54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index 1e35c43..41dbdfa 100644
--- a/include/net/bluetooth/sco.h
+++ b/include/net/bluetooth/sco.h
@@ -43,6 +43,7 @@ struct sockaddr_sco {
 #define SCO_OPTIONS	0x01
 struct sco_options {
 	__u16 mtu;
+	__u16 voice_setting;
 };
 
 #define SCO_CONNINFO	0x02
@@ -73,6 +74,7 @@ struct sco_conn {
 struct sco_pinfo {
 	struct bt_sock	bt;
 	__u32		flags;
+	__u16		voice_setting;
 	struct sco_conn	*conn;
 };
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 2738014..dc92073 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)->voice_setting = 0;
+
 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
 
 	bt_sock_link(&sco_sk_list, sk);
@@ -711,6 +713,54 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
 }
 
+static int sco_sock_setsockopt_old(struct socket *sock, int optname,
+				   char __user *optval, unsigned int optlen)
+{
+	struct sock *sk = sock->sk;
+	struct sco_options opts;
+	int len, err = 0;
+
+	BT_DBG("sk %p", sk);
+
+	lock_sock(sk);
+
+	switch (optname) {
+	case SCO_OPTIONS:
+		if (sk->sk_state != BT_OPEN &&
+		    sk->sk_state != BT_BOUND &&
+		    sk->sk_state != BT_CONNECT2) {
+			err = -EINVAL;
+			break;
+		}
+
+		opts.mtu = 0;
+		opts.voice_setting = 0;
+
+		len = min_t(unsigned int, sizeof(opts), optlen);
+		if (copy_from_user((char *) &opts, optval, len)) {
+			err = -EFAULT;
+			break;
+		}
+
+		if (opts.mtu != 0) {
+			err = -EINVAL;
+			break;
+		}
+
+		BT_DBG("voice_setting %d", opts.voice_setting);
+
+		sco_pi(sk)->voice_setting = opts.voice_setting;
+		break;
+
+	default:
+		err = -ENOPROTOOPT;
+		break;
+	}
+
+	release_sock(sk);
+	return err;
+}
+
 static int sco_sock_setsockopt(struct socket *sock, int level, int optname, char __user *optval, unsigned int optlen)
 {
 	struct sock *sk = sock->sk;
@@ -719,6 +769,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname, char
 
 	BT_DBG("sk %p", sk);
 
+	if (level == SOL_SCO)
+		return sco_sock_setsockopt_old(sock, optname, optval, optlen);
+
 	lock_sock(sk);
 
 	switch (optname) {
@@ -771,6 +824,7 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname, char __user
 		}
 
 		opts.mtu = sco_pi(sk)->conn->mtu;
+		opts.voice_setting = sco_pi(sk)->voice_setting;
 
 		BT_DBG("mtu %d", opts.mtu);
 
-- 
1.7.9.5

[PATCH v4 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.
---
 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 e6aa0f1..69785c2 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 voice_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 b9f9016..ad551e2 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -551,13 +551,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 voice_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;
 
@@ -603,9 +603,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 dc92073..7593a8e 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)->voice_setting);
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
 		goto done;
-- 
1.7.9.5

[PATCH v4 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. Mode 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.
---
 net/bluetooth/sco.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 7593a8e..50be879 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -654,7 +654,8 @@ 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 voice_setting)
 {
 	struct hci_dev *hdev = conn->hdev;
 
@@ -681,9 +682,19 @@ 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;
+
+		if (voice_setting == 0x0003) {
+			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;
+		} else if (voice_setting == 0x0000) {
+			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);
@@ -700,7 +711,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->voice_setting);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

[PATCH v4 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.
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |   19 +++++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 69785c2..70e0364 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		voice_setting;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ad551e2..95c69a6 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -175,13 +175,22 @@ 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;
+
+	if (conn->voice_setting == 0x0003) {
+		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;
+	} else {
+		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;
+	}
 
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
@@ -575,6 +584,8 @@ struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
 
 	hci_conn_hold(sco);
 
+	sco->voice_setting = voice_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

[PATCH v4 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, but still test conn->fallback as a preventive
measure.
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_conn.c         |   12 ++++++++++--
 net/bluetooth/hci_event.c        |    3 ++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 70e0364..ba3fd72 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		voice_setting;
+	__u8		fallback;
 	unsigned long	flags;
 
 	__u8		remote_cap;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 95c69a6..83b38e3 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -179,13 +179,21 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
 	cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
 
-	if (conn->voice_setting == 0x0003) {
+	if (conn->fallback == 0 && conn->voice_setting == 0x0003) {
 		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;
-	} else {
+		conn->fallback = 1;
+	} else if (conn->fallback == 1 && conn->voice_setting == 0x0003) {
+		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK |
+							   ESCO_EV3);
+		cp.max_latency    = __constant_cpu_to_le16(0x0007);
+		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
+		cp.retrans_effort = 0x02;
+		conn->fallback = 2;
+	} else if (conn->voice_setting == 0x0000) {
 		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);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2fe3ccd..f3b070e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2953,11 +2953,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 != 2) {
 			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 v4 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.

mention that you are making it static because of that.

And we need a Signed-off-by line.

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

Regards

Marcel

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Marcel Holtmann]

Hi Fred,

> This patch extends the current SCO socket option to add a 'voice_setting'
> member. This member is intended to choose data type at runtime.
> Incoming connections will be setup during defered setup. Outgoing connections
> have to be setup before connect(). The desired setting is stored in the sco
> socket info.
> This patch declares needed members, modifies getsockopt() and implements
> setsockopt(). Setting the mtu is not supported.

Signed-off-by line.

> ---
> include/net/bluetooth/sco.h |    2 ++
> net/bluetooth/sco.c         |   54 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+)
> 
> diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
> index 1e35c43..41dbdfa 100644
> --- a/include/net/bluetooth/sco.h
> +++ b/include/net/bluetooth/sco.h
> @@ -43,6 +43,7 @@ struct sockaddr_sco {
> #define SCO_OPTIONS	0x01
> struct sco_options {
> 	__u16 mtu;
> +	__u16 voice_setting;
> };

I find this parameter name a bit long. What about just "setting" or "settings" or "voice". I am open for suggestions.

Also should this be part of options or the socket address structure.

Another option would be to introduce a SCO_SETTINGS or SCO_VOICE socket option. With just this one parameter.

Since the default value of voice setting is not 0x0000, it might make actually more sense to introduce a new socket option. Playing the memset handling would only work nicely if the default would be 0x0000, but it isn't.

Regards

Marcel

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Dalleau, Frederic]

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Hi Marcel,

On Tue, Mar 26, 2013 at 9:22 PM, Marcel Holtmann <marcel@holtmann.org>wrote:

> I find this parameter name a bit long. What about just "setting" or
> "settings" or "voice". I am open for suggestions.
>
> Also should this be part of options or the socket address structure.
>
> Another option would be to introduce a SCO_SETTINGS or SCO_VOICE socket
> option. With just this one parameter.
>
> Since the default value of voice setting is not 0x0000, it might make
> actually more sense to introduce a new socket option. Playing the memset
> handling would only work nicely if the default would be 0x0000, but it
> isn't.
>

I agree that having a separate socket option is better. IMHO, having it in
the
sockaddr is not enough because you don't know in advance what stream is
flowing for incoming connections. It could improve the API for outgoing
connections.

My concern is how do we handle user values.
If the user do not set this option, hci_conn->settings contains 0x0000.
In this case, it is possible to start the sco connection as we have it
today.
If the user set 0x0060 (cvsd 16 bits) or 0x0003 (transparent data), then it
is
possible to start S3, S2, S1 or T2, T1 sequences.

For other values, IMHO, returning an error would be the way to go.

If the above is fine for you, we can discuss the naming. What about the
following ?

#define SCO_SETTINGS  0x03
struct sco_settings {
        __u16           settings;
};

Regards,
Fred

[-- Attachment #2: Type: text/html, Size: 1947 bytes --]

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Frédéric DALLEAU]

Hi Marcel

Le 26/03/2013 21:22, Marcel Holtmann a écrit :
I find this parameter name a bit long. What about just "setting" or 
"settings" or "voice". I am open for suggestions.
>
> Also should this be part of options or the socket address structure.
>
> Another option would be to introduce a SCO_SETTINGS or SCO_VOICE socket option. With just this one parameter.
>
> Since the default value of voice setting is not 0x0000, it might make actually more sense to introduce a new socket option. Playing the memset handling would only work nicely if the default would be 0x0000, but it isn't.
>
> Regards
>
> Marcel
>

  I agree that having a separate socket option is better. IMHO, having 
it in the
sockaddr is not enough because you don't know in advance what stream is
flowing for incoming connections. It could improve the API for outgoing
connections.

My concern is how do we handle user values.
If the user do not set this option, hci_conn->settings contains 0x0000.
In this case, it is possible to start the sco connection as we have it 
today.
If the user set 0x0060 (cvsd 16 bits) or 0x0003 (transparent data), then 
it is
possible to start S3, S2, S1 or T2, T1 sequences.

For other values, IMHO, returning an error would be the way to go.

If the above is fine for you, we can discuss the naming. What about the
following ?

#define SCO_SETTINGS  0x03
struct sco_settings {
         __u16           settings;
};

Regards,
Frédéric

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Marcel Holtmann]

Hi Fred,

> I find this parameter name a bit long. What about just "setting" or "settings" or "voice". I am open for suggestions.
>> 
>> Also should this be part of options or the socket address structure.
>> 
>> Another option would be to introduce a SCO_SETTINGS or SCO_VOICE socket option. With just this one parameter.
>> 
>> Since the default value of voice setting is not 0x0000, it might make actually more sense to introduce a new socket option. Playing the memset handling would only work nicely if the default would be 0x0000, but it isn't.
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> I agree that having a separate socket option is better. IMHO, having it in the
> sockaddr is not enough because you don't know in advance what stream is
> flowing for incoming connections. It could improve the API for outgoing
> connections.

fair enough. Being able to set a socket option before calling accept() allows us to pick and choose the SCO air mode we require.

> My concern is how do we handle user values.
> If the user do not set this option, hci_conn->settings contains 0x0000.
> In this case, it is possible to start the sco connection as we have it today.
> If the user set 0x0060 (cvsd 16 bits) or 0x0003 (transparent data), then it is
> possible to start S3, S2, S1 or T2, T1 sequences.
> 
> For other values, IMHO, returning an error would be the way to go.

The default value for voice settings comes from hci_dev and is normally 0x0060. So that would be the value that we have initially. Which means CVSD air encoding.

If you create a SCO socket with socket() and call getsockopt on it, then it should return the default voice setting from the controller. Actually our global default and only once you bound it with bind() it will tell you the controller specific value. And with Synchronous setup, it also does not matter much since there it is part of the HCI command anyway. It does get a bit tricky for legacy controllers that do not support eSCO. Here we would need to write default voice setting first before calling add SCO.

And yes, we can limit the possible valid values. It is more important to also return the value we are getting back from the kernel. So that systems like PA know exactly what kind of link is configured.

> If the above is fine for you, we can discuss the naming. What about the
> following ?
> 
> #define SCO_SETTINGS  0x03
> struct sco_settings {
>        __u16           settings;
> };

That is my current thinking. However we might start using SOL_BLUETOOTH and start using BT_VOICE or similar as a socket option. I do want to be able to retire SOL_SCO and SOL_L2CAP at some point.

Regards

Marcel

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Frédéric DALLEAU]

Hi Marcel,

 >> #define SCO_SETTINGS  0x03
>> struct sco_settings {
>>         __u16           settings;
>> };
>
> That is my current thinking. However we might start using SOL_BLUETOOTH and start using BT_VOICE or similar as a socket option. I do want to be able to retire SOL_SCO and SOL_L2CAP at some point.

I just forgot about this because I don't use it and this API satisfy my 
needs, but it does not allow to distinguish between host side and 
adapter side mSBC.
Do you still care about this?

Thanks,
Fred

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Marcel Holtmann]

Hi Fred,

> >> #define SCO_SETTINGS  0x03
>>> struct sco_settings {
>>>        __u16           settings;
>>> };
>> 
>> That is my current thinking. However we might start using SOL_BLUETOOTH and start using BT_VOICE or similar as a socket option. I do want to be able to retire SOL_SCO and SOL_L2CAP at some point.
> 
> I just forgot about this because I don't use it and this API satisfy my needs, but it does not allow to distinguish between host side and adapter side mSBC.
> Do you still care about this?

what do you mean by host side and adapter side difference? I am not following.

Regards

Marcel

Re: [PATCH v4 2/6] Bluetooth: Add SCO socket voice_setting option

[Frédéric DALLEAU]

Le 09/04/2013 17:30, Marcel Holtmann a écrit :
> Hi Fred,
>
>>>> #define SCO_SETTINGS  0x03
>>>> struct sco_settings {
>>>>         __u16           settings;
>>>> };
>>>
>>> That is my current thinking. However we might start using SOL_BLUETOOTH and start using BT_VOICE or similar as a socket option. I do want to be able to retire SOL_SCO and SOL_L2CAP at some point.
>>
>> I just forgot about this because I don't use it and this API satisfy my needs, but it does not allow to distinguish between host side and adapter side mSBC.
>> Do you still care about this?
>
> what do you mean by host side and adapter side difference? I am not following.

I was thinking that an adapter could do some adapter side mSBC similar 
to the way SCO over PCM works. (ie when connected the adapter 
automatically encode and forward packet to/from his PCM port).
If it doesn't make sense, just forget about it.

The socket option would look like this (in bluetooth.h) :

#define BT_VOICE  11
struct bt_voice {
	__u16 setting;
};

#define BT_VOICE_TRANSPARENT  0x0003
#define BT_VOICE_CVSD         0x0060

Regards,
Fred