2011-12-27 17:22:26

by Andre Guedes

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

On Thu, Dec 22, 2011 at 8:18 PM, Rene Herman <[email protected]> wrote:
> Good day.
>
> A while ago, my noname USB Bluetooth adapter stopped working, with from that
> point on only "Bluetooth: hci0 command tx timeout" appearing in the kernel
> messages.
>
> I just now got around to looking at it and bisected this to commit
> eead27da60df80a112d1ac3ea482226e9794c26b:
>
> ===
> Author: Andre Guedes <[email protected]>
> Date: ? Thu Jun 30 19:20:55 2011 -0300
>
> ? ?Bluetooth: Add lmp_host_le_capable() macro
>
> ? ?Since we have the extended LMP features properly implemented, we
> ? ?should check the LMP_HOST_LE bit to know if the host supports LE.
>
> ? ?Signed-off-by: Andre Guedes <[email protected]>
> ? ?Signed-off-by: Gustavo F. Padovan <[email protected]>
> ===
>
> while that commit cannot be reverted outright due to dependencies, reverting
> it functionally fixes things for me. That is, Bluetooth works again after
> applying (to linux-3.1.6):
>
> ===
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 56943ad..c7f794e 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -540,7 +540,7 @@ int hci_dev_open(__u16 dev)
> ? ? ? ? ? ? ? ?ret = __hci_request(hdev, hci_init_req, 0,
>
> msecs_to_jiffies(HCI_INIT_TIMEOUT));
>
> - ? ? ? ? ? ? ? if (lmp_host_le_capable(hdev))
> + ? ? ? ? ? ? ? if (lmp_le_capable(hdev))
> ? ? ? ? ? ? ? ? ? ? ? ?ret = __hci_request(hdev, hci_le_init_req, 0,
>
> msecs_to_jiffies(HCI_INIT_TIMEOUT));
> ===
>
> It seems that my adapter wasn't ready for the change. It is a:
>
> 002 Device 002: ID 1131:1004 Integrated System Solution Corp. Bluetooth
> Device
>
> If more specific information is wanted (to add adapter-specific quirk
> handling, perhaps?) please be verbose on how to obtain it. I don't know
> anything about Bluetooth

Could you please send us the kernel log messages with Bluetooth debug
enabled? Here goes the instructions:

1. Compile kernel with CONFIG_DYNAMIC_DEBUG=y.
2. Load Bluetooth module.
3. Run the following commands to turn debug on (remember to mount debugfs
first):
--
$ echo "file hci_core.c +pf" > /sys/kernel/debug/dynamic_debug/control
$ echo "file hci_event.c +pf" > /sys/kernel/debug/dynamic_debug/control
$ echo "file mgmt.c +pf" > /sys/kernel/debug/dynamic_debug/control
$ echo "file btusb.c +pf" > /sys/kernel/debug/dynamic_debug/control
--
4. Plug the Bluetooth adapter.

Thanks in advance,

Andre


2011-12-29 00:22:01

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 29-12-11 00:07, Marcel Holtmann wrote:

>> By the way, while the bluetooth (2.0) spec seems to consist of a 1230
>> page document that I am most certainly not going to study ...
>>
>> ... I couldn't help noticing that the HCI_Read_Local_Extended_Features
>> command is in fact specified to return "The highest features page number
>> which contains non-zero bits for the local device", and if I look at the
>> above, it seems to indeed return max=0.
>>
>> Is it as such not in fact the case that the dongle is spec-compliant,
>> whereas it's the linux code that neglects to check that return value in
>> order to make sure that it requested a valid page?
>
> the Linux code indeed is broken for not checking the returned page, but
> it is broken for different reasons.
>
> However the expected result from the dongle would be an error code and
> not just returning page 0 instead.

Okay. Just as an additional data point:

I came across the HCI_Read_Local_Supported_Commands command in the spec
a few pages up and quickly verified that the adapter doesn't in fact
deny support for the HCI_Read_Local_Extended_Features command.

That is, after applying the below, I do get:

hci_cc_read_local_commands:hci0 status 0x0
hci_cc_read_local_commands:hci0 commands[14] 0xe8

which does claim support for the command (bit 6 of commands[14]).

Thought I'd check since if it hadn't, that would imply a certain fix but
that can't be used then.

I'll stand by to test whatever final solution is arrived at.

(although I don't personally care, linux 3.0.y and 3.1.y may want it as
well at that point I guess).

