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>
In-Reply-To: <CALWDO_XT=O4NiqMur+_u1z4o0868ZzBr4gpUikgmgw2U4zqMzw@mail.gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Thu, 2 Jul 2020 11:01:09 -0400
Message-ID: <CALWDO_Wf5gftarhsxq-itzWhvtjj171hVYezsRN-GU_dv-zaVg@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

Resending as plaintext.


On Thu, Jul 2, 2020 at 10:59 AM Alain Michaud <alainmichaud@google.com> wro=
te:
>
> Hi Marcel
>
> On Thu, Jul 2, 2020 at 9:18 AM Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>>
>> Hi Alain,
>>
>> > This patch adds an le_supported_roles features which allows a
>> > clients to determine if the controller is able to support peripheral a=
nd
>> > 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. In=
dividual features can not overwrite it.
>>
>> So if you only want to support a the =E2=80=9Cread" functionality, then =
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, would=
 you suggest if LE is supported that the Central and Peripheral roles are s=
upported and just use this to query the simultaneous support?
>
>>
>> > +
>> > static int read_exp_features_info(struct sock *sk, struct hci_dev *hde=
v,
>> >                                 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_fla=
gs(hdev));
>> > +             ++idx;
>> > +     }
>> > +
>> >       rp->feature_count =3D cpu_to_le16(idx);
>> >
>> >       /* After reading the experimental features information, enable
>>
>> Regards
>>
>> Marcel
>>