All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] bluetooth: add support for some old headsets
@ 2020-07-16 11:08 Sergey Shtylyov
  2020-07-16 13:14 ` Marcel Holtmann
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2020-07-16 11:08 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, linux-bluetooth
  Cc: David S. Miller, Jakub Kicinski, netdev

The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
for the eSCO/SCO connection via BT/EDR: the host controller returns error
code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
(0x0028) command without actually trying to setup connection with a remote
device in case such device (like Digma BT-14 headset) didn't advertise its
supported features.  Even though this doesn't break compatibility with the
Bluetooth standard it breaks the compatibility with the Hands-Free Profile
(HFP).

This patch returns the compatibility with the HFP profile and actually
tries to check all available connection parameters despite of the specific
MediaTek implementation. Without it one was unable to establish eSCO/SCO
connection with some headsets.

Based on the patch by Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>

---
This patch is against the 'bluetooth-next.git' repo.

 net/bluetooth/hci_event.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: bluetooth-next/net/bluetooth/hci_event.c
===================================================================
--- bluetooth-next.orig/net/bluetooth/hci_event.c
+++ bluetooth-next/net/bluetooth/hci_event.c
@@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
 	if (acl) {
 		sco = acl->link;
 		if (sco) {
+			if (status == 0x20 && /* Unsupported LMP Parameter value */
+			    sco->out) {
+				sco->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
+						(hdev->esco_type & EDR_ESCO_MASK);
+				if (hci_setup_sync(sco, sco->link->handle))
+					goto unlock;
+			}
 			sco->state = BT_CLOSED;
 
 			hci_connect_cfm(sco, status);
@@ -2194,6 +2201,7 @@ static void hci_cs_setup_sync_conn(struc
 		}
 	}
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-16 11:08 [PATCH RFC] bluetooth: add support for some old headsets Sergey Shtylyov
@ 2020-07-16 13:14 ` Marcel Holtmann
  2020-07-16 20:33   ` Sergey Shtylyov
  0 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2020-07-16 13:14 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev

Hi Sergey,

> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
> for the eSCO/SCO connection via BT/EDR: the host controller returns error
> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
> (0x0028) command without actually trying to setup connection with a remote
> device in case such device (like Digma BT-14 headset) didn't advertise its
> supported features.  Even though this doesn't break compatibility with the
> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
> (HFP).
> 
> This patch returns the compatibility with the HFP profile and actually
> tries to check all available connection parameters despite of the specific
> MediaTek implementation. Without it one was unable to establish eSCO/SCO
> connection with some headsets.

please include the parts of btmon output that show this issue.

> Based on the patch by Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>.
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
> 
> ---
> This patch is against the 'bluetooth-next.git' repo.
> 
> net/bluetooth/hci_event.c |    8 ++++++++
> 1 file changed, 8 insertions(+)
> 
> Index: bluetooth-next/net/bluetooth/hci_event.c
> ===================================================================
> --- bluetooth-next.orig/net/bluetooth/hci_event.c
> +++ bluetooth-next/net/bluetooth/hci_event.c
> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
> 	if (acl) {
> 		sco = acl->link;
> 		if (sco) {
> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
> +			    sco->out) {
> +				sco->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
> +						(hdev->esco_type & EDR_ESCO_MASK);
> +				if (hci_setup_sync(sco, sco->link->handle))
> +					goto unlock;
> +			}
> 			sco->state = BT_CLOSED;

since this is the command status event, I doubt that sco->out check is needed. And I would start with a switch statement right away.

I also think that we need to re-structure this hci_cs_setup_sync_conn function a little to avoid the deep indentation. Make it look more like hci_sync_conn_complete_evt also use a switch statement even if right now we only have one entry.

Regards

Marcel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-16 13:14 ` Marcel Holtmann
@ 2020-07-16 20:33   ` Sergey Shtylyov
  2020-07-17  6:59     ` Marcel Holtmann
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2020-07-16 20:33 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev

Hello!

On 7/16/20 4:14 PM, Marcel Holtmann wrote:

>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>> (0x0028) command without actually trying to setup connection with a remote
>> device in case such device (like Digma BT-14 headset) didn't advertise its
>> supported features.  Even though this doesn't break compatibility with the
>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>> (HFP).
>>
>> This patch returns the compatibility with the HFP profile and actually
>> tries to check all available connection parameters despite of the specific
>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>> connection with some headsets.
> 
> please include the parts of btmon output that show this issue.

   Funny, I had removed that part from the original patch. Here's that log:

< HCI Command: Setup Synchronous Connection (0x01|0x0028) plen 17                                  #1 [hci0] 6.705320
        Handle: 50
        Transmit bandwidth: 8000
        Receive bandwidth: 8000
        Max latency: 10
        Setting: 0x0060
          Input Coding: Linear
          Input Data Format: 2's complement
          Input Sample Size: 16-bit
            of bits padding at MSB: 0
          Air Coding Format: CVSD
        Retransmission effort: Optimize for power consumption (0x01)
        Packet type: 0x0380
          3-EV3 may not be used
          2-EV5 may not be used
          3-EV5 may not be used
> HCI Event: Command Status (0x0f) plen 4                                                          #2 [hci0] 6.719598
      Setup Synchronous Connection (0x01|0x0028) ncmd 1
        Status: Unsupported LMP Parameter Value / Unsupported LL Parameter Value (0x20)

>> Based on the patch by Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>
>> ---
>> This patch is against the 'bluetooth-next.git' repo.
>>
>> net/bluetooth/hci_event.c |    8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> Index: bluetooth-next/net/bluetooth/hci_event.c
>> ===================================================================
>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>> +++ bluetooth-next/net/bluetooth/hci_event.c
>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>> 	if (acl) {
>> 		sco = acl->link;
>> 		if (sco) {
>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>> +			    sco->out) {
>> +				sco->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
>> +						(hdev->esco_type & EDR_ESCO_MASK);
>> +				if (hci_setup_sync(sco, sco->link->handle))
>> +					goto unlock;
>> +			}
>> 			sco->state = BT_CLOSED;
> 
> since this is the command status event, I doubt that sco->out check is needed.

   Can't comment oin this, my BT fu is too weak... 

> And I would start with a switch statement right away.

   Funny, I had removed the *switch* statement from the original patch... :-)

> I also think that we need to re-structure this hci_cs_setup_sync_conn function a little to avoid the deep indentation.
> Make it look more like hci_sync_conn_complete_evt also use a switch statement even if right now we only have one 
> entry.

    Indeed, done now. :-)
 
> Regards
> 
> Marcel

MBR, Sergey

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-16 20:33   ` Sergey Shtylyov
@ 2020-07-17  6:59     ` Marcel Holtmann
  2020-07-17 19:12       ` Sergey Shtylyov
  0 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2020-07-17  6:59 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev

Hi Sergey,

>>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>>> (0x0028) command without actually trying to setup connection with a remote
>>> device in case such device (like Digma BT-14 headset) didn't advertise its
>>> supported features.  Even though this doesn't break compatibility with the
>>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>>> (HFP).
>>> 
>>> This patch returns the compatibility with the HFP profile and actually
>>> tries to check all available connection parameters despite of the specific
>>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>>> connection with some headsets.
>> 
>> please include the parts of btmon output that show this issue.
> 
>   Funny, I had removed that part from the original patch. Here's that log:
> 
> < HCI Command: Setup Synchronous Connection (0x01|0x0028) plen 17                                  #1 [hci0] 6.705320
>        Handle: 50
>        Transmit bandwidth: 8000
>        Receive bandwidth: 8000
>        Max latency: 10
>        Setting: 0x0060
>          Input Coding: Linear
>          Input Data Format: 2's complement
>          Input Sample Size: 16-bit
>            of bits padding at MSB: 0
>          Air Coding Format: CVSD
>        Retransmission effort: Optimize for power consumption (0x01)
>        Packet type: 0x0380
>          3-EV3 may not be used
>          2-EV5 may not be used
>          3-EV5 may not be used
>> HCI Event: Command Status (0x0f) plen 4                                                          #2 [hci0] 6.719598
>      Setup Synchronous Connection (0x01|0x0028) ncmd 1
>        Status: Unsupported LMP Parameter Value / Unsupported LL Parameter Value (0x20)

I double check with the specification and it is not precise that errors should be reported via sync conn complete events. My assumption would be that your headset only supports SCO and thus the controller realizes that eSCO request can not be completed anyway. So the controller opts for quickest path to get out of this.

> 
>>> Based on the patch by Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>.
>>> 
>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>> 
>>> ---
>>> This patch is against the 'bluetooth-next.git' repo.
>>> 
>>> net/bluetooth/hci_event.c |    8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>> 
>>> Index: bluetooth-next/net/bluetooth/hci_event.c
>>> ===================================================================
>>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>>> +++ bluetooth-next/net/bluetooth/hci_event.c
>>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>>> 	if (acl) {
>>> 		sco = acl->link;
>>> 		if (sco) {
>>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>>> +			    sco->out) {
>>> +				sco->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
>>> +						(hdev->esco_type & EDR_ESCO_MASK);
>>> +				if (hci_setup_sync(sco, sco->link->handle))
>>> +					goto unlock;
>>> +			}
>>> 			sco->state = BT_CLOSED;
>> 
>> since this is the command status event, I doubt that sco->out check is needed.
> 
>   Can't comment oin this, my BT fu is too weak... 

It is the case. Command status is only local to command we issued and thus in this case it is the connection creation attempt from our side. Meaning it is always outgoing.

Regards

Marcel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-17  6:59     ` Marcel Holtmann
@ 2020-07-17 19:12       ` Sergey Shtylyov
  2020-07-21 18:56         ` Sergey Shtylyov
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2020-07-17 19:12 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev, Ildar Kamaletdinov

On 7/17/20 9:59 AM, Marcel Holtmann wrote:

>>>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>>>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>>>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>>>> (0x0028) command without actually trying to setup connection with a remote
>>>> device in case such device (like Digma BT-14 headset) didn't advertise its
>>>> supported features.  Even though this doesn't break compatibility with the
>>>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>>>> (HFP).
>>>>
>>>> This patch returns the compatibility with the HFP profile and actually
>>>> tries to check all available connection parameters despite of the specific
>>>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>>>> connection with some headsets.
>>>
>>> please include the parts of btmon output that show this issue.
>>
>>   Funny, I had removed that part from the original patch. Here's that log:
>>
>> < HCI Command: Setup Synchronous Connection (0x01|0x0028) plen 17                                  #1 [hci0] 6.705320
>>        Handle: 50
>>        Transmit bandwidth: 8000
>>        Receive bandwidth: 8000
>>        Max latency: 10
>>        Setting: 0x0060
>>          Input Coding: Linear
>>          Input Data Format: 2's complement
>>          Input Sample Size: 16-bit
>>            of bits padding at MSB: 0
>>          Air Coding Format: CVSD
>>        Retransmission effort: Optimize for power consumption (0x01)
>>        Packet type: 0x0380
>>          3-EV3 may not be used
>>          2-EV5 may not be used
>>          3-EV5 may not be used
>>> HCI Event: Command Status (0x0f) plen 4                                                          #2 [hci0] 6.719598
>>      Setup Synchronous Connection (0x01|0x0028) ncmd 1
>>        Status: Unsupported LMP Parameter Value / Unsupported LL Parameter Value (0x20)
 
> I double check with the specification and it is not precise that errors should be reported
> via sync conn complete events. My assumption would be that your headset only supports SCO and
> thus the controller realizes that eSCO request can not be completed anyway. So the controller
> opts for quickest path to get out of this.

>>>> Based on the patch by Ildar Kamaletdinov <i.kamaletdinov@omprussia.ru>.

   Adding him to CC...

>>>>
>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>>>
>>>> ---
>>>> This patch is against the 'bluetooth-next.git' repo.
>>>>
>>>> net/bluetooth/hci_event.c |    8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> Index: bluetooth-next/net/bluetooth/hci_event.c
>>>> ===================================================================
>>>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>>>> +++ bluetooth-next/net/bluetooth/hci_event.c
>>>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>>>> 	if (acl) {
>>>> 		sco = acl->link;
>>>> 		if (sco) {
>>>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>>>> +			    sco->out) {

    Actually, I was expecting that you'd tell me to create a HCI quirk for this situation.
I have a patch doing that but I haven't been able to locate the driver in which to set this
quirk flag...

>>>> +				sco->pkt_type = (hdev->esco_type & SCO_ESCO_MASK) |
>>>> +						(hdev->esco_type & EDR_ESCO_MASK);
>>>> +				if (hci_setup_sync(sco, sco->link->handle))
>>>> +					goto unlock;
>>>> +			}
>>>> 			sco->state = BT_CLOSED;
>>>
>>> since this is the command status event, I doubt that sco->out check is needed.
>>
>>   Can't comment oin this, my BT fu is too weak... 

> It is the case. Command status is only local to command we issued and thus in this case it
> is the connection creation attempt from our side. Meaning it is always outgoing.

   Ildar, what do you think?

> Regards
> 
> Marcel

MBR, Sergei

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-17 19:12       ` Sergey Shtylyov
@ 2020-07-21 18:56         ` Sergey Shtylyov
  2020-07-28  7:16           ` Marcel Holtmann
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Shtylyov @ 2020-07-21 18:56 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev, Ildar Kamaletdinov

Hello!

On 7/17/20 10:12 PM, Sergey Shtylyov wrote:

>>>>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>>>>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>>>>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>>>>> (0x0028) command without actually trying to setup connection with a remote
>>>>> device in case such device (like Digma BT-14 headset) didn't advertise its
>>>>> supported features.  Even though this doesn't break compatibility with the
>>>>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>>>>> (HFP).
>>>>>
>>>>> This patch returns the compatibility with the HFP profile and actually
>>>>> tries to check all available connection parameters despite of the specific
>>>>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>>>>> connection with some headsets.
[...]
>>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>>>>
>>>>> ---
>>>>> This patch is against the 'bluetooth-next.git' repo.
>>>>>
>>>>> net/bluetooth/hci_event.c |    8 ++++++++
>>>>> 1 file changed, 8 insertions(+)
>>>>>
>>>>> Index: bluetooth-next/net/bluetooth/hci_event.c
>>>>> ===================================================================
>>>>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>>>>> +++ bluetooth-next/net/bluetooth/hci_event.c
>>>>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>>>>> 	if (acl) {
>>>>> 		sco = acl->link;
>>>>> 		if (sco) {
>>>>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>>>>> +			    sco->out) {
> 
>     Actually, I was expecting that you'd tell me to create a HCI quirk for this situation.
> I have a patch doing that but I haven't been able to locate the driver in which to set this
> quirk flag...

   And that's no wonder! The BT driver that needs this patch is out-of-tree (and not even open
source, it seems) as we have finally ascertained with Ildar... Is there any interest in the
"preparatory" patch that lowers the indentation levels in hci_cs_setup_sync_conn()?

[...]

MBR, Sergei

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-21 18:56         ` Sergey Shtylyov
@ 2020-07-28  7:16           ` Marcel Holtmann
  2020-07-28 13:27             ` Ildar Kamaletdinov
  0 siblings, 1 reply; 8+ messages in thread
From: Marcel Holtmann @ 2020-07-28  7:16 UTC (permalink / raw)
  To: Sergey Shtylyov
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev, Ildar Kamaletdinov

Hi Sergey,

>>>>>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>>>>>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>>>>>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>>>>>> (0x0028) command without actually trying to setup connection with a remote
>>>>>> device in case such device (like Digma BT-14 headset) didn't advertise its
>>>>>> supported features.  Even though this doesn't break compatibility with the
>>>>>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>>>>>> (HFP).
>>>>>> 
>>>>>> This patch returns the compatibility with the HFP profile and actually
>>>>>> tries to check all available connection parameters despite of the specific
>>>>>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>>>>>> connection with some headsets.
> [...]
>>>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>>>>> 
>>>>>> ---
>>>>>> This patch is against the 'bluetooth-next.git' repo.
>>>>>> 
>>>>>> net/bluetooth/hci_event.c |    8 ++++++++
>>>>>> 1 file changed, 8 insertions(+)
>>>>>> 
>>>>>> Index: bluetooth-next/net/bluetooth/hci_event.c
>>>>>> ===================================================================
>>>>>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>>>>>> +++ bluetooth-next/net/bluetooth/hci_event.c
>>>>>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>>>>>> 	if (acl) {
>>>>>> 		sco = acl->link;
>>>>>> 		if (sco) {
>>>>>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>>>>>> +			    sco->out) {
>> 
>>    Actually, I was expecting that you'd tell me to create a HCI quirk for this situation.
>> I have a patch doing that but I haven't been able to locate the driver in which to set this
>> quirk flag...
> 
>   And that's no wonder! The BT driver that needs this patch is out-of-tree (and not even open
> source, it seems) as we have finally ascertained with Ildar... Is there any interest in the
> "preparatory" patch that lowers the indentation levels in hci_cs_setup_sync_conn()?

how is it possible that there is an out-of-tree Bluetooth driver. Seems odd. Maybe want to submit that upstream first.

Regards

Marcel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RFC] bluetooth: add support for some old headsets
  2020-07-28  7:16           ` Marcel Holtmann
@ 2020-07-28 13:27             ` Ildar Kamaletdinov
  0 siblings, 0 replies; 8+ messages in thread
From: Ildar Kamaletdinov @ 2020-07-28 13:27 UTC (permalink / raw)
  To: Marcel Holtmann, Sergey Shtylyov
  Cc: Johan Hedberg, Bluetooth Kernel Mailing List, David S. Miller,
	Jakub Kicinski, netdev

Hello everyone,

Let me add a little bit more details related to this patch.

According to Bluetooth Core Specification Vol 2 part F page 776 and  Bluetooth Core Specification Vol4 Part E page 1978 after `HCI_Setup_Synchronous_Connection()` was sent `HCI_Command_Status()` must be received from BDR/EDR controller.

According to Bluetooth Core Specification Vol 2 part F page 364 HCI_Command_Status description:

"Some HCI commands may generate errors that need to be reported to the Host, but there is insufficient information to determine how the command would normally be processed. In this case, two events can be used to indicate this to the Host, the HCI_Command_Complete event and HCI_Command_Status events. Which of the two events is used is implementation-dependent."

Mediatek's implementation reports error `Unsupported LMP feature` in `HCI_Command_Status()` event (not in `HCI_Command_Complete()` event as in other implementations).
So that behavior is a little bit odd but don't break compatibility with Bluetooth Core Specification. Actually Mediatek's BDR/EDR controller reports error without trying to actually setup connection with headset.

But according to Hands-Free profile specification 1.8 p. 113 Synchronous Connection Interoperability Requirements Bluetooth Host MUST try all features T1->T2->S1->S2->D0 or D1 before considering connection as `failed`. And it is true if error is reported in `HCI_Command_Complete()` event.
Eventually If error is reported in `HCI_Command_Status()` event connection considered as 'failed' just after first error which breaks compatibility with HFP profile specification v1.8 in Linux Kernel. That leads to problems when SCo/eSCO connection could not be established when using Mediatek's BDR/EDR controllers.

So that patch should add support for correct SCO/eSCO connection behavior when work with at least Mediatek controllers. (a little bit peculiar behavior but compatible with Bluetooth Core spec).

If any objections or questions please fill free to contact me or Sergey anytime.
> Hi Sergey,
>
>>>>>>> The MediaTek Bluetooth platform (MT6630 etc.) has a peculiar implementation
>>>>>>> for the eSCO/SCO connection via BT/EDR: the host controller returns error
>>>>>>> code 0x20 (LMP feature not supported) for HCI_Setup_Synchronous_Connection
>>>>>>> (0x0028) command without actually trying to setup connection with a remote
>>>>>>> device in case such device (like Digma BT-14 headset) didn't advertise its
>>>>>>> supported features.  Even though this doesn't break compatibility with the
>>>>>>> Bluetooth standard it breaks the compatibility with the Hands-Free Profile
>>>>>>> (HFP).
>>>>>>>
>>>>>>> This patch returns the compatibility with the HFP profile and actually
>>>>>>> tries to check all available connection parameters despite of the specific
>>>>>>> MediaTek implementation. Without it one was unable to establish eSCO/SCO
>>>>>>> connection with some headsets.
>> [...]
>>>>>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru>
>>>>>>>
>>>>>>> ---
>>>>>>> This patch is against the 'bluetooth-next.git' repo.
>>>>>>>
>>>>>>> net/bluetooth/hci_event.c |    8 ++++++++
>>>>>>> 1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> Index: bluetooth-next/net/bluetooth/hci_event.c
>>>>>>> ===================================================================
>>>>>>> --- bluetooth-next.orig/net/bluetooth/hci_event.c
>>>>>>> +++ bluetooth-next/net/bluetooth/hci_event.c
>>>>>>> @@ -2187,6 +2187,13 @@ static void hci_cs_setup_sync_conn(struc
>>>>>>> 	if (acl) {
>>>>>>> 		sco = acl->link;
>>>>>>> 		if (sco) {
>>>>>>> +			if (status == 0x20 && /* Unsupported LMP Parameter value */
>>>>>>> +			    sco->out) {
>>>    Actually, I was expecting that you'd tell me to create a HCI quirk for this situation.
>>> I have a patch doing that but I haven't been able to locate the driver in which to set this
>>> quirk flag...
>>   And that's no wonder! The BT driver that needs this patch is out-of-tree (and not even open
>> source, it seems) as we have finally ascertained with Ildar... Is there any interest in the
>> "preparatory" patch that lowers the indentation levels in hci_cs_setup_sync_conn()?
> how is it possible that there is an out-of-tree Bluetooth driver. Seems odd. Maybe want to submit that upstream first.
>
> Regards
>
> Marcel
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-28 13:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 11:08 [PATCH RFC] bluetooth: add support for some old headsets Sergey Shtylyov
2020-07-16 13:14 ` Marcel Holtmann
2020-07-16 20:33   ` Sergey Shtylyov
2020-07-17  6:59     ` Marcel Holtmann
2020-07-17 19:12       ` Sergey Shtylyov
2020-07-21 18:56         ` Sergey Shtylyov
2020-07-28  7:16           ` Marcel Holtmann
2020-07-28 13:27             ` Ildar Kamaletdinov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.