Re: [PATCH] Bluetooth: HCI: Introduce HCI_QUIRK_BROKEN_LE_CODED

[Bruno Pitrus <brunopitrus@hotmail.com>]

Dnia środa, 23 sierpnia 2023 18:35:27 CEST Luiz Augusto von Dentz pisze:
> Hi Bruno,
> 
> On Wed, Aug 23, 2023 at 2:21 AM Bruno Pitrus <brunopitrus@hotmail.com> wrote:
> >
> > Dnia środa, 23 sierpnia 2023 10:26:31 CEST Paul Menzel pisze:
> > > 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 <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.
> > >
> > > Understood. Maybe you could add the known broken firmware versions to
> > > the commit message for posterity though.
> > >
> > >
> > > Kind regards,
> > >
> > > Paul
> > >
> > Thanks,
> > This patch partly works for me. The mouse now takes several dozen seconds to detect where the computer did not find it at all before.
> > But note that in kernel 6.3.x it was always detected immediately.
> 
> What version did you try, v2 didn't have any effect for me, only v3
> worked which had the same behavior of AX210 so it discovered it quite
> quickly.
> 
> 

I only tried v1 patch ( https://www.spinics.net/lists/linux-bluetooth/msg106406.html ). Should i try a later version instead?