Skip to contentSkip to conversation

forcing SCO connection patch

Bluez-devel Louis JANG 5 participants 22 messages Dec 17, 2007 – Feb 28, 2008

Subsystems (4)
Networking Network drivers Bluetooth drivers Bluetooth
lore
Share & tools
lore

Louis JANG

author
#0 lore raw

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

bluetooth/sco.c renamed from bluetooth/sco.c.orig +25 -1
@@ -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;
bluetooth/hci_conn.c renamed from bluetooth/hci_conn.c.orig +2 -1
@@ -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);
net/bluetooth/sco.h renamed from net/bluetooth/sco.h.orig +3 -0
@@ -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 */
hci_event.c renamed from ../../kernel-2.6-bluez-patch-2.6.23-mh1/net/bluetooth/hci_event.c +10 -2
@@ -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);

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

_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#1 lore raw

Louis

Quoted 4 lines

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.

Signature · 10 lines
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
#2 lore raw

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#6 lore raw
In reply to Louis JANG

Quote mail from [email protected] :

2007/12/17 Louis JANG <[email protected]>:

Quoted 22 lines

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.

Quoted 4 lines

BRs
Louis JANG

Reply from [email protected]:

Quoted 12 lines · Brad Midgley

Louis

Quoted 4 lines

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.

Quoted 8 lines

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

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Please rework your patch and resend if you fixed them.

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

(Thanks for andrew and davem)

Regards
dave

Regards
dave

#7 lore raw
Quoted 68 lines · Dave Young

Quote mail from [email protected] :

2007/12/17 Louis JANG <[email protected]>:

Quoted 22 lines

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.

Quoted 4 lines

BRs
Louis JANG

Reply from [email protected]:

Quoted 14 lines · Brad Midgley

Louis

Quoted 4 lines

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.

Quoted 8 lines

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

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Quoted 6 lines

Please rework your patch and resend if you fixed them.

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

(Thanks for andrew and davem)

On bugzilla, bug 9871 are same problem as yours.

add davem and netdev in cc-list

Quoted 7 lines

Regards
dave

Regards
dave

#8 lore raw

Sorry, [email protected] was missed in cc

Quoted 72 lines · Dave Young
Quoted 70 lines · Dave Young

Quote mail from louis-6CAt/[email protected] :

2007/12/17 Louis JANG <louis-6CAt/[email protected]>:

Quoted 22 lines

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.

Quoted 4 lines

BRs
Louis JANG

Reply from [email protected]:

Quoted 14 lines · Brad Midgley

Louis

Quoted 4 lines

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.

Quoted 8 lines

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

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Please rework your patch and resend if you fixed them.

Quoted 17 lines
Quoted 4 lines

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

(Thanks for andrew and davem)

On bugzilla, bug 9871 are same problem as yours.

add davem and netdev in cc-list

Quoted 7 lines

Regards
dave

Regards
dave

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#9 lore raw
In reply to Dave Young
Quoted 15 lines

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Please rework your patch and resend if you fixed them.

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

Quoted 9 lines

(Thanks for andrew and davem)

Regards
dave

Regards
dave

Hi all,

I adjusted indentation of the patches but I'm not sure what's wrong about second comment of Marcel. please let me know if there are another problems in this patch.

Thanks in advance,
Louis JANG

