Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200701220853.421445-1-alainm@chromium.org> <427B6E1C-178C-405D-88F8-899EC48AC8A9@holtmann.org>
 <CALWDO_XT=O4NiqMur+_u1z4o0868ZzBr4gpUikgmgw2U4zqMzw@mail.gmail.com> <CALWDO_Wf5gftarhsxq-itzWhvtjj171hVYezsRN-GU_dv-zaVg@mail.gmail.com>
In-Reply-To: <CALWDO_Wf5gftarhsxq-itzWhvtjj171hVYezsRN-GU_dv-zaVg@mail.gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Thu, 2 Jul 2020 13:37:31 -0400
Message-ID: <CALWDO_VcbYuWV7yoJBNM17up8Wb+TXnYQMAe9oexhv3zFt5y0g@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: le_supported_roles experimental feature
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 2, 2020 at 11:01 AM Alain Michaud <alainmichaud@google.com> wro=
te:
>
> Resending as plaintext.
>
>
> On Thu, Jul 2, 2020 at 10:59 AM Alain Michaud <alainmichaud@google.com> w=
rote:
> >
> > Hi Marcel
> >
> > On Thu, Jul 2, 2020 at 9:18 AM Marcel Holtmann <marcel@holtmann.org> wr=
ote:
> >>
> >> Hi Alain,
> >>
> >> > This patch adds an le_supported_roles features which allows a
> >> > clients to determine if the controller is able to support peripheral=
 and
> >> > central connections separately and at the same time.
> >> >
> >> > Signed-off-by: Alain Michaud <alainm@chromium.org>
> >> > ---
> >> >
> >> > Changes in v2:
> >> > - Slight change of design based on offline feedback
> >> >
> >> > net/bluetooth/mgmt.c | 36 +++++++++++++++++++++++++++++++++++-
> >> > 1 file changed, 35 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> > index 5e9b9728eeac..c13fcc21745f 100644
> >> > --- a/net/bluetooth/mgmt.c
> >> > +++ b/net/bluetooth/mgmt.c
> >> > @@ -3753,10 +3753,36 @@ static const u8 debug_uuid[16] =3D {
> >> > };
> >> > #endif
> >> >
> >> > +/* 671b10b5-42c0-4696-9227-eb28d1b049d6 */
> >> > +static const u8 le_supported_roles[16] =3D {
> >> > +     0xd6, 0x49, 0xb0, 0xd1, 0x28, 0xeb, 0x27, 0x92,
> >> > +     0x96, 0x46, 0xc0, 0x42, 0xb5, 0x10, 0x1b, 0x67,
> >> > +};
> >> > +
> >> > +static u32 get_le_roles_flags(struct hci_dev *hdev)
> >> > +{
> >> > +     u32 flags =3D 0;
> >> > +
> >> > +     /* Central connections supported */
> >> > +     if (hdev->le_states[4] & 0x08)
> >> > +             flags |=3D BIT(0);
> >> > +
> >> > +     /* Peripheral connections supported */
> >> > +     if (hdev->le_states[4] & 0x40)
> >> > +             flags |=3D BIT(1);
> >> > +
> >> > +     /* Simult central and peripheral connections supported */
> >> > +     if (test_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks) &&
> >> > +         (hdev->le_states[3] & 0x10))
> >> > +             flags |=3D BIT(2);
> >> > +
> >> > +     return flags;
> >> > +}
> >>
> >> this is not what we can do here. The flags are defined like this.
> >>
> >>         The following bits are defined for the Flags parameter:
> >>
> >>                 0       Feature active
> >>                 1       Causes change in supported settings
> >>
> >> And I want these flags for generic handling of experimental features. =
Individual features can not overwrite it.
> >>
> >> So if you only want to support a the =E2=80=9Cread" functionality, the=
n something like this please.
> >>
> >>         if ((hdev->le_states[4] & 0x08) &&      /* Central */
> >>             (hdev->le_states[4] & 0x40) &&      /* Peripheral */
> >>             (hdev->le_states[3] & 0x10) &&      /* Simultaneous */
> >>             test_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks))
> >>                 flags |=3D BIT(0);
> >>
> > OK, Since the userspace Api we discussed reports individual states, wou=
ld you suggest if LE is supported that the Central and Peripheral roles are=
 supported and just use this to query the simultaneous support? Also, seems=
 like what you are suggesting looks a lot like v1.
> >
> >>
> >> > +
> >> > static int read_exp_features_info(struct sock *sk, struct hci_dev *h=
dev,
> >> >                                 void *data, u16 data_len)
> >> > {
> >> > -     char buf[42];
> >> > +     char buf[44];
> >> >       struct mgmt_rp_read_exp_features_info *rp =3D (void *)buf;
> >> >       u16 idx =3D 0;
> >> >
> >> > @@ -3774,6 +3800,14 @@ static int read_exp_features_info(struct sock=
 *sk, struct hci_dev *hdev,
> >> >       }
> >> > #endif
> >> >
> >> > +     if (hdev) {
> >> > +             memcpy(rp->features[idx].uuid, le_supported_roles,
> >> > +                    sizeof(le_supported_roles));
> >> > +
> >>
> >> And I would put it all here instead of a separate function.
> >>
> >> > +             rp->features[idx].flags =3D cpu_to_le32(get_le_roles_f=
lags(hdev));
> >> > +             ++idx;
> >> > +     }
> >> > +
> >> >       rp->feature_count =3D cpu_to_le16(idx);
> >> >
> >> >       /* After reading the experimental features information, enable
> >>
> >> Regards
> >>
> >> Marcel
> >>