[RFC 0/4] sco: Another SCO socket option :)

[RFC 0/4] sco: Another SCO socket option :)

[Frédéric Dalleau]

Hi,

This patch implements codec socket option on SCO sockets.
Currently it proposes T2 codec settins (see HFP 1.6 p.102)


How to test:
$ scotest -W 1 -C 1

on another machine with same patches run:
$ scotest -n $ADDR -C 1

Check result with btmon.

This took more time than I expected for I met the following issues :
* bluetooth-next branch crashed in apparmor after login.
* At some point, unplugging my usb dongle would also crash the kernel. It is
possible this one was a mix between different versions of kernel modules.
* if the connection request is not accepted, the initiator gets in timeout, and
there is a crash. I think this one will need be fixed.

Next step is to implement fallback between T2 and T1. I'm thinking about using
another bit in hconn->flags to store current settings selection.

Let me know what you think.
Best regards,
Frédéric


Frédéric Dalleau (4):
  Bluetooth: Add option for SCO socket codec
  Bluetooth: Add option for SCO socket socket
  Bluetooth: Use codec to create SCO connection
  Bluetooth: Set link parameters for outgoing connections

 include/net/bluetooth/hci_core.h |    6 +++--
 include/net/bluetooth/sco.h      |    2 ++
 net/bluetooth/hci_conn.c         |   24 ++++++++++++++----
 net/bluetooth/hci_event.c        |   23 ++++++++++++++---
 net/bluetooth/l2cap_core.c       |    4 +--
 net/bluetooth/mgmt.c             |    4 +--
 net/bluetooth/sco.c              |   51 ++++++++++++++++++++++++++++++++++++--
 7 files changed, 97 insertions(+), 17 deletions(-)

-- 
1.7.9.5

[RFC 1/4] Bluetooth: Add option for SCO socket codec

[Frédéric Dalleau]

This patch extends the current SCO socket option to add a codec field. This
field is intended to choose between mSBC and CVSD codec at runtime. Incoming
connections will be setup during defered setup. Outgoing connections have to
be setup before connect(). The selected codec is stored in the sco socket info.
This patch declares needed members and implements getsockopt().
---
 include/net/bluetooth/sco.h |    2 ++
 net/bluetooth/sco.c         |    3 +++
 2 files changed, 5 insertions(+)

diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
index 1e35c43..c283de0 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;
+	__u8 codec;
 };
 
 #define SCO_CONNINFO	0x02
@@ -73,6 +74,7 @@ struct sco_conn {
 struct sco_pinfo {
 	struct bt_sock	bt;
 	__u32		flags;
+	__u8		codec;
 	struct sco_conn	*conn;
 };
 
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index eea17cd..c2c0c13 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -427,6 +427,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)->codec = 0;
+
 	setup_timer(&sk->sk_timer, sco_sock_timeout, (unsigned long)sk);
 
 	bt_sock_link(&sco_sk_list, sk);
@@ -745,6 +747,7 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname, char __user
 		}
 
 		opts.mtu = sco_pi(sk)->conn->mtu;
+		opts.codec = sco_pi(sk)->codec;
 
 		BT_DBG("mtu %d", opts.mtu);
 
-- 
1.7.9.5

[RFC 2/4] Bluetooth: Add option for SCO socket socket

[Frédéric Dalleau]

This patch implements setsockopt().
---
 net/bluetooth/sco.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index c2c0c13..b363712 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -687,6 +687,47 @@ 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.codec = 0;
+
+		len = min_t(unsigned int, sizeof(opts), optlen);
+		if (copy_from_user((char *) &opts, optval, len)) {
+			err = -EFAULT;
+			break;
+		}
+
+		sco_pi(sk)->codec = opts.codec;
+		BT_DBG("codec %d", opts.codec);
+		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;
@@ -695,6 +736,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) {
-- 
1.7.9.5

[RFC 3/4] Bluetooth: Use codec to create SCO connection

[Frédéric Dalleau]

When an incoming SCO connection is requested, check the selected codec, and
reply appropriately. Codec should have been negotiated previously.
Note that this patch only changes the reply for defered setup.
---
 include/net/bluetooth/hci_core.h |    2 +-
 net/bluetooth/hci_event.c        |   23 +++++++++++++++++++----
 net/bluetooth/sco.c              |    2 +-
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2f2b743..cb41552 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -577,7 +577,7 @@ 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);
+void hci_conn_accept(struct hci_conn *conn, int mask, int codec);
 
 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 705078a..57ed7d3 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2047,7 +2047,7 @@ unlock:
 	hci_conn_check_pending(hdev);
 }
 
-void hci_conn_accept(struct hci_conn *conn, int mask)
+void hci_conn_accept(struct hci_conn *conn, int mask, int codec)
 {
 	struct hci_dev *hdev = conn->hdev;
 
@@ -2070,13 +2070,28 @@ void hci_conn_accept(struct hci_conn *conn, int mask)
 		struct hci_cp_accept_sync_conn_req cp;
 
 		bacpy(&cp.bdaddr, &conn->dst);
-		cp.pkt_type = cpu_to_le16(conn->pkt_type);
 
+		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;
+
+		switch (codec) {
+		case 0:
+			cp.max_latency    = __constant_cpu_to_le16(0xffff);
+			cp.retrans_effort = 0xff;
+			break;
+		case 1 /* mSBC */:
+			/* Transparent */
+			cp.content_format |= cpu_to_le16(3);
+			 /* 2EV3 unsupported */
+			if (conn->pkt_type & ESCO_2EV3)
+				cp.max_latency = __constant_cpu_to_le16(0x0008);
+			else
+				cp.max_latency = __constant_cpu_to_le16(0x000D);
+			cp.retrans_effort = 0x02;
+			break;
+		}
 
 		hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
 			     sizeof(cp), &cp);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index b363712..be3e0b2 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -675,7 +675,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);
+		hci_conn_accept(pi->conn->hcon, 0, pi->codec);
 		sk->sk_state = BT_CONFIG;
 
 		release_sock(sk);
-- 
1.7.9.5

[RFC 4/4] Bluetooth: Set link parameters for outgoing connections

[Frédéric Dalleau]

In order to establish the connection, the outgoing connection must also
request the codec. Here we need to set a bit in ACL connection flag to set up
the correct parameters if the ACL connection is not up yet.
---
 include/net/bluetooth/hci_core.h |    4 +++-
 net/bluetooth/hci_conn.c         |   24 +++++++++++++++++++-----
 net/bluetooth/l2cap_core.c       |    4 ++--
 net/bluetooth/mgmt.c             |    4 ++--
 net/bluetooth/sco.c              |    2 +-
 5 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cb41552..e286196 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -438,6 +438,7 @@ enum {
 	HCI_CONN_SSP_ENABLED,
 	HCI_CONN_POWER_SAVE,
 	HCI_CONN_REMOTE_OOB,
+	HCI_CONN_SCO_TRANSP_DATA,
 };
 
 static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
@@ -585,7 +586,8 @@ void hci_chan_list_flush(struct hci_conn *conn);
 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);
+			     __u8 dst_type, __u8 sec_level,
+			     __u8 auth_type, __u8 codec);
 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 25bfce0..1636dd5 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -179,9 +179,16 @@ 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);
-	cp.max_latency    = __constant_cpu_to_le16(0xffff);
 	cp.voice_setting  = cpu_to_le16(hdev->voice_setting);
-	cp.retrans_effort = 0xff;
+
+	if (test_and_clear_bit(HCI_CONN_SCO_TRANSP_DATA, &conn->flags)) {
+		cp.voice_setting |= 3;
+		cp.max_latency    = __constant_cpu_to_le16(0x000d);
+		cp.retrans_effort = 0x02;
+	} else {
+		cp.max_latency    = __constant_cpu_to_le16(0xffff);
+		cp.retrans_effort = 0xff;
+	}
 
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
@@ -552,7 +559,8 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 }
 
 static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
-				bdaddr_t *dst, u8 sec_level, u8 auth_type)
+					bdaddr_t *dst, u8 sec_level,
+					u8 auth_type, u8 codec)
 {
 	struct hci_conn *acl;
 	struct hci_conn *sco;
@@ -575,6 +583,10 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
 
 	hci_conn_hold(sco);
 
+	if (codec)
+		set_bit(HCI_CONN_SCO_TRANSP_DATA, &sco->flags);
+
+
 	if (acl->state == BT_CONNECTED &&
 	    (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
 		set_bit(HCI_CONN_POWER_SAVE, &acl->flags);
@@ -594,7 +606,8 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
 
 /* Create SCO, ACL or LE connection. */
 struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
-			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
+			     __u8 dst_type, __u8 sec_level, __u8 auth_type,
+			     __u8 codec)
 {
 	BT_DBG("%s dst %pMR type 0x%x", hdev->name, dst, type);
 
@@ -605,7 +618,8 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 		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 hci_connect_sco(hdev, type, dst, sec_level, auth_type,
+				       codec);
 	}
 
 	return ERR_PTR(-EINVAL);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b52f66d..2ca70a4 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1690,10 +1690,10 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 	if (chan->dcid == L2CAP_CID_LE_DATA)
 		hcon = hci_connect(hdev, LE_LINK, dst, dst_type,
-				   chan->sec_level, auth_type);
+				   chan->sec_level, auth_type, 0);
 	else
 		hcon = hci_connect(hdev, ACL_LINK, dst, dst_type,
-				   chan->sec_level, auth_type);
+				   chan->sec_level, auth_type, 0);
 
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5d0ef75..d03405d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1917,10 +1917,10 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	if (cp->addr.type == BDADDR_BREDR)
 		conn = hci_connect(hdev, ACL_LINK, &cp->addr.bdaddr,
-				   cp->addr.type, sec_level, auth_type);
+				   cp->addr.type, sec_level, auth_type, 0);
 	else
 		conn = hci_connect(hdev, LE_LINK, &cp->addr.bdaddr,
-				   cp->addr.type, sec_level, auth_type);
+				   cp->addr.type, sec_level, auth_type, 0);
 
 	memset(&rp, 0, sizeof(rp));
 	bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index be3e0b2..e86ae29 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -186,7 +186,7 @@ static int sco_connect(struct sock *sk)
 		type = SCO_LINK;
 
 	hcon = hci_connect(hdev, type, dst, BDADDR_BREDR, BT_SECURITY_LOW,
-			   HCI_AT_NO_BONDING);
+			   HCI_AT_NO_BONDING, sco_pi(sk)->codec);
 	if (IS_ERR(hcon)) {
 		err = PTR_ERR(hcon);
 		goto done;
-- 
1.7.9.5

Re: [RFC 0/4] sco: Another SCO socket option :)

[Arnaud Mouiche]

Hello Frédéric,

my natural feeling is to be generic as much as possible, and to not try 
to implement directly a particular profile in the kernel:
ex:
- I'm a hobbyist or academic and I want to use SCO socket for something 
else than standard profiles like an asymmetric synchronous connection (?)
- tomorrow, a new X or Y codec will be selected by the SIG and some new 
settings will be required (may be OPUS, now we have a high quality 
patent free, royalty-free and standardized codec available. 
http://opus-codec.org/)
- I've a BT module doesn't behave correctly when using exactly the 
parameters specified by the HFP specs (I think I have already seen one 
but don't remember which)

also, BT specs provided a generic API (HCI_Setup_Synchronous_Connection 
command & co). why try to remove the flexibility ?

Except this "codec only vs. every parameter" question, I like the 
implementation.

regards,
arnaud


On 11/28/2012 07:28 PM, Frédéric Dalleau wrote:
> Hi,
>
> This patch implements codec socket option on SCO sockets.
> Currently it proposes T2 codec settins (see HFP 1.6 p.102)
>
>
> How to test:
> $ scotest -W 1 -C 1
>
> on another machine with same patches run:
> $ scotest -n $ADDR -C 1
>
> Check result with btmon.
>
> This took more time than I expected for I met the following issues :
> * bluetooth-next branch crashed in apparmor after login.
> * At some point, unplugging my usb dongle would also crash the kernel. It is
> possible this one was a mix between different versions of kernel modules.
> * if the connection request is not accepted, the initiator gets in timeout, and
> there is a crash. I think this one will need be fixed.
>
> Next step is to implement fallback between T2 and T1. I'm thinking about using
> another bit in hconn->flags to store current settings selection.
>
> Let me know what you think.
> Best regards,
> Frédéric
>
>
> Frédéric Dalleau (4):
>    Bluetooth: Add option for SCO socket codec
>    Bluetooth: Add option for SCO socket socket
>    Bluetooth: Use codec to create SCO connection
>    Bluetooth: Set link parameters for outgoing connections
>
>   include/net/bluetooth/hci_core.h |    6 +++--
>   include/net/bluetooth/sco.h      |    2 ++
>   net/bluetooth/hci_conn.c         |   24 ++++++++++++++----
>   net/bluetooth/hci_event.c        |   23 ++++++++++++++---
>   net/bluetooth/l2cap_core.c       |    4 +--
>   net/bluetooth/mgmt.c             |    4 +--
>   net/bluetooth/sco.c              |   51 ++++++++++++++++++++++++++++++++++++--
>   7 files changed, 97 insertions(+), 17 deletions(-)
>

Re: [RFC 4/4] Bluetooth: Set link parameters for outgoing connections

[Andrei Emeltchenko]

Hi Frédéric,

On Wed, Nov 28, 2012 at 07:28:36PM +0100, Frédéric Dalleau wrote:
> In order to establish the connection, the outgoing connection must also
> request the codec. Here we need to set a bit in ACL connection flag to set up
> the correct parameters if the ACL connection is not up yet.

...

> @@ -585,7 +586,8 @@ void hci_chan_list_flush(struct hci_conn *conn);
>  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);
> +			     __u8 dst_type, __u8 sec_level,
> +			     __u8 auth_type, __u8 codec);

somehow I do not like this new parameter for hci_connect as it is only
needed for SCO.

Now I am not sure we need hci_connect super-function if it only calls
appropriate hci_connect_{sco,acl,le} based on type.

...
> @@ -552,7 +559,8 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>  }
>  
>  static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
> -				bdaddr_t *dst, u8 sec_level, u8 auth_type)
> +					bdaddr_t *dst, u8 sec_level,
> +					u8 auth_type, u8 codec)

what about directly calling hci_connect_sco with the parameter?

...

Best regards 
Andrei Emeltchenko