bluetooth/hci_event.c renamed from bluetooth/hci_event.c.orig +11 -2
@@ -1313,8 +1313,17 @@
 	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) {
+			__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) 
+				conn->type = ev->link_type;
+	    	}
+		if (!conn) 
+			goto unlock;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
net/bluetooth/sco.h renamed from net/bluetooth-orig/sco.h +3 -0
diff -uNr include/net/bluetooth-orig/sco.h include/net/bluetooth/sco.h
@@ -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 */
bluetooth/hci_conn.c renamed from bluetooth-orig/hci_conn.c +1 -1
diff -uNr net/bluetooth-orig/hci_conn.c net/bluetooth/hci_conn.c
@@ -354,7 +354,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);
bluetooth/sco.c renamed from bluetooth-orig/sco.c +21 -1
diff -uNr net/bluetooth-orig/sco.c net/bluetooth/sco.c
@@ -200,7 +200,10 @@
 
 	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 +663,21 @@
 {
 	struct sock *sk = sock->sk;
 	int err = 0;
+	unsigned int force_sco;
 
 	BT_DBG("sk %p", sk);
 
 	lock_sock(sk);
 
 	switch (optname) {
+	case SCO_FORCESCO:
+		if (copy_from_user((char *)&force_sco, optval, sizeof(unsigned int))) {
+			err = -EFAULT;
+			break;
+		}
+		sco_pi(sk)->force_sco = (force_sco != 0);
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;

@@ -681,6 +693,7 @@
 	struct sco_options opts;
 	struct sco_conninfo cinfo;
 	int len, err = 0;
+	unsigned int force_sco;
 
 	BT_DBG("sk %p", sk);
 

@@ -721,6 +734,13 @@
 
 		break;
 
+	case SCO_FORCESCO:
+		force_sco = sco_pi(sock)->force_sco;
+		if (copy_to_user(optval, (char *)&force_sco, sizeof(unsigned int)))
+			err = -EFAULT;
+
+		break;
+		
 	default:
 		err = -ENOPROTOOPT;
 		break;
#10 lore raw
Quoted 20 lines · Louis JANG
Quoted 20 lines

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Please rework your patch and resend if you fixed them.

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

(Thanks for andrew and davem)

Regards
dave

Quoted 8 lines
Quoted 4 lines

Regards
dave

Hi all,

I adjusted indentation of the patches

Not enough.

Please first read Documentation/CodingStyle, fix them, and then use scripts/checkpatch.pl to check your patch.

Quoted 119 lines

but I'm not sure what's wrong
about second comment of Marcel. please let me know if there are another problems in this patch.

Thanks in advance,
Louis JANG

--- net/bluetooth/hci_event.c.orig 2008-02-25 17:17:11.000000000 +0900
+++ net/bluetooth/hci_event.c 2008-02-25 17:30:23.000000000 +0900
@@ -1313,8 +1313,17 @@

        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) {
+ __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)
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }

        if (!ev->status) {
                conn->handle = __le16_to_cpu(ev->handle);

diff -uNr include/net/bluetooth-orig/sco.h include/net/bluetooth/sco.h
--- include/net/bluetooth-orig/sco.h 2007-10-10 05:31:38.000000000 +0900
+++ include/net/bluetooth/sco.h 2008-02-25 18:04:20.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 */
diff -uNr net/bluetooth-orig/hci_conn.c net/bluetooth/hci_conn.c
--- net/bluetooth-orig/hci_conn.c 2008-02-25 17:58:27.000000000 +0900
+++ net/bluetooth/hci_conn.c 2008-02-25 18:02:04.000000000 +0900
@@ -354,7 +354,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);

