MIME-Version: 1.0
References: <61c10192.1c69fb81.96a67.06a8@mx.google.com> <CABBYNZLQ_b8Aw21QnUzmuEedny4wh4LSu6DLQvw1pynFGvHMTA@mail.gmail.com>
 <CACGnfjRA2Co6OpTJfosbx0GdLJwk3dVHQ8edWPqkKSo_q4JyPQ@mail.gmail.com> <CABBYNZK+32F-AxVsFYXKXWfzbFRDoR9PXJNL6UOvKv8+H+DoUw@mail.gmail.com>
In-Reply-To: <CABBYNZK+32F-AxVsFYXKXWfzbFRDoR9PXJNL6UOvKv8+H+DoUw@mail.gmail.com>
From:   Jesse Melhuish <melhuishj@chromium.org>
Date:   Tue, 21 Dec 2021 16:27:59 -0600
Message-ID: <CACGnfjTJm4GP9kTbUgAnxO=+RrLWEq+=EfMLh5=DAQmJBuBnQg@mail.gmail.com>
Subject: Re: [RFC BlueZ PATCH] Bluetooth: Use driver status and experiment
 value for central-peripheral support.
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Jesse Melhuish <melhuishj@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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