diff mbox series

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

Message ID 20220907170935.11757-4-sven@svenpeter.dev (mailing list archive)
State New, archived
Headers show
Series Broadcom/Apple Bluetooth driver for Apple Silicon | expand

Commit Message

Sven Peter Sept. 7, 2022, 5:09 p.m. UTC 
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@svenpeter.dev>
---
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(+)

Comments

Luiz Augusto von Dentz Sept. 7, 2022, 6:49 p.m. UTC | #1 
Hi Sven,

On Wed, Sep 7, 2022 at 10:10 AM Sven Peter <sven@svenpeter.dev> 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@svenpeter.dev>
> ---
> 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.

>                 legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
>                 if (legacy_evt_type != LE_ADV_INVALID) {
>                         process_adv_report(hdev, legacy_evt_type, &info->bdaddr,
> --
> 2.25.1
>
Sven Peter Sept. 7, 2022, 7:21 p.m. UTC | #2 
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@svenpeter.dev> 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@svenpeter.dev>
>> ---
>> 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
diff mbox series

Patch

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;
+
 		legacy_evt_type = ext_evt_type_to_legacy(hdev, evt_type);
 		if (legacy_evt_type != LE_ADV_INVALID) {
 			process_adv_report(hdev, legacy_evt_type, &info->bdaddr,