diff mbox series

[v3,3/4] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

Message ID 20211120012448.1476960-3-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v3,1/4] Bluetooth: MGMT: Use hci_dev_test_and_{set,clear}_flag | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS

Commit Message

Luiz Augusto von Dentz Nov. 20, 2021, 1:24 a.m. UTC 
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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@intel.com>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/mgmt.c             | 12 ++++++++++++
 2 files changed, 13 insertions(+)

Comments

Marcel Holtmann Nov. 24, 2021, 3:27 p.m. UTC | #1 
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@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/mgmt.c             | 12 ++++++++++++
> 2 files changed, 13 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index fc93a1907c90..9c94d1c49b25 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -153,6 +153,7 @@ struct bdaddr_list_with_irk {
> 
> enum hci_conn_flags {
> 	HCI_CONN_FLAG_REMOTE_WAKEUP,
> +	HCI_CONN_FLAG_DEVICE_PRIVACY,

coming this now, I wonder if we better call them FLAG_REMOTE_WAKEUP_SUPPORT and FLAG_DEVICE_PRIVACY_SUPPORT. If I am not mistaken, then these are for indicating support for it.

Regards

Marcel
Luiz Augusto von Dentz Dec. 1, 2021, 7:19 p.m. UTC | #2 
Hi Marcel,

On Wed, Nov 24, 2021 at 7:27 AM Marcel Holtmann <marcel@holtmann.org> 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@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h |  1 +
> > net/bluetooth/mgmt.c             | 12 ++++++++++++
> > 2 files changed, 13 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index fc93a1907c90..9c94d1c49b25 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -153,6 +153,7 @@ struct bdaddr_list_with_irk {
> >
> > enum hci_conn_flags {
> >       HCI_CONN_FLAG_REMOTE_WAKEUP,
> > +     HCI_CONN_FLAG_DEVICE_PRIVACY,
>
> coming this now, I wonder if we better call them FLAG_REMOTE_WAKEUP_SUPPORT and FLAG_DEVICE_PRIVACY_SUPPORT. If I am not mistaken, then these are for indicating support for it.

These flags are used in multiple places:

hci_dev->conn_flags
hci_conn_params->conn_flags
bdaddr_list_with_flags->flags

Which is one of the reason I made them all use  DECLARE_BITMAP(flags,
__HCI_CONN_NUM_FLAGS) so they are in sync, the use of them in
hci_dev->conn_flags means they are supported but in the other 2 it
means they are in use, so I prefer leave as they are.
Marcel Holtmann Dec. 3, 2021, 9:12 p.m. UTC | #3 
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@intel.com>
>>> ---
>>> include/net/bluetooth/hci_core.h |  1 +
>>> net/bluetooth/mgmt.c             | 12 ++++++++++++
>>> 2 files changed, 13 insertions(+)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index fc93a1907c90..9c94d1c49b25 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -153,6 +153,7 @@ struct bdaddr_list_with_irk {
>>> 
>>> enum hci_conn_flags {
>>>      HCI_CONN_FLAG_REMOTE_WAKEUP,
>>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
>> 
>> coming this now, I wonder if we better call them FLAG_REMOTE_WAKEUP_SUPPORT and FLAG_DEVICE_PRIVACY_SUPPORT. If I am not mistaken, then these are for indicating support for it.
> 
> These flags are used in multiple places:
> 
> hci_dev->conn_flags
> hci_conn_params->conn_flags
> bdaddr_list_with_flags->flags
> 
> Which is one of the reason I made them all use  DECLARE_BITMAP(flags,
> __HCI_CONN_NUM_FLAGS) so they are in sync, the use of them in
> hci_dev->conn_flags means they are supported but in the other 2 it
> means they are in use, so I prefer leave as they are.

is my comment wrong? Don’t they always indicate the support for it?

Regards

Marcel
Luiz Augusto von Dentz Dec. 3, 2021, 9:41 p.m. UTC | #4 
Hi Marcel,

On Fri, Dec 3, 2021 at 1:12 PM Marcel Holtmann <marcel@holtmann.org> 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@intel.com>
> >>> ---
> >>> include/net/bluetooth/hci_core.h |  1 +
> >>> net/bluetooth/mgmt.c             | 12 ++++++++++++
> >>> 2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>> index fc93a1907c90..9c94d1c49b25 100644
> >>> --- a/include/net/bluetooth/hci_core.h
> >>> +++ b/include/net/bluetooth/hci_core.h
> >>> @@ -153,6 +153,7 @@ struct bdaddr_list_with_irk {
> >>>
> >>> enum hci_conn_flags {
> >>>      HCI_CONN_FLAG_REMOTE_WAKEUP,
> >>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
> >>
> >> coming this now, I wonder if we better call them FLAG_REMOTE_WAKEUP_SUPPORT and FLAG_DEVICE_PRIVACY_SUPPORT. If I am not mistaken, then these are for indicating support for it.
> >
> > These flags are used in multiple places:
> >
> > hci_dev->conn_flags
> > hci_conn_params->conn_flags
> > bdaddr_list_with_flags->flags
> >
> > Which is one of the reason I made them all use  DECLARE_BITMAP(flags,
> > __HCI_CONN_NUM_FLAGS) so they are in sync, the use of them in
> > hci_dev->conn_flags means they are supported but in the other 2 it
> > means they are in use, so I prefer leave as they are.
>
> is my comment wrong? Don’t they always indicate the support for it?

Support vs Use, anyway I always think about the shortest form for
defines and having some term like SUPPORT sounds a little superfluous
for me, but I'm fine adding it if you really think that is necessary
in this case.

> Regards
>
> Marcel
>
Marcel Holtmann Dec. 3, 2021, 9:47 p.m. UTC | #5 
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@intel.com>
>>>>> ---
>>>>> include/net/bluetooth/hci_core.h |  1 +
>>>>> net/bluetooth/mgmt.c             | 12 ++++++++++++
>>>>> 2 files changed, 13 insertions(+)
>>>>> 
>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>>> index fc93a1907c90..9c94d1c49b25 100644
>>>>> --- a/include/net/bluetooth/hci_core.h
>>>>> +++ b/include/net/bluetooth/hci_core.h
>>>>> @@ -153,6 +153,7 @@ struct bdaddr_list_with_irk {
>>>>> 
>>>>> enum hci_conn_flags {
>>>>>     HCI_CONN_FLAG_REMOTE_WAKEUP,
>>>>> +     HCI_CONN_FLAG_DEVICE_PRIVACY,
>>>> 
>>>> coming this now, I wonder if we better call them FLAG_REMOTE_WAKEUP_SUPPORT and FLAG_DEVICE_PRIVACY_SUPPORT. If I am not mistaken, then these are for indicating support for it.
>>> 
>>> These flags are used in multiple places:
>>> 
>>> hci_dev->conn_flags
>>> hci_conn_params->conn_flags
>>> bdaddr_list_with_flags->flags
>>> 
>>> Which is one of the reason I made them all use  DECLARE_BITMAP(flags,
>>> __HCI_CONN_NUM_FLAGS) so they are in sync, the use of them in
>>> hci_dev->conn_flags means they are supported but in the other 2 it
>>> means they are in use, so I prefer leave as they are.
>> 
>> is my comment wrong? Don’t they always indicate the support for it?
> 
> Support vs Use, anyway I always think about the shortest form for
> defines and having some term like SUPPORT sounds a little superfluous
> for me, but I'm fine adding it if you really think that is necessary
> in this case.

I already applied 1/4 and 2/4. So just re-spin them to make them apply to bluetooth-next.

Regards

Marcel
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index fc93a1907c90..9c94d1c49b25 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -153,6 +153,7 @@  struct bdaddr_list_with_irk {
 
 enum hci_conn_flags {
 	HCI_CONN_FLAG_REMOTE_WAKEUP,
+	HCI_CONN_FLAG_DEVICE_PRIVACY,
 
 	__HCI_CONN_NUM_FLAGS,
 };
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0f4b620bc630..925a549e448c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3978,6 +3978,11 @@  static int exp_ll_privacy_feature_changed(bool enabled, struct hci_dev *hdev,
 	memcpy(ev.uuid, rpa_resolution_uuid, 16);
 	ev.flags = cpu_to_le32((enabled ? BIT(0) : 0) | BIT(1));
 
+	if (enabled && privacy_mode_capable(hdev))
+		set_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags);
+	else
+		clear_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, hdev->conn_flags);
+
 	return mgmt_limited_event(MGMT_EV_EXP_FEATURE_CHANGED, hdev,
 				  &ev, sizeof(ev),
 				  HCI_MGMT_EXP_FEATURE_EVENTS, skip);
@@ -4461,6 +4466,13 @@  static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (params) {
 			bitmap_from_u64(params->flags, current_flags);
 			status = MGMT_STATUS_SUCCESS;
+
+			/* Update passive scan if HCI_CONN_FLAG_DEVICE_PRIVACY
+			 * has been set.
+			 */
+			if (test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY,
+				     params->flags))
+				hci_update_passive_scan(hdev);
 		} else {
 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
 				    &cp->addr.bdaddr,