* [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. @ 2021-12-20 22:19 Jesse Melhuish 2021-12-21 20:57 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Jesse Melhuish @ 2021-12-20 22:19 UTC (permalink / raw) To: linux-bluetooth; +Cc: Jesse Melhuish --- 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))); } } } -- 2.31.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. 2021-12-20 22:19 [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support Jesse Melhuish @ 2021-12-21 20:57 ` Luiz Augusto von Dentz 2021-12-21 21:19 ` Jesse Melhuish 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2021-12-21 20:57 UTC (permalink / raw) To: Jesse Melhuish; +Cc: linux-bluetooth@vger.kernel.org Hi Jesse, On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@chromium.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. 2021-12-21 20:57 ` Luiz Augusto von Dentz @ 2021-12-21 21:19 ` Jesse Melhuish 2021-12-21 21:35 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Jesse Melhuish @ 2021-12-21 21:19 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Jesse Melhuish, linux-bluetooth@vger.kernel.org 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). On Tue, Dec 21, 2021 at 2:57 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Jesse, > > On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@chromium.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. 2021-12-21 21:19 ` Jesse Melhuish @ 2021-12-21 21:35 ` Luiz Augusto von Dentz 2021-12-21 22:27 ` Jesse Melhuish 0 siblings, 1 reply; 6+ messages in thread From: Luiz Augusto von Dentz @ 2021-12-21 21:35 UTC (permalink / raw) To: Jesse Melhuish; +Cc: linux-bluetooth@vger.kernel.org Hi Jesse, On Tue, Dec 21, 2021 at 1:20 PM Jesse Melhuish <melhuishj@chromium.org> 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@gmail.com> wrote: > > > > Hi Jesse, > > > > On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@chromium.org> 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. 2021-12-21 21:35 ` Luiz Augusto von Dentz @ 2021-12-21 22:27 ` Jesse Melhuish 2021-12-22 20:12 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 6+ messages in thread From: Jesse Melhuish @ 2021-12-21 22:27 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Jesse Melhuish, linux-bluetooth@vger.kernel.org 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@gmail.com> wrote: > > Hi Jesse, > > On Tue, Dec 21, 2021 at 1:20 PM Jesse Melhuish <melhuishj@chromium.org> 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@gmail.com> wrote: > > > > > > Hi Jesse, > > > > > > On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@chromium.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support. 2021-12-21 22:27 ` Jesse Melhuish @ 2021-12-22 20:12 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 6+ messages in thread From: Luiz Augusto von Dentz @ 2021-12-22 20:12 UTC (permalink / raw) To: Jesse Melhuish; +Cc: linux-bluetooth@vger.kernel.org HI Jesse, On Tue, Dec 21, 2021 at 2:28 PM Jesse Melhuish <melhuishj@chromium.org> wrote: > > 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@gmail.com> wrote: > > > > Hi Jesse, > > > > On Tue, Dec 21, 2021 at 1:20 PM Jesse Melhuish <melhuishj@chromium.org> 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: I end up sending a patch myself: https://patchwork.kernel.org/project/bluetooth/patch/20211221223357.742863-4-luiz.dentz@gmail.com/ > > 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@gmail.com> wrote: > > > > > > > > Hi Jesse, > > > > > > > > On Mon, Dec 20, 2021 at 5:59 PM Jesse Melhuish <melhuishj@chromium.org> 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 -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-22 20:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-12-20 22:19 [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment value for central-peripheral support Jesse Melhuish 2021-12-21 20:57 ` Luiz Augusto von Dentz 2021-12-21 21:19 ` Jesse Melhuish 2021-12-21 21:35 ` Luiz Augusto von Dentz 2021-12-21 22:27 ` Jesse Melhuish 2021-12-22 20:12 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox