[PATCH v0 0/6] Multiple Bluetooth SCO connections

[PATCH v0 0/6] Multiple Bluetooth SCO connections

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patch series includes patches that have been useful to connect two HCI-based SCO links simultaneously. This can be used for example to connect to HSP headsets at the same time.

The patch series is divided in three groups: kernel patches, BlueZ userspace patches and PulseAudio patches for module-bluetooth-device.

The kernel patches include some code cleanup and more importantly a dynamically changing alternate setting in btusb driver. These ideas have been taken from the patches I found in [1]. The last patch, “Bluetooth: Remove outgoing MTU check” should be considered with care, since there probably are better approaches to solve this (WIP).

The BlueZ userspace patches add some necessary infrastructure to support such use-cases.

The PulseAudio patches provide some changes to be able to test the rest of the code. The first three patches have been reused from a previously submitted patch series, and only the last two patches are relevant for this purpose. They provide some simple workarounds and should not be considered a proper solution.

As I said, the easiest may to test these patches is by using two Bluetooth headsets. You should use the Media API (Enable=Media in audio.conf), connect both headsets, and use pacmd to set their profile to hsp.

[1] http://bluetooth-alsa.sourceforge.net/future.html

Mikel Astiz (6):
  Bluetooth: Use unsigned int instead of signed int
  Bluetooth: Remove unnecessary check
  Bluetooth: Remove unused HCI event handling
  Bluetooth: Simplify outgoing SCO scheduling code
  Bluetooth: btusb: Dynamic alternate setting
  Bluetooth: Remove outgoing MTU check

 drivers/bluetooth/btusb.c |   13 +++++-
 net/bluetooth/hci_core.c  |  100 ++++++++++++++-------------------------------
 net/bluetooth/hci_event.c |    6 ---
 net/bluetooth/sco.c       |   14 ++-----
 4 files changed, 46 insertions(+), 87 deletions(-)

-- 
1.7.7.6

[PATCH v0 1/6] Bluetooth: Use unsigned int instead of signed int

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The involved values are all unsigned and thus unsigned int should be
used instead of signed int. Assigning ~0 to a signed int results in -1,
which is confusing and error-prone, while the code is trying to set the
maximum value possible.

The code still works because the C standard defines that unsigned
comparison will be performed in these cases, when comparing an unsigned
int and a signed int.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 net/bluetooth/hci_core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 52c7abf..43760f1 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2336,7 +2336,7 @@ static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn *conn = NULL, *c;
-	int num = 0, min = ~0;
+	unsigned int num = 0, min = ~0;
 
 	/* We don't have to lock device here. Connections are always
 	 * added and removed with TX task disabled. */
@@ -2417,7 +2417,7 @@ static inline struct hci_chan *hci_chan_sent(struct hci_dev *hdev, __u8 type,
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_chan *chan = NULL;
-	int num = 0, min = ~0, cur_prio = 0;
+	unsigned int num = 0, min = ~0, cur_prio = 0;
 	struct hci_conn *conn;
 	int cnt, q, conn_num = 0;
 
-- 
1.7.7.6

[PATCH v0 2/6] Bluetooth: Remove unnecessary check

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The function already fails if the given size is greater than the MTU, so
there is no need to consider that case afterwards.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 net/bluetooth/sco.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 8bf26d1..b48370b 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -234,7 +234,7 @@ static inline int sco_send_frame(struct sock *sk, struct msghdr *msg, int len)
 {
 	struct sco_conn *conn = sco_pi(sk)->conn;
 	struct sk_buff *skb;
-	int err, count;
+	int err;
 
 	/* Check outgoing MTU */
 	if (len > conn->mtu)
@@ -242,20 +242,18 @@ static inline int sco_send_frame(struct sock *sk, struct msghdr *msg, int len)
 
 	BT_DBG("sk %p len %d", sk, len);
 
-	count = min_t(unsigned int, conn->mtu, len);
-	skb = bt_skb_send_alloc(sk, count,
-			msg->msg_flags & MSG_DONTWAIT, &err);
+	skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		return err;
 
-	if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count)) {
+	if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {
 		kfree_skb(skb);
 		return -EFAULT;
 	}
 
 	hci_send_sco(conn->hcon, skb);
 
-	return count;
+	return len;
 }
 
 static inline void sco_recv_frame(struct sco_conn *conn, struct sk_buff *skb)
-- 
1.7.7.6

[PATCH v0 3/6] Bluetooth: Remove unused HCI event handling

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

Function hci_num_comp_pkts_evt() doesn't need to handle SCO connections
unless SCO flow control is explicitly enabled, as described in the
Bluetooth Core Specification, Volume 4, Part B, section 7.7.19.

Even in the unlikely case that the hardware supports this feature, the
kernel does not enable it.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 net/bluetooth/hci_event.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bb6d802..78f0d58 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2538,12 +2538,6 @@ static inline void hci_num_comp_pkts_evt(struct hci_dev *hdev, struct sk_buff *s
 			}
 			break;
 
