[PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Simon Fels]

Some controllers doesn't seem to clear the bitfield to indicate which
HCI commands are supported correctly. This leads to wrong conclusions
which HCI commands we can send at initialization time. In this case
the problem appeared with the command to retrieve the MWS transport
configuration which is only available since version 4.1

Signed-off-by: Simon Fels <simon.fels@canonical.com>
---
 include/net/bluetooth/bluetooth.h | 2 +-
 net/bluetooth/hci_core.c          | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index fcf2ae7..0861477 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -37,7 +37,7 @@
 /* Bluetooth versions */
 #define BLUETOOTH_VER_1_1	1
 #define BLUETOOTH_VER_1_2	2
-#define BLUETOOTH_VER_2_0	3
+#define BLUETOOTH_VER_4_1	7
 
 /* Reserv for core and drivers use */
 #define BT_SKB_RESERVE	8
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index a7cdd99..f376ab6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -824,8 +824,13 @@ static void hci_init4_req(struct hci_request *req, unsigned long opt)
 	if (hdev->commands[29] & 0x20)
 		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
 
-	/* Get MWS transport configuration if the HCI command is supported */
-	if (hdev->commands[30] & 0x08)
+	/* Get MWS transport configuration if the HCI command is supported
+	 * but only when reported HCI version is at minimum 4.1. Some
+	 * controllers doesn't seem to clear the bit field they transmit
+	 * correctly so all resevered fields are invalid and let to
+	 * failing commands */
+	if (hdev->hci_ver >= BLUETOOTH_VER_4_1 &&
+		hdev->commands[30] & 0x08)
 		hci_req_add(req, HCI_OP_GET_MWS_TRANSPORT_CONFIG, 0, NULL);
 
 	/* Check for Synchronization Train support */
-- 
2.1.4

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Marcel Holtmann]

Hi Simon,

> Some controllers doesn't seem to clear the bitfield to indicate which
> HCI commands are supported correctly. This leads to wrong conclusions
> which HCI commands we can send at initialization time. In this case
> the problem appeared with the command to retrieve the MWS transport
> configuration which is only available since version 4.1

provide btmon of the failing case and information about the controller that has this bug.

> Signed-off-by: Simon Fels <simon.fels@canonical.com>
> ---
> include/net/bluetooth/bluetooth.h | 2 +-
> net/bluetooth/hci_core.c          | 9 +++++++--
> 2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index fcf2ae7..0861477 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -37,7 +37,7 @@
> /* Bluetooth versions */
> #define BLUETOOTH_VER_1_1	1
> #define BLUETOOTH_VER_1_2	2
> -#define BLUETOOTH_VER_2_0	3
> +#define BLUETOOTH_VER_4_1	7
> 
> /* Reserv for core and drivers use */
> #define BT_SKB_RESERVE	8
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index a7cdd99..f376ab6 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -824,8 +824,13 @@ static void hci_init4_req(struct hci_request *req, unsigned long opt)
> 	if (hdev->commands[29] & 0x20)
> 		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
> 
> -	/* Get MWS transport configuration if the HCI command is supported */
> -	if (hdev->commands[30] & 0x08)
> +	/* Get MWS transport configuration if the HCI command is supported
> +	 * but only when reported HCI version is at minimum 4.1. Some
> +	 * controllers doesn't seem to clear the bit field they transmit
> +	 * correctly so all resevered fields are invalid and let to
> +	 * failing commands */
> +	if (hdev->hci_ver >= BLUETOOTH_VER_4_1 &&
> +		hdev->commands[30] & 0x08)
> 		hci_req_add(req, HCI_OP_GET_MWS_TRANSPORT_CONFIG, 0, NULL);

We are not doing it this way. The version checks we have in the core are for really old controllers and times when the Bluetooth Core specification was unclear in certain behaviors.

Any new broken behavior of a controller will not be hidden behind a version check. It will be hidden behind a quirk and we will call out the manufacturer for not doing this right in the driver by setting needed quirks.

Regards

Marcel

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Simon Fels]

