[Bluez-devel] forcing SCO connection patch

[Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

Hello everybody,

I attached two patches. the first one(bluez-kernel-forcesco.patch) is to
force using HCI_OP_ADD_SCO instead of HCI_OP_SETUP_SYNC_CONN, and the
second one is to handle SCO connection complete event but its request
was ESCO.

1.
I'm developing bluetooth functions in my linux phone project, and I'm
using bluez for my job. I've tested lots of headsets, and found that I
coudn't connect SCO channel with HCI_OP_SETUP_SYNC_CONN in some old
headsets. I could connect SCO channel with HCI_OP_ADD_SCO in this case.
however, there is no api to force using SCO instead of ESCO in bluez. so
I added SCO_FORCESCO to handle this old headsets

2.
When I tried to connect SCO channel with
HCI_OP_SETUP_SYNC_CONN(LINK_TYPE_ESCO), some bluetooth headsets responds
with LINK_TYPE_SCO because it did not support ESCO. But bluez couldn't
handle this situation, and patch_hci_event.c is for this.


BRs
Louis JANG


[-- Attachment #2: bluez-kernel-forcesco.patch --]
[-- Type: text/x-patch, Size: 2106 bytes --]

--- net/bluetooth/sco.c.orig	2007-12-17 18:03:14.000000000 +0900
+++ net/bluetooth/sco.c	2007-12-17 18:18:53.000000000 +0900
@@ -200,7 +200,11 @@
 
 	err = -ENOMEM;
 
-	type = lmp_esco_capable(hdev) ? ESCO_LINK : SCO_LINK;
+	if (sco_pi(sk)->force_sco) {
+	    type = SCO_LINK;
+	} else {
+	    type = lmp_esco_capable(hdev) ? ESCO_LINK : SCO_LINK;
+	}
 
 	hcon = hci_connect(hdev, type, dst);
 	if (!hcon)
@@ -660,12 +664,23 @@
 {
 	struct sock *sk = sock->sk;
 	int err = 0;
+	int len;
+	unsigned int force_sco;
 
 	BT_DBG("sk %p", sk);
 
 	lock_sock(sk);
 
 	switch (optname) {
+	case SCO_FORCESCO:
+		len = sizeof(unsigned int);
+		if (copy_from_user((char *)&force_sco, optval, len)) {
+		    err = -EFAULT;
+		    break;
+		}
+		sco_pi(sk)->force_sco = (force_sco != 0);
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
@@ -681,6 +696,7 @@
 	struct sco_options opts;
 	struct sco_conninfo cinfo;
 	int len, err = 0; 
+	unsigned int force_sco;
 
 	BT_DBG("sk %p", sk);
 
@@ -721,6 +737,14 @@
 
 		break;
 
+	case SCO_FORCESCO:
+		force_sco = sco_pi(sock)->force_sco;
+		len = sizeof(unsigned int);
+		if (copy_to_user(optval, (char *)&force_sco, len))
+			err = -EFAULT;
+
+		break;
+		
 	default:
 		err = -ENOPROTOOPT;
 		break;
--- net/bluetooth/hci_conn.c.orig	2007-11-29 15:53:15.000000000 +0900
+++ net/bluetooth/hci_conn.c	2007-12-17 18:03:28.000000000 +0900
@@ -355,7 +355,8 @@
 
 		if (acl->state == BT_CONNECTED && 
 				(sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
-			if (lmp_esco_capable(hdev))
+			//if (lmp_esco_capable(hdev))
+			if (type == ESCO_LINK)
 				hci_setup_sync(sco, acl->handle);
 			else
 				hci_add_sco(sco, acl->handle);
--- include/net/bluetooth/sco.h.orig	2007-12-17 18:02:54.000000000 +0900
+++ include/net/bluetooth/sco.h	2007-12-17 18:04:17.000000000 +0900
@@ -51,6 +51,8 @@
 	__u8  dev_class[3];
 };
 
+#define SCO_FORCESCO	0x03
+
 /* ---- SCO connections ---- */
 struct sco_conn {
 	struct hci_conn	*hcon;
@@ -74,6 +76,7 @@
 	struct bt_sock	bt;
 	__u32		flags;
 	struct sco_conn	*conn;
+	unsigned int    force_sco :1;
 };
 
 #endif /* __SCO_H */

[-- Attachment #3: patch_hci_event.c --]
[-- Type: text/x-csrc, Size: 647 bytes --]

--- ../../../kernel-2.6-bluez-patch-2.6.23-mh1/net/bluetooth/hci_event.c	2007-11-21 14:15:41.000000000 +0900
+++ hci_event.c	2007-12-05 14:39:52.000000000 +0900
@@ -1316,8 +1316,16 @@
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
-	if (!conn)
-		goto unlock;
+	if (!conn) {
+	    if (ev->link_type != ACL_LINK) {
+		int t = (ev->link_type == ESCO_LINK) ? SCO_LINK : ESCO_LINK;
+		conn = hci_conn_hash_lookup_ba(hdev, t, &ev->bdaddr);
+		if (conn) 
+		    conn->type = ev->link_type;
+	    }
+	    if (!conn) 
+	    	goto unlock;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);

[-- Attachment #4: Type: text/plain, Size: 308 bytes --]

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

[-- Attachment #5: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Brad Midgley]

Louis

>  When I tried to connect SCO channel with
>  HCI_OP_SETUP_SYNC_CONN(LINK_TYPE_ESCO), some bluetooth headsets responds
>  with LINK_TYPE_SCO because it did not support ESCO. But bluez couldn't
>  handle this situation, and patch_hci_event.c is for this.

Marcel looked at this patch and came back with the comments below. Can
you revisit it? I think some other people are seeing the same issues.
The patch won't go upstream until Marcel likes it.

the patch you sent me is fully broken. First of all the coding style
is wrong. Does nobody have learned this by now? I always look for that
first before even reading the patch. Second the case where an
ESCO_LINK returns NULL is broken and will fall over and crash.

-- 
Brad

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Brad Midgley]

Louis

I should clarify. It looks like the one Marcel saw was:

-	if (!conn)
-		goto unlock;
+	if (!conn) {
+		if (ev->link_type==SCO_LINK) {
+			conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
+			if (!conn)
+				goto unlock;
+			else
+				conn->type=SCO_LINK;
+			}
+	}

but yours was

-	if (!conn)
-		goto unlock;
+	if (!conn) {
+	    if (ev->link_type != ACL_LINK) {
+		int t = (ev->link_type == ESCO_LINK) ? SCO_LINK : ESCO_LINK;
+		conn = hci_conn_hash_lookup_ba(hdev, t, &ev->bdaddr);
+		if (conn)
+		    conn->type = ev->link_type;
+	    }
+	    if (!conn)
+	    	goto unlock;
+	}

they both have indent/space issues but I'm not sure Marcel's other
comment makes sense for what you did.

It seems like the problem is ev->link_type might have the wrong value
for the lookup to succeed, but I'm not sure what the big picture looks
like here. Is this the best place to solve it or was this a quick
workaround?

Brad

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Guillaume Bedot]

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

Hello,

Le dimanche 24 février 2008 à 23:43 -0700, Brad Midgley a écrit :
[...] 
> Marcel looked at this patch and came back with the comments below. Can
> you revisit it? I think some other people are seeing the same issues.
> The patch won't go upstream until Marcel likes it.
> 
> the patch you sent me is fully broken. First of all the coding style
> is wrong. Does nobody have learned this by now? I always look for that
> first before even reading the patch. Second the case where an
> ESCO_LINK returns NULL is broken and will fall over and crash.
> 
It was about the patch on the wiki (one of mine, shame on me).

Marcel is right, this patch is an awful and moreover buggy patch (and I
found nothing in the specification stating a SCO connection shall be
opened as a fallback for an eSCO one, or shall not, so I abandonned that
way).

However, I think the first patch
(force-sco-link-until-headset-features-are-known.patch), despite badly
styled and named, was correct.

Everywhere I found a eSCO feature test for the local device, I added a
test for the remote device eSCO/EV3 feature.

An eSCO connection is then used only when both the local and the remote
devices supports the feature, else a SCO connection is requested with
headsets not supporting eSCO.

The headset's features mask should be already available, as the request
is sent just after establishing the underlying ACL connection.

And it shall be known before establishing a synchronous connection with
the right packet type(s), according to the spec (maybe we should ensure
we get a response to this features request ?).

I tried to fix style issues in this new patch, let me now if some
remain.

The other patch (also-accept-sco-links-v2.patch) is not needed at all,
as the case should not occur anymore with the attached patch applied.

Best regards,

Guillaume B.

PS:
I have sometimes a hci reset after the "synchronous connection complete"
event, when playing with the two headsets alternatively (as with the
other patch) but didn't understand why yet.
I hope it was not too long.

[-- Attachment #2: also_use_headset_features.patch --]
[-- Type: text/x-patch, Size: 1908 bytes --]

--- net/bluetooth/hci_conn.c.orig	2008-02-26 17:19:55.000000000 +0100
+++ net/bluetooth/hci_conn.c	2008-02-26 17:24:22.000000000 +0100
@@ -326,6 +326,9 @@
 	if (type == ACL_LINK)
 		return acl;
 
+	if (lmp_esco_capable(hdev) && lmp_esco_capable(acl))
+		type = ESCO_LINK;
+
 	if (!(sco = hci_conn_hash_lookup_ba(hdev, type, dst))) {
 		if (!(sco = hci_conn_add(hdev, type, dst))) {
 			hci_conn_put(acl);
@@ -340,7 +343,7 @@
 
 	if (acl->state == BT_CONNECTED &&
 			(sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
-		if (lmp_esco_capable(hdev))
+		if (type == ESCO_LINK)
 			hci_setup_sync(sco, acl->handle);
 		else
 			hci_add_sco(sco, acl->handle);
--- net/bluetooth/hci_event.c.orig	2008-02-26 17:20:11.000000000 +0100
+++ net/bluetooth/hci_event.c	2008-02-26 17:24:22.000000000 +0100
@@ -720,7 +720,7 @@
 		struct hci_conn *sco = conn->link;
 		if (sco) {
 			if (!ev->status) {
-				if (lmp_esco_capable(hdev))
+				if (lmp_esco_capable(hdev) && lmp_esco_capable(conn))
 					hci_setup_sync(sco, conn->handle);
 				else
 					hci_add_sco(sco, conn->handle);
@@ -771,7 +771,7 @@
 
 		hci_dev_unlock(hdev);
 
-		if (ev->link_type == ACL_LINK || !lmp_esco_capable(hdev)) {
+		if (ev->link_type == ACL_LINK || !lmp_esco_capable(hdev) || !lmp_esco_capable(conn)) {
 			struct hci_cp_accept_conn_req cp;
 
 			bacpy(&cp.bdaddr, &ev->bdaddr);
--- net/bluetooth/sco.c.orig	2008-02-26 17:20:21.000000000 +0100
+++ net/bluetooth/sco.c	2008-02-26 17:24:22.000000000 +0100
@@ -182,7 +182,7 @@
 	struct sco_conn *conn;
 	struct hci_conn *hcon;
 	struct hci_dev  *hdev;
-	int err, type;
+	int err;
 
 	BT_DBG("%s -> %s", batostr(src), batostr(dst));
 
@@ -193,9 +193,8 @@
 
 	err = -ENOMEM;
 
-	type = lmp_esco_capable(hdev) ? ESCO_LINK : SCO_LINK;
-
-	hcon = hci_connect(hdev, type, dst);
+	/* SCO or ESCO is decided in hci_connect */
+	hcon = hci_connect(hdev, SCO_LINK, dst);
 	if (!hcon)
 		goto done;
 

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Marcel Holtmann]

Hi Guillaume,

>> Marcel looked at this patch and came back with the comments below.  
>> Can
>> you revisit it? I think some other people are seeing the same issues.
>> The patch won't go upstream until Marcel likes it.
>>
>> the patch you sent me is fully broken. First of all the coding style
>> is wrong. Does nobody have learned this by now? I always look for  
>> that
>> first before even reading the patch. Second the case where an
>> ESCO_LINK returns NULL is broken and will fall over and crash.
>>
> It was about the patch on the wiki (one of mine, shame on me).
>
> Marcel is right, this patch is an awful and moreover buggy patch  
> (and I
> found nothing in the specification stating a SCO connection shall be
> opened as a fallback for an eSCO one, or shall not, so I abandonned  
> that
> way).
>
> However, I think the first patch
> (force-sco-link-until-headset-features-are-known.patch), despite badly
> styled and named, was correct.

No. That is wrong. Using sync setup commands apply to SCO and eSCO. No  
need to force anything here. Let the Bluetooth firmware inside the  
chip do the right thing.

> Everywhere I found a eSCO feature test for the local device, I added a
> test for the remote device eSCO/EV3 feature.
>
> An eSCO connection is then used only when both the local and the  
> remote
> devices supports the feature, else a SCO connection is requested with
> headsets not supporting eSCO.

As I said, wrong approach. Don't put that logic into the host stack.  
Let the firmware negotiate that.

> The headset's features mask should be already available, as the  
> request
> is sent just after establishing the underlying ACL connection.
>
> And it shall be known before establishing a synchronous connection  
> with
> the right packet type(s), according to the spec (maybe we should  
> ensure
> we get a response to this features request ?).
>
> I tried to fix style issues in this new patch, let me now if some
> remain.
>
> The other patch (also-accept-sco-links-v2.patch) is not needed at all,
> as the case should not occur anymore with the attached patch applied.

Wrong. This case can still happen. Nobody says that the remote device  
has to establish an eSCO link only because the link manager feature  
says that the chip supports. In short it means that if the host stack  
on the remote doesn't want eSCO that is perfectly fine and valid.

Regards

Marcel


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Guillaume Bedot]

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

Re,

Le mardi 26 février 2008 à 20:28 +0100, Marcel Holtmann a écrit :
> No. That is wrong. Using sync setup commands apply to SCO and eSCO.
> No  
> need to force anything here. Let the Bluetooth firmware inside the  
> chip do the right thing.

So I have to assume LM=device firmware and Host=device driver, the
separation between the two finally makes sense.

Here is a corrected version of the other patch.

Regards,

Guillaume B.

[-- Attachment #2: also-accept-sco-links-v3.patch --]
[-- Type: text/x-patch, Size: 432 bytes --]

--- net/bluetooth/hci_event.c.orig	2008-02-26 17:20:11.000000000 +0100
+++ net/bluetooth/hci_event.c	2008-02-26 21:19:04.000000000 +0100
@@ -1313,6 +1313,11 @@
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
+	if (!conn && ev->link_type==SCO_LINK) {
+		conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
+		if (conn)
+			conn->type=SCO_LINK;
+	}
 	if (!conn)
 		goto unlock;
 

[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 164 bytes --]

_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] forcing SCO connection patch

[Guillaume Bedot]

Re,

Le mercredi 27 février 2008 à 21:21 +0900, Louis JANG a écrit :
>  	conn = hci_conn_hash_lookup_ba(hdev, ev->link_type, &ev->bdaddr);
> -	if (!conn)
> -		goto unlock;
> +	if (!conn) {
> +		__u8 link_type = (ev->link_type == ESCO_LINK) ? SCO_LINK : ESCO_LINK;
> +
> +		conn = hci_conn_hash_lookup_ba(hdev, link_type, &ev->bdaddr);
> +		if (!conn)
> +			goto unlock;

Does your patch handle re-negotiation of the SCO link back to an eSCO
one ? Great.

And the style in your patch is better according to Marcel rules.

I vote for yours.

Best regards,

Guillaume B.

Re: [Bluez-devel] forcing SCO connection patch

[Guillaume Bedot]

On jeu, 2008-02-28 at 11:49 +0900, Louis JANG wrote:
> Hi Marcel,
> 
> I changed what you said below and attached patch again.
> 
> And did you check Guillaume's patch for this issue? his patch try to
> find again when ev->link_type is SCO_LINK only. I think his way is
> better than mine. I haven't saw other case (LM is not support esco but
> ESCO_LINK is connected) and I think it shouldn't happen in a normal
> situation.

Perhaps my patch fixes half of the issue, as it does not get a handle on
the other case, and I just miss the point.

The cases represented in the "Messages Sequences Chart" are said to be
"examples", and they cover all the possible cases.
Maybe I just lack imagination.

If I have no example, it doesn't matter : if it happens, the same issue
will occur if the two link types aren't tried.

Best regards,

Guillaume B.