-		case SCO_LINK:
-			hdev->sco_cnt += count;
-			if (hdev->sco_cnt > hdev->sco_pkts)
-				hdev->sco_cnt = hdev->sco_pkts;
-			break;
-
 		default:
 			BT_ERR("Unknown type %d conn %p", conn->type, conn);
 			break;
-- 
1.7.7.6

[PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

This patch refactors the SCO scheduling algorithm to remove code that is
not making any difference. There is no HCI SCO flow control in the
kernel so the scheduling can be simplified.

All data produced by userspace is sent to the controller without any
flow control. This was also the case before this patch, but now it's
more obvious and readable.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 net/bluetooth/hci_core.c |   98 ++++++++++++++--------------------------------
 1 files changed, 30 insertions(+), 68 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 43760f1..7eb8c3f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2331,66 +2331,6 @@ EXPORT_SYMBOL(hci_send_sco);
 
 /* ---- HCI TX task (outgoing data) ---- */
 
-/* HCI Connection scheduler */
-static inline struct hci_conn *hci_low_sent(struct hci_dev *hdev, __u8 type, int *quote)
-{
-	struct hci_conn_hash *h = &hdev->conn_hash;
-	struct hci_conn *conn = NULL, *c;
-	unsigned int num = 0, min = ~0;
-
-	/* We don't have to lock device here. Connections are always
-	 * added and removed with TX task disabled. */
-
-	rcu_read_lock();
-
-	list_for_each_entry_rcu(c, &h->list, list) {
-		if (c->type != type || skb_queue_empty(&c->data_q))
-			continue;
-
-		if (c->state != BT_CONNECTED && c->state != BT_CONFIG)
-			continue;
-
-		num++;
-
-		if (c->sent < min) {
-			min  = c->sent;
-			conn = c;
-		}
-
-		if (hci_conn_num(hdev, type) == num)
-			break;
-	}
-
-	rcu_read_unlock();
-
-	if (conn) {
-		int cnt, q;
-
-		switch (conn->type) {
-		case ACL_LINK:
-			cnt = hdev->acl_cnt;
-			break;
-		case SCO_LINK:
-		case ESCO_LINK:
-			cnt = hdev->sco_cnt;
-			break;
-		case LE_LINK:
-			cnt = hdev->le_mtu ? hdev->le_cnt : hdev->acl_cnt;
-			break;
-		default:
-			cnt = 0;
-			BT_ERR("Unknown link type");
-		}
-
-		q = cnt / num;
-		*quote = q ? q : 1;
-	} else
-		*quote = 0;
-
-	BT_DBG("conn %p quote %d", conn, *quote);
-	return conn;
-}
-
 static inline void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
 {
 	struct hci_conn_hash *h = &hdev->conn_hash;
@@ -2663,17 +2603,26 @@ static inline void hci_sched_acl(struct hci_dev *hdev)
 /* Schedule SCO */
 static inline void hci_sched_sco(struct hci_dev *hdev)
 {
+	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn *conn;
-	struct sk_buff *skb;
-	int quote;
 
 	BT_DBG("%s", hdev->name);
 
 	if (!hci_conn_num(hdev, SCO_LINK))
 		return;
 
-	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, SCO_LINK, &quote))) {
-		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(conn, &h->list, list) {
+		struct sk_buff *skb;
+
+		if (conn->type != SCO_LINK)
+			continue;
+
+		if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+			continue;
+
+		while ((skb = skb_dequeue(&conn->data_q)) != NULL) {
 			BT_DBG("skb %p len %d", skb, skb->len);
 			hci_send_frame(skb);
 
@@ -2682,21 +2631,32 @@ static inline void hci_sched_sco(struct hci_dev *hdev)
 				conn->sent = 0;
 		}
 	}
+
+	rcu_read_unlock();
 }
 
 static inline void hci_sched_esco(struct hci_dev *hdev)
 {
+	struct hci_conn_hash *h = &hdev->conn_hash;
 	struct hci_conn *conn;
-	struct sk_buff *skb;
-	int quote;
 
 	BT_DBG("%s", hdev->name);
 
 	if (!hci_conn_num(hdev, ESCO_LINK))
 		return;
 
-	while (hdev->sco_cnt && (conn = hci_low_sent(hdev, ESCO_LINK, &quote))) {
-		while (quote-- && (skb = skb_dequeue(&conn->data_q))) {
+	rcu_read_lock();
+
+	list_for_each_entry_rcu(conn, &h->list, list) {
+		struct sk_buff *skb;
+
+		if (conn->type != ESCO_LINK)
+			continue;
+
+		if (conn->state != BT_CONNECTED && conn->state != BT_CONFIG)
+			continue;
+
+		while ((skb = skb_dequeue(&conn->data_q)) != NULL) {
 			BT_DBG("skb %p len %d", skb, skb->len);
 			hci_send_frame(skb);
 
@@ -2705,6 +2665,8 @@ static inline void hci_sched_esco(struct hci_dev *hdev)
 				conn->sent = 0;
 		}
 	}
+
+	rcu_read_unlock();
 }
 
 static inline void hci_sched_le(struct hci_dev *hdev)
-- 
1.7.7.6

[PATCH v0 5/6] Bluetooth: btusb: Dynamic alternate setting

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

The alternate setting must be dynamically set according to the number of
active SCO links, and the bit depth of the audio. The possible values
for the alternate setting are described in the Bluetooth Core
Specification, Volume 4, Part B, section 2.1.1.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 drivers/bluetooth/btusb.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3311b81..10d02fa 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -849,6 +849,7 @@ static void btusb_work(struct work_struct *work)
 {
 	struct btusb_data *data = container_of(work, struct btusb_data, work);
 	struct hci_dev *hdev = data->hdev;
+	int new_alts;
 	int err;
 
 	if (hdev->conn_hash.sco_num > 0) {
@@ -862,11 +863,19 @@ static void btusb_work(struct work_struct *work)
 
 			set_bit(BTUSB_DID_ISO_RESUME, &data->flags);
 		}
-		if (data->isoc_altsetting != 2) {
+
+		if (hdev->voice_setting & 0x0020) {
+			static const int alts[3] = { 2, 4, 5 };
+			new_alts = alts[hdev->conn_hash.sco_num - 1];
+		} else {
+			new_alts = hdev->conn_hash.sco_num;
+		}
+
+		if (data->isoc_altsetting != new_alts) {
 			clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
 			usb_kill_anchored_urbs(&data->isoc_anchor);
 
-			if (__set_isoc_interface(hdev, 2) < 0)
+			if (__set_isoc_interface(hdev, new_alts) < 0)
 				return;
 		}
 
-- 
1.7.7.6

[PATCH v0 6/6] Bluetooth: Remove outgoing MTU check

[Mikel Astiz]

From: Mikel Astiz <mikel.astiz@bmw-carit.de>

MTU is known in userland so there is no real need to check this in the
kernel, specially because no SCO flow control is done anyway.

In addition, when multiple SCO links are active, many BT chips will work
only if bigger packets are being sent. In this case the MTU is not
relevant any more and typically the size of the incoming packets can be
used also for outgoing packets. This is often greater than the MTU so it
is necessary that we remove this check.

Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
---
 net/bluetooth/sco.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index b48370b..9ae9e31 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -236,10 +236,6 @@ static inline int sco_send_frame(struct sock *sk, struct msghdr *msg, int len)
 	struct sk_buff *skb;
 	int err;
 
-	/* Check outgoing MTU */
-	if (len > conn->mtu)
-		return -EINVAL;
-
 	BT_DBG("sk %p len %d", sk, len);
 
 	skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
-- 
1.7.7.6

Re: [PATCH v0 1/6] Bluetooth: Use unsigned int instead of signed int

[Marcel Holtmann]

Hi Mikel,

> The involved values are all unsigned and thus unsigned int should be
> used instead of signed int. Assigning ~0 to a signed int results in -1,
> which is confusing and error-prone, while the code is trying to set the
> maximum value possible.
> 
> The code still works because the C standard defines that unsigned
> comparison will be performed in these cases, when comparing an unsigned
> int and a signed int.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

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

Regards

Marcel

Re: [PATCH v0 2/6] Bluetooth: Remove unnecessary check

[Marcel Holtmann]

Hi Mikel,

> The function already fails if the given size is greater than the MTU, so
> there is no need to consider that case afterwards.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  net/bluetooth/sco.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)

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

Regards

Marcel

Re: [PATCH v0 3/6] Bluetooth: Remove unused HCI event handling

[Marcel Holtmann]

Hi Mikel,

> Function hci_num_comp_pkts_evt() doesn't need to handle SCO connections
> unless SCO flow control is explicitly enabled, as described in the
> Bluetooth Core Specification, Volume 4, Part B, section 7.7.19.
> 
> Even in the unlikely case that the hardware supports this feature, the
> kernel does not enable it.

I am not sure that we should just remove this. What happens if you run
on a controller that has this enabled? I rather have the support for it
added then.

Regards

Marcel

Re: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Marcel Holtmann]

Hi Mikel,

> This patch refactors the SCO scheduling algorithm to remove code that is
> not making any difference. There is no HCI SCO flow control in the
> kernel so the scheduling can be simplified.
> 
> All data produced by userspace is sent to the controller without any
> flow control. This was also the case before this patch, but now it's
> more obvious and readable.

as in the previous patch. You can only do this if you make sure that the
controller does not have flow control enabled. You need another basic
check here to ensure this first. Otherwise a lot of things might break.

Regards

Marcel

Re: [PATCH v0 5/6] Bluetooth: btusb: Dynamic alternate setting

[Marcel Holtmann]

Hi Mikel,

> The alternate setting must be dynamically set according to the number of
> active SCO links, and the bit depth of the audio. The possible values
> for the alternate setting are described in the Bluetooth Core
> Specification, Volume 4, Part B, section 2.1.1.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  drivers/bluetooth/btusb.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)

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

Regards

Marcel

Re: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Luiz Augusto von Dentz]

