diff mbox series

[v2,1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

Message ID 20211118231302.1000168-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Delegated to: Luiz Von Dentz
Headers show
Series [v2,1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester fail Total: 493, Passed: 490 (99.4%), Failed: 2, Not Run: 1
tedd_an/testrunnerrfcomm-tester success Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester success Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 0

Commit Message

Luiz Augusto von Dentz Nov. 18, 2021, 11:13 p.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>
---
v2: Fix supported flags not actually checking if the hdev really
supports the flags.

 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/mgmt.c             | 30 ++++++++++++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

Comments

Marcel Holtmann Nov. 19, 2021, 10:01 a.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>
> ---
> v2: Fix supported flags not actually checking if the hdev really
> supports the flags.
> 
> include/net/bluetooth/hci_core.h |  4 ++++
> net/bluetooth/mgmt.c             | 30 ++++++++++++++++++++++++++----
> 2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2560cfe80db8..42ba40df6e20 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
> };
> 
> @@ -1465,6 +1466,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 f8f74d344297..d82d1a62754a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4349,7 +4349,22 @@ 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 = 0;
> +
> +	/* Check if adapter can wakeup the system */
> +	if (hdev->wakeup && hdev->wakeup(hdev))
> +		flags |= BIT(HCI_CONN_FLAG_REMOTE_WAKEUP);

I would do this change as a separate patch since it has nothing to do with the device privacy setting.

Do we have to call hdev->wakeup() as well here? Isn’t the existence of the callback enough indication.

That also said, doesn’t it make sense to store the supported_flags in the device params struct. It would make it certainly easy to return. Potentially we have a few calls to Get Device Flags and its notifications that have to rebuild this flags field over and over again.

Regards

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

On Fri, Nov 19, 2021 at 2:01 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>
> > ---
> > v2: Fix supported flags not actually checking if the hdev really
> > supports the flags.
> >
> > include/net/bluetooth/hci_core.h |  4 ++++
> > net/bluetooth/mgmt.c             | 30 ++++++++++++++++++++++++++----
> > 2 files changed, 30 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 2560cfe80db8..42ba40df6e20 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
> > };
> >
> > @@ -1465,6 +1466,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 f8f74d344297..d82d1a62754a 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -4349,7 +4349,22 @@ 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 = 0;
> > +
> > +     /* Check if adapter can wakeup the system */
> > +     if (hdev->wakeup && hdev->wakeup(hdev))
> > +             flags |= BIT(HCI_CONN_FLAG_REMOTE_WAKEUP);
>
> I would do this change as a separate patch since it has nothing to do with the device privacy setting.

Will do.

> Do we have to call hdev->wakeup() as well here? Isn’t the existence of the callback enough indication.

Hmm, right I guess we want to know if the driver is capable of waking
up, not that it is actually enabled at that point since we are
evaluating if it is supported. I will change that.

> That also said, doesn’t it make sense to store the supported_flags in the device params struct. It would make it certainly easy to return. Potentially we have a few calls to Get Device Flags and its notifications that have to rebuild this flags field over and over again.

But we only perform a lookup, so in case there are no hci_conn_params
we would have to store it somewhere else, perhaps in hci_dev?

> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2560cfe80db8..42ba40df6e20 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
 };
 
@@ -1465,6 +1466,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 f8f74d344297..d82d1a62754a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4349,7 +4349,22 @@  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 = 0;
+
+	/* Check if adapter can wakeup the system */
+	if (hdev->wakeup && hdev->wakeup(hdev))
+		flags |= BIT(HCI_CONN_FLAG_REMOTE_WAKEUP);
+
+	/* Check if Privacy Mode can be set */
+	if (privacy_mode_capable(hdev))
+		flags |= BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
+
+	bt_dev_err(hdev, "flag 0x%8x", flags);
+
+	return flags;
+}
 
 static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
@@ -4358,7 +4373,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;
 
@@ -4422,7 +4437,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",
@@ -4455,6 +4470,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,
@@ -7060,7 +7082,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);
 
 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
 				MGMT_STATUS_SUCCESS, &cp->addr,