[PATCH v3] Bluetooth: Configure controller address resolution if available

[PATCH v3] Bluetooth: Configure controller address resolution if available

[Marcel Holtmann]

When the LL Privacy support is available, then as part of enabling or
disabling passive background scanning, it is required to set up the
controller based address resolution as well.

Since only passive background scanning is utilizing the whitelist, the
address resolution is now bound to the whitelist and passive background
scanning. All other resolution can be easily done by the host stack.

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci.h      |  1 +
 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/hci_request.c      | 26 +++++++++++++++++++++++---
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 5ef4547760db..58360538d42b 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -459,6 +459,7 @@ enum {
 #define HCI_LE_SLAVE_FEATURES		0x08
 #define HCI_LE_PING			0x10
 #define HCI_LE_DATA_LEN_EXT		0x20
+#define HCI_LE_LL_PRIVACY		0x40
 #define HCI_LE_EXT_SCAN_POLICY		0x80
 #define HCI_LE_PHY_2M			0x01
 #define HCI_LE_PHY_CODED		0x08
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2f3275f1d1c4..663ffde9bd1d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -645,6 +645,7 @@ extern struct mutex hci_cb_list_lock;
 	do {							\
 		hci_dev_clear_flag(hdev, HCI_LE_SCAN);		\
 		hci_dev_clear_flag(hdev, HCI_LE_ADV);		\
+		hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\
 		hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ);	\
 	} while (0)
 
@@ -1277,6 +1278,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define scan_coded(dev) (((dev)->le_tx_def_phys & HCI_LE_SET_PHY_CODED) || \
 			 ((dev)->le_rx_def_phys & HCI_LE_SET_PHY_CODED))
 
+/* Use LL Privacy based address resolution if supported */
+#define use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
+
 /* Use ext scanning if set ext scan param and ext scan enable is supported */
 #define use_ext_scan(dev) (((dev)->commands[37] & 0x20) && \
 			   ((dev)->commands[37] & 0x40))
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 9ea40106ef17..efec2a0bb824 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -676,6 +676,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
 		cp.enable = LE_SCAN_DISABLE;
 		hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
 	}
+
+	if (use_ll_privacy(hdev) &&
+	    hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION)) {
+		__u8 enable = 0x00;
+		hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
+	}
 }
 
 static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
@@ -808,10 +814,16 @@ static bool scan_use_rpa(struct hci_dev *hdev)
 }
 
 static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
-			       u16 window, u8 own_addr_type, u8 filter_policy)
+			       u16 window, u8 own_addr_type, u8 filter_policy,
+			       bool addr_resolv)
 {
 	struct hci_dev *hdev = req->hdev;
 
+	if (use_ll_privacy(hdev) && addr_resolv) {
+		__u8 enable = 0x01;
+		hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
+	}
+
 	/* Use ext scanning if set ext scan param and ext scan enable is
 	 * supported
 	 */
@@ -885,12 +897,18 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
 	}
 }
 
+/* Ensure to call hci_req_add_le_scan_disable() first to disable the
+ * controller based address resolution to be able to reconfigure
+ * resolving list.
+ */
 void hci_req_add_le_passive_scan(struct hci_request *req)
 {
 	struct hci_dev *hdev = req->hdev;
 	u8 own_addr_type;
 	u8 filter_policy;
 	u8 window, interval;
+	/* Background scanning should run with address resolution */
+	bool addr_resolv = true;
 
 	if (hdev->scanning_paused) {
 		bt_dev_dbg(hdev, "Scanning is paused for suspend");
@@ -936,7 +954,7 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
 
 	bt_dev_dbg(hdev, "LE passive scan with whitelist = %d", filter_policy);
 	hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
-			   own_addr_type, filter_policy);
+			   own_addr_type, filter_policy, addr_resolv);
 }
 
 static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
@@ -2725,6 +2743,8 @@ static int active_scan(struct hci_request *req, unsigned long opt)
 	u8 own_addr_type;
 	/* White list is not used for discovery */
 	u8 filter_policy = 0x00;
+	/* Discovery doesn't require controller address resolution */
+	bool addr_resolv = false;
 	int err;
 
 	BT_DBG("%s", hdev->name);