Hi Mikel, Marcel,

On Wed, Apr 11, 2012 at 12:24 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Mikel,
>
>> This patch refactors the SCO scheduling algorithm to remove code that is
>> not making any difference. There is no HCI SCO flow control in the
>> kernel so the scheduling can be simplified.
>>
>> All data produced by userspace is sent to the controller without any
>> flow control. This was also the case before this patch, but now it's
>> more obvious and readable.
>
> as in the previous patch. You can only do this if you make sure that the
> controller does not have flow control enabled. You need another basic
> check here to ensure this first. Otherwise a lot of things might break.

It also seems to remove any quote/fairness of the scheduling, actually
Im not sure how it works because it sends the frames regardless of the
number of available slots/buffers indicated by sco_cnt, I also wonder
how it will scale with multiple SCO connections, even though it is
quite rare to have more than one who knows what crazy ideas people
will come.

-- 
Luiz Augusto von Dentz

RE: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Mikel Astiz]

Hi Marcel, Luiz,

> >>
> >> All data produced by userspace is sent to the controller without any
> >> flow control. This was also the case before this patch, but now it's
> >> more obvious and readable.
> >
> > as in the previous patch. You can only do this if you make sure that
> > the controller does not have flow control enabled. You need another
> > basic check here to ensure this first. Otherwise a lot of things
> might break.

These patches should break nothing for several reasons:

1. HCI SCO flow control is NOT enabled by default, as described in the spec=
 volume 2 part E section 7.3.37.

2. The kernel does not read or write this parameter, which means it wouldn'=
t even know if the controller would have it enabled for some strange reason=
 (however I haven't managed to find any controller that even supports this)=
.

>=20
> It also seems to remove any quote/fairness of the scheduling, actually
> Im not sure how it works because it sends the frames regardless of the
> number of available slots/buffers indicated by sco_cnt, I also wonder
> how it will scale with multiple SCO connections, even though it is
> quite rare to have more than one who knows what crazy ideas people will
> come.
>=20

3. The previous implementation in the kernel seems to be totally useless, g=
iven that sco_cnt is never updated (grep for "sco_cnt--" or check hci_sched=
_sco() and hci_sched_esco()). So the idea of a quota is just an illusion he=
re.

Maybe I'm missing something here, that's why I need your feedback :-)

In a nutshell: if some controller does support SCO flow control, it would b=
e straightforward to support it in the Kernel (I actually have an implement=
ation but wasn't able to test it).

If the controller doesn't have this feature, which is the most common case,=
 then we either do in the kernel (time-based), in userspace, or both. For t=
he moment I just propose to do it in userspace, as we've been doing recentl=
y despite the illusion I mention above.

Regarding multiple SCOs, it's not a problem at all. It's actually what I'm =
trying to achieve with these patches.

Cheers,
Mikel

RE: [PATCH v0 3/6] Bluetooth: Remove unused HCI event handling

[Mikel Astiz]

SGkgTWFyY2VsLA0KDQo+IA0KPiBJIGFtIG5vdCBzdXJlIHRoYXQgd2Ugc2hvdWxkIGp1c3QgcmVt
b3ZlIHRoaXMuIFdoYXQgaGFwcGVucyBpZiB5b3UgcnVuDQo+IG9uIGEgY29udHJvbGxlciB0aGF0
IGhhcyB0aGlzIGVuYWJsZWQ/IEkgcmF0aGVyIGhhdmUgdGhlIHN1cHBvcnQgZm9yIGl0DQo+IGFk
ZGVkIHRoZW4uDQo+IA0KDQpQbGVhc2UgY2hlY2sgbXkgYW5zd2VyIHRvIEx1aXogYW5kIHlvdSBy
ZWdhcmRpbmcgcGF0Y2ggdjAgNC82Lg0KDQpDaGVlcnMsDQpNaWtlbA0KDQo=

RE: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Marcel Holtmann]

Hi Mikel,

> > >> All data produced by userspace is sent to the controller without any
> > >> flow control. This was also the case before this patch, but now it's
> > >> more obvious and readable.
> > >
> > > as in the previous patch. You can only do this if you make sure that
> > > the controller does not have flow control enabled. You need another
> > > basic check here to ensure this first. Otherwise a lot of things
> > might break.
> 
> These patches should break nothing for several reasons:
> 
> 1. HCI SCO flow control is NOT enabled by default, as described in the spec volume 2 part E section 7.3.37.
> 
> 2. The kernel does not read or write this parameter, which means it wouldn't even know if the controller would have it enabled for some strange reason (however I haven't managed to find any controller that even supports this).

this does not mean we just remove code now. We should start fixing that
and read the setting actually.