diff -uNr net/bluetooth-orig/sco.c net/bluetooth/sco.c
--- net/bluetooth-orig/sco.c 2008-02-25 17:58:27.000000000 +0900
+++ net/bluetooth/sco.c 2008-02-25 18:08:51.000000000 +0900 @@ -200,7 +200,10 @@

        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 +663,21 @@
  {
        struct sock *sk = sock->sk;
        int err = 0;

+ unsigned int force_sco;

        BT_DBG("sk %p", sk);

        lock_sock(sk);

        switch (optname) {
 +       case SCO_FORCESCO:
 +               if (copy_from_user((char *)&force_sco, optval, sizeof(unsigned int))) {
 +                       err = -EFAULT;
 +                       break;
 +               }
 +               sco_pi(sk)->force_sco = (force_sco != 0);
 +               break;
 +
        default:
                err = -ENOPROTOOPT;
                break;
 @@ -681,6 +693,7 @@
        struct sco_options opts;
        struct sco_conninfo cinfo;
        int len, err = 0;

+ unsigned int force_sco;

        BT_DBG("sk %p", sk);

@@ -721,6 +734,13 @@

                break;

+ case SCO_FORCESCO:
+ force_sco = sco_pi(sock)->force_sco;
+ if (copy_to_user(optval, (char *)&force_sco, sizeof(unsigned int)))
+ err = -EFAULT;
+
+ break;
+

Quoted 5 lines
        default:
                err = -ENOPROTOOPT;
                break;

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#11 lore raw

Dave Young 쓴 글:

Quoted 28 lines · Louis JANG
Quoted 27 lines
Quoted 15 lines

I ever asked marcel about the coding style. please see following thread:
http://lkml.org/lkml/2008/1/22/91

I think the style problem marcel said is
1. using kernel codeing style
2. marcel's style
container_of or get_user_data calls at the top of the variable declaration using the empty lines to seperate code blocks

Please rework your patch and resend if you fixed them.

BTW, please use the new bluetooth mailing list for kerne issue. [email protected]

Quoted 9 lines

(Thanks for andrew and davem)

Regards
dave

Regards
dave

Hi all,

Quoted 8 lines
Quoted 2 lines

I adjusted indentation of the patches

Not enough.

Please first read Documentation/CodingStyle, fix them, and then use scripts/checkpatch.pl to check your patch.

I fixed all of errors except 80 characters warning. Thanks

Louis JANG

Signed-off-by Louis JANG <[email protected]>
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +11 -2
@@ -1313,8 +1313,17 @@
 	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) {
+			__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)
+				conn->type = ev->link_type;
+		}
+		if (!conn)
+			goto unlock;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
Signed-off-by Louis JANG <[email protected]>
include/net/bluetooth/sco.h renamed from include/net/bluetooth-orig/sco.h +3 -0
diff -uNr linux-2.6.23/include/net/bluetooth-orig/sco.h linux-2.6.23/include/net/bluetooth/sco.h
@@ -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 */
net/bluetooth/hci_conn.c renamed from net/bluetooth-orig/hci_conn.c +1 -1
diff -uNr linux-2.6.23/net/bluetooth-orig/hci_conn.c linux-2.6.23/net/bluetooth/hci_conn.c
@@ -354,7 +354,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/sco.c renamed from net/bluetooth-orig/sco.c +21 -1
diff -uNr linux-2.6.23/net/bluetooth-orig/sco.c linux-2.6.23/net/bluetooth/sco.c
@@ -200,7 +200,10 @@
 
 	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 +663,21 @@
 {
 	struct sock *sk = sock->sk;
 	int err = 0;
+	int force_sco;
 
 	BT_DBG("sk %p", sk);
 
 	lock_sock(sk);
 
 	switch (optname) {
+	case SCO_FORCESCO:
+		if (copy_from_user(&force_sco, optval, sizeof(int))) {
+			err = -EFAULT;
+			break;
+		}
+		sco_pi(sk)->force_sco = (force_sco != 0);
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;

@@ -681,6 +693,7 @@
 	struct sco_options opts;
 	struct sco_conninfo cinfo;
 	int len, err = 0;
+	int force_sco;
 
 	BT_DBG("sk %p", sk);
 

@@ -721,6 +734,13 @@
 
 		break;
 
+	case SCO_FORCESCO:
+		force_sco = sco_pi(sock)->force_sco;
+		if (copy_to_user(optval, &force_sco, sizeof(int)))
+			err = -EFAULT;
+
+		break;
+
 	default:
 		err = -ENOPROTOOPT;
 		break;
maintainer
#12 lore raw

Hi Louis,

Quoted 19 lines

I fixed all of errors except 80 characters warning. Thanks

Louis JANG

Signed-off-by Louis JANG <louis-6CAt/[email protected]>

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-25
17:17:11.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-25
17:30:23.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +2 -2
@@ -1313,8 +1313,17 @@
 	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) {

net/bluetooth/hci_event.c line 1327

Quoted 10 lines
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -0
+			__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)
+				conn->type = ev->link_type;
+		}
+		if (!conn)
+			goto unlock;
+	}

NAK. There is no need to check for ACL_LINK. The sync_complete will only be called for SCO or eSCO connections.

include/net/bluetooth/sco.h line 55