On 15.09.2015 11:32, Marcel Holtmann wrote:
> Hi Simon,
>
>> Some controllers doesn't seem to clear the bitfield to indicate which
>> HCI commands are supported correctly. This leads to wrong conclusions
>> which HCI commands we can send at initialization time. In this case
>> the problem appeared with the command to retrieve the MWS transport
>> configuration which is only available since version 4.1
>
> provide btmon of the failing case and information about the controller that has this bug.
>
>> Signed-off-by: Simon Fels <simon.fels@canonical.com>
>> ---
>> include/net/bluetooth/bluetooth.h | 2 +-
>> net/bluetooth/hci_core.c          | 9 +++++++--
>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index fcf2ae7..0861477 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -37,7 +37,7 @@
>> /* Bluetooth versions */
>> #define BLUETOOTH_VER_1_1	1
>> #define BLUETOOTH_VER_1_2	2
>> -#define BLUETOOTH_VER_2_0	3
>> +#define BLUETOOTH_VER_4_1	7
>>
>> /* Reserv for core and drivers use */
>> #define BT_SKB_RESERVE	8
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index a7cdd99..f376ab6 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -824,8 +824,13 @@ static void hci_init4_req(struct hci_request *req, unsigned long opt)
>> 	if (hdev->commands[29] & 0x20)
>> 		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
>>
>> -	/* Get MWS transport configuration if the HCI command is supported */
>> -	if (hdev->commands[30] & 0x08)
>> +	/* Get MWS transport configuration if the HCI command is supported
>> +	 * but only when reported HCI version is at minimum 4.1. Some
>> +	 * controllers doesn't seem to clear the bit field they transmit
>> +	 * correctly so all resevered fields are invalid and let to
>> +	 * failing commands */
>> +	if (hdev->hci_ver >= BLUETOOTH_VER_4_1 &&
>> +		hdev->commands[30] & 0x08)
>> 		hci_req_add(req, HCI_OP_GET_MWS_TRANSPORT_CONFIG, 0, NULL);
>
> We are not doing it this way. The version checks we have in the core are for really old controllers and times when the Bluetooth Core specification was unclear in certain behaviors.
>
> Any new broken behavior of a controller will not be hidden behind a version check. It will be hidden behind a quirk and we will call out the manufacturer for not doing this right in the driver by setting needed quirks.

That is fine for me. Was just sending this out to get the discussion 
started and a bit unsure which patterns you use for such things.

btmon log is at http://paste.ubuntu.com/12416578/

The failure case is simply that the HCI is never getting up. It fails 
during a

$ hciconfig hci0 up

within the kernel side initialization phase.

regards,
Simon

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Marcel Holtmann]

Hi Simon,