@@ -2746,7 +2766,7 @@ static int active_scan(struct hci_request *req, unsigned long opt)
 		own_addr_type = ADDR_LE_DEV_PUBLIC;
 
 	hci_req_start_scan(req, LE_SCAN_ACTIVE, interval, DISCOV_LE_SCAN_WIN,
-			   own_addr_type, filter_policy);
+			   own_addr_type, filter_policy, addr_resolv);
 	return 0;
 }
 
-- 
2.25.2

Re: [PATCH v3] Bluetooth: Configure controller address resolution if available

[Abhishek Pandit-Subedi]

Hi Marcel,

This looks good to me.

Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>


On Tue, Apr 7, 2020 at 1:52 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> When the LL Privacy support is available, then as part of enabling or
> disabling passive background scanning, it is required to set up the
> controller based address resolution as well.
>
> Since only passive background scanning is utilizing the whitelist, the
> address resolution is now bound to the whitelist and passive background
> scanning. All other resolution can be easily done by the host stack.
>
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/hci.h      |  1 +
>  include/net/bluetooth/hci_core.h |  4 ++++
>  net/bluetooth/hci_request.c      | 26 +++++++++++++++++++++++---
>  3 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 5ef4547760db..58360538d42b 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -459,6 +459,7 @@ enum {
>  #define HCI_LE_SLAVE_FEATURES          0x08
>  #define HCI_LE_PING                    0x10
>  #define HCI_LE_DATA_LEN_EXT            0x20
> +#define HCI_LE_LL_PRIVACY              0x40
>  #define HCI_LE_EXT_SCAN_POLICY         0x80
>  #define HCI_LE_PHY_2M                  0x01
>  #define HCI_LE_PHY_CODED               0x08
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2f3275f1d1c4..663ffde9bd1d 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -645,6 +645,7 @@ extern struct mutex hci_cb_list_lock;
>         do {                                                    \
>                 hci_dev_clear_flag(hdev, HCI_LE_SCAN);          \
>                 hci_dev_clear_flag(hdev, HCI_LE_ADV);           \
> +               hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\
>                 hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ);     \
>         } while (0)
>
> @@ -1277,6 +1278,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>  #define scan_coded(dev) (((dev)->le_tx_def_phys & HCI_LE_SET_PHY_CODED) || \
>                          ((dev)->le_rx_def_phys & HCI_LE_SET_PHY_CODED))
>
> +/* Use LL Privacy based address resolution if supported */
> +#define use_ll_privacy(dev) ((dev)->le_features[0] & HCI_LE_LL_PRIVACY)
> +
>  /* Use ext scanning if set ext scan param and ext scan enable is supported */
>  #define use_ext_scan(dev) (((dev)->commands[37] & 0x20) && \
>                            ((dev)->commands[37] & 0x40))
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 9ea40106ef17..efec2a0bb824 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -676,6 +676,12 @@ void hci_req_add_le_scan_disable(struct hci_request *req)
>                 cp.enable = LE_SCAN_DISABLE;
>                 hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>         }
> +
> +       if (use_ll_privacy(hdev) &&
> +           hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION)) {
> +               __u8 enable = 0x00;
> +               hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
> +       }
>  }
>
>  static void del_from_white_list(struct hci_request *req, bdaddr_t *bdaddr,
> @@ -808,10 +814,16 @@ static bool scan_use_rpa(struct hci_dev *hdev)
>  }
>
>  static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
> -                              u16 window, u8 own_addr_type, u8 filter_policy)
> +                              u16 window, u8 own_addr_type, u8 filter_policy,
> +                              bool addr_resolv)
>  {
>         struct hci_dev *hdev = req->hdev;
>
> +       if (use_ll_privacy(hdev) && addr_resolv) {
> +               __u8 enable = 0x01;
> +               hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
> +       }
> +
>         /* Use ext scanning if set ext scan param and ext scan enable is
>          * supported
>          */
> @@ -885,12 +897,18 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
>         }
>  }
>
> +/* Ensure to call hci_req_add_le_scan_disable() first to disable the
> + * controller based address resolution to be able to reconfigure
> + * resolving list.
> + */
>  void hci_req_add_le_passive_scan(struct hci_request *req)
>  {
>         struct hci_dev *hdev = req->hdev;
>         u8 own_addr_type;
>         u8 filter_policy;
>         u8 window, interval;
> +       /* Background scanning should run with address resolution */
> +       bool addr_resolv = true;
>
>         if (hdev->scanning_paused) {
>                 bt_dev_dbg(hdev, "Scanning is paused for suspend");
> @@ -936,7 +954,7 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
>
>         bt_dev_dbg(hdev, "LE passive scan with whitelist = %d", filter_policy);
>         hci_req_start_scan(req, LE_SCAN_PASSIVE, interval, window,
> -                          own_addr_type, filter_policy);
> +                          own_addr_type, filter_policy, addr_resolv);
>  }
>
>  static u8 get_adv_instance_scan_rsp_len(struct hci_dev *hdev, u8 instance)
> @@ -2725,6 +2743,8 @@ static int active_scan(struct hci_request *req, unsigned long opt)
>         u8 own_addr_type;
>         /* White list is not used for discovery */
>         u8 filter_policy = 0x00;
> +       /* Discovery doesn't require controller address resolution */
> +       bool addr_resolv = false;
>         int err;
>
>         BT_DBG("%s", hdev->name);
> @@ -2746,7 +2766,7 @@ static int active_scan(struct hci_request *req, unsigned long opt)
>                 own_addr_type = ADDR_LE_DEV_PUBLIC;
>
>         hci_req_start_scan(req, LE_SCAN_ACTIVE, interval, DISCOV_LE_SCAN_WIN,
> -                          own_addr_type, filter_policy);
> +                          own_addr_type, filter_policy, addr_resolv);
>         return 0;
>  }
>
> --
> 2.25.2
>

Re: [PATCH v3] Bluetooth: Configure controller address resolution if available

[Marcel Holtmann]

Hi Abhishek,

> This looks good to me.
> 
> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

however it is not enough, we also have to enable address resolution before calling LE Create Connection. It is actually a bit tricky to enable / disable address resolution correctly.

Regards

Marcel

Re: [PATCH v3] Bluetooth: Configure controller address resolution if available

[Sathish Narasimman]

Hi Marcel

On Wed, Apr 8, 2020 at 11:46 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > This looks good to me.
> >
> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> however it is not enough, we also have to enable address resolution before calling LE Create Connection. It is actually a bit tricky to enable / disable address resolution correctly.
When we receive directed_adv we disable the scan. which disables
address_resolution.
immediately I was trying to enable address resolution inside
hci_req_add_le_create_conn
@@ -813,6 +813,12 @@ static void hci_req_add_le_create_conn(struct
hci_request *req,
                        return;
        }

+       if (use_ll_privacy(hdev) &&
+           !hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION)) {
+               __u8 enable = 0x01;
+               hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
+       }
+
        if (use_ext_conn(hdev)) {
                struct hci_cp_le_ext_create_conn *cp;
                struct hci_cp_le_ext_conn_param *p;
where this fails as the flag is not cleared yet.
where the idea is to bool addr_resolv as below
->hci_req_add_le_scan_disable(req, addr_resolv)
In which we can stop disabling addr_resolution and continue
le_create_conn during hci_connect_le
>
> Regards
>
> Marcel
>

Regards
Sathish N

Re: [PATCH v3] Bluetooth: Configure controller address resolution if available

[Sathish Narasimman]

Hi marcel

On Tue, May 5, 2020 at 7:14 PM Sathish Narasimman <nsathish41@gmail.com> wrote:
>
> Hi Marcel
>
> On Wed, Apr 8, 2020 at 11:46 AM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Abhishek,
> >
> > > This looks good to me.
> > >
> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >
> > however it is not enough, we also have to enable address resolution before calling LE Create Connection. It is actually a bit tricky to enable / disable address resolution correctly.
> When we receive directed_adv we disable the scan. which disables
> address_resolution.
> immediately I was trying to enable address resolution inside
> hci_req_add_le_create_conn
> @@ -813,6 +813,12 @@ static void hci_req_add_le_create_conn(struct
> hci_request *req,
>                         return;
>         }
>
> +       if (use_ll_privacy(hdev) &&
> +           !hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION)) {
> +               __u8 enable = 0x01;
> +               hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
> +       }
> +
>         if (use_ext_conn(hdev)) {
>                 struct hci_cp_le_ext_create_conn *cp;
>                 struct hci_cp_le_ext_conn_param *p;
> where this fails as the flag is not cleared yet.
> where the idea is to bool addr_resolv as below
> ->hci_req_add_le_scan_disable(req, addr_resolv)
> In which we can stop disabling addr_resolution and continue
> le_create_conn during hci_connect_le
Uploaded patch v2 please review

> >
> > Regards
> >
> > Marcel
> >
>
> Regards
> Sathish N

Regards
Sathish N