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