> > It also seems to remove any quote/fairness of the scheduling, actually
> > Im not sure how it works because it sends the frames regardless of the
> > number of available slots/buffers indicated by sco_cnt, I also wonder
> > how it will scale with multiple SCO connections, even though it is
> > quite rare to have more than one who knows what crazy ideas people will
> > come.
> > 
> 
> 3. The previous implementation in the kernel seems to be totally useless, given that sco_cnt is never updated (grep for "sco_cnt--" or check hci_sched_sco() and hci_sched_esco()). So the idea of a quota is just an illusion here.
> 
> Maybe I'm missing something here, that's why I need your feedback :-)
> 
> In a nutshell: if some controller does support SCO flow control, it would be straightforward to support it in the Kernel (I actually have an implementation but wasn't able to test it).
> 
> If the controller doesn't have this feature, which is the most common case, then we either do in the kernel (time-based), in userspace, or both. For the moment I just propose to do it in userspace, as we've been doing recently despite the illusion I mention above.
> 
> Regarding multiple SCOs, it's not a problem at all. It's actually what I'm trying to achieve with these patches.

Lets read the settings first and see what current controllers are doing.
That part needs to be fixed first. It is something we have been lazy and
ignoring it.

Regards

Marcel

RE: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Mikel Astiz]

SGkgTWFyY2VsLA0KDQo+IA0KPiBIaSBNaWtlbCwNCj4gDQo+ID4gPiA+PiBBbGwgZGF0YSBwcm9k
dWNlZCBieSB1c2Vyc3BhY2UgaXMgc2VudCB0byB0aGUgY29udHJvbGxlciB3aXRob3V0DQo+ID4g
PiA+PiBhbnkgZmxvdyBjb250cm9sLiBUaGlzIHdhcyBhbHNvIHRoZSBjYXNlIGJlZm9yZSB0aGlz
IHBhdGNoLCBidXQNCj4gPiA+ID4+IG5vdyBpdCdzIG1vcmUgb2J2aW91cyBhbmQgcmVhZGFibGUu
DQo+ID4gPiA+DQo+ID4gPiA+IGFzIGluIHRoZSBwcmV2aW91cyBwYXRjaC4gWW91IGNhbiBvbmx5
IGRvIHRoaXMgaWYgeW91IG1ha2Ugc3VyZQ0KPiA+ID4gPiB0aGF0IHRoZSBjb250cm9sbGVyIGRv
ZXMgbm90IGhhdmUgZmxvdyBjb250cm9sIGVuYWJsZWQuIFlvdSBuZWVkDQo+ID4gPiA+IGFub3Ro
ZXIgYmFzaWMgY2hlY2sgaGVyZSB0byBlbnN1cmUgdGhpcyBmaXJzdC4gT3RoZXJ3aXNlIGEgbG90
IG9mDQo+ID4gPiA+IHRoaW5ncw0KPiA+ID4gbWlnaHQgYnJlYWsuDQo+ID4NCj4gPiBUaGVzZSBw
YXRjaGVzIHNob3VsZCBicmVhayBub3RoaW5nIGZvciBzZXZlcmFsIHJlYXNvbnM6DQo+ID4NCj4g
PiAxLiBIQ0kgU0NPIGZsb3cgY29udHJvbCBpcyBOT1QgZW5hYmxlZCBieSBkZWZhdWx0LCBhcyBk
ZXNjcmliZWQgaW4NCj4gdGhlIHNwZWMgdm9sdW1lIDIgcGFydCBFIHNlY3Rpb24gNy4zLjM3Lg0K
PiA+DQo+ID4gMi4gVGhlIGtlcm5lbCBkb2VzIG5vdCByZWFkIG9yIHdyaXRlIHRoaXMgcGFyYW1l
dGVyLCB3aGljaCBtZWFucyBpdA0KPiB3b3VsZG4ndCBldmVuIGtub3cgaWYgdGhlIGNvbnRyb2xs
ZXIgd291bGQgaGF2ZSBpdCBlbmFibGVkIGZvciBzb21lDQo+IHN0cmFuZ2UgcmVhc29uIChob3dl
dmVyIEkgaGF2ZW4ndCBtYW5hZ2VkIHRvIGZpbmQgYW55IGNvbnRyb2xsZXIgdGhhdA0KPiBldmVu
IHN1cHBvcnRzIHRoaXMpLg0KPiANCj4gdGhpcyBkb2VzIG5vdCBtZWFuIHdlIGp1c3QgcmVtb3Zl
IGNvZGUgbm93LiBXZSBzaG91bGQgc3RhcnQgZml4aW5nIHRoYXQNCj4gYW5kIHJlYWQgdGhlIHNl
dHRpbmcgYWN0dWFsbHkuDQo+IA0KPiA+ID4gSXQgYWxzbyBzZWVtcyB0byByZW1vdmUgYW55IHF1
b3RlL2ZhaXJuZXNzIG9mIHRoZSBzY2hlZHVsaW5nLA0KPiA+ID4gYWN0dWFsbHkgSW0gbm90IHN1
cmUgaG93IGl0IHdvcmtzIGJlY2F1c2UgaXQgc2VuZHMgdGhlIGZyYW1lcw0KPiA+ID4gcmVnYXJk
bGVzcyBvZiB0aGUgbnVtYmVyIG9mIGF2YWlsYWJsZSBzbG90cy9idWZmZXJzIGluZGljYXRlZCBi
eQ0KPiA+ID4gc2NvX2NudCwgSSBhbHNvIHdvbmRlciBob3cgaXQgd2lsbCBzY2FsZSB3aXRoIG11
bHRpcGxlIFNDTw0KPiA+ID4gY29ubmVjdGlvbnMsIGV2ZW4gdGhvdWdoIGl0IGlzIHF1aXRlIHJh
cmUgdG8gaGF2ZSBtb3JlIHRoYW4gb25lIHdobw0KPiA+ID4ga25vd3Mgd2hhdCBjcmF6eSBpZGVh
cyBwZW9wbGUgd2lsbCBjb21lLg0KPiA+ID4NCj4gPg0KPiA+IDMuIFRoZSBwcmV2aW91cyBpbXBs
ZW1lbnRhdGlvbiBpbiB0aGUga2VybmVsIHNlZW1zIHRvIGJlIHRvdGFsbHkNCj4gdXNlbGVzcywg
Z2l2ZW4gdGhhdCBzY29fY250IGlzIG5ldmVyIHVwZGF0ZWQgKGdyZXAgZm9yICJzY29fY250LS0i
IG9yDQo+IGNoZWNrIGhjaV9zY2hlZF9zY28oKSBhbmQgaGNpX3NjaGVkX2VzY28oKSkuIFNvIHRo
ZSBpZGVhIG9mIGEgcXVvdGEgaXMNCj4ganVzdCBhbiBpbGx1c2lvbiBoZXJlLg0KPiA+DQo+ID4g
TWF5YmUgSSdtIG1pc3Npbmcgc29tZXRoaW5nIGhlcmUsIHRoYXQncyB3aHkgSSBuZWVkIHlvdXIg
ZmVlZGJhY2sgOi0pDQo+ID4NCj4gPiBJbiBhIG51dHNoZWxsOiBpZiBzb21lIGNvbnRyb2xsZXIg
ZG9lcyBzdXBwb3J0IFNDTyBmbG93IGNvbnRyb2wsIGl0DQo+IHdvdWxkIGJlIHN0cmFpZ2h0Zm9y
d2FyZCB0byBzdXBwb3J0IGl0IGluIHRoZSBLZXJuZWwgKEkgYWN0dWFsbHkgaGF2ZQ0KPiBhbiBp
bXBsZW1lbnRhdGlvbiBidXQgd2Fzbid0IGFibGUgdG8gdGVzdCBpdCkuDQo+ID4NCj4gPiBJZiB0
aGUgY29udHJvbGxlciBkb2Vzbid0IGhhdmUgdGhpcyBmZWF0dXJlLCB3aGljaCBpcyB0aGUgbW9z
dCBjb21tb24NCj4gY2FzZSwgdGhlbiB3ZSBlaXRoZXIgZG8gaW4gdGhlIGtlcm5lbCAodGltZS1i
YXNlZCksIGluIHVzZXJzcGFjZSwgb3INCj4gYm90aC4gRm9yIHRoZSBtb21lbnQgSSBqdXN0IHBy
b3Bvc2UgdG8gZG8gaXQgaW4gdXNlcnNwYWNlLCBhcyB3ZSd2ZQ0KPiBiZWVuIGRvaW5nIHJlY2Vu
dGx5IGRlc3BpdGUgdGhlIGlsbHVzaW9uIEkgbWVudGlvbiBhYm92ZS4NCj4gPg0KPiA+IFJlZ2Fy
ZGluZyBtdWx0aXBsZSBTQ09zLCBpdCdzIG5vdCBhIHByb2JsZW0gYXQgYWxsLiBJdCdzIGFjdHVh
bGx5DQo+IHdoYXQgSSdtIHRyeWluZyB0byBhY2hpZXZlIHdpdGggdGhlc2UgcGF0Y2hlcy4NCj4g
DQo+IExldHMgcmVhZCB0aGUgc2V0dGluZ3MgZmlyc3QgYW5kIHNlZSB3aGF0IGN1cnJlbnQgY29u
dHJvbGxlcnMgYXJlDQo+IGRvaW5nLg0KPiBUaGF0IHBhcnQgbmVlZHMgdG8gYmUgZml4ZWQgZmly
c3QuIEl0IGlzIHNvbWV0aGluZyB3ZSBoYXZlIGJlZW4gbGF6eQ0KPiBhbmQgaWdub3JpbmcgaXQu
DQo+IA0KDQpXZSBkb24ndCBhY3R1YWxseSBuZWVkIHRvIHJlYWQgYW55dGhpbmcgYmVjYXVzZSB0
aGUgZmVhdHVyZSBpcyBkaXNhYmxlZCBieSBkZWZhdWx0LiBBcyBJIHNhaWQsIHRoYXQncyBjbGVh
ciBpbiB0aGUgc3BlY2lmaWNhdGlvbi4NCg0KV2UgbWlnaHQgdHJ5IHRvIHdyaXRlIGl0IChzZXQg
aXQgdG8gMSkgYnV0IHRoaXMgaXMgbm90IHN1cHBvcnRlZCBieSB0aGUgbWFpbiBjb250cm9sbGVy
IG1hbnVmYWN0dXJlcnMgKHRvIGJlIGNvbmZpcm1lZCwgYnV0IHRoYXQncyB0aGUgZmVlZGJhY2sg
SSBoYXZlKS4gU29tZSBjb250cm9sbGVycyBsaXN0IHRoaXMgZmVhdHVyZSBpbiB0aGUgc3VwcG9y
dGVkIGNvbW1hbmQgbGlzdCAoc2VjdGlvbiA2LjI3IG9jdGV0IDEwIGJpdCA0KSwgYnV0IHN0aWxs
IGl0J3Mgbm90IHBvc3NpYmxlIHRvIGVuYWJsZSBpdC4NCg0KU28gSSBkb24ndCByZWFsbHkgZ2V0
IHdoeSB3ZSBzaG91bGQgY29tcGxpY2F0ZSB0aGUga2VybmVsIGNvZGUgZm9yIHNvbWV0aGluZyB0
aGF0IGlzIHZlcnkgcmFyZSwgYW5kIHRoYXQgcHJvYmFibHkgbmV2ZXIgYWN0dWFsbHkgd29ya2Vk
Lg0KDQpVc2Vyc3BhY2UgY29kZSBuZWVkcyB0byBkbyBmbG93IGNvbnRyb2wgYW55d2F5ICh0aW1l
LWJhc2VkKSwgYW5kIEkgZG9uJ3QgdGhpbmsgd2Ugd2FudCBpdCBpbiB0d28gZGlmZmVyZW50IHBs
YWNlcy4NCg0KVGhlIG9ubHkgcmVhc29uYWJsZSBhbHRlcm5hdGl2ZSBJIHNlZSBpcyB0aGF0IHRo
ZSBrZXJuZWwgd291bGQgZG8gZmxvdyBjb250cm9sIGFsd2F5cywgd2l0aCBvciB3aXRob3V0IGhh
cmR3YXJlIHN1cHBvcnQuIEFzIEkgYWxyZWFkeSBzYWlkLCB0aGlzIHNob3VsZCBiZSBxdWl0ZSBz
aW1wbGUsIGJ1dCBzdGlsbCB3ZSBhcmUgYWRkaW5nIGxpbmVzIG9mIGNvZGUgd2l0aG91dCBhbnkg
cmVhbCBiZW5lZml0Lg0KDQpDaGVlcnMsDQpNaWtlbA0KDQo=