Quoted 11 lines

diff -uNr linux-2.6.23/include/net/bluetooth-orig/sco.h linux-2.6.23/include/net/bluetooth/sco.h
--- linux-2.6.23/include/net/bluetooth-orig/sco.h 2007-10-10
05:31:38.000000000 +0900
+++ linux-2.6.23/include/net/bluetooth/sco.h 2008-02-25
18:04:20.000000000 +0900

include/net/bluetooth/sco.h renamed from include/net/bluetooth-orig/sco.h +2 -0
@@ -51,6 +51,8 @@
 	__u8  dev_class[3];
 };
 
+#define SCO_FORCESCO	0x03
+

NAK. We don't need this. And even if we really would want this, we would do it via extra parameters inside sockaddr_sco. In that case we would do it right and exposing eSCO settings and not some boolean parameter.

Regards

Marcel

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#13 lore raw

Hi Marcel

Quoted 16 lines
Quoted 16 lines

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-25 17:17:11.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-25 17:30:23.000000000 +0900

(quoted diff) +6 -2
@@ -1313,8 +1313,17 @@
 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) {
+ __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)
Quoted 8 lines

Review line 1326

Quoted 5 lines
(quoted diff) +5 -0
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }

NAK. There is no need to check for ACL_LINK. The sync_complete will only be called for SCO or eSCO connections.

I see. I removed this check line in the patch.

Thanks.
Louis JANG

Signed-off-by Louis JANG <[email protected]>
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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)
+			conn->type = ev->link_type;
+		else
+			goto unlock;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
#3 lore raw
In reply to Brad Midgley

Hello,

Le dimanche 24 février 2008 à 23:43 -0700, Brad Midgley a écrit :
[...]

Quoted 9 lines

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.

bluetooth/hci_conn.c renamed from bluetooth/hci_conn.c.orig +4 -1
@@ -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);
bluetooth/hci_event.c renamed from bluetooth/hci_event.c.orig +2 -2
@@ -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);
bluetooth/sco.c renamed from bluetooth/sco.c.orig +3 -4
@@ -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;

-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#4 lore raw

Hi Guillaume,

Quoted 20 lines
Quoted 11 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 10 lines

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 ?).

Quoted 5 lines

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 [email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#14 lore raw
In reply to Louis JANG

Hi Loius,

Quoted 52 lines
Quoted 17 lines
Quoted 17 lines

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-25 17:17:11.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-25 17:30:23.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +6 -2
@@ -1313,8 +1313,17 @@
 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) {
+ __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)
Quoted 8 lines

net/bluetooth/hci_event.c line 1324

Quoted 5 lines
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +5 -0
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }

NAK. There is no need to check for ACL_LINK. The sync_complete will only be called for SCO or eSCO connections.

I see. I removed this check line in the patch.

Thanks.
Louis JANG

Signed-off-by Louis JANG <louis-6CAt/[email protected]>

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-26
12:46:36.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-26
12:47:23.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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)
+			conn->type = ev->link_type;
+		else
+			goto unlock;
+	}
 
 	if (!ev->status) {
Quoted 1 line
		conn->handle = __le16_to_cpu(ev->handle);

do something like this:

if (!conn) {
	....

	conn = ....
	if (!conn)
		goto unlock;

	conn->type = ev->link_type;
}

And include a description when submitting a patch.

Regards

Marcel

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Marcel Holtmann

Re,

Quoted 4 lines · Marcel Holtmann

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.

bluetooth/hci_event.c renamed from bluetooth/hci_event.c.orig +5 -0
@@ -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;

-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

author
#15 lore raw
In reply to Marcel Holtmann

Marcel Holtmann 쓴 글:

Quoted 64 lines

Hi Loius,

Quoted 42 lines
Quoted 17 lines
Quoted 17 lines

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-25 17:17:11.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-25 17:30:23.000000000 +0900

(quoted diff) +6 -2
@@ -1313,8 +1313,17 @@
 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) {
+ __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)
Quoted 8 lines

Review line 1327

Quoted 5 lines
(quoted diff) +5 -0
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }

NAK. There is no need to check for ACL_LINK. The sync_complete will only be called for SCO or eSCO connections.

I see. I removed this check line in the patch.

Thanks.
Louis JANG

Signed-off-by Louis JANG <louis-6CAt/[email protected]>

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-26 12:46:36.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-26 12:47:23.000000000 +0900

(quoted diff) +2 -2
@@ -1313,8 +1313,15 @@
 hci_dev_lock(hdev);
 
 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;

Review line 1327

Quoted 10 lines
(quoted diff) +7 -0
+
+ conn = hci_conn_hash_lookup_ba(hdev, link_type, &ev->bdaddr);
+ if (conn)
+ conn->type = ev->link_type;
+ else
+ goto unlock;
+ }
 
 if (!ev->status) {
 conn->handle = __le16_to_cpu(ev->handle);

do something like this:

if (!conn) {
....

conn = ....
if (!conn)
goto unlock;

Quoted 8 lines

conn->type = ev->link_type;
}

And include a description when submitting a patch.

Regards

Marcel

I changed code with this style and included patch description. Thanks

Louis JANG

This patch is to handle different type of synchronous link is estabilished with its request.

Signed-off-by Louis JANG <[email protected]>
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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;
+
+		conn->type = ev->link_type;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
maintainer
#16 lore raw

Hi Louis,

Quoted 101 lines
Quoted 63 lines
Quoted 52 lines
Quoted 17 lines
Quoted 17 lines

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-25 17:17:11.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-25 17:30:23.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +6 -2
@@ -1313,8 +1313,17 @@
 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) {
+ __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)
Quoted 8 lines

net/bluetooth/hci_event.c line 1324

Quoted 5 lines
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +5 -0
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }

NAK. There is no need to check for ACL_LINK. The sync_complete will only be called for SCO or eSCO connections.

I see. I removed this check line in the patch.

Thanks.
Louis JANG

Signed-off-by Louis JANG <[email protected]>

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-26 12:46:36.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-26 12:47:23.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 hci_dev_lock(hdev);
 
 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)
+ conn->type = ev->link_type;
+ else
+ goto unlock;
+ }
 
 if (!ev->status) {
Quoted 1 line

conn->handle = __le16_to_cpu(ev->handle);

do something like this:

if (!conn) {
....

conn = ....
if (!conn)
goto unlock;

Quoted 8 lines

conn->type = ev->link_type;
}

And include a description when submitting a patch.

Regards

Marcel

I changed code with this style and included patch description. Thanks

Louis JANG
This patch is to handle different type of synchronous link is estabilished with its request.

Signed-off-by Louis JANG <[email protected]>

--- linux-2.6.23/net/bluetooth/hci_event.c.orig 2008-02-26
12:46:36.000000000 +0900
+++ linux-2.6.23/net/bluetooth/hci_event.c 2008-02-27
10:48:29.000000000 +0900

net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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;
+
+		conn->type = ev->link_type;
+	}
 
 	if (!ev->status) {
Quoted 1 line
		conn->handle = __le16_to_cpu(ev->handle);

so the patch is fine, but the description of what this patch is doing, why it is doing it this way and what it fixes is fully missing. Please update it with a proper description.

Regards

Marcel

author
#17 lore raw

Marcel Holtmann 쓴 글:

Quoted 4 lines

so the patch is fine, but the description of what this patch is doing, why it is doing it this way and what it fixes is fully missing. Please update it with a proper description.

I added more information. please check it, and please let me know if there are any missings.

Thanks
Louis JANG

Quoted 4 lines

Regards

Marcel

When bluez tried to connect SCO channel with HCI_OP_SETUP_SYNC_CONN(ESCO_LINK), a real connection may be connected with SCO_LINK instead of ESCO_LINK when peer device doesn't support eSCO. however bluez try to find connection handle by event's link type(SCO_LINK in above case) and the valid connection handle for this event is waiting for ESCO_LINK. so bluez can't find connection handle and discarded the event.

This patch is to handle different type of synchronous link is estabilished with its request.

If bluez can't find connection handle, it try to find with different link type again. (For the above case, if bluez can't find connection handle with SCO_LINK, it try to find connection handle with ESCO_LINK again.) and update link type of this connection handle with event's link type.

Signed-off-by Louis JANG <[email protected]>
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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;
+
+		conn->type = ev->link_type;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
#18 lore raw

Re,

net/bluetooth/hci_event.c line 1321

Quoted 9 lines · Louis JANG
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +6 -2
 	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.

maintainer
#19 lore raw
In reply to Louis JANG

Hi Louis,

Quoted 6 lines

When bluez tried to connect SCO channel with HCI_OP_SETUP_SYNC_CONN(ESCO_LINK), a real connection may be connected with SCO_LINK instead of ESCO_LINK when peer device doesn't support eSCO. however bluez try to find connection handle by event's link type(SCO_LINK in above case) and the valid connection handle for this event is waiting for ESCO_LINK. so bluez can't find connection handle and discarded the event.

using HCI_OP_SETUP_SYNC_CONN doesn't mean eSCO. It is perfectly fine to request SCO links via that command. The difference here is Bluetooth 1.1 or 1.2 and later.

Quoted 7 lines

This patch is to handle different type of synchronous link is estabilished with its request.

If bluez can't find connection handle, it try to find with different link type again. (For the above case, if bluez can't find connection handle with SCO_LINK, it try to find connection handle with ESCO_LINK again.) and update link type of this connection handle with event's link type.

Inside the kernel it is not called BlueZ :) It simply is the Bluetooth subsystem and in case the HCI core.

Regards

Marcel

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#20 lore raw

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.

Best regards,
Louis JANG

Marcel Holtmann 쓴 글:

Quoted 33 lines

Hi Louis,

Quoted 11 lines

When bluez tried to connect SCO channel with HCI_OP_SETUP_SYNC_CONN(ESCO_LINK),
a real connection may be connected with SCO_LINK instead of ESCO_LINK when
peer device doesn't support eSCO. however bluez try to find connection handle
by event's link type(SCO_LINK in above case) and the valid connection handle
for this event is waiting for ESCO_LINK. so bluez can't find connection handle
and discarded the event.

using HCI_OP_SETUP_SYNC_CONN doesn't mean eSCO. It is perfectly fine to request SCO links via that command. The difference here is Bluetooth 1.1 or 1.2 and later.

Quoted 9 lines

This patch is to handle different type of synchronous link is estabilished
with its request.

If bluez can't find connection handle, it try to find with different link type again. (For the above case, if bluez can't find connection handle with SCO_LINK, it try to find connection handle with ESCO_LINK again.)
and update link type of this connection handle with event's link type.

Inside the kernel it is not called BlueZ :) It simply is the Bluetooth subsystem and in case the HCI core.

Regards

Quoted 8 lines

Marcel

-
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

When HCI core tried to connect SCO channel with HCI_OP_SETUP_SYNC_CONN, HCI core is using ESCO_LINK link type if LM supports eSCO. however a real connection may be connected with SCO_LINK instead of ESCO_LINK when peer device doesn't support eSCO.

However HCI core try to find connection handle by event's link type (SCO_LINK in above case) in this case, the valid connection handle for this event is waiting for ESCO_LINK. HCI core can't find connection handle and discarded the event.

This patch is to handle different type of synchronous link is estabilished with its request.

If HCI core can't find connection handle, it try to find with different link type again. (For the above case, if HCI core can't find connection handle with SCO_LINK, it try to find connection handle with ESCO_LINK again.) and update link type of this connection handle with event's link type.

Signed-off-by Louis JANG <[email protected]>
net/bluetooth/hci_event.c renamed from net/bluetooth/hci_event.c.orig +9 -2
@@ -1313,8 +1313,15 @@
 	hci_dev_lock(hdev);
 
 	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;
+
+		conn->type = ev->link_type;
+	}
 
 	if (!ev->status) {
 		conn->handle = __le16_to_cpu(ev->handle);
#21 lore raw
Quoted 9 lines · Louis JANG

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.