[PATCH v2 1/4] Bluetooth: Refactor advertising report processing into its own function

[PATCH v2 1/4] Bluetooth: Refactor advertising report processing into its own function

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

As preparation for merging ADV_IND/ADV_SCAN_IND and SCAN_RSP together
into a single mgmt Device Found event refactor individual advertising
report handling into a separate function. This will help keep the code
more readable as more logic gets added.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 9ee081b9c064..43872af20aa4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3941,25 +3941,30 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 	}
 }
 
+static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
+			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
+{
+	if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
+		check_pending_le_conn(hdev, bdaddr, bdaddr_type);
+
+	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
+			  data, len);
+}
+
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	u8 num_reports = skb->data[0];
 	void *ptr = &skb->data[1];
-	s8 rssi;
 
 	hci_dev_lock(hdev);
 
 	while (num_reports--) {
 		struct hci_ev_le_advertising_info *ev = ptr;
-
-		if (ev->evt_type == LE_ADV_IND ||
-		    ev->evt_type == LE_ADV_DIRECT_IND)
-			check_pending_le_conn(hdev, &ev->bdaddr,
-					      ev->bdaddr_type);
+		s8 rssi;
 
 		rssi = ev->data[ev->length];
-		mgmt_device_found(hdev, &ev->bdaddr, LE_LINK, ev->bdaddr_type,
-				  NULL, rssi, 0, 1, ev->data, ev->length);
+		process_adv_report(hdev, ev->evt_type, &ev->bdaddr,
+				   ev->bdaddr_type, rssi, ev->data, ev->length);
 
 		ptr += sizeof(*ev) + ev->length + 1;
 	}
-- 
1.8.5.3

[PATCH v2 2/4] Bluetooth: Don't send device found events during passive scanning

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Passive LE scanning is only used by the kernel-internal connection
establishment procedure. It makes therefore little sense to send device
found events to user space.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_event.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 43872af20aa4..403c1d96331a 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3944,8 +3944,12 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
 {
-	if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
-		check_pending_le_conn(hdev, bdaddr, bdaddr_type);
+	/* Passive scanning shouldn't trigger any device found events */
+	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
+		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
+			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
+	    return;
+	}
 
 	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
 			  data, len);
-- 
1.8.5.3

Re: [PATCH v2 2/4] Bluetooth: Don't send device found events during passive scanning

[Andre Guedes]

Hi Johan,

On 03/24/2014 05:48 AM, johan.hedberg@gmail.com wrote:
> From: Johan Hedberg <johan.hedberg@intel.com>
>
> Passive LE scanning is only used by the kernel-internal connection
> establishment procedure. It makes therefore little sense to send device
> found events to user space.

Not sure this patch is necessary. This feature is already garanteed by 
this patch 12602d0cc005354a519b3eba443d7912ab71313a .

> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>   net/bluetooth/hci_event.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 43872af20aa4..403c1d96331a 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3944,8 +3944,12 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
>   static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
>   			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
>   {
> -	if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> -		check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> +	/* Passive scanning shouldn't trigger any device found events */
> +	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> +		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> +			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> +	    return;
> +	}

Additionally, as a side effect of this change, automatic connection 
won't be establish while discovery is running anymore. So it can add 
even more delay to our reconnection procedure (10.24 seconds max). Did 
you considered this?

BR,

Andre

Re: [PATCH v2 2/4] Bluetooth: Don't send device found events during passive scanning

[Johan Hedberg]

Hi Andre,

On Mon, Mar 24, 2014, Andre Guedes wrote:
> On 03/24/2014 05:48 AM, johan.hedberg@gmail.com wrote:
> >From: Johan Hedberg <johan.hedberg@intel.com>
> >
> >Passive LE scanning is only used by the kernel-internal connection
> >establishment procedure. It makes therefore little sense to send device
> >found events to user space.
> 
> Not sure this patch is necessary. This feature is already garanteed
> by this patch 12602d0cc005354a519b3eba443d7912ab71313a .