Rene.

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8fe270c..0ac87ea 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -622,6 +622,7 @@ static void hci_cc_read_local_commands(struct
hci_dev *hdev, struct sk_buff *skb
goto done;

memcpy(hdev->commands, rp->commands, sizeof(hdev->commands));
+ BT_DBG("%s commands[14] 0x%x", hdev->name, rp->commands[14]);

if (test_bit(HCI_INIT, &hdev->flags) && (hdev->commands[5] & 0x10))
hci_setup_link_policy(hdev);

2011-12-28 23:07:03

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

> > < HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
> > page 1
> > > HCI Event: Command Complete (0x0e) plen 14
> > Read Local Extended Features (0x04|0x0004) ncmd 1
> > status 0x00 page 0 max 0
> > Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
> >
> > And this is correct. Page 0 is the same as local features. Storing this
> > as page 1 is the issue here. We request page 1, but we are getting page
> > 0 instead. So yes, the controller is broken, but not as broken as it
> > gets us false information.
>
> By the way, while the bluetooth (2.0) spec seems to consist of a 1230
> page document that I am most certainly not going to study ...
>
> ... I couldn't help noticing that the HCI_Read_Local_Extended_Features
> command is in fact specified to return "The highest features page number
> which contains non-zero bits for the local device", and if I look at the
> above, it seems to indeed return max=0.
>
> Is it as such not in fact the case that the dongle is spec-compliant,
> whereas it's the linux code that neglects to check that return value in
> order to make sure that it requested a valid page?

the Linux code indeed is broken for not checking the returned page, but
it is broken for different reasons.

However the expected result from the dongle would be an error code and
not just returning page 0 instead.

And the max value just tells you about the max page value. So that the
host does not have to go ahead and read 255 pages before realizing that
there is nothing there.

Regards

Marcel



2011-12-28 22:17:30

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 28-12-11 17:48, Marcel Holtmann wrote:

> < HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
> page 1
> > HCI Event: Command Complete (0x0e) plen 14
> Read Local Extended Features (0x04|0x0004) ncmd 1
> status 0x00 page 0 max 0
> Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
>
> And this is correct. Page 0 is the same as local features. Storing this
> as page 1 is the issue here. We request page 1, but we are getting page
> 0 instead. So yes, the controller is broken, but not as broken as it
> gets us false information.

By the way, while the bluetooth (2.0) spec seems to consist of a 1230
page document that I am most certainly not going to study ...

... I couldn't help noticing that the HCI_Read_Local_Extended_Features
command is in fact specified to return "The highest features page number
which contains non-zero bits for the local device", and if I look at the
above, it seems to indeed return max=0.

Is it as such not in fact the case that the dongle is spec-compliant,
whereas it's the linux code that neglects to check that return value in
order to make sure that it requested a valid page?

Rene.

2011-12-28 17:24:18

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 28-12-11 17:48, Marcel Holtmann wrote:

> The correct patch would be something like this:
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 919e3c0..b6eb9c5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -711,7 +711,8 @@ static void hci_cc_read_local_ext_features(struct hci_dev *h
> if (rp->status)
> return;
>
> - memcpy(hdev->extfeatures, rp->features, 8);
> + if (rp->page == 1)
> + memcpy(hdev->extfeatures, rp->features, 8);
>
> hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
> }
>
> The current patch is actually fully broken anyway. So besides switching
> LE on when requesting page 0, you will also switch it off when
> requesting any other page and it accidentally returns 0x00 since we
> unconditionally overwrite it.

Don't know anything about pages, but... I can confirm that the above
works for me as well.

Rene.

2011-12-28 17:12:25

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

> > I think this patch can do handling, let's see what others think.
>
> I can confirm that this works for me.
>
> Might it be useful to specify the currently observed crap dongle (ie,
> USB 1131:1004) in the changelog or comment? That way, if the handling
> ever changes in the manner that David suggests, maybe this specific
> dongle will find its way into the list immediately.
>
> But... works for me as is.

the patch is wrong. It just fixes a symptom and not the problem. See my
other email.

Regards

Marcel



2011-12-28 16:54:58

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 28-12-11 16:52, Gustavo Padovan wrote:

> I think this patch can do handling, let's see what others think.

I can confirm that this works for me.

Might it be useful to specify the currently observed crap dongle (ie,
USB 1131:1004) in the changelog or comment? That way, if the handling
ever changes in the manner that David suggests, maybe this specific
dongle will find its way into the list immediately.

But... works for me as is.

Rene.

2011-12-28 16:48:43

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Gustavo,

> > >>For some reason your adapter is returning here the same value of
> > >>Read Local Features. I would say your device is broken. The only
> > >>fix I have in mind now is throw away the device.
> > >
> > >Yes, I have one of these ISSC devices, it's completely broken
> > >device.
> >
> > It worked well upto and including 2.6.29 though.
>
> We never used Extended Features before 2.6.39
>
> >
> > This would seem to be the kind of thing other subsystems use quirk
> > handling for. The device identifies itself as "1131:1004":
> >
> > Bus 002 Device 002: ID 1131:1004 Integrated System Solution Corp.
> > Bluetooth Device
> >
> > Do you guys have infra-structure in place for adapter-specific
> > (quitk) handling?
>
> I think this patch can do handling, let's see what others think.
>
> Gustavo
>
>
> ---
> Author: Gustavo F. Padovan <[email protected]>
> Date: Wed Dec 28 13:40:02 2011 -0200
>
> Bluetooth: Fix lmp_host_le_capable() check for broken devices
>
> Some dongles reports a wrong Local Extended Features leading the kernel
> think that dongle support LE while it don't.
>
> The fix here is just rely on a bit in Local Features (LE Capable) to tell
> us if the device really supports LE.
>
> LE Host Capable is the only bit used from Local Extended Features in our
> kernel.
>
> Signed-off-by: Gustavo F. Padovan <[email protected]>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5e2e984..c693111 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -676,7 +676,11 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>
> /* ----- Extended LMP capabilities ----- */
> -#define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
> +/* Some crap dongles does not report a proper Local Extended Features causing

the page 1 is called host features and not local extended features.

> + * the kernel to wrongly init it as a LE device. So first check if it is LE
> + * capable (controller) which is a info from the Local Features */
> +#define lmp_host_le_capable(dev) ( lmp_le_capable(dev) && \
> + (dev)->extfeatures[0] & LMP_HOST_LE)

< HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
page 1
> HCI Event: Command Complete (0x0e) plen 14
Read Local Extended Features (0x04|0x0004) ncmd 1
status 0x00 page 0 max 0
Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80

And this is correct. Page 0 is the same as local features. Storing this
as page 1 is the issue here. We request page 1, but we are getting page
0 instead. So yes, the controller is broken, but not as broken as it
gets us false information. We are just getting different information
than we requested. And we are being stupid here.

Here is the original patch that introduced this behavior:

commit 971e3a4bbcbf7378315b85150853d86be59cffe0
Author: Andre Guedes <[email protected]>
Date: Thu Jun 30 19:20:52 2011 -0300

Bluetooth: Add extfeatures to struct hci_dev

And even here we should have called this hostfeatures since that is what
page 1 really is.

The correct patch would be something like this:

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 919e3c0..b6eb9c5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -711,7 +711,8 @@ static void hci_cc_read_local_ext_features(struct hci_dev *h
if (rp->status)
return;

- memcpy(hdev->extfeatures, rp->features, 8);
+ if (rp->page == 1)
+ memcpy(hdev->extfeatures, rp->features, 8);

hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
}

The current patch is actually fully broken anyway. So besides switching
LE on when requesting page 0, you will also switch it off when
requesting any other page and it accidentally returns 0x00 since we
unconditionally overwrite it.

Regards

Marcel



2011-12-28 16:16:44

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi David,

* David Herrmann <[email protected]> [2011-12-28 17:04:11 +0100]:

> On Wed, Dec 28, 2011 at 4:52 PM, Gustavo Padovan <[email protected]>=
wrote:
> ..snip..
> > I think this patch can do handling, let's see what others think.
> >
> > =A0 =A0 =A0 =A0Gustavo
> >
> >
> > ---
> > Author: Gustavo F. Padovan <[email protected]>
> > Date: =A0 Wed Dec 28 13:40:02 2011 -0200
> >
> > =A0 =A0Bluetooth: Fix lmp_host_le_capable() check for broken devices
> >
> > =A0 =A0Some dongles reports a wrong Local Extended Features leading the=
kernel
> > =A0 =A0think that dongle support LE while it don't.
> >
> > =A0 =A0The fix here is just rely on a bit in Local Features (LE Capable=
) to tell
> > =A0 =A0us if the device really supports LE.
> >
> > =A0 =A0LE Host Capable is the only bit used from Local Extended Feature=
s in our
> > =A0 =A0kernel.
> >
> > =A0 =A0Signed-off-by: Gustavo F. Padovan <[email protected]>
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/h=
ci_core.h
> > index 5e2e984..c693111 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -676,7 +676,11 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> > =A0#define lmp_le_capable(dev) =A0 =A0 =A0 =A0((dev)->features[4] & LMP=
_LE)
> >
> > =A0/* ----- Extended LMP capabilities ----- */
> > -#define lmp_host_le_capable(dev) =A0 ((dev)->extfeatures[0] & LMP_HOST=
_LE)
> > +/* Some crap dongles does not report a proper Local Extended Features =
causing
> > + * the kernel to wrongly init it as a LE device. So first check if it =
is LE
> > + * capable (controller) which is a info from the Local Features */
> > +#define lmp_host_le_capable(dev) =A0 =A0( lmp_le_capable(dev) && \
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 (dev)->extfeatures[0] & LMP_HOST_LE)
> >
> > =A0/* ----- HCI protocols ----- */
> > =A0static inline int hci_proto_connect_ind(struct hci_dev *hdev, bdaddr=
_t *bdaddr,
>=20
> Next time we use the extfeatures for something else, we will break
> this device again. Can't we fix dev->extfeatures on initialization
> based on a blacklist/quirklist? Or is dev->extfeatures modified after
> initialzation of the device somewhere?

Sure, we will, but I don't see us using another bit of extfeatures in the n=
ear
future (after the next Core Spec release maybe) and I don't wanna got that =
far
in this fix right now.

Gustavo

2011-12-28 16:04:11

by David Herrmann

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On Wed, Dec 28, 2011 at 4:52 PM, Gustavo Padovan <[email protected]> w=
rote:
..snip..
> I think this patch can do handling, let's see what others think.
>
> =A0 =A0 =A0 =A0Gustavo
>
>
> ---
> Author: Gustavo F. Padovan <[email protected]>
> Date: =A0 Wed Dec 28 13:40:02 2011 -0200
>
> =A0 =A0Bluetooth: Fix lmp_host_le_capable() check for broken devices
>
> =A0 =A0Some dongles reports a wrong Local Extended Features leading the k=
ernel
> =A0 =A0think that dongle support LE while it don't.
>
> =A0 =A0The fix here is just rely on a bit in Local Features (LE Capable) =
to tell
> =A0 =A0us if the device really supports LE.
>
> =A0 =A0LE Host Capable is the only bit used from Local Extended Features =
in our
> =A0 =A0kernel.
>
> =A0 =A0Signed-off-by: Gustavo F. Padovan <[email protected]>
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci=
_core.h
> index 5e2e984..c693111 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -676,7 +676,11 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> =A0#define lmp_le_capable(dev) =A0 =A0 =A0 =A0((dev)->features[4] & LMP_L=
E)
>
> =A0/* ----- Extended LMP capabilities ----- */
> -#define lmp_host_le_capable(dev) =A0 ((dev)->extfeatures[0] & LMP_HOST_L=
E)
> +/* Some crap dongles does not report a proper Local Extended Features ca=
using
> + * the kernel to wrongly init it as a LE device. So first check if it is=
LE
> + * capable (controller) which is a info from the Local Features */
> +#define lmp_host_le_capable(dev) =A0 =A0( lmp_le_capable(dev) && \
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
=A0 (dev)->extfeatures[0] & LMP_HOST_LE)
>
> =A0/* ----- HCI protocols ----- */
> =A0static inline int hci_proto_connect_ind(struct hci_dev *hdev, bdaddr_t=
*bdaddr,

Next time we use the extfeatures for something else, we will break
this device again. Can't we fix dev->extfeatures on initialization
based on a blacklist/quirklist? Or is dev->extfeatures modified after
initialzation of the device somewhere?

Regards
David

2011-12-28 15:52:25

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

* Rene Herman <[email protected]> [2011-12-28 02:53:26 +0100]:

> On 28-12-11 02:28, Gustavo Padovan wrote:
>
> >>For some reason your adapter is returning here the same value of
> >>Read Local Features. I would say your device is broken. The only
> >>fix I have in mind now is throw away the device.
> >
> >Yes, I have one of these ISSC devices, it's completely broken
> >device.
>
> It worked well upto and including 2.6.29 though.

We never used Extended Features before 2.6.39

>
> This would seem to be the kind of thing other subsystems use quirk
> handling for. The device identifies itself as "1131:1004":
>
> Bus 002 Device 002: ID 1131:1004 Integrated System Solution Corp.
> Bluetooth Device
>
> Do you guys have infra-structure in place for adapter-specific
> (quitk) handling?

I think this patch can do handling, let's see what others think.

Gustavo


---
Author: Gustavo F. Padovan <[email protected]>
Date: Wed Dec 28 13:40:02 2011 -0200

Bluetooth: Fix lmp_host_le_capable() check for broken devices

Some dongles reports a wrong Local Extended Features leading the kernel
think that dongle support LE while it don't.

The fix here is just rely on a bit in Local Features (LE Capable) to tell
us if the device really supports LE.

LE Host Capable is the only bit used from Local Extended Features in our
kernel.

Signed-off-by: Gustavo F. Padovan <[email protected]>

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5e2e984..c693111 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -676,7 +676,11 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
#define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)

/* ----- Extended LMP capabilities ----- */
-#define lmp_host_le_capable(dev) ((dev)->extfeatures[0] & LMP_HOST_LE)
+/* Some crap dongles does not report a proper Local Extended Features causing
+ * the kernel to wrongly init it as a LE device. So first check if it is LE
+ * capable (controller) which is a info from the Local Features */
+#define lmp_host_le_capable(dev) ( lmp_le_capable(dev) && \
+ (dev)->extfeatures[0] & LMP_HOST_LE)

