Hi Luiz, That looks good to me. I will verify and send a patch. Thanks! On Tue, Dec 21, 2021 at 3:35 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote: > > Hi Jesse, > > On Tue, Dec 21, 2021 at 1:20 PM Jesse Melhuish <melhuishj@xxxxxxxxxxxx> wrote: > > > > Hi Luiz, > > > > Thanks for looking at this. I did see the main.conf file, and the > > documentation is fairly clear, but I think there is still a bug here. > > Mainly: if one enables the experiment via main.conf, but the driver > > for the kernel has not set the relevant quirk indicating that the > > device supports the functionality, we are still seeing > > "central-peripheral" listed as supported. Ultimately this leads to > > test failures for us as, believing that the functionality is > > supported, we execute tests that then fail (this is reproducible if > > you modify a test device's driver to not support this feature (for > > btusb I removed BTUSB_VALID_LE_STATES as an attribute) and enable the > > feature via main.conf). In code, I think the culprit is seen in this > > function in that the value of "action" is set exclusively by the > > main.conf file, and is then passed into "feat->func" which adds it to > > "adapter->exps" (at least for this feature). Given what you've said > > here, I believe the right solution would be to require that both the > > feature be supported (as derived from the driver) and enabled (as > > derived from main.conf) in order to consider the feature enabled (as > > reported to peers/via query). > > This might be a kernel bug then, the UUID shall be omitted if not > supported, if it is being returned by MGMT_OP_READ_EXP_FEATURES_INFO > then bluetoothd will consider it supported thus causing the problem > you are describing: > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/mgmt.c#n3919 > > It looks like only it is indeed a problem for > simult_central_periph_uuid, so we shall probably change it to: > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > index c8baf6141026..28b873df9084 100644 > --- a/net/bluetooth/mgmt.c > +++ b/net/bluetooth/mgmt.c > @@ -3921,7 +3921,7 @@ static int read_exp_features_info(struct sock > *sk, struct hci_dev *hdev, > } > #endif > > - if (hdev) { > + if (hdev & test_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks)) { > if (hci_dev_le_state_simultaneous(hdev)) > flags = BIT(0); > else > > > On Tue, Dec 21, 2021 at 2:57 PM Luiz Augusto von Dentz > > <luiz.dentz@xxxxxxxxx> wrote: > > > > > > Hi Jesse, > > > > > > On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@xxxxxxxxxxxx> wrote: > > > > > > > > --- > > > > The observed behavior without any change is that support for the > > > > central-peripheral role can be enabled through an experiment flag in > > > > BlueZ regardless of whether the controller can actually support it. > > > > Additionally, if the controller has enabled this feature but the > > > > experiment flag has not been set the central-peripheral role is not > > > > listed as supported. I'm not certain what the expected behavior should > > > > be, but enabling if either source says to enable (this patch) or only > > > > when both enable the feature both seem like reasonable options to start > > > > with. > > > > > > > > src/adapter.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/adapter.c b/src/adapter.c > > > > index 9fc6853c9..60325015b 100644 > > > > --- a/src/adapter.c > > > > +++ b/src/adapter.c > > > > @@ -10434,7 +10434,8 @@ static void read_exp_features_complete(uint8_t status, uint16_t length, > > > > } > > > > > > > > if (feat->func) > > > > - feat->func(adapter, action); > > > > + feat->func(adapter, action || > > > > + (rp->features[i].flags & BIT(0))); > > > > > > Feature being supported doesn't mean the system wants it to be enabled > > > since this is experimental after all there could be side effects, the > > > proper way to enable it is via main.conf: > > > > > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/main.conf#n115 > > > > > > > } > > > > } > > > > } > > > > -- > > > > 2.31.0 > > > > > > -- > > > Luiz Augusto von Dentz > > > > -- > Luiz Augusto von Dentz
