[PATCH v3 1/4] Bluetooth: MGMT: Use hci_dev_test_and_{set,clear}_flag

[PATCH v3 1/4] Bluetooth: MGMT: Use hci_dev_test_and_{set,clear}_flag

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This make use of hci_dev_test_and_{set,clear}_flag instead of doing 2
operations in a row.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Fix marking Device Privacy Flag even when adapter is not capable of
handling Set Privacy Mode.
v3: Add patch for using hci_dev_test_and_{set,clear}_flag and split
changes reworking how HCI_CONN_FLAG_REMOTE_WAKEUP is set and make use of
bitmap to store the supported flags.

 net/bluetooth/mgmt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f8f74d344297..0f91bf15e260 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4041,10 +4041,10 @@ static int set_zero_key_func(struct sock *sk, struct hci_dev *hdev,
 #endif
 
 	if (hdev && use_ll_privacy(hdev) && !hdev_is_powered(hdev)) {
-		bool changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
-
-		hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+		bool changed;
 
+		changed = hci_dev_test_and_clear_flag(hdev,
+						      HCI_ENABLE_LL_PRIVACY);
 		if (changed)
 			exp_ll_privacy_feature_changed(false, hdev, sk);
 	}
@@ -4139,15 +4139,15 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
 	val = !!cp->param[0];
 
 	if (val) {
-		changed = !hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
-		hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+		changed = !hci_dev_test_and_set_flag(hdev,
+						     HCI_ENABLE_LL_PRIVACY);
 		hci_dev_clear_flag(hdev, HCI_ADVERTISING);
 
 		/* Enable LL privacy + supported settings changed */
 		flags = BIT(0) | BIT(1);
 	} else {
-		changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
-		hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
+		changed = hci_dev_test_and_clear_flag(hdev,
+						      HCI_ENABLE_LL_PRIVACY);
 
 		/* Disable LL privacy + supported settings changed */
 		flags = BIT(1);
-- 
2.33.1

[PATCH v3 2/4] Bluetooth: hci_core: Rework hci_conn_params flags

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This reworks hci_conn_params flags to use bitmap_* helpers and add
support for setting the supported flags in hdev->conn_flags so it can
easily be accessed.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci_core.h | 48 +++++++++++++++++++-------------
 net/bluetooth/hci_core.c         |  8 +++++-
 net/bluetooth/hci_request.c      |  4 +--
 net/bluetooth/hci_sync.c         |  7 ++---
 net/bluetooth/mgmt.c             | 30 +++++++++++++-------
 5 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2560cfe80db8..fc93a1907c90 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -151,22 +151,21 @@ struct bdaddr_list_with_irk {
 	u8 local_irk[16];
 };
 
-struct bdaddr_list_with_flags {
-	struct list_head list;
-	bdaddr_t bdaddr;
-	u8 bdaddr_type;
-	u32 current_flags;
-};
-
 enum hci_conn_flags {
 	HCI_CONN_FLAG_REMOTE_WAKEUP,
-	HCI_CONN_FLAG_MAX
-};
 
-#define hci_conn_test_flag(nr, flags) ((flags) & (1U << nr))
+	__HCI_CONN_NUM_FLAGS,
+};
 
 /* Make sure number of flags doesn't exceed sizeof(current_flags) */
-static_assert(HCI_CONN_FLAG_MAX < 32);
+static_assert(__HCI_CONN_NUM_FLAGS < 32);
+
+struct bdaddr_list_with_flags {
+	struct list_head list;
+	bdaddr_t bdaddr;
+	u8 bdaddr_type;
+	DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
+};
 
 struct bt_uuid {
 	struct list_head list;
@@ -559,6 +558,7 @@ struct hci_dev {
 	struct rfkill		*rfkill;
 
 	DECLARE_BITMAP(dev_flags, __HCI_NUM_FLAGS);
+	DECLARE_BITMAP(conn_flags, __HCI_CONN_NUM_FLAGS);
 
 	__s8			adv_tx_power;
 	__u8			adv_data[HCI_MAX_EXT_AD_LENGTH];
@@ -754,7 +754,7 @@ struct hci_conn_params {
 
 	struct hci_conn *conn;
 	bool explicit_connect;
-	u32 current_flags;
+	DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
 };
 
 extern struct list_head hci_dev_list;
@@ -762,13 +762,20 @@ extern struct list_head hci_cb_list;
 extern rwlock_t hci_dev_list_lock;
 extern struct mutex hci_cb_list_lock;
 
-#define hci_dev_set_flag(hdev, nr)             set_bit((nr), (hdev)->dev_flags)
-#define hci_dev_clear_flag(hdev, nr)           clear_bit((nr), (hdev)->dev_flags)
-#define hci_dev_change_flag(hdev, nr)          change_bit((nr), (hdev)->dev_flags)
-#define hci_dev_test_flag(hdev, nr)            test_bit((nr), (hdev)->dev_flags)
-#define hci_dev_test_and_set_flag(hdev, nr)    test_and_set_bit((nr), (hdev)->dev_flags)
-#define hci_dev_test_and_clear_flag(hdev, nr)  test_and_clear_bit((nr), (hdev)->dev_flags)
-#define hci_dev_test_and_change_flag(hdev, nr) test_and_change_bit((nr), (hdev)->dev_flags)
+#define hci_dev_set_flag(hdev, nr) \
+	set_bit((nr), (hdev)->dev_flags)
+#define hci_dev_clear_flag(hdev, nr) \
+	clear_bit((nr), (hdev)->dev_flags)
+#define hci_dev_change_flag(hdev, nr) \
+	change_bit((nr), (hdev)->dev_flags)
+#define hci_dev_test_flag(hdev, nr) \
+	test_bit((nr), (hdev)->dev_flags)
+#define hci_dev_test_and_set_flag(hdev, nr) \
+	test_and_set_bit((nr), (hdev)->dev_flags)
+#define hci_dev_test_and_clear_flag(hdev, nr) \
+	test_and_clear_bit((nr), (hdev)->dev_flags)
+#define hci_dev_test_and_change_flag(hdev, nr) \
+	test_and_change_bit((nr), (hdev)->dev_flags)
 
 #define hci_dev_clear_volatile_flags(hdev)			\
 	do {							\
@@ -1465,6 +1472,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/hci_core.c b/net/bluetooth/hci_core.c
index fdc0dcf8ee36..7c4af0b34b62 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2153,7 +2153,7 @@ int hci_bdaddr_list_add_with_flags(struct list_head *list, bdaddr_t *bdaddr,
 
 	bacpy(&entry->bdaddr, bdaddr);
 	entry->bdaddr_type = type;
-	entry->current_flags = flags;
+	bitmap_from_u64(entry->flags, flags);
 
 	list_add(&entry->list, list);
 
@@ -2629,6 +2629,12 @@ int hci_register_dev(struct hci_dev *hdev)
 	if (test_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks))
 		hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
 
+	/* Mark Remote Wakeup connection flag as supported if driver has wakeup
+	 * callback.
+	 */
+	if (hdev->wakeup)
+		set_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, hdev->conn_flags);
+
 	hci_sock_dev_event(hdev, HCI_DEV_REG);
 	hci_dev_hold(hdev);
 
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 8b3205e4b23e..fee88214606e 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -492,8 +492,8 @@ static int add_to_accept_list(struct hci_request *req,
 	}
 
 	/* During suspend, only wakeable devices can be in accept list */
-	if (hdev->suspended && !hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
-						   params->current_flags))
+	if (hdev->suspended &&
+	    !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
 		return 0;
 
 	*num_entries += 1;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index ad86caf41f91..5f44ff0b8910 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1606,8 +1606,8 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 	}
 
 	/* During suspend, only wakeable devices can be in acceptlist */
-	if (hdev->suspended && !hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
-						   params->current_flags))
+	if (hdev->suspended &&
+	    !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
 		return 0;
 
 	/* Attempt to program the device in the resolving list first to avoid
@@ -4749,8 +4749,7 @@ static int hci_update_event_filter_sync(struct hci_dev *hdev)
 	hci_clear_event_filter_sync(hdev);
 
 	list_for_each_entry(b, &hdev->accept_list, list) {
-		if (!hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
-					b->current_flags))
+		if (!test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, b->flags))
 			continue;
 
 		bt_dev_dbg(hdev, "Adding event filters for %pMR", &b->bdaddr);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0f91bf15e260..0f4b620bc630 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4349,8 +4349,6 @@ 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 int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
 {
@@ -4358,7 +4356,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;
 	u32 current_flags = 0;
 	u8 status = MGMT_STATUS_INVALID_PARAMS;
 
@@ -4367,6 +4365,9 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	hci_dev_lock(hdev);
 
+	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
+			__HCI_CONN_NUM_FLAGS);
+
 	memset(&rp, 0, sizeof(rp));
 
 	if (cp->addr.type == BDADDR_BREDR) {
@@ -4376,7 +4377,8 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (!br_params)
 			goto done;
 
-		current_flags = br_params->current_flags;
+		bitmap_to_arr32(&current_flags, br_params->flags,
+				__HCI_CONN_NUM_FLAGS);
 	} else {
 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
 						le_addr_type(cp->addr.type));
@@ -4384,7 +4386,8 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (!params)
 			goto done;
 
-		current_flags = params->current_flags;
+		bitmap_to_arr32(&current_flags, params->flags,
+				__HCI_CONN_NUM_FLAGS);
 	}
 
 	bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
@@ -4422,13 +4425,16 @@ 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;
 	u32 current_flags = __le32_to_cpu(cp->current_flags);
 
 	bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
 		   &cp->addr.bdaddr, cp->addr.type,
 		   __le32_to_cpu(current_flags));
 
+	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
+			__HCI_CONN_NUM_FLAGS);
+
 	if ((supported_flags | current_flags) != supported_flags) {
 		bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)",
 			    current_flags, supported_flags);
@@ -4443,7 +4449,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 							      cp->addr.type);
 
 		if (br_params) {
-			br_params->current_flags = current_flags;
+			bitmap_from_u64(br_params->flags, current_flags);
 			status = MGMT_STATUS_SUCCESS;
 		} else {
 			bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)",
@@ -4453,7 +4459,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
 						le_addr_type(cp->addr.type));
 		if (params) {
-			params->current_flags = current_flags;
+			bitmap_from_u64(params->flags, current_flags);
 			status = MGMT_STATUS_SUCCESS;
 		} else {
 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
@@ -6979,6 +6985,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 	struct hci_conn_params *params;
 	int err;
 	u32 current_flags = 0;
+	u32 supported_flags;
 
 	bt_dev_dbg(hdev, "sock %p", sk);
 
@@ -7050,7 +7057,8 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
 						addr_type);
 		if (params)
-			current_flags = params->current_flags;
+			bitmap_to_arr32(&current_flags, params->flags,
+					__HCI_CONN_NUM_FLAGS);
 	}
 
 	err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL);
@@ -7059,8 +7067,10 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 
 added:
 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
+	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
+			__HCI_CONN_NUM_FLAGS);
 	device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
-			     SUPPORTED_DEVICE_FLAGS(), current_flags);
+			     supported_flags, current_flags);
 
 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
 				MGMT_STATUS_SUCCESS, &cp->addr,
-- 
2.33.1

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

[Luiz Augusto von Dentz]

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(+)

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,
-- 
2.33.1

[PATCH v3 4/4] Bluetooth: hci_sync: Set Privacy Mode when updating the resolving list

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for Set Privacy Mode when updating the resolving list
when HCI_CONN_FLAG_DEVICE_PRIVACY so the controller shall use Device
Mode for devices programmed in the resolving list, Device Mode is
actually required when the remote device are not able to use RPA as
otherwise the default mode is Network Privacy Mode in which only
allows RPAs thus the controller would filter out advertisement using
identity addresses for which there is an IRK.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/hci.h      | 10 +++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_event.c        | 29 ++++++++++++++++++
 net/bluetooth/hci_sync.c         | 51 ++++++++++++++++++++++++++++----
 4 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 84db6b275231..7444d286e6be 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1931,6 +1931,16 @@ struct hci_rp_le_read_transmit_power {
 	__s8  max_le_tx_power;
 } __packed;
 
+#define HCI_NETWORK_PRIVACY		0x00
+#define HCI_DEVICE_PRIVACY		0x01
+
+#define HCI_OP_LE_SET_PRIVACY_MODE	0x204e
+struct hci_cp_le_set_privacy_mode {
+	__u8  bdaddr_type;
+	bdaddr_t  bdaddr;
+	__u8  mode;
+} __packed;
+
 #define HCI_OP_LE_READ_BUFFER_SIZE_V2	0x2060
 struct hci_rp_le_read_buffer_size_v2 {
 	__u8    status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9c94d1c49b25..427c5af9eb41 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -756,6 +756,7 @@ struct hci_conn_params {
 	struct hci_conn *conn;
 	bool explicit_connect;
 	DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
+	u8  privacy_mode;
 };
 
 extern struct list_head hci_dev_list;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index efc5458b1345..51c88f4f1274 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1300,6 +1300,31 @@ static void hci_cc_le_read_transmit_power(struct hci_dev *hdev,
 	hdev->max_le_tx_power = rp->max_le_tx_power;
 }
 
+static void hci_cc_le_set_privacy_mode(struct hci_dev *hdev,
+				       struct sk_buff *skb)
+{
+	__u8 status = *((__u8 *)skb->data);
+	struct hci_cp_le_set_privacy_mode *cp;
+	struct hci_conn_params *params;
+
+	bt_dev_dbg(hdev, "status 0x%2.2x", status);
+
+	if (status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_PRIVACY_MODE);
+	if (!cp)
+		return;
+
+	hci_dev_lock(hdev);
+
+	params = hci_conn_params_lookup(hdev, &cp->bdaddr, cp->bdaddr_type);
+	if (params)
+		params->privacy_mode = cp->mode;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	__u8 *sent, status = *((__u8 *) skb->data);
@@ -3812,6 +3837,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cc_le_read_transmit_power(hdev, skb);
 		break;
 
+	case HCI_OP_LE_SET_PRIVACY_MODE:
+		hci_cc_le_set_privacy_mode(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode);
 		break;
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 5f44ff0b8910..eada6a39068e 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1580,8 +1580,40 @@ static int hci_le_add_resolve_list_sync(struct hci_dev *hdev,
 				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
 }
 
+/* Set Device Privacy Mode. */
+static int hci_le_set_privacy_mode_sync(struct hci_dev *hdev,
+					struct hci_conn_params *params)
+{
+	struct hci_cp_le_set_privacy_mode cp;
+	struct smp_irk *irk;
+
+	/* If device privacy mode has already been set there is nothing to do */
+	if (params->privacy_mode == HCI_DEVICE_PRIVACY)
+		return 0;
+
+	/* Check if HCI_CONN_FLAG_DEVICE_PRIVACY has been set as it also
+	 * indicates that LL Privacy has been enabled and
+	 * HCI_OP_LE_SET_PRIVACY_MODE is supported.
+	 */
+	if (!test_bit(HCI_CONN_FLAG_DEVICE_PRIVACY, params->flags))
+		return 0;
+
+	irk = hci_find_irk_by_addr(hdev, &params->addr, params->addr_type);
+	if (!irk)
+		return 0;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.bdaddr_type = irk->addr_type;
+	bacpy(&cp.bdaddr, &irk->bdaddr);
+	cp.mode = HCI_DEVICE_PRIVACY;
+
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PRIVACY_MODE,
+				     sizeof(cp), &cp, HCI_CMD_TIMEOUT);
+}
+
 /* Adds connection to allow list if needed, if the device uses RPA (has IRK)
- * this attempts to program the device in the resolving list as well.
+ * this attempts to program the device in the resolving list as well and
+ * properly set the privacy mode.
  */
 static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 				       struct hci_conn_params *params,
@@ -1590,11 +1622,6 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 	struct hci_cp_le_add_to_accept_list cp;
 	int err;
 
-	/* Already in accept list */
-	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
-				   params->addr_type))
-		return 0;
-
 	/* Select filter policy to accept all advertising */
 	if (*num_entries >= hdev->le_accept_list_size)
 		return -ENOSPC;
