Re: [PATCH] Bluetooth: HCI: Introduce HCI_QUIRK_BROKEN_LE_CODED

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Paul,

On Tue, Aug 22, 2023 at 1:01 PM Paul Menzel <pmenzel@molgen.mpg.de> 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 <luiz.von.dentz@intel.com>
> >
> > 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 <luiz.von.dentz@intel.com>
> > ---
> >   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.

>
> Kind regards,
>
> Paul



-- 
Luiz Augusto von Dentz