First of all, I'm glad you're reviewing my patches :)

Secondly, yes I'm aware of that other patch and discovery check. I added
this extra one for clarity and for simplifying the logic of the
advertising report processing function (so it can assume that we're
doing active scanning once we've gone past this first if-statement).

> >diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >index 43872af20aa4..403c1d96331a 100644
> >--- a/net/bluetooth/hci_event.c
> >+++ b/net/bluetooth/hci_event.c
> >@@ -3944,8 +3944,12 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> >  static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> >  			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
> >  {
> >-	if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> >-		check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> >+	/* Passive scanning shouldn't trigger any device found events */
> >+	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> >+		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> >+			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> >+	    return;
> >+	}
> 
> Additionally, as a side effect of this change, automatic connection
> won't be establish while discovery is running anymore. So it can add
> even more delay to our reconnection procedure (10.24 seconds max).
> Did you considered this?

Yes, I've considered it, however no one seems to be 100% sure either way
what is the best policy. Either you abort the discovery procedure the
user has started in hopes of pairing/connecting a new device, or you
delay the connection to an existing device. For now, I decided to be
consistent with what the "old" user space based reconnection code is
doing, i.e. ignoring events while active discovery has been requested.

Johan

Re: [PATCH v2 2/4] Bluetooth: Don't send device found events during passive scanning

[Marcel Holtmann]

Hi Johan,

> Passive LE scanning is only used by the kernel-internal connection
> establishment procedure. It makes therefore little sense to send device
> found events to user space.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_event.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 43872af20aa4..403c1d96331a 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3944,8 +3944,12 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> 			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
> {
> -	if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> -		check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> +	/* Passive scanning shouldn't trigger any device found events */
> +	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> +		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> +			check_pending_le_conn(hdev, bdaddr, bdaddr_type);
> +	    return;

I still have no idea on how you are indenting this return statement ;)

> +	}
> 
> 	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
> 			  data, len);

Regards

Marcel

[PATCH v2 3/4] Bluetooth: Add scan_rsp parameter to mgmt_device_found()

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

In preparation for being able to merge ADV_IND/ADV_SCAN_IND and SCAN_RSP
together into a single device found event add a second parameter to the
mgmt_device_found function. For now all callers pass NULL as this
parameters since we don't yet have caching of the last received
advertising report.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  5 +++--
 net/bluetooth/hci_event.c        | 10 +++++-----
 net/bluetooth/mgmt.c             | 10 +++++++---
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5f8bc05694ac..a1b8eab8a47d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1264,8 +1264,9 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
 				       u8 *randomizer192, u8 *hash256,
 				       u8 *randomizer256, u8 status);
 void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
-		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name,
-		       u8 ssp, u8 *eir, u16 eir_len);
+		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
+		       ssp, u8 *eir, u16 eir_len, u8 *scan_rsp,
+		       u8 scan_rsp_len);
 void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		      u8 addr_type, s8 rssi, u8 *name, u8 name_len);
 void mgmt_discovering(struct hci_dev *hdev, u8 discovering);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 403c1d96331a..e7e9c1688402 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1797,7 +1797,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		name_known = hci_inquiry_cache_update(hdev, &data, false, &ssp);
 		mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
 				  info->dev_class, 0, !name_known, ssp, NULL,
-				  0);
+				  0, NULL, 0);
 	}
 
 	hci_dev_unlock(hdev);
@@ -3068,7 +3068,7 @@ static void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev,
 							      false, &ssp);
 			mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
 					  info->dev_class, info->rssi,
-					  !name_known, ssp, NULL, 0);
+					  !name_known, ssp, NULL, 0, NULL, 0);
 		}
 	} else {
 		struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
@@ -3086,7 +3086,7 @@ static void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev,
 							      false, &ssp);
 			mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
 					  info->dev_class, info->rssi,
-					  !name_known, ssp, NULL, 0);
+					  !name_known, ssp, NULL, 0, NULL, 0);
 		}
 	}
 
@@ -3275,7 +3275,7 @@ static void hci_extended_inquiry_result_evt(struct hci_dev *hdev,
 		eir_len = eir_get_length(info->data, sizeof(info->data));
 		mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
 				  info->dev_class, info->rssi, !name_known,
-				  ssp, info->data, eir_len);
+				  ssp, info->data, eir_len, NULL, 0);
 	}
 
 	hci_dev_unlock(hdev);
@@ -3952,7 +3952,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 	}
 
 	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
-			  data, len);
+			  data, len, NULL, 0);
 }
 
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d2d4e0d5aed0..360822554e07 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5669,7 +5669,8 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
 
 void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
-		       ssp, u8 *eir, u16 eir_len)
+		       ssp, u8 *eir, u16 eir_len, u8 *scan_rsp,
+		       u8 scan_rsp_len)
 {
 	char buf[512];
 	struct mgmt_ev_device_found *ev = (void *) buf;
@@ -5707,8 +5708,11 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		eir_len = eir_append_data(ev->eir, eir_len, EIR_CLASS_OF_DEV,
 					  dev_class, 3);
 
-	ev->eir_len = cpu_to_le16(eir_len);
-	ev_size = sizeof(*ev) + eir_len;
+	if (scan_rsp_len > 0)
+		memcpy(ev->eir + eir_len, scan_rsp, scan_rsp_len);
+
+	ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
+	ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
 
 	mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
 }
-- 
1.8.5.3

Re: [PATCH v2 3/4] Bluetooth: Add scan_rsp parameter to mgmt_device_found()

[Marcel Holtmann]

Hi Johan,

> In preparation for being able to merge ADV_IND/ADV_SCAN_IND and SCAN_RSP
> together into a single device found event add a second parameter to the
> mgmt_device_found function. For now all callers pass NULL as this
> parameters since we don't yet have caching of the last received
> advertising report.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  5 +++--
> net/bluetooth/hci_event.c        | 10 +++++-----
> net/bluetooth/mgmt.c             | 10 +++++++---
> 3 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5f8bc05694ac..a1b8eab8a47d 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1264,8 +1264,9 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
> 				       u8 *randomizer192, u8 *hash256,
> 				       u8 *randomizer256, u8 status);
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> -		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name,
> -		       u8 ssp, u8 *eir, u16 eir_len);
> +		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
> +		       ssp, u8 *eir, u16 eir_len, u8 *scan_rsp,
> +		       u8 scan_rsp_len);
> void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 		      u8 addr_type, s8 rssi, u8 *name, u8 name_len);
> void mgmt_discovering(struct hci_dev *hdev, u8 discovering);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 403c1d96331a..e7e9c1688402 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1797,7 +1797,7 @@ static void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		name_known = hci_inquiry_cache_update(hdev, &data, false, &ssp);
> 		mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
> 				  info->dev_class, 0, !name_known, ssp, NULL,
> -				  0);
> +				  0, NULL, 0);
> 	}
> 
> 	hci_dev_unlock(hdev);
> @@ -3068,7 +3068,7 @@ static void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev,
> 							      false, &ssp);
> 			mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
> 					  info->dev_class, info->rssi,
> -					  !name_known, ssp, NULL, 0);
> +					  !name_known, ssp, NULL, 0, NULL, 0);
> 		}
> 	} else {
> 		struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
> @@ -3086,7 +3086,7 @@ static void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev,
> 							      false, &ssp);
> 			mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
> 					  info->dev_class, info->rssi,
> -					  !name_known, ssp, NULL, 0);
> +					  !name_known, ssp, NULL, 0, NULL, 0);
> 		}
> 	}
> 
> @@ -3275,7 +3275,7 @@ static void hci_extended_inquiry_result_evt(struct hci_dev *hdev,
> 		eir_len = eir_get_length(info->data, sizeof(info->data));
> 		mgmt_device_found(hdev, &info->bdaddr, ACL_LINK, 0x00,
> 				  info->dev_class, info->rssi, !name_known,
> -				  ssp, info->data, eir_len);
> +				  ssp, info->data, eir_len, NULL, 0);
> 	}
> 
> 	hci_dev_unlock(hdev);
> @@ -3952,7 +3952,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> 	}
> 
> 	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
> -			  data, len);
> +			  data, len, NULL, 0);
> }
> 
> static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index d2d4e0d5aed0..360822554e07 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5669,7 +5669,8 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
> 
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 		       u8 addr_type, u8 *dev_class, s8 rssi, u8 cfm_name, u8
> -		       ssp, u8 *eir, u16 eir_len)
> +		       ssp, u8 *eir, u16 eir_len, u8 *scan_rsp,
> +		       u8 scan_rsp_len)
> {
> 	char buf[512];
> 	struct mgmt_ev_device_found *ev = (void *) buf;
> @@ -5707,8 +5708,11 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 		eir_len = eir_append_data(ev->eir, eir_len, EIR_CLASS_OF_DEV,
> 					  dev_class, 3);

        /* Leave 5 bytes for a potential CoD field */                            
        if (sizeof(*ev) + eir_len + 5 > sizeof(buf))                             
                return;       

doesn’t this check also need to be adjusted?

> 
> -	ev->eir_len = cpu_to_le16(eir_len);
> -	ev_size = sizeof(*ev) + eir_len;
> +	if (scan_rsp_len > 0)
> +		memcpy(ev->eir + eir_len, scan_rsp, scan_rsp_len);
> +
> +	ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> +	ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
> 
> 	mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> }

Regards

Marcel

[PATCH v2 4/4] Bluetooth: Merge ADV_IND/ADV_SCAN_IND and SCAN_RSP together

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

To avoid too many events being sent to user space and to help parsing of
all available remote device data it makes sense for us to wait for the
scan response and send a single merged Device Found event to user space.

This patch adds a few new variables to hci_dev to track the last
received ADV_IND/ADV_SCAN_IND, i.e. those which will cause a SCAN_REQ to
be send in the case of active scanning. When the SCAN_RSP is received
the cached data is passed together with the SCAN_RSP to the
mgmt_device_found function which takes care of merging them into a
single Device Found event.

We also need a bit of extra logic to handle situations where we don't
receive a SCAN_RSP after caching some data. In such a scenario we simply
have to send out the cached data as it is and then operate on the new
report as if the cache had been empty.

We also need to send out any pending cached data when scanning stops as
well as ensure that the cache is empty at the start of a new active
scanning session. These both cases are covered by the update to the
hci_cc_le_set_scan_enable function in this patch.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  4 +++
 net/bluetooth/hci_event.c        | 76 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a1b8eab8a47d..59b112397d39 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -68,6 +68,10 @@ struct discovery_state {
 	struct list_head	unknown;	/* Name state not known */
 	struct list_head	resolve;	/* Name needs to be resolved */
 	__u32			timestamp;
+	bdaddr_t		last_adv_addr;
+	u8			last_adv_addr_type;
+	u8			last_adv_data[HCI_MAX_AD_LENGTH];
+	u8			last_adv_data_len;
 };
 
 struct hci_conn_hash {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e7e9c1688402..addc44f28c87 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1018,6 +1018,12 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_dev_unlock(hdev);
 }
 
+#define ADV_CACHE_EMPTY(d)	(!bacmp(&(d)->last_adv_addr, BDADDR_ANY))
+#define ADV_CACHE_CLEAR(d)	do { \
+					bacpy(&(d)->last_adv_addr, BDADDR_ANY); \
+					(d)->last_adv_data_len = 0; \
+				} while (0)
+
 static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 				      struct sk_buff *skb)
 {
@@ -1036,9 +1042,21 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 	switch (cp->enable) {
 	case LE_SCAN_ENABLE:
 		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
+		if (hdev->le_scan_type == LE_SCAN_ACTIVE)
+			ADV_CACHE_CLEAR(&hdev->discovery);
 		break;
 
 	case LE_SCAN_DISABLE:
+		if (!ADV_CACHE_EMPTY(&hdev->discovery)) {
+			struct discovery_state *d = &hdev->discovery;
+
+			mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
+						  d->last_adv_addr_type, NULL,
+						  0, 0, 1, d->last_adv_data,
+						  d->last_adv_data_len,
+						  NULL, 0);
+		}
+
 		/* Cancel this timer so that we don't try to disable scanning
 		 * when it's already disabled.
 		 */
@@ -3944,6 +3962,8 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
 static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
 {
+	struct discovery_state *d = &hdev->discovery;
+
 	/* Passive scanning shouldn't trigger any device found events */
 	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
 		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
@@ -3951,8 +3971,60 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 	    return;
 	}
 
-	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
-			  data, len, NULL, 0);
+	/* If there's nothing cached either cache the data from this
+	 * event or send an immediate device found event if the data is
+	 * not cachable.
+	 */
+	if (ADV_CACHE_EMPTY(d)) {
+		if (type == LE_ADV_IND || type == LE_ADV_SCAN_IND)
+			goto cache_data;
+
+		mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL,
+				  rssi, 0, 1, data, len, NULL, 0);
+		return;
+	}
+
+	/* If the cached data doesn't match this report or this isn't a
+	 * scan response (e.g. we got a duplicate ADV_IND) then force
+	 * sending of the cached data.
+	 */
+	if (type != LE_ADV_SCAN_RSP || bacmp(bdaddr, &d->last_adv_addr) ||
+	    bdaddr_type != d->last_adv_addr_type) {
+		/* Send out whatever is in the cache */
+		mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
+				  d->last_adv_addr_type, NULL, 0, 0, 1,
+				  d->last_adv_data, d->last_adv_data_len,
+				  NULL, 0);
+
+		/* If the new event should be cached store it there */
+		if (type == LE_ADV_IND || type == LE_ADV_SCAN_IND)
+			goto cache_data;
+
+		/* The event can't be cached so clear the cache and send
+		 * an immediate event based on this report.
+		 */
+		ADV_CACHE_CLEAR(d);
+		mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
+				  d->last_adv_addr_type, NULL, rssi, 0, 1,
+				  data, len, NULL, 0);
+		return;
+	}
+
+	/* If we get here we've got a cached ADV_IND or ADV_SCAN_IND and
+	 * the new event is a SCAN_RSP. We can therefore proceed with
+	 * sending a merged device found event.
+	 */
+	mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
+			  d->last_adv_addr_type, NULL, rssi, 0, 1, data, len,
+			  d->last_adv_data, d->last_adv_data_len);
+	ADV_CACHE_CLEAR(d);
+	return;
+
+cache_data:
+	bacpy(&d->last_adv_addr, bdaddr);
+	d->last_adv_addr_type = bdaddr_type;
+	memcpy(d->last_adv_data, data, len);
+	d->last_adv_data_len = len;
 }
 
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
-- 
1.8.5.3

Re: [PATCH v2 4/4] Bluetooth: Merge ADV_IND/ADV_SCAN_IND and SCAN_RSP together

[Marcel Holtmann]

Hi Johan,