@@ -1620,6 +1647,18 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
 		return err;
 	}
 
+	/* Set Privacy Mode */
+	err = hci_le_set_privacy_mode_sync(hdev, params);
+	if (err) {
+		bt_dev_err(hdev, "Unable to set privacy mode: %d", err);
+		return err;
+	}
+
+	/* Check if already in accept list */
+	if (hci_bdaddr_list_lookup(&hdev->le_accept_list, &params->addr,
+				   params->addr_type))
+		return 0;
+
 	*num_entries += 1;
 	cp.bdaddr_type = params->addr_type;
 	bacpy(&cp.bdaddr, &params->addr);
-- 
2.33.1

Re: [PATCH v3 1/4] Bluetooth: MGMT: Use hci_dev_test_and_{set,clear}_flag

[Marcel Holtmann]

Hi Luiz,

> This make use of hci_dev_test_and_{set,clear}_flag instead of doing 2
> operations in a row.

I really Fixes: tags for these.

> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Fix marking Device Privacy Flag even when adapter is not capable of
> handling Set Privacy Mode.
> v3: Add patch for using hci_dev_test_and_{set,clear}_flag and split
> changes reworking how HCI_CONN_FLAG_REMOTE_WAKEUP is set and make use of
> bitmap to store the supported flags.
> 
> net/bluetooth/mgmt.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index f8f74d344297..0f91bf15e260 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4041,10 +4041,10 @@ static int set_zero_key_func(struct sock *sk, struct hci_dev *hdev,
> #endif
> 
> 	if (hdev && use_ll_privacy(hdev) && !hdev_is_powered(hdev)) {
> -		bool changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> -
> -		hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> +		bool changed;
> 
> +		changed = hci_dev_test_and_clear_flag(hdev,
> +						      HCI_ENABLE_LL_PRIVACY);
> 		if (changed)
> 			exp_ll_privacy_feature_changed(false, hdev, sk);
> 	}
> @@ -4139,15 +4139,15 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
> 	val = !!cp->param[0];
> 
> 	if (val) {
> -		changed = !hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> -		hci_dev_set_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> +		changed = !hci_dev_test_and_set_flag(hdev,
> +						     HCI_ENABLE_LL_PRIVACY);
> 		hci_dev_clear_flag(hdev, HCI_ADVERTISING);
> 
> 		/* Enable LL privacy + supported settings changed */
> 		flags = BIT(0) | BIT(1);
> 	} else {
> -		changed = hci_dev_test_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> -		hci_dev_clear_flag(hdev, HCI_ENABLE_LL_PRIVACY);
> +		changed = hci_dev_test_and_clear_flag(hdev,
> +						      HCI_ENABLE_LL_PRIVACY);
> 
> 		/* Disable LL privacy + supported settings changed */
> 		flags = BIT(1);

Regards

Marcel

Re: [PATCH v3 2/4] Bluetooth: hci_core: Rework hci_conn_params flags

[Marcel Holtmann]

Hi Luiz,

> This reworks hci_conn_params flags to use bitmap_* helpers and add
> support for setting the supported flags in hdev->conn_flags so it can
> easily be accessed.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 48 +++++++++++++++++++-------------
> net/bluetooth/hci_core.c         |  8 +++++-
> net/bluetooth/hci_request.c      |  4 +--
> net/bluetooth/hci_sync.c         |  7 ++---
> net/bluetooth/mgmt.c             | 30 +++++++++++++-------
> 5 files changed, 61 insertions(+), 36 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2560cfe80db8..fc93a1907c90 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -151,22 +151,21 @@ struct bdaddr_list_with_irk {
> 	u8 local_irk[16];
> };
> 
> -struct bdaddr_list_with_flags {
> -	struct list_head list;
> -	bdaddr_t bdaddr;
> -	u8 bdaddr_type;
> -	u32 current_flags;
> -};
> -
> enum hci_conn_flags {
> 	HCI_CONN_FLAG_REMOTE_WAKEUP,
> -	HCI_CONN_FLAG_MAX
> -};
> 
> -#define hci_conn_test_flag(nr, flags) ((flags) & (1U << nr))
> +	__HCI_CONN_NUM_FLAGS,
> +};
> 
> /* Make sure number of flags doesn't exceed sizeof(current_flags) */
> -static_assert(HCI_CONN_FLAG_MAX < 32);
> +static_assert(__HCI_CONN_NUM_FLAGS < 32);
> +
> +struct bdaddr_list_with_flags {
> +	struct list_head list;
> +	bdaddr_t bdaddr;
> +	u8 bdaddr_type;
> +	DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
> +};
> 
> struct bt_uuid {
> 	struct list_head list;
> @@ -559,6 +558,7 @@ struct hci_dev {
> 	struct rfkill		*rfkill;
> 
> 	DECLARE_BITMAP(dev_flags, __HCI_NUM_FLAGS);
> +	DECLARE_BITMAP(conn_flags, __HCI_CONN_NUM_FLAGS);
> 
> 	__s8			adv_tx_power;
> 	__u8			adv_data[HCI_MAX_EXT_AD_LENGTH];
> @@ -754,7 +754,7 @@ struct hci_conn_params {
> 
> 	struct hci_conn *conn;
> 	bool explicit_connect;
> -	u32 current_flags;
> +	DECLARE_BITMAP(flags, __HCI_CONN_NUM_FLAGS);
> };
> 
> extern struct list_head hci_dev_list;
> @@ -762,13 +762,20 @@ extern struct list_head hci_cb_list;
> extern rwlock_t hci_dev_list_lock;
> extern struct mutex hci_cb_list_lock;
> 
> -#define hci_dev_set_flag(hdev, nr)             set_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_clear_flag(hdev, nr)           clear_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_change_flag(hdev, nr)          change_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_test_flag(hdev, nr)            test_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_test_and_set_flag(hdev, nr)    test_and_set_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_test_and_clear_flag(hdev, nr)  test_and_clear_bit((nr), (hdev)->dev_flags)
> -#define hci_dev_test_and_change_flag(hdev, nr) test_and_change_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_set_flag(hdev, nr) \
> +	set_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_clear_flag(hdev, nr) \
> +	clear_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_change_flag(hdev, nr) \
> +	change_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_test_flag(hdev, nr) \
> +	test_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_test_and_set_flag(hdev, nr) \
> +	test_and_set_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_test_and_clear_flag(hdev, nr) \
> +	test_and_clear_bit((nr), (hdev)->dev_flags)
> +#define hci_dev_test_and_change_flag(hdev, nr) \
> +	test_and_change_bit((nr), (hdev)->dev_flags)

actually in these cases, break the 80 column rule.

> 
> #define hci_dev_clear_volatile_flags(hdev)			\
> 	do {							\
> @@ -1465,6 +1472,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))
> +

This shouldn’t be in this patch then.

> /* Use enhanced synchronous connection if command is supported */
> #define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index fdc0dcf8ee36..7c4af0b34b62 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2153,7 +2153,7 @@ int hci_bdaddr_list_add_with_flags(struct list_head *list, bdaddr_t *bdaddr,
> 
> 	bacpy(&entry->bdaddr, bdaddr);
> 	entry->bdaddr_type = type;
> -	entry->current_flags = flags;
> +	bitmap_from_u64(entry->flags, flags);
> 
> 	list_add(&entry->list, list);
> 
> @@ -2629,6 +2629,12 @@ int hci_register_dev(struct hci_dev *hdev)
> 	if (test_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks))
> 		hci_dev_set_flag(hdev, HCI_UNCONFIGURED);
> 
> +	/* Mark Remote Wakeup connection flag as supported if driver has wakeup
> +	 * callback.
> +	 */
> +	if (hdev->wakeup)
> +		set_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, hdev->conn_flags);
> +
> 	hci_sock_dev_event(hdev, HCI_DEV_REG);
> 	hci_dev_hold(hdev);
> 
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 8b3205e4b23e..fee88214606e 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -492,8 +492,8 @@ static int add_to_accept_list(struct hci_request *req,
> 	}
> 
> 	/* During suspend, only wakeable devices can be in accept list */
> -	if (hdev->suspended && !hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
> -						   params->current_flags))
> +	if (hdev->suspended &&
> +	    !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
> 		return 0;
> 
> 	*num_entries += 1;
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index ad86caf41f91..5f44ff0b8910 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1606,8 +1606,8 @@ static int hci_le_add_accept_list_sync(struct hci_dev *hdev,
> 	}
> 
> 	/* During suspend, only wakeable devices can be in acceptlist */
> -	if (hdev->suspended && !hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
> -						   params->current_flags))
> +	if (hdev->suspended &&
> +	    !test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, params->flags))
> 		return 0;
> 
> 	/* Attempt to program the device in the resolving list first to avoid
> @@ -4749,8 +4749,7 @@ static int hci_update_event_filter_sync(struct hci_dev *hdev)
> 	hci_clear_event_filter_sync(hdev);
> 
> 	list_for_each_entry(b, &hdev->accept_list, list) {
> -		if (!hci_conn_test_flag(HCI_CONN_FLAG_REMOTE_WAKEUP,
> -					b->current_flags))
> +		if (!test_bit(HCI_CONN_FLAG_REMOTE_WAKEUP, b->flags))
> 			continue;
> 
> 		bt_dev_dbg(hdev, "Adding event filters for %pMR", &b->bdaddr);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 0f91bf15e260..0f4b620bc630 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4349,8 +4349,6 @@ 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 int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 			    u16 data_len)
> {
> @@ -4358,7 +4356,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;
> 	u32 current_flags = 0;
> 	u8 status = MGMT_STATUS_INVALID_PARAMS;
> 
> @@ -4367,6 +4365,9 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 
> 	hci_dev_lock(hdev);
> 
> +	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
> +			__HCI_CONN_NUM_FLAGS);
> +
> 	memset(&rp, 0, sizeof(rp));
> 
> 	if (cp->addr.type == BDADDR_BREDR) {
> @@ -4376,7 +4377,8 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 		if (!br_params)
> 			goto done;
> 
> -		current_flags = br_params->current_flags;
> +		bitmap_to_arr32(&current_flags, br_params->flags,
> +				__HCI_CONN_NUM_FLAGS);
> 	} else {
> 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> 						le_addr_type(cp->addr.type));
> @@ -4384,7 +4386,8 @@ static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 		if (!params)
> 			goto done;
> 
> -		current_flags = params->current_flags;
> +		bitmap_to_arr32(&current_flags, params->flags,
> +				__HCI_CONN_NUM_FLAGS);
> 	}
> 
> 	bacpy(&rp.addr.bdaddr, &cp->addr.bdaddr);
> @@ -4422,13 +4425,16 @@ 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;
> 	u32 current_flags = __le32_to_cpu(cp->current_flags);
> 
> 	bt_dev_dbg(hdev, "Set device flags %pMR (type 0x%x) = 0x%x",
> 		   &cp->addr.bdaddr, cp->addr.type,
> 		   __le32_to_cpu(current_flags));
> 
> +	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
> +			__HCI_CONN_NUM_FLAGS);
> +
> 	if ((supported_flags | current_flags) != supported_flags) {
> 		bt_dev_warn(hdev, "Bad flag given (0x%x) vs supported (0x%0x)",
> 			    current_flags, supported_flags);
> @@ -4443,7 +4449,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 							      cp->addr.type);
> 
> 		if (br_params) {
> -			br_params->current_flags = current_flags;
> +			bitmap_from_u64(br_params->flags, current_flags);
> 			status = MGMT_STATUS_SUCCESS;
> 		} else {
> 			bt_dev_warn(hdev, "No such BR/EDR device %pMR (0x%x)",
> @@ -4453,7 +4459,7 @@ static int set_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> 						le_addr_type(cp->addr.type));
> 		if (params) {
> -			params->current_flags = current_flags;
> +			bitmap_from_u64(params->flags, current_flags);
> 			status = MGMT_STATUS_SUCCESS;
> 		} else {
> 			bt_dev_warn(hdev, "No such LE device %pMR (0x%x)",
> @@ -6979,6 +6985,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 	struct hci_conn_params *params;
> 	int err;
> 	u32 current_flags = 0;
> +	u32 supported_flags;
> 
> 	bt_dev_dbg(hdev, "sock %p", sk);
> 
> @@ -7050,7 +7057,8 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 		params = hci_conn_params_lookup(hdev, &cp->addr.bdaddr,
> 						addr_type);
> 		if (params)
> -			current_flags = params->current_flags;
> +			bitmap_to_arr32(&current_flags, params->flags,
> +					__HCI_CONN_NUM_FLAGS);
> 	}
> 
> 	err = hci_cmd_sync_queue(hdev, add_device_sync, NULL, NULL);
> @@ -7059,8 +7067,10 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 
> added:
> 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> +	bitmap_to_arr32(&supported_flags, hdev->conn_flags,
> +			__HCI_CONN_NUM_FLAGS);
> 	device_flags_changed(NULL, hdev, &cp->addr.bdaddr, cp->addr.type,
> -			     SUPPORTED_DEVICE_FLAGS(), current_flags);
> +			     supported_flags, current_flags);
> 
> 	err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
> 				MGMT_STATUS_SUCCESS, &cp->addr,

Regards

Marcel

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

[Marcel Holtmann]

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

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

[Luiz Augusto von Dentz]

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.

-- 
Luiz Augusto von Dentz

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

[Marcel Holtmann]

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

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

[Luiz Augusto von Dentz]

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
>


-- 
Luiz Augusto von Dentz

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

[Marcel Holtmann]

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