| Message ID | 6f461412-a6c0-aa53-5e74-394e278ee9b1@omprussia.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] bluetooth: add support for some old headsets | expand |
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
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
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
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
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
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
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 >
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); }
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(+)