Quote mail from [email protected] :
2007/12/17 Louis JANG <[email protected]>:
> 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 [email protected]:
On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <[email protected]> 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.
[email protected]
(Thanks for andrew and davem)
Regards
dave
Regards
dave
On Mon, Feb 25, 2008 at 3:30 PM, Dave Young <[email protected]> wrote:
> Quote mail from [email protected] :
>
> 2007/12/17 Louis JANG <[email protected]>:
>
>
> > 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 [email protected]:
>
> On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <[email protected]> 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.
> [email protected]
>
> (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
>
Sorry, [email protected] was missed in cc
On Mon, Feb 25, 2008 at 3:34 PM, Dave Young <[email protected]> wrote:
>
> On Mon, Feb 25, 2008 at 3:30 PM, Dave Young <[email protected]> wrote:
> > Quote mail from [email protected] :
> >
> > 2007/12/17 Louis JANG <[email protected]>:
> >
> >
> > > 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 [email protected]:
> >
> > On Mon, Feb 25, 2008 at 2:43 PM, Brad Midgley <[email protected]> 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.
> > [email protected]
> >
> > (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
> >
>
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;
On Mon, Feb 25, 2008 at 5:28 PM, Louis JANG <[email protected]> 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.
> > [email protected]
> >
> > (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;
>
>
Signed-off-by: Louis JANG <[email protected]>
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;
Hi Louis,
> I fixed all of errors except 80 characters warning.
> Thanks
>
> Louis JANG
>
> Signed-off-by: Louis JANG <[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
> @@ -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
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
@@ -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);
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 <[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
> @@ -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
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
@@ -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);
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 <[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
>>> @@ -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 <[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
> @@ -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
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]>
--- 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);
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
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]>
--- 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);