/* ----- HCI protocols ----- */
static inline int hci_proto_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr,

2011-12-28 01:57:49

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 28-12-11 02:53, Rene Herman wrote:

> On 28-12-11 02:28, Gustavo Padovan wrote:
>
>>> For some reason your adapter is returning here the same value of
>>> Read Local Features. I would say your device is broken. The only
>>> fix I have in mind now is throw away the device.
>>
>> Yes, I have one of these ISSC devices, it's completely broken
>> device.
>
> It worked well upto and including 2.6.29 though.

2.6.39, that is.

> This would seem to be the kind of thing other subsystems use quirk
> handling for. The device identifies itself as "1131:1004":
>
> Bus 002 Device 002: ID 1131:1004 Integrated System Solution Corp.
> Bluetooth Device
>
> Do you guys have infra-structure in place for adapter-specific (quirk)
> handling?
>
> Rene.

2011-12-28 01:53:26

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 28-12-11 02:28, Gustavo Padovan wrote:

>> For some reason your adapter is returning here the same value of
>> Read Local Features. I would say your device is broken. The only
>> fix I have in mind now is throw away the device.
>
> Yes, I have one of these ISSC devices, it's completely broken
> device.

It worked well upto and including 2.6.29 though.

This would seem to be the kind of thing other subsystems use quirk
handling for. The device identifies itself as "1131:1004":

Bus 002 Device 002: ID 1131:1004 Integrated System Solution Corp.
Bluetooth Device

Do you guys have infra-structure in place for adapter-specific (quitk)
handling?

Rene.

2011-12-28 01:28:50

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

* Gustavo Padovan <[email protected]> [2011-12-27 23:22:48 -0200]:

> Hi Rene,
>
> * Rene Herman <[email protected]> [2011-12-27 23:19:40 +0100]:
>
> > On 27-12-11 21:30, Gustavo Padovan wrote:
> >
> > >Seems your extfeatures is telling you something different from what it really
> > >supports. Can you try with the following diff (use the dynamic debug that Andre
> > >told you)
> >
> > Attached. The result is "features: 0xff".
> >
> > I suppose that (should) mean(s) "all features"; if that's not
> > actually a valid possibility due, perhaps, to undefined or mutually
> > exclusive bits then maybe it's an option to take 0xff to mean that
> > the adapter doesn't in fact support any of them?
> >
> > >Also the output of hcidump would nice.
> >
> > If the below (which is what hcidump spit back at me at some as far
> > as I could determine random moment) is not what you wanted, I
> > believe you will need to specify which action to take while
> > monitoring things with hcidump.
> >
> > ==============
> > HCI sniffer - Bluetooth packet analyzer ver 2.1
> > device: hci0 snap_len: 1028 filter: 0xffffffff
> > < HCI Command: Reset (0x03|0x0003) plen 0
> > > HCI Event: Command Complete (0x0e) plen 4
> > Reset (0x03|0x0003) ncmd 1
> > status 0x00
> > < HCI Command: Read Local Supported Features (0x04|0x0003) plen 0
> > > HCI Event: Command Complete (0x0e) plen 12
> > Read Local Supported Features (0x04|0x0003) ncmd 1
> > status 0x00
> > Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
> > < HCI Command: Read Local Version Information (0x04|0x0001) plen 0
> > > HCI Event: Command Complete (0x0e) plen 12
> > Read Local Version Information (0x04|0x0001) ncmd 1
> > status 0x00
> > HCI Version: 2.0 (0x3) HCI Revision: 0x2da
> > LMP Version: 2.0 (0x3) LMP Subversion: 0x2da
> > Manufacturer: Integrated System Solution Corp. (57)
> > < HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> > > HCI Event: Command Complete (0x0e) plen 11
> > Read Buffer Size (0x04|0x0005) ncmd 1
> > status 0x00
> > ACL MTU 1021:4 SCO MTU 48:10
> > < HCI Command: Read BD ADDR (0x04|0x0009) plen 0
> > > HCI Event: Command Complete (0x0e) plen 10
> > Read BD ADDR (0x04|0x0009) ncmd 1
> > status 0x00 bdaddr 00:11:67:C0:12:9E
> > < HCI Command: Read Class of Device (0x03|0x0023) plen 0
> > > HCI Event: Command Complete (0x0e) plen 7
> > Read Class of Device (0x03|0x0023) ncmd 1
> > status 0x00 class 0x000000
> > < HCI Command: Read Local Name (0x03|0x0014) plen 0
> > > HCI Event: Command Complete (0x0e) plen 252
> > Read Local Name (0x03|0x0014) ncmd 1
> > status 0x00 name 'ISSCEDRBTA'
> > < HCI Command: Read Voice Setting (0x03|0x0025) plen 0
> > > HCI Event: Command Complete (0x0e) plen 6
> > Read Voice Setting (0x03|0x0025) ncmd 1
> > status 0x00 voice setting 0x0060
> > < HCI Command: Set Event Filter (0x03|0x0005) plen 1
> > type 0 condition 0
> > Clear all filters
> > > HCI Event: Command Complete (0x0e) plen 4
> > Set Event Filter (0x03|0x0005) ncmd 1
> > status 0x00
> > < HCI Command: Write Connection Accept Timeout (0x03|0x0016) plen 2
> > timeout 32000
> > > HCI Event: Command Complete (0x0e) plen 4
> > Write Connection Accept Timeout (0x03|0x0016) ncmd 1
> > status 0x00
> > < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
> > bdaddr 00:00:00:00:00:00 all 1
> > > HCI Event: Command Complete (0x0e) plen 6
> > Delete Stored Link Key (0x03|0x0012) ncmd 1
> > status 0x00 deleted 0
> > < HCI Command: Set Event Mask (0x03|0x0001) plen 8
> > Mask: 0xfffffbff07180000
> > > HCI Event: Command Complete (0x0e) plen 4
> > Set Event Mask (0x03|0x0001) ncmd 1
> > status 0x00
> > < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> > > HCI Event: Command Complete (0x0e) plen 68
> > Read Local Supported Commands (0x04|0x0002) ncmd 1
> > status 0x00
> > Commands: ffffef03ceffffffffffff9ff30fe8ff3c
> > < HCI Command: Write Inquiry Mode (0x03|0x0045) plen 1
> > mode 1
> > > HCI Event: Command Complete (0x0e) plen 4
> > Write Inquiry Mode (0x03|0x0045) ncmd 1
> > status 0x00
> > < HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
> > page 1
> > > HCI Event: Command Complete (0x0e) plen 14
> > Read Local Extended Features (0x04|0x0004) ncmd 1
> > status 0x00 page 0 max 0
> > Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
>
> For some reason your adapter is returning here the same value of Read Local
> Features. I would say your device is broken. The only fix I have in mind now
> is throw away the device.

Yes, I have one of these ISSC devices, it's completely broken device.

Gustavo

2011-12-28 01:22:48

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

* Rene Herman <[email protected]> [2011-12-27 23:19:40 +0100]:

> On 27-12-11 21:30, Gustavo Padovan wrote:
>
> >Seems your extfeatures is telling you something different from what it really
> >supports. Can you try with the following diff (use the dynamic debug that Andre
> >told you)
>
> Attached. The result is "features: 0xff".
>
> I suppose that (should) mean(s) "all features"; if that's not
> actually a valid possibility due, perhaps, to undefined or mutually
> exclusive bits then maybe it's an option to take 0xff to mean that
> the adapter doesn't in fact support any of them?
>
> >Also the output of hcidump would nice.
>
> If the below (which is what hcidump spit back at me at some as far
> as I could determine random moment) is not what you wanted, I
> believe you will need to specify which action to take while
> monitoring things with hcidump.
>
> ==============
> HCI sniffer - Bluetooth packet analyzer ver 2.1
> device: hci0 snap_len: 1028 filter: 0xffffffff
> < HCI Command: Reset (0x03|0x0003) plen 0
> > HCI Event: Command Complete (0x0e) plen 4
> Reset (0x03|0x0003) ncmd 1
> status 0x00
> < HCI Command: Read Local Supported Features (0x04|0x0003) plen 0
> > HCI Event: Command Complete (0x0e) plen 12
> Read Local Supported Features (0x04|0x0003) ncmd 1
> status 0x00
> Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
> < HCI Command: Read Local Version Information (0x04|0x0001) plen 0
> > HCI Event: Command Complete (0x0e) plen 12
> Read Local Version Information (0x04|0x0001) ncmd 1
> status 0x00
> HCI Version: 2.0 (0x3) HCI Revision: 0x2da
> LMP Version: 2.0 (0x3) LMP Subversion: 0x2da
> Manufacturer: Integrated System Solution Corp. (57)
> < HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> > HCI Event: Command Complete (0x0e) plen 11
> Read Buffer Size (0x04|0x0005) ncmd 1
> status 0x00
> ACL MTU 1021:4 SCO MTU 48:10
> < HCI Command: Read BD ADDR (0x04|0x0009) plen 0
> > HCI Event: Command Complete (0x0e) plen 10
> Read BD ADDR (0x04|0x0009) ncmd 1
> status 0x00 bdaddr 00:11:67:C0:12:9E
> < HCI Command: Read Class of Device (0x03|0x0023) plen 0
> > HCI Event: Command Complete (0x0e) plen 7
> Read Class of Device (0x03|0x0023) ncmd 1
> status 0x00 class 0x000000
> < HCI Command: Read Local Name (0x03|0x0014) plen 0
> > HCI Event: Command Complete (0x0e) plen 252
> Read Local Name (0x03|0x0014) ncmd 1
> status 0x00 name 'ISSCEDRBTA'
> < HCI Command: Read Voice Setting (0x03|0x0025) plen 0
> > HCI Event: Command Complete (0x0e) plen 6
> Read Voice Setting (0x03|0x0025) ncmd 1
> status 0x00 voice setting 0x0060
> < HCI Command: Set Event Filter (0x03|0x0005) plen 1
> type 0 condition 0
> Clear all filters
> > HCI Event: Command Complete (0x0e) plen 4
> Set Event Filter (0x03|0x0005) ncmd 1
> status 0x00
> < HCI Command: Write Connection Accept Timeout (0x03|0x0016) plen 2
> timeout 32000
> > HCI Event: Command Complete (0x0e) plen 4
> Write Connection Accept Timeout (0x03|0x0016) ncmd 1
> status 0x00
> < HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
> bdaddr 00:00:00:00:00:00 all 1
> > HCI Event: Command Complete (0x0e) plen 6
> Delete Stored Link Key (0x03|0x0012) ncmd 1
> status 0x00 deleted 0
> < HCI Command: Set Event Mask (0x03|0x0001) plen 8
> Mask: 0xfffffbff07180000
> > HCI Event: Command Complete (0x0e) plen 4
> Set Event Mask (0x03|0x0001) ncmd 1
> status 0x00
> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> > HCI Event: Command Complete (0x0e) plen 68
> Read Local Supported Commands (0x04|0x0002) ncmd 1
> status 0x00
> Commands: ffffef03ceffffffffffff9ff30fe8ff3c
> < HCI Command: Write Inquiry Mode (0x03|0x0045) plen 1
> mode 1
> > HCI Event: Command Complete (0x0e) plen 4
> Write Inquiry Mode (0x03|0x0045) ncmd 1
> status 0x00
> < HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
> page 1
> > HCI Event: Command Complete (0x0e) plen 14
> Read Local Extended Features (0x04|0x0004) ncmd 1
> status 0x00 page 0 max 0
> Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80

For some reason your adapter is returning here the same value of Read Local
Features. I would say your device is broken. The only fix I have in mind now
is throw away the device.

Marcel, do you have any idea or have seen this before?

Gustavo

2011-12-27 22:19:40

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 27-12-11 21:30, Gustavo Padovan wrote:

> Seems your extfeatures is telling you something different from what it really
> supports. Can you try with the following diff (use the dynamic debug that Andre
> told you)

Attached. The result is "features: 0xff".

I suppose that (should) mean(s) "all features"; if that's not actually a
valid possibility due, perhaps, to undefined or mutually exclusive bits
then maybe it's an option to take 0xff to mean that the adapter doesn't
in fact support any of them?

> Also the output of hcidump would nice.

If the below (which is what hcidump spit back at me at some as far as I
could determine random moment) is not what you wanted, I believe you
will need to specify which action to take while monitoring things with
hcidump.

==============
HCI sniffer - Bluetooth packet analyzer ver 2.1
device: hci0 snap_len: 1028 filter: 0xffffffff
< HCI Command: Reset (0x03|0x0003) plen 0
> HCI Event: Command Complete (0x0e) plen 4
Reset (0x03|0x0003) ncmd 1
status 0x00
< HCI Command: Read Local Supported Features (0x04|0x0003) plen 0
> HCI Event: Command Complete (0x0e) plen 12
Read Local Supported Features (0x04|0x0003) ncmd 1
status 0x00
Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
< HCI Command: Read Local Version Information (0x04|0x0001) plen 0
> HCI Event: Command Complete (0x0e) plen 12
Read Local Version Information (0x04|0x0001) ncmd 1
status 0x00
HCI Version: 2.0 (0x3) HCI Revision: 0x2da
LMP Version: 2.0 (0x3) LMP Subversion: 0x2da
Manufacturer: Integrated System Solution Corp. (57)
< HCI Command: Read Buffer Size (0x04|0x0005) plen 0
> HCI Event: Command Complete (0x0e) plen 11
Read Buffer Size (0x04|0x0005) ncmd 1
status 0x00
ACL MTU 1021:4 SCO MTU 48:10
< HCI Command: Read BD ADDR (0x04|0x0009) plen 0
> HCI Event: Command Complete (0x0e) plen 10
Read BD ADDR (0x04|0x0009) ncmd 1
status 0x00 bdaddr 00:11:67:C0:12:9E
< HCI Command: Read Class of Device (0x03|0x0023) plen 0
> HCI Event: Command Complete (0x0e) plen 7
Read Class of Device (0x03|0x0023) ncmd 1
status 0x00 class 0x000000
< HCI Command: Read Local Name (0x03|0x0014) plen 0
> HCI Event: Command Complete (0x0e) plen 252
Read Local Name (0x03|0x0014) ncmd 1
status 0x00 name 'ISSCEDRBTA'
< HCI Command: Read Voice Setting (0x03|0x0025) plen 0
> HCI Event: Command Complete (0x0e) plen 6
Read Voice Setting (0x03|0x0025) ncmd 1
status 0x00 voice setting 0x0060
< HCI Command: Set Event Filter (0x03|0x0005) plen 1
type 0 condition 0
Clear all filters
> HCI Event: Command Complete (0x0e) plen 4
Set Event Filter (0x03|0x0005) ncmd 1
status 0x00
< HCI Command: Write Connection Accept Timeout (0x03|0x0016) plen 2
timeout 32000
> HCI Event: Command Complete (0x0e) plen 4
Write Connection Accept Timeout (0x03|0x0016) ncmd 1
status 0x00
< HCI Command: Delete Stored Link Key (0x03|0x0012) plen 7
bdaddr 00:00:00:00:00:00 all 1
> HCI Event: Command Complete (0x0e) plen 6
Delete Stored Link Key (0x03|0x0012) ncmd 1
status 0x00 deleted 0
< HCI Command: Set Event Mask (0x03|0x0001) plen 8
Mask: 0xfffffbff07180000
> HCI Event: Command Complete (0x0e) plen 4
Set Event Mask (0x03|0x0001) ncmd 1
status 0x00
< HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> HCI Event: Command Complete (0x0e) plen 68
Read Local Supported Commands (0x04|0x0002) ncmd 1
status 0x00
Commands: ffffef03ceffffffffffff9ff30fe8ff3c
< HCI Command: Write Inquiry Mode (0x03|0x0045) plen 1
mode 1
> HCI Event: Command Complete (0x0e) plen 4
Write Inquiry Mode (0x03|0x0045) ncmd 1
status 0x00
< HCI Command: Read Local Extended Features (0x04|0x0004) plen 1
page 1
> HCI Event: Command Complete (0x0e) plen 14
Read Local Extended Features (0x04|0x0004) ncmd 1
status 0x00 page 0 max 0
Features: 0xff 0xfe 0xff 0x7e 0x98 0x19 0x00 0x80
< HCI Command: Write Default Link Policy Settings (0x02|0x000f) plen 2
policy 0x07
Link policy: RSWITCH HOLD SNIFF
> HCI Event: Command Complete (0x0e) plen 4
Write Default Link Policy Settings (0x02|0x000f) ncmd 1
status 0x00
< HCI Command: LE Read Buffer Size (0x08|0x0002) plen 0
==============

Rene.


Attachments:
bluetooth-3.1.6-dbg.dmesg (9.24 kB)

2011-12-27 20:30:08

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

* Rene Herman <[email protected]> [2011-12-27 20:38:32 +0100]:

> On 27-12-11 18:22, Andre Guedes wrote:
>
> >Could you please send us the kernel log messages with Bluetooth debug
> >enabled? Here goes the instructions:
> >
> >1. Compile kernel with CONFIG_DYNAMIC_DEBUG=y.
> >2. Load Bluetooth module.
> >3. Run the following commands to turn debug on (remember to mount debugfs
> >first):
> >--
> >$ echo "file hci_core.c +pf"> /sys/kernel/debug/dynamic_debug/control
> >$ echo "file hci_event.c +pf"> /sys/kernel/debug/dynamic_debug/control
> >$ echo "file mgmt.c +pf"> /sys/kernel/debug/dynamic_debug/control
> >$ echo "file btusb.c +pf"> /sys/kernel/debug/dynamic_debug/control
> >--
> >4. Plug the Bluetooth adapter.
>
> Thanks for the verbosity. Here's a -- full, just in case it's useful
> -- dmesg from vanilla 3.1.6 (i.e., when bluetooth does not work)
> generated per the above. The bluetooth bit is the last part.

snip

>
> hci_le_init_req:hci0
> hci_send_cmd:hci0 opcode 0x2002 plen 0
> hci_send_cmd:skb len 3
> hci_cmd_task:hci0 cmd 1
> hci_send_frame:hci0 type 1 len 3
> btusb_send_frame:hci0
> btusb_tx_complete:hci0 urb e6900700 status 0 count 3
> Bluetooth: hci0 command tx timeout
> hci_cmd_task:hci0 cmd 1
> __hci_request:hci0 end: err -110

Seems your extfeatures is telling you something different from what it really
supports. Can you try with the following diff (use the dynamic debug that Andre
told you)

Also the output of hcidump would nice.


diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 919e3c0..4c9f1aa 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -711,6 +711,8 @@ static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
if (rp->status)
return;

+ BT_DBG("features 0x%2.2x", rp->features[0]);
+
memcpy(hdev->extfeatures, rp->features, 8);

hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);


Gustavo

2011-12-27 19:38:32

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 27-12-11 18:22, Andre Guedes wrote:

> Could you please send us the kernel log messages with Bluetooth debug
> enabled? Here goes the instructions:
>
> 1. Compile kernel with CONFIG_DYNAMIC_DEBUG=y.
> 2. Load Bluetooth module.
> 3. Run the following commands to turn debug on (remember to mount debugfs
> first):
> --
> $ echo "file hci_core.c +pf"> /sys/kernel/debug/dynamic_debug/control
> $ echo "file hci_event.c +pf"> /sys/kernel/debug/dynamic_debug/control
> $ echo "file mgmt.c +pf"> /sys/kernel/debug/dynamic_debug/control
> $ echo "file btusb.c +pf"> /sys/kernel/debug/dynamic_debug/control
> --
> 4. Plug the Bluetooth adapter.

Thanks for the verbosity. Here's a -- full, just in case it's useful --
dmesg from vanilla 3.1.6 (i.e., when bluetooth does not work) generated
per the above. The bluetooth bit is the last part.

Regards,
Rene


Attachments:
bluetooth-3.1.6.dmesg (42.23 kB)

2012-01-04 15:12:16

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

On 04-01-12 15:16, Andre Guedes wrote:

>> Any word on a final fix for this?
>
> Yes,this couple of patches fixes this issue:
> http://article.gmane.org/gmane.linux.bluez.kernel/19950/
> http://article.gmane.org/gmane.linux.bluez.kernel/19951/
>
> They were already pushed to bluetooth-next tree.

Thank you. Just in case anyone cares, versions against 3.1.7 re-attached
(and confirmed, of course).

Regards,
Rene


Attachments:
bluetooth-3.1.7_1-2.diff (1.41 kB)
bluetooth-3.1.7_2-2.diff (725.00 B)
Download all attachments

2012-01-04 14:16:00

by Andre Guedes

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)

Hi Rene,

On Wed, Jan 4, 2012 at 9:04 AM, Rene Herman <[email protected]> wrote:
>
> On 28-12-11 17:48, Marcel Holtmann wrote:
>
>> The correct patch would be something like this:
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 919e3c0..b6eb9c5 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -711,7 +711,8 @@ static void hci_cc_read_local_ext_features(struct
>> hci_dev *h
>> =A0 =A0 =A0 =A0 if (rp->status)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
>>
>> - =A0 =A0 =A0 memcpy(hdev->extfeatures, rp->features, 8);
>> + =A0 =A0 =A0 if (rp->page =3D=3D 1)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 memcpy(hdev->extfeatures, rp->features, 8)=
;
>>
>> =A0 =A0 =A0 =A0 hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES,
>> rp->status);
>> =A0}
>>
>> The current patch is actually fully broken anyway. So besides switching
>> LE on when requesting page 0, you will also switch it off when
>> requesting any other page and it accidentally returns 0x00 since we
>> unconditionally overwrite it.
>
>
> Any word on a final fix for this?

Yes,this couple of patches fixes this issue:
http://article.gmane.org/gmane.linux.bluez.kernel/19950/
http://article.gmane.org/gmane.linux.bluez.kernel/19951/

They were already pushed to bluetooth-next tree.

BR,

Andre

2012-01-04 12:04:18

by Rene Herman

[permalink] [raw]
Subject: Re: [bluetooth] linux-3.x regression (bisected)


On 28-12-11 17:48, Marcel Holtmann wrote:

> The correct patch would be something like this:
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 919e3c0..b6eb9c5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -711,7 +711,8 @@ static void hci_cc_read_local_ext_features(struct hci_dev *h
> if (rp->status)
> return;
>
> - memcpy(hdev->extfeatures, rp->features, 8);
> + if (rp->page == 1)
> + memcpy(hdev->extfeatures, rp->features, 8);
>
> hci_req_complete(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, rp->status);
> }
>
> The current patch is actually fully broken anyway. So besides switching
> LE on when requesting page 0, you will also switch it off when
> requesting any other page and it accidentally returns 0x00 since we
> unconditionally overwrite it.

Any word on a final fix for this?

Regards,
Rene Herman