Re: [Bluez-devel] forcing SCO connection patch

Re: [Bluez-devel] forcing SCO connection patch

[Dave Young]

Quote mail from louis@mizi.com :

2007/12/17 Louis JANG <louis@mizi.com>:
> 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
>
>

Reply from bmidgley@gmail.com:

On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <bmidgley@gmail.com> wrote:
> 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
>


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.
linux-bluetooth@vger.kernel.org

(Thanks for andrew and davem)

Regards
dave

Regards
dave

Re: [Bluez-devel] forcing SCO connection patch

[Dave Young]

On Mon, Feb 25, 2008 at 3:30 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
> Quote mail from louis@mizi.com :
>
>  2007/12/17 Louis JANG <louis@mizi.com>:
>
>
> > 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
>  >
>  >
>
>  Reply from bmidgley@gmail.com:
>
>  On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <bmidgley@gmail.com> wrote:
>  > 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
>  >
>
>
>  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.
>  linux-bluetooth@vger.kernel.org
>
>  (Thanks for andrew and davem)

On bugzilla, bug 9871 are same problem as yours.

add davem and netdev in cc-list

>
>  Regards
>  dave
>
>  Regards
>  dave
>

Re: [Bluez-devel] forcing SCO connection patch

[Dave Young]

Sorry, bmidgley@gmail.com was missed in cc

On Mon, Feb 25, 2008 at 3:34 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>
> On Mon, Feb 25, 2008 at 3:30 PM, Dave Young <hidave.darkstar@gmail.com> wrote:
>  > Quote mail from louis@mizi.com :
>  >
>  >  2007/12/17 Louis JANG <louis@mizi.com>:
>  >
>  >
>  > > 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
>  >  >
>  >  >
>  >
>  >  Reply from bmidgley@gmail.com:
>  >
>  >  On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <bmidgley@gmail.com> wrote:
>  >  > 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
>  >  >
>  >
>  >
>  >  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.
>  >  linux-bluetooth@vger.kernel.org
>  >
>  >  (Thanks for andrew and davem)
>
>  On bugzilla, bug 9871 are same problem as yours.
>
>  add davem and netdev in cc-list
>
>  >
>  >  Regards
>  >  dave
>  >
>  >  Regards
>  >  dave
>  >
>

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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


> 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.
> linux-bluetooth@vger.kernel.org
>
> (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


[-- Attachment #2: patch_hci_event.c2 --]
[-- Type: text/plain, Size: 635 bytes --]

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

[-- Attachment #3: bluez-kernel-forcesco.patch2 --]
[-- Type: text/plain, Size: 2191 bytes --]

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;
+		
 	default:
 		err = -ENOPROTOOPT;
 		break;

Re: [Bluez-devel] forcing SCO connection patch

[Dave Young]

On Mon, Feb 25, 2008 at 5:28 PM, Louis JANG <louis@mizi.com> wrote:
>
>  > 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.
>  > linux-bluetooth@vger.kernel.org
>  >
>  > (Thanks for andrew and davem)
>  >
>  > Regards
>  > dave
>  >
>  > 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.

>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;
>  +
>         default:
>                 err = -ENOPROTOOPT;
>                 break;
>
>

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

Dave Young 쓴 글:
> On Mon, Feb 25, 2008 at 5:28 PM, Louis JANG <louis@mizi.com> wrote:
>   
>>  > 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.
>>  > linux-bluetooth@vger.kernel.org
>>  >
>>  > (Thanks for andrew and davem)
>>  >
>>  > Regards
>>  > dave
>>  >
>>  > 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.
>   
I fixed all of errors except 80 characters warning.
Thanks

Louis JANG


[-- Attachment #2: patch_hci_event.c3 --]
[-- Type: text/plain, Size: 699 bytes --]

Signed-off-by: Louis JANG <louis@mizi.com>

--- 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
@@ -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);

[-- Attachment #3: bluez-kernel-forcesco.patch3 --]
[-- Type: text/plain, Size: 2336 bytes --]

Signed-off-by: Louis JANG <louis@mizi.com>

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
@@ -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 linux-2.6.23/net/bluetooth-orig/hci_conn.c linux-2.6.23/net/bluetooth/hci_conn.c
--- linux-2.6.23/net/bluetooth-orig/hci_conn.c	2008-02-25 17:58:27.000000000 +0900
+++ linux-2.6.23/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 linux-2.6.23/net/bluetooth-orig/sco.c linux-2.6.23/net/bluetooth/sco.c
--- linux-2.6.23/net/bluetooth-orig/sco.c	2008-02-25 17:58:27.000000000 +0900
+++ linux-2.6.23/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;
+	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;

Re: [Bluez-devel] forcing SCO connection patch

[Marcel Holtmann]

Hi Louis,

> I fixed all of errors except 80 characters warning.
> Thanks
>
> Louis JANG
>
> Signed-off-by: Louis JANG <louis@mizi.com>
>
> --- 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
> @@ -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;
> +	}

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

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

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

Hi Marcel


>> --- 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
>> @@ -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;
>> + }
>
> 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

[-- Attachment #2: patch_hci_event.c4 --]
[-- Type: text/plain, Size: 647 bytes --]

Signed-off-by: Louis JANG <louis@mizi.com>
--- 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
@@ -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);

Re: [Bluez-devel] forcing SCO connection patch

[Marcel Holtmann]

Hi Loius,

>>> --- 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
>>> @@ -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;
>>> + }
>>
>> 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@mizi.com>
> --- 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
> @@ -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);

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

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

Marcel Holtmann 쓴 글:
> Hi Loius,
>
>>>> --- 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
>>>> @@ -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;
>>>> + }
>>>
>>> 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@mizi.com>
>> --- 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
>> @@ -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);
>
> 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
I changed code with this style and included patch description.
Thanks

Louis JANG

[-- Attachment #2: patch_hci_event.c5 --]
[-- Type: text/plain, Size: 736 bytes --]

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

Signed-off-by: Louis JANG <louis@mizi.com>
--- 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
@@ -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);

Re: [Bluez-devel] forcing SCO connection patch

[Marcel Holtmann]

Hi Louis,

>>>>> --- 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
>>>>> @@ -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;
>>>>> + }
>>>>
>>>> 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@mizi.com>
>>> --- 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
>>> @@ -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);
>>
>> 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
> 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 <louis@mizi.com>
> --- 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
> @@ -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);

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

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

Marcel Holtmann 쓴 글:
>
> 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

>
> Regards
>
> Marcel


[-- Attachment #2: patch_hci_event.c6 --]
[-- Type: text/plain, Size: 1449 bytes --]

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 <louis@mizi.com>
--- 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
@@ -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);

Re: [Bluez-devel] forcing SCO connection patch

[Marcel Holtmann]

Hi Louis,

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

> 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

Re: [Bluez-devel] forcing SCO connection patch

[Louis JANG]

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

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 쓴 글:
> Hi Louis,
>
>> 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.
>
>> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>


[-- Attachment #2: patch_hci_event.c7 --]
[-- Type: text/plain, Size: 1529 bytes --]

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 <louis@mizi.com>
--- 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
@@ -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);