> To avoid too many events being sent to user space and to help parsing of
> all available remote device data it makes sense for us to wait for the
> scan response and send a single merged Device Found event to user space.
> 
> This patch adds a few new variables to hci_dev to track the last
> received ADV_IND/ADV_SCAN_IND, i.e. those which will cause a SCAN_REQ to
> be send in the case of active scanning. When the SCAN_RSP is received
> the cached data is passed together with the SCAN_RSP to the
> mgmt_device_found function which takes care of merging them into a
> single Device Found event.
> 
> We also need a bit of extra logic to handle situations where we don't
> receive a SCAN_RSP after caching some data. In such a scenario we simply
> have to send out the cached data as it is and then operate on the new
> report as if the cache had been empty.
> 
> We also need to send out any pending cached data when scanning stops as
> well as ensure that the cache is empty at the start of a new active
> scanning session. These both cases are covered by the update to the
> hci_cc_le_set_scan_enable function in this patch.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  4 +++
> net/bluetooth/hci_event.c        | 76 ++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index a1b8eab8a47d..59b112397d39 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -68,6 +68,10 @@ struct discovery_state {
> 	struct list_head	unknown;	/* Name state not known */
> 	struct list_head	resolve;	/* Name needs to be resolved */
> 	__u32			timestamp;
> +	bdaddr_t		last_adv_addr;
> +	u8			last_adv_addr_type;
> +	u8			last_adv_data[HCI_MAX_AD_LENGTH];
> +	u8			last_adv_data_len;
> };
> 
> struct hci_conn_hash {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index e7e9c1688402..addc44f28c87 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1018,6 +1018,12 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
> 	hci_dev_unlock(hdev);
> }
> 
> +#define ADV_CACHE_EMPTY(d)	(!bacmp(&(d)->last_adv_addr, BDADDR_ANY))

maybe using something like has_pending_adv_report() is a bit better. I have a bit of a weird feeling see ADV_CACHE_EMPTY calls since it is not really a cache.

> +#define ADV_CACHE_CLEAR(d)	do { \
> +					bacpy(&(d)->last_adv_addr, BDADDR_ANY); \
> +					(d)->last_adv_data_len = 0; \
> +				} while (0)

clear_pending_adv_report() seems like a bit better name.

> +
> static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> 				      struct sk_buff *skb)
> {
> @@ -1036,9 +1042,21 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> 	switch (cp->enable) {
> 	case LE_SCAN_ENABLE:
> 		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
> +		if (hdev->le_scan_type == LE_SCAN_ACTIVE)
> +			ADV_CACHE_CLEAR(&hdev->discovery);
> 		break;
> 
> 	case LE_SCAN_DISABLE:
> +		if (!ADV_CACHE_EMPTY(&hdev->discovery)) {
> +			struct discovery_state *d = &hdev->discovery;
> +

A comment why we do this here might be a good idea.

> +			mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
> +						  d->last_adv_addr_type, NULL,
> +						  0, 0, 1, d->last_adv_data,
> +						  d->last_adv_data_len,
> +						  NULL, 0);
> +		}
> +
> 		/* Cancel this timer so that we don't try to disable scanning
> 		 * when it's already disabled.
> 		 */
> @@ -3944,6 +3962,8 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> 			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
> {
> +	struct discovery_state *d = &hdev->discovery;
> +
> 	/* Passive scanning shouldn't trigger any device found events */
> 	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> 		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> @@ -3951,8 +3971,60 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> 	    return;
> 	}
> 
> -	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
> -			  data, len, NULL, 0);
> +	/* If there's nothing cached either cache the data from this
> +	 * event or send an immediate device found event if the data is
> +	 * not cachable.
> +	 */

I would not use the word cache since it is not really a cache. It is just a pending report.

