netdev - Re: [PATCH RFC] bluetooth: add support for some old headsets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e9f32310-2728-60a2-adc7-3a7418ce54e3@omprussia.ru>
Date:   Thu, 16 Jul 2020 23:33:40 +0300
From:   Sergey Shtylyov <s.shtylyov@...russia.ru>
To:     Marcel Holtmann <marcel@...tmann.org>
CC:     Johan Hedberg <johan.hedberg@...il.com>,
        Bluetooth Kernel Mailing List 
        <linux-bluetooth@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC] bluetooth: add support for some old headsets

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@...russia.ru>.
>>
>> Signed-off-by: Sergey Shtylyov <s.shtylyov@...russia.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