Dear Luiz,
Thank you for your answer.
Am 22.08.23 um 22:20 schrieb Luiz Augusto von Dentz:
> Hi Paul,
>
> On Tue, Aug 22, 2023 at 1:01 PM Paul Menzel <[email protected]> wrote:
>>
>> [CC: +Bruno]
>>
>> Dear Luiz,
>>
>>
>> Thank you for the patch.
>>
>> Am 22.08.23 um 21:14 schrieb Luiz Augusto von Dentz:
>>> From: Luiz Augusto von Dentz <[email protected]>
>>>
>>> This introduces HCI_QUIRK_BROKEN_LE_CODED is is used to indicate that
>>
>> …. It is used …
>>
>>> LE Coded PHY shall not be used, it is then set for some Intel models
>>> that claims to support it but when used causes many problems.
>>
>> that claim to …
>>
>>> Link: https://github.com/bluez/bluez/issues/577
>>> Link: https://github.com/bluez/bluez/issues/582
>>> Link: https://lore.kernel.org/linux-bluetooth/CABBYNZKco-v7wkjHHexxQbgwwSz-S=GZ=dZKbRE1qxT1h4fFbQ@mail.gmail.com/T/#
>>> Fixes: 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")
>>> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
>>> ---
>>> drivers/bluetooth/btintel.c | 2 ++
>>> include/net/bluetooth/hci.h | 10 ++++++++++
>>> include/net/bluetooth/hci_core.h | 4 +++-
>>> 3 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>>> index 9b239ce96fa4..3ed60b2b0340 100644
>>> --- a/drivers/bluetooth/btintel.c
>>> +++ b/drivers/bluetooth/btintel.c
>>> @@ -2776,6 +2776,8 @@ static int btintel_setup_combined(struct hci_dev *hdev)
>>> case 0x11: /* JfP */
>>> case 0x12: /* ThP */
>>> case 0x13: /* HrP */
>>> + set_bit(HCI_QUIRK_BROKEN_LE_CODED, &hdev->quirks);
>>> + fallthrough;
>>> case 0x14: /* CcP */
>>> set_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks);
>>> fallthrough;
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index c58425d4c592..767921d7f5c1 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -319,6 +319,16 @@ enum {
>>> * This quirk must be set before hci_register_dev is called.
>>> */
>>> HCI_QUIRK_USE_MSFT_EXT_ADDRESS_FILTER,
>>> +
>>> + /*
>>> + * When this quirk is set, LE Coded PHY is shall not be used. This is
>>
>> s/is shall/shall/
>>
>>> + * required for some Intel controllers which erroneously claim to
>>> + * support it but it causes problems with extended scanning.
>>> + *
>>> + * This quirk can be set before hci_register_dev is called or
>>> + * during the hdev->setup vendor callback.
>>> + */
>>> + HCI_QUIRK_BROKEN_LE_CODED,
>>> };
>>>
>>> /* HCI device flags */
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 6e2988b11f99..e6359f7346f1 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -1817,7 +1817,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>>> #define scan_2m(dev) (((dev)->le_tx_def_phys & HCI_LE_SET_PHY_2M) || \
>>> ((dev)->le_rx_def_phys & HCI_LE_SET_PHY_2M))
>>>
>>> -#define le_coded_capable(dev) (((dev)->le_features[1] & HCI_LE_PHY_CODED))
>>> +#define le_coded_capable(dev) (((dev)->le_features[1] & HCI_LE_PHY_CODED) && \
>>> + !test_bit(HCI_QUIRK_BROKEN_LE_CODED, \
>>> + &(dev)->quirks))
>>>
>>> #define scan_coded(dev) (((dev)->le_tx_def_phys & HCI_LE_SET_PHY_CODED) || \
>>> ((dev)->le_rx_def_phys & HCI_LE_SET_PHY_CODED))
>>
>> Will this be future proof, once firmware for the broken controllers are
>> fixed?
>
> Yes, it won't cause any regressions if the firmware is fixed in the
> future, but LE coded PHY will need to be re-enabled by removing the
> quirks, this is why we say it is broken but we can't depend on runtime
> detection and should probably print a warning until we have a proper
> fix for it lands at the firmware side. We could in theory set
> HCI_QUIRK_BROKEN_LE_CODED based on the firmware version but firmware
> versioning schemes tend to change so I'm afraid that could also cause
> regression like this in the future.
Understood. Maybe you could add the known broken firmware versions to
the commit message for posterity though.
Kind regards,
Paul