Hi Marcel,
On Wed, Nov 17, 2021 at 8:45 PM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Luiz,
>
> > This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
> > userspace to indicate to the controller to use Device Privacy Mode to a
> > specific device.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > ---
> > v2: Fix marking Device Privacy Flag even when adapter is not capable of
> > handling Set Privacy Mode.
> >
> > include/net/bluetooth/hci_core.h | 4 ++++
> > net/bluetooth/mgmt.c | 24 ++++++++++++++++++++----
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index b5f061882c10..07d2d099dc2a 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {
> >
> > enum hci_conn_flags {
> > HCI_CONN_FLAG_REMOTE_WAKEUP,
> > + HCI_CONN_FLAG_DEVICE_PRIVACY,
> > HCI_CONN_FLAG_MAX
> > };
> >
> > @@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> > #define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
> > hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))
> >
> > +#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
> > + (hdev->commands[39] & 0x04))
> > +
> > /* Use enhanced synchronous connection if command is supported */
> > #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> >
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 06384d761928..8a8376d32be3 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > MGMT_STATUS_NOT_SUPPORTED);
> > }
> >
> > -#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
> > +static u32 supported_device_flags(struct hci_dev *hdev)
> > +{
> > + u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
> > +
> > + /* Check if Privacy Mode can be set */
> > + if (!privacy_mode_capable(hdev))
> > + flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
> > +
> > + return flags;
> > +}
>
> I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.
But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
capable of setting it, anyway the general idea of the supported_flags
is to indicate to userspace what flags the kernel is capable of
settings, so yeah I would expected it to be capable of setting every
flag except for those the controller don't have proper support for.
Maybe you got confused by the logic of first enabling everything and
then toggle back the bits that are not supported by the hdev.
> If we really set all bits on the supported device flags, then that is a bug. Or is it too early for me to grok this code?
>
> >
> > static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > u16 data_len)
> > @@ -4359,7 +4368,7 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > struct mgmt_rp_get_device_flags rp;
> > struct bdaddr_list_with_flags *br_params;
> > struct hci_conn_params *params;
> > - u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> > + u32 supported_flags = supported_device_flags(hdev);
> > u32 current_flags = 0;
> > u8 status = MGMT_STATUS_INVALID_PARAMS;
> >
> > @@ -4423,7 +4432,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > struct bdaddr_list_with_flags *br_params;
> > struct hci_conn_params *params;
> > u8 status = MGMT_STATUS_INVALID_PARAMS;
> > - u32 supported_flags = SUPPORTED_DEVICE_FLAGS();
> > + u32 supported_flags = supported_device_flags(hdev);
> > u32 current_flags = __le32_to_cpu(cp->current_flags);
> >
> > bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
> > @@ -4456,6 +4465,13 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > if (params) {
> > params->current_flags = current_flags;
> > status = MGMT_STATUS_SUCCESS;
> > +
> > + /* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
> > + * has been set.
> > + */
> > + if (hci_conn_test_flag(HCI_CONN_FLAG_DEVICE_PRIVACY,
> > + params->current_flags))
> > + hci_update_passive_scan(hdev);
> > } else {
> > bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
> > &cp->addr.bdaddr,
> > @@ -7061,7 +7077,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> > added:
> > device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> > device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
> > - SUPPORTED_DEVICE_FLAGS(), current_flags);
> > + supported_device_flags(hdev), current_flags);
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