>>> Some controllers doesn't seem to clear the bitfield to indicate which
>>> HCI commands are supported correctly. This leads to wrong conclusions
>>> which HCI commands we can send at initialization time. In this case
>>> the problem appeared with the command to retrieve the MWS transport
>>> configuration which is only available since version 4.1
>> 
>> provide btmon of the failing case and information about the controller that has this bug.
>> 
>>> Signed-off-by: Simon Fels <simon.fels@canonical.com>
>>> ---
>>> include/net/bluetooth/bluetooth.h | 2 +-
>>> net/bluetooth/hci_core.c          | 9 +++++++--
>>> 2 files changed, 8 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>>> index fcf2ae7..0861477 100644
>>> --- a/include/net/bluetooth/bluetooth.h
>>> +++ b/include/net/bluetooth/bluetooth.h
>>> @@ -37,7 +37,7 @@
>>> /* Bluetooth versions */
>>> #define BLUETOOTH_VER_1_1	1
>>> #define BLUETOOTH_VER_1_2	2
>>> -#define BLUETOOTH_VER_2_0	3
>>> +#define BLUETOOTH_VER_4_1	7
>>> 
>>> /* Reserv for core and drivers use */
>>> #define BT_SKB_RESERVE	8
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index a7cdd99..f376ab6 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -824,8 +824,13 @@ static void hci_init4_req(struct hci_request *req, unsigned long opt)
>>> 	if (hdev->commands[29] & 0x20)
>>> 		hci_req_add(req, HCI_OP_READ_LOCAL_CODECS, 0, NULL);
>>> 
>>> -	/* Get MWS transport configuration if the HCI command is supported */
>>> -	if (hdev->commands[30] & 0x08)
>>> +	/* Get MWS transport configuration if the HCI command is supported
>>> +	 * but only when reported HCI version is at minimum 4.1. Some
>>> +	 * controllers doesn't seem to clear the bit field they transmit
>>> +	 * correctly so all resevered fields are invalid and let to
>>> +	 * failing commands */
>>> +	if (hdev->hci_ver >= BLUETOOTH_VER_4_1 &&
>>> +		hdev->commands[30] & 0x08)
>>> 		hci_req_add(req, HCI_OP_GET_MWS_TRANSPORT_CONFIG, 0, NULL);
>> 
>> We are not doing it this way. The version checks we have in the core are for really old controllers and times when the Bluetooth Core specification was unclear in certain behaviors.
>> 
>> Any new broken behavior of a controller will not be hidden behind a version check. It will be hidden behind a quirk and we will call out the manufacturer for not doing this right in the driver by setting needed quirks.
> 
> That is fine for me. Was just sending this out to get the discussion started and a bit unsure which patterns you use for such things.
> 
> btmon log is at http://paste.ubuntu.com/12416578/
> 
> The failure case is simply that the HCI is never getting up. It fails during a
> 
> $ hciconfig hci0 up
> 
> within the kernel side initialization phase.

so it is actually not that this chip does not understand this command, it is that it is being stupid.

< HCI Command: Get MWS Transport Layer.. (0x05|0x000c) plen 0
> HCI Event: Command Complete (0x0e) plen 5
      Get MWS Transport Layer Configuration (0x05|0x000c) ncmd 1
        Status: Unknown Connection Identifier (0x02)
        Number of transports: 0
        Baud rate list: 0 entries

The command is properly supported. I just don't know why it needs to return with an error. And especially error 0x02 which makes no sense in this case since the command is not even taking a connection handle in the first place.

< HCI Command: Read Local Version Info.. (0x04|0x0001) plen 0
> HCI Event: Command Complete (0x0e) plen 12
      Read Local Version Information (0x04|0x0001) ncmd 1
        Status: Success (0x00)
        HCI version: Bluetooth 4.0 (0x06) - Revision 0 (0x0000)
        LMP version: Bluetooth 4.0 (0x06) - Subversion 0 (0x0000)
        Manufacturer: MediaTek, Inc. (70)
< HCI Command: Read BD ADDR (0x04|0x0009) plen 0
> HCI Event: Command Complete (0x0e) plen 10
      Read BD ADDR (0x04|0x0009) ncmd 1
        Status: Success (0x00)
        Address: 38:BC:1A:1F:7E:96 (Meizu technology co.,ltd)

So it seems that MediaTek needs to work a bit on their HCI command support. Which module is this anyway? Who builds it and where can it be found? Is this USB or UART? If it is USB, please provide /sys/kernel/debug/usb/devices entry for this one.

I have the feeling we are missing some sort of firmware update here that would allow to fix this controller.

And I forgot to note that MWS actually was a CSA. So it is valid for Bluetooth 4.0 controllers. It is just that Bluetooth 4.1 included all previous CSA.

Regards

Marcel

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Simon Fels]

Hey Marcel,

> so it is actually not that this chip does not understand this command, it is that it is being stupid.

It lead me to the assumption that the chip does something pretty wired 
here as I didn't found this command in the 4.0 spec and when saying "I 
am supporting 4.0" I would assume it doesn't mix any of the new commands 
from newer spec versions in. However didn't saw that it was added in the 
addendum ...

> < HCI Command: Get MWS Transport Layer.. (0x05|0x000c) plen 0
>> HCI Event: Command Complete (0x0e) plen 5
>        Get MWS Transport Layer Configuration (0x05|0x000c) ncmd 1
>          Status: Unknown Connection Identifier (0x02)
>          Number of transports: 0
>          Baud rate list: 0 entries
>
> The command is properly supported. I just don't know why it needs to return with an error. And especially error 0x02 which makes no sense in this case since the command is not even taking a connection handle in the first place.
>
> < HCI Command: Read Local Version Info.. (0x04|0x0001) plen 0
>> HCI Event: Command Complete (0x0e) plen 12
>        Read Local Version Information (0x04|0x0001) ncmd 1
>          Status: Success (0x00)
>          HCI version: Bluetooth 4.0 (0x06) - Revision 0 (0x0000)
>          LMP version: Bluetooth 4.0 (0x06) - Subversion 0 (0x0000)
>          Manufacturer: MediaTek, Inc. (70)
> < HCI Command: Read BD ADDR (0x04|0x0009) plen 0
>> HCI Event: Command Complete (0x0e) plen 10
>        Read BD ADDR (0x04|0x0009) ncmd 1
>          Status: Success (0x00)
>          Address: 38:BC:1A:1F:7E:96 (Meizu technology co.,ltd)
>
> So it seems that MediaTek needs to work a bit on their HCI command support. Which module is this anyway? Who builds it and where can it be found? Is this USB or UART? If it is USB, please provide /sys/kernel/debug/usb/devices entry for this one.

As far as I know it is the MT6630QP chip and its connected over UART.

> I have the feeling we are missing some sort of firmware update here that would allow to fix this controller.

Generally I would say this is the way to go too but that isn't really an 
option in this case. So either we come up with a general solution by 
adding a quirk for this or I patch this just on our kernel.

> And I forgot to note that MWS actually was a CSA. So it is valid for Bluetooth 4.0 controllers. It is just that Bluetooth 4.1 included all previous CSA.

I see. That is why I didn't found it in the 4.0 spec. Thanks for the hint!

regards,
Simon

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Marcel Holtmann]

Hi Simon,

>> so it is actually not that this chip does not understand this command, it is that it is being stupid.
> 
> It lead me to the assumption that the chip does something pretty wired here as I didn't found this command in the 4.0 spec and when saying "I am supporting 4.0" I would assume it doesn't mix any of the new commands from newer spec versions in. However didn't saw that it was added in the addendum ...
> 
>> < HCI Command: Get MWS Transport Layer.. (0x05|0x000c) plen 0
>>> HCI Event: Command Complete (0x0e) plen 5
>>       Get MWS Transport Layer Configuration (0x05|0x000c) ncmd 1
>>         Status: Unknown Connection Identifier (0x02)
>>         Number of transports: 0
>>         Baud rate list: 0 entries
>> 
>> The command is properly supported. I just don't know why it needs to return with an error. And especially error 0x02 which makes no sense in this case since the command is not even taking a connection handle in the first place.
>> 
>> < HCI Command: Read Local Version Info.. (0x04|0x0001) plen 0
>>> HCI Event: Command Complete (0x0e) plen 12
>>       Read Local Version Information (0x04|0x0001) ncmd 1
>>         Status: Success (0x00)
>>         HCI version: Bluetooth 4.0 (0x06) - Revision 0 (0x0000)
>>         LMP version: Bluetooth 4.0 (0x06) - Subversion 0 (0x0000)
>>         Manufacturer: MediaTek, Inc. (70)
>> < HCI Command: Read BD ADDR (0x04|0x0009) plen 0
>>> HCI Event: Command Complete (0x0e) plen 10
>>       Read BD ADDR (0x04|0x0009) ncmd 1
>>         Status: Success (0x00)
>>         Address: 38:BC:1A:1F:7E:96 (Meizu technology co.,ltd)
>> 
>> So it seems that MediaTek needs to work a bit on their HCI command support. Which module is this anyway? Who builds it and where can it be found? Is this USB or UART? If it is USB, please provide /sys/kernel/debug/usb/devices entry for this one.
> 
> As far as I know it is the MT6630QP chip and its connected over UART.
> 
>> I have the feeling we are missing some sort of firmware update here that would allow to fix this controller.
> 
> Generally I would say this is the way to go too but that isn't really an option in this case. So either we come up with a general solution by adding a quirk for this or I patch this just on our kernel.

if this an UART chip, what hciattach command are you using? Plain H:4 or 3-Wire UART?

We started supporting multiple manufactures inside the upstream kernel with proper baud rate switching, flow control, low power modes and also firmware loading. With these properly integrated devices the new btattach command can be used.

So where did you get this hardware from anyway? And we need to get Mediatek to support this in the upstream kernel. Otherwise this is not sustainable. These days you need to have firmware patching support at least to fix all the nasty issues you can run into after the chips have been includes in devices.

Regards

Marcel

Re: [PATCH] bluetooth: ask for MWS transport config only if controller supports 4.1

[Simon Fels]

On 16.09.2015 11:01, Marcel Holtmann wrote:
> Hi Simon,
>
>>> so it is actually not that this chip does not understand this command, it is that it is being stupid.
>>
>> It lead me to the assumption that the chip does something pretty wired here as I didn't found this command in the 4.0 spec and when saying "I am supporting 4.0" I would assume it doesn't mix any of the new commands from newer spec versions in. However didn't saw that it was added in the addendum ...
>>
>>> < HCI Command: Get MWS Transport Layer.. (0x05|0x000c) plen 0
>>>> HCI Event: Command Complete (0x0e) plen 5
>>>        Get MWS Transport Layer Configuration (0x05|0x000c) ncmd 1
>>>          Status: Unknown Connection Identifier (0x02)
>>>          Number of transports: 0
>>>          Baud rate list: 0 entries
>>>
>>> The command is properly supported. I just don't know why it needs to return with an error. And especially error 0x02 which makes no sense in this case since the command is not even taking a connection handle in the first place.
>>>
>>> < HCI Command: Read Local Version Info.. (0x04|0x0001) plen 0
>>>> HCI Event: Command Complete (0x0e) plen 12
>>>        Read Local Version Information (0x04|0x0001) ncmd 1
>>>          Status: Success (0x00)
>>>          HCI version: Bluetooth 4.0 (0x06) - Revision 0 (0x0000)
>>>          LMP version: Bluetooth 4.0 (0x06) - Subversion 0 (0x0000)
>>>          Manufacturer: MediaTek, Inc. (70)
>>> < HCI Command: Read BD ADDR (0x04|0x0009) plen 0
>>>> HCI Event: Command Complete (0x0e) plen 10
>>>        Read BD ADDR (0x04|0x0009) ncmd 1
>>>          Status: Success (0x00)
>>>          Address: 38:BC:1A:1F:7E:96 (Meizu technology co.,ltd)
>>>
>>> So it seems that MediaTek needs to work a bit on their HCI command support. Which module is this anyway? Who builds it and where can it be found? Is this USB or UART? If it is USB, please provide /sys/kernel/debug/usb/devices entry for this one.
>>
>> As far as I know it is the MT6630QP chip and its connected over UART.
>>
>>> I have the feeling we are missing some sort of firmware update here that would allow to fix this controller.
>>
>> Generally I would say this is the way to go too but that isn't really an option in this case. So either we come up with a general solution by adding a quirk for this or I patch this just on our kernel.
>
> if this an UART chip, what hciattach command are you using? Plain H:4 or 3-Wire UART?
>
> We started supporting multiple manufactures inside the upstream kernel with proper baud rate switching, flow control, low power modes and also firmware loading. With these properly integrated devices the new btattach command can be used.
>
> So where did you get this hardware from anyway? And we need to get Mediatek to support this in the upstream kernel. Otherwise this is not sustainable. These days you need to have firmware patching support at least to fix all the nasty issues you can run into after the chips have been includes in devices.

As summary: I had a quick discussion with Marcel on IRC and I will go 
with a small workaround on our tree for now.

regards,
Simon