Re: [PATCH v2 3/5] Bluetooth: hci_event: Add quirk to ignore byte in LE Extended Adv Report

[Sven Peter]

Hi Luiz,

On Wed, Sep 7, 2022, at 20:49, Luiz Augusto von Dentz wrote:
> Hi Sven,
>
> On Wed, Sep 7, 2022 at 10:10 AM Sven Peter <sven@xxxxxxxxxxxxx> wrote:
>>
>> Broadcom controllers present on Apple Silicon devices use the upper
>> 8 bits of the event type in the LE Extended Advertising Report for
>> the channel on which the frame has been received.
>> Add a quirk to drop the upper byte to ensure that the advertising
>> results are parsed correctly.
>>
>> The following excerpt from a btmon trace shows a report received on
>> channel 37 by these controllers:
>>
>> > HCI Event: LE Meta Event (0x3e) plen 55
>>       LE Extended Advertising Report (0x0d)
>>         Num reports: 1
>>         Entry 0
>>           Event type: 0x2513
>>             Props: 0x0013
>>               Connectable
>>               Scannable
>>               Use legacy advertising PDUs
>>             Data status: Complete
>>             Reserved (0x2500)
>>           Legacy PDU Type: Reserved (0x2513)
>>           Address type: Public (0x00)
>>           Address: XX:XX:XX:XX:XX:XX (Shenzhen Jingxun Software [...])
>>           Primary PHY: LE 1M
>>           Secondary PHY: No packets
>>           SID: no ADI field (0xff)
>>           TX power: 127 dBm
>>           RSSI: -76 dBm (0xb4)
>>           Periodic advertising interval: 0.00 msec (0x0000)
>>           Direct address type: Public (0x00)
>>           Direct address: 00:00:00:00:00:00 (OUI 00-00-00)
>>           Data length: 0x1d
>>           [...]
>>         Flags: 0x18
>>           Simultaneous LE and BR/EDR (Controller)
>>           Simultaneous LE and BR/EDR (Host)
>>         Company: Harman International Industries, Inc. (87)
>>           Data: [...]
>>         Service Data (UUID 0xfddf):
>>         Name (complete): JBL Flip 5
>>
>> Signed-off-by: Sven Peter <sven@xxxxxxxxxxxxx>
>> ---
>> changes from v1:
>>   - adjusted the commit message a bit to make checkpatch happy
>>
>>  include/net/bluetooth/hci.h | 11 +++++++++++
>>  net/bluetooth/hci_event.c   |  4 ++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index cf29511b25a8..62539c1a6bf2 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -263,6 +263,17 @@ enum {
>>          * during the hdev->setup vendor callback.
>>          */
>>         HCI_QUIRK_BROKEN_ENHANCED_SETUP_SYNC_CONN,
>> +
>> +       /*
>> +        * When this quirk is set, the upper 8 bits of the evt_type field of
>> +        * the LE Extended Advertising Report events are discarded.
>> +        * Some Broadcom controllers found in Apple machines put the channel
>> +        * the report was received on into these reserved bits.
>> +        *
>> +        * This quirk can be set before hci_register_dev is called or
>> +        * during the hdev->setup vendor callback.
>> +        */
>> +       HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_EVT_TYPE,
>>  };
>>
>>  /* HCI device flags */
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 485c814cf44a..b50d05211f0d 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -6471,6 +6471,10 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, void *data,
>>                         break;
>>
>>                 evt_type = __le16_to_cpu(info->type);
>> +               if (test_bit(HCI_QUIRK_FIXUP_LE_EXT_ADV_REPORT_EVT_TYPE,
>> +                            &hdev->quirks))
>> +                       evt_type &= 0xff;
>> +
>
> Don't think we really need to quirk in order to mask the reserved
> bits, according to the 5.3 spec only bits 0-6 are actually valid, that
> said the usage of the upper byte is sort of non-standard so I don't
> know what is broadcom/apple thinking that they could use like this,
> instead this should probably be placed in a vendor command or even add
> as part of the data itself with a vendor type.

Sure, I'll just mask them unconditionally then for v3.

I originally thought it was a strange bug in their firmware but
then I found their btmon-like tool called "PacketLogger" which
actually decodes that byte and shows it in the UI as "Channel".
It seems to be intentional :/



Best,

Sven