RE: [PATCH v0 4/6] Bluetooth: Simplify outgoing SCO scheduling code

[Marcel Holtmann]

Hi Mikel,

> > > > >> All data produced by userspace is sent to the controller without
> > > > >> any flow control. This was also the case before this patch, but
> > > > >> now it's more obvious and readable.
> > > > >
> > > > > as in the previous patch. You can only do this if you make sure
> > > > > that the controller does not have flow control enabled. You need
> > > > > another basic check here to ensure this first. Otherwise a lot of
> > > > > things
> > > > might break.
> > >
> > > These patches should break nothing for several reasons:
> > >
> > > 1. HCI SCO flow control is NOT enabled by default, as described in
> > the spec volume 2 part E section 7.3.37.
> > >
> > > 2. The kernel does not read or write this parameter, which means it
> > wouldn't even know if the controller would have it enabled for some
> > strange reason (however I haven't managed to find any controller that
> > even supports this).
> > 
> > this does not mean we just remove code now. We should start fixing that
> > and read the setting actually.
> > 
> > > > It also seems to remove any quote/fairness of the scheduling,
> > > > actually Im not sure how it works because it sends the frames
> > > > regardless of the number of available slots/buffers indicated by
> > > > sco_cnt, I also wonder how it will scale with multiple SCO
> > > > connections, even though it is quite rare to have more than one who
> > > > knows what crazy ideas people will come.
> > > >
> > >
> > > 3. The previous implementation in the kernel seems to be totally
> > useless, given that sco_cnt is never updated (grep for "sco_cnt--" or
> > check hci_sched_sco() and hci_sched_esco()). So the idea of a quota is
> > just an illusion here.
> > >
> > > Maybe I'm missing something here, that's why I need your feedback :-)
> > >
> > > In a nutshell: if some controller does support SCO flow control, it
> > would be straightforward to support it in the Kernel (I actually have
> > an implementation but wasn't able to test it).
> > >
> > > If the controller doesn't have this feature, which is the most common
> > case, then we either do in the kernel (time-based), in userspace, or
> > both. For the moment I just propose to do it in userspace, as we've
> > been doing recently despite the illusion I mention above.
> > >
> > > Regarding multiple SCOs, it's not a problem at all. It's actually
> > what I'm trying to achieve with these patches.
> > 
> > Lets read the settings first and see what current controllers are
> > doing.
> > That part needs to be fixed first. It is something we have been lazy
> > and ignoring it.
> > 
> 
> We don't actually need to read anything because the feature is disabled by default. As I said, that's clear in the specification.

you have no idea in what state your controller is when we are not
sending HCI Reset. And there are still some controllers where we do not
do that.

> We might try to write it (set it to 1) but this is not supported by the main controller manufacturers (to be confirmed, but that's the feedback I have). Some controllers list this feature in the supported command list (section 6.27 octet 10 bit 4), but still it's not possible to enable it.
> 
> So I don't really get why we should complicate the kernel code for something that is very rare, and that probably never actually worked.
> 
> Userspace code needs to do flow control anyway (time-based), and I don't think we want it in two different places.
> 
> The only reasonable alternative I see is that the kernel would do flow control always, with or without hardware support. As I already said, this should be quite simple, but still we are adding lines of code without any real benefit.

I do not wanna remove code from the kernel that seemed to be reasonable.
Just extend it with your case.

Regards

Marcel

Re: [PATCH v0 1/6] Bluetooth: Use unsigned int instead of signed int

[Gustavo Padovan]

Hi Mikel,

* Mikel Astiz <mikel.astiz.oss@gmail.com> [2012-04-11 08:48:47 +0200]:

> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
> 
> The involved values are all unsigned and thus unsigned int should be
> used instead of signed int. Assigning ~0 to a signed int results in -1,
> which is confusing and error-prone, while the code is trying to set the
> maximum value possible.
> 
> The code still works because the C standard defines that unsigned
> comparison will be performed in these cases, when comparing an unsigned
> int and a signed int.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Applied to bluetooth-next thanks.

	Gustavo

Re: [PATCH v0 2/6] Bluetooth: Remove unnecessary check

[Gustavo Padovan]

Hi Mikel,

* Mikel Astiz <mikel.astiz.oss@gmail.com> [2012-04-11 08:48:48 +0200]:

> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
> 
> The function already fails if the given size is greater than the MTU, so
> there is no need to consider that case afterwards.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  net/bluetooth/sco.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)

Applied to bluetooth-next, thanks.

	Gustavo

Re: [PATCH v0 5/6] Bluetooth: btusb: Dynamic alternate setting

[Gustavo Padovan]

Hi Mikel,

* Mikel Astiz <mikel.astiz.oss@gmail.com> [2012-04-11 08:48:51 +0200]:

> From: Mikel Astiz <mikel.astiz@bmw-carit.de>
> 
> The alternate setting must be dynamically set according to the number of
> active SCO links, and the bit depth of the audio. The possible values
> for the alternate setting are described in the Bluetooth Core
> Specification, Volume 4, Part B, section 2.1.1.
> 
> Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> ---
>  drivers/bluetooth/btusb.c |   13 +++++++++++--
>  1 files changed, 11 insertions(+), 2 deletions(-)

Applied, thanks.

	Gustavo

Re: [PATCH v0 5/6] Bluetooth: btusb: Dynamic alternate setting

[Marcel Holtmann]

Hi Gustavo,

> > The alternate setting must be dynamically set according to the number of
> > active SCO links, and the bit depth of the audio. The possible values
> > for the alternate setting are described in the Bluetooth Core
> > Specification, Volume 4, Part B, section 2.1.1.
> > 
> > Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> > ---
> >  drivers/bluetooth/btusb.c |   13 +++++++++++--
> >  1 files changed, 11 insertions(+), 2 deletions(-)
> 
> Applied, thanks.

applied to what? It is not part of the share bluetooth-next tree. You
are confusing me here.

Regards

Marcel

Re: [PATCH v0 5/6] Bluetooth: btusb: Dynamic alternate setting

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2012-04-17 11:26:54 +0200]:

> Hi Gustavo,
> 
> > > The alternate setting must be dynamically set according to the number of
> > > active SCO links, and the bit depth of the audio. The possible values
> > > for the alternate setting are described in the Bluetooth Core
> > > Specification, Volume 4, Part B, section 2.1.1.
> > > 
> > > Signed-off-by: Mikel Astiz <mikel.astiz.oss@gmail.com>
> > > ---
> > >  drivers/bluetooth/btusb.c |   13 +++++++++++--
> > >  1 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > Applied, thanks.
> 
> applied to what? It is not part of the share bluetooth-next tree. You
> are confusing me here.

Sorry, I was in a hurry to go to a class at University and forgot to
push it, it is now on bluetooth-next tree.

	Gustavo