Hi Paul, On Tue, Aug 22, 2023 at 1:01 PM Paul Menzel <pmenzel@xxxxxxxxxxxxx> 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@xxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx/T/# > > Fixes: 288c90224eec ("Bluetooth: Enable all supported LE PHY by default") > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx> > > --- > > 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