> +	if (ADV_CACHE_EMPTY(d)) {

Do we really need to check if the pending report is empty here. It will not match the bacmp() change anyway.

> +		if (type == LE_ADV_IND || type == LE_ADV_SCAN_IND)
> +			goto cache_data;
> +
> +		mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL,
> +				  rssi, 0, 1, data, len, NULL, 0);
> +		return;
> +	}
> +
> +	/* If the cached data doesn't match this report or this isn't a
> +	 * scan response (e.g. we got a duplicate ADV_IND) then force
> +	 * sending of the cached data.
> +	 */
> +	if (type != LE_ADV_SCAN_RSP || bacmp(bdaddr, &d->last_adv_addr) ||
> +	    bdaddr_type != d->last_adv_addr_type) {
> +		/* Send out whatever is in the cache */
> +		mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
> +				  d->last_adv_addr_type, NULL, 0, 0, 1,
> +				  d->last_adv_data, d->last_adv_data_len,
> +				  NULL, 0);
> +
> +		/* If the new event should be cached store it there */
> +		if (type == LE_ADV_IND || type == LE_ADV_SCAN_IND)
> +			goto cache_data;
> +
> +		/* The event can't be cached so clear the cache and send
> +		 * an immediate event based on this report.
> +		 */
> +		ADV_CACHE_CLEAR(d);
> +		mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
> +				  d->last_adv_addr_type, NULL, rssi, 0, 1,
> +				  data, len, NULL, 0);
> +		return;
> +	}
> +
> +	/* If we get here we've got a cached ADV_IND or ADV_SCAN_IND and
> +	 * the new event is a SCAN_RSP. We can therefore proceed with
> +	 * sending a merged device found event.
> +	 */
> +	mgmt_device_found(hdev, &d->last_adv_addr, LE_LINK,
> +			  d->last_adv_addr_type, NULL, rssi, 0, 1, data, len,
> +			  d->last_adv_data, d->last_adv_data_len);
> +	ADV_CACHE_CLEAR(d);
> +	return;
> +
> +cache_data:
> +	bacpy(&d->last_adv_addr, bdaddr);
> +	d->last_adv_addr_type = bdaddr_type;
> +	memcpy(d->last_adv_data, data, len);
> +	d->last_adv_data_len = len;
> }

Maybe have this as a store_pending_adv_report() is a nice idea.

> 
> static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
> — 

Regards

Marcel

Re: [PATCH v2 4/4] Bluetooth: Merge ADV_IND/ADV_SCAN_IND and SCAN_RSP together

[Johan Hedberg]

Hi Marcel,

On Mon, Mar 24, 2014, Marcel Holtmann wrote:
> > @@ -3944,6 +3962,8 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> > static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> > 			       u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
> > {
> > +	struct discovery_state *d = &hdev->discovery;
> > +
> > 	/* Passive scanning shouldn't trigger any device found events */
> > 	if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> > 		if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> > @@ -3951,8 +3971,60 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> > 	    return;
> > 	}
> > 
> > -	mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
> > -			  data, len, NULL, 0);
> > +	/* If there's nothing cached either cache the data from this
> > +	 * event or send an immediate device found event if the data is
> > +	 * not cachable.
> > +	 */
> 
> I would not use the word cache since it is not really a cache. It is
> just a pending report.
> 
> > +	if (ADV_CACHE_EMPTY(d)) {
> 
> Do we really need to check if the pending report is empty here. It
> will not match the bacmp() change anyway.

I agree with your other suggestions, but here I feel like the logic
stays more understandable with keeping this separate. E.g. one
difference between the two branches is the check for SCAN_RSP. It
prevents the code from trying to merge together two consecutive ADV_IND
from the same device. Furthermore, either way need to compare against
BDADDR_ANY first to know if a mismatch of bdaddrs means we should send
what's pending or not (if there's nothing pending).

I just have a hard time wrapping my head around a simpler solution
that's both understandable and correct. Feel free to suggest your own
solution, but this part of my patch will remain unchanged in the next
revision.

Johan

Re: [PATCH v2 1/4] Bluetooth: Refactor advertising report processing into its own function

[Marcel Holtmann]

Hi Johan,

> As preparation for merging ADV_IND/ADV_SCAN_IND and SCAN_RSP together
> into a single mgmt Device Found event refactor individual advertising
> report handling into a separate function. This will help keep the code
> more readable as more logic gets added.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/hci_event.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel