[RFC v4 1/2] Bluetooth: Add le_scan_restart

[RFC v4 1/2] Bluetooth: Add le_scan_restart

[Jakub Pawlowski]

Currently there is no way to restart le scan, and it's needed in
service scan method. The way it work: it disable, and then enable le
scan on controller. During this restart special flag is set to make
sure we won't remove disable scan work from workqueue.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 include/net/bluetooth/hci_core.h |  9 +++++++++
 net/bluetooth/hci_core.c         | 42 ++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        |  9 ++++++---
 3 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3c78270..53e7975 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -56,6 +56,13 @@ struct inquiry_entry {
 	struct inquiry_data	data;
 };
 
+/* BR/EDR and/or LE discovery state flags: the flags defined here should
+ * represent state of discovery
+ */
+enum {
+	HCI_LE_SCAN_RESTARTING,
+};
+
 struct discovery_state {
 	int			type;
 	enum {
@@ -79,6 +86,7 @@ struct discovery_state {
 	s8			rssi;
 	u16			uuid_count;
 	u8			(*uuids)[16];
+	unsigned long		flags;
 };
 
 struct hci_conn_hash {
@@ -343,6 +351,7 @@ struct hci_dev {
 	unsigned long		dev_flags;
 
 	struct delayed_work	le_scan_disable;
+	struct delayed_work	le_scan_restart;
 
 	__s8			adv_tx_power;
 	__u8			adv_data[HCI_MAX_AD_LENGTH];
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 93f92a0..273311f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2624,6 +2624,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 		cancel_delayed_work(&hdev->service_cache);
 
 	cancel_delayed_work_sync(&hdev->le_scan_disable);
+	cancel_delayed_work_sync(&hdev->le_scan_restart);
 
 	if (test_bit(HCI_MGMT, &hdev->dev_flags))
 		cancel_delayed_work_sync(&hdev->rpa_expired);
@@ -3892,6 +3893,8 @@ static void le_scan_disable_work(struct work_struct *work)
 
 	BT_DBG("%s", hdev->name);
 
+	cancel_delayed_work_sync(&hdev->le_scan_restart);
+
 	hci_req_init(&req, hdev);
 
 	hci_req_add_le_scan_disable(&req);
@@ -3901,6 +3904,44 @@ static void le_scan_disable_work(struct work_struct *work)
 		BT_ERR("Disable LE scanning request failed: err %d", err);
 }
 
+static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
+{
+	clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
+
+	if (status)
+		BT_ERR("Failed to restart LE scan: status %d", status);
+}
+
+static void le_scan_restart_work(struct work_struct *work)
+{
+	struct hci_dev *hdev = container_of(work, struct hci_dev,
+					    le_scan_restart.work);
+	struct hci_request req;
+	struct hci_cp_le_set_scan_enable cp;
+	int err;
+
+	BT_DBG("%s", hdev->name);
+
+	/* If controller is not scanning we are done. */
+	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return;
+
+	hci_req_init(&req, hdev);
+
+	hci_req_add_le_scan_disable(&req);
+
+	memset(&cp, 0, sizeof(cp));
+	cp.enable = LE_SCAN_ENABLE;
+	cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
+	hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+	set_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
+
+	err = hci_req_run(&req, le_scan_restart_work_complete);
+	if (err)
+		BT_ERR("Restart LE scan request failed: err %d", err);
+}
+
 static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
 {
 	struct hci_dev *hdev = req->hdev;
@@ -4078,6 +4119,7 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
 	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
 	INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
+	INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
 
 	skb_queue_head_init(&hdev->rx_q);
 	skb_queue_head_init(&hdev->cmd_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 322abbb..cefb35d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 					  d->last_adv_data_len, NULL, 0);
 		}
 
-		/* Cancel this timer so that we don't try to disable scanning
-		 * when it's already disabled.
+		/* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
+		 * because we're just restarting scan. Otherwise cancel it so
+		 * that we don't try to disable scanning when it's already
+		 * disabled.
 		 */
-		cancel_delayed_work(&hdev->le_scan_disable);
+		if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags))
+			cancel_delayed_work(&hdev->le_scan_disable);
 
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
-- 
2.2.0.rc0.207.ga3a616c

[RFC v4 2/2] Bluetooth: Add restarting to service discovery

[Jakub Pawlowski]

When using LE_SCAN_FILTER_DUP_ENABLE, controllers would send
advertising report from each LE device only once. That means that we
don't get any updates on RSSI value, and makes Service Discovery very
slow. This patch adds restarting scan when in Service Discovery, and
device with filtered uuid is found, but it's not in RSSI range to send
event yet. This way if device moves into range, we will quickly get RSSI
update.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/mgmt.c             | 34 ++++++++++++++++++++++------------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 53e7975..96cf380 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1353,6 +1353,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
 #define DISCOV_INTERLEAVED_TIMEOUT	5120	/* msec */
 #define DISCOV_INTERLEAVED_INQUIRY_LEN	0x04
 #define DISCOV_BREDR_INQUIRY_LEN	0x08
+#define DISCOV_LE_RESTART_DELAY		200	/* msec */
 
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
 int mgmt_new_settings(struct hci_dev *hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index a91e484..d40e253 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6983,6 +6983,12 @@ static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
 	return false;
 }
 
+static void restart_le_scan(struct hci_dev *hdev)
+{
+	queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
+			   msecs_to_jiffies(DISCOV_LE_RESTART_DELAY));
+}
+
 void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 		       u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
 		       u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
@@ -7003,18 +7009,6 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 			return;
 	}
 
-	/* When using service discovery with a RSSI threshold, then check
-	 * if such a RSSI threshold is specified. If a RSSI threshold has
-	 * been specified, then all results with a RSSI smaller than the
-	 * RSSI threshold will be dropped.
-	 *
-	 * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
-	 * the results are also dropped.
-	 */
-	if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
-	    (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
-		return;
-
 	/* Make sure that the buffer is big enough. The 5 extra bytes
 	 * are for the potential CoD field.
 	 */
@@ -7052,6 +7046,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 					      hdev->discovery.uuids);
 			if (!match)
 				return;
+
+			restart_le_scan(hdev);
 		}
 
 		/* Copy EIR or advertising data into event */
@@ -7080,6 +7076,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 						     hdev->discovery.uuid_count,
 						     hdev->discovery.uuids))
 				return;
+
+			restart_le_scan(hdev);
 		}
 
 		/* Append scan response data to event */
@@ -7093,6 +7091,18 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 			return;
 	}
 
+	/* When using service discovery with a RSSI threshold, then check
+	 * if such a RSSI threshold is specified. If a RSSI threshold has
+	 * been specified, then all results with a RSSI smaller than the
+	 * RSSI threshold will be dropped.
+	 *
+	 * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
+	 * the results are also dropped.
+	 */
+	if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
+	    (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
+		return;
+
 	ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
 	ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
 
-- 
2.2.0.rc0.207.ga3a616c

Re: [RFC v4 2/2] Bluetooth: Add restarting to service discovery

[Marcel Holtmann]

Hi Jakub,

> When using LE_SCAN_FILTER_DUP_ENABLE, controllers would send
> advertising report from each LE device only once. That means that we
> don't get any updates on RSSI value, and makes Service Discovery very
> slow. This patch adds restarting scan when in Service Discovery, and
> device with filtered uuid is found, but it's not in RSSI range to send
> event yet. This way if device moves into range, we will quickly get RSSI
> update.
> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/mgmt.c             | 34 ++++++++++++++++++++++------------
> 2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 53e7975..96cf380 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1353,6 +1353,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> #define DISCOV_INTERLEAVED_TIMEOUT	5120	/* msec */
> #define DISCOV_INTERLEAVED_INQUIRY_LEN	0x04
> #define DISCOV_BREDR_INQUIRY_LEN	0x08
> +#define DISCOV_LE_RESTART_DELAY		200	/* msec */
> 
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
> int mgmt_new_settings(struct hci_dev *hdev);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index a91e484..d40e253 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -6983,6 +6983,12 @@ static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
> 	return false;
> }
> 
> +static void restart_le_scan(struct hci_dev *hdev)
> +{

so here you should check if we are scanning or not and if this extra handling of restarts is actually needed.

In addition you might really want to check that this is the LE scanning phase. I know that checking for LE_SCAN would do that, but this is a bit dangerous since this code path can also be entered from BR/EDR inquiry results.

> +	queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
> +			   msecs_to_jiffies(DISCOV_LE_RESTART_DELAY));
> +}
> +
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 		       u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
> 		       u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -7003,18 +7009,6 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 			return;
> 	}
> 
> -	/* When using service discovery with a RSSI threshold, then check
> -	 * if such a RSSI threshold is specified. If a RSSI threshold has
> -	 * been specified, then all results with a RSSI smaller than the
> -	 * RSSI threshold will be dropped.
> -	 *
> -	 * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
> -	 * the results are also dropped.
> -	 */
> -	if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
> -	    (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
> -		return;
> -

We are you moving this check around. There is no point in checking for the UUID when the RSSI threshold is not reached. This advertising event should be ignored.

Especially if this is on BR/EDR or the controller is doing the multiple reporting for us, then there is no point in bothering with looking for the UUID.

So I realize it is desired to only restart when the UUIDs we are looking for is actually found, but that does not mean we want to penalize all other paths or controllers that do not need a scan restart in the first place.

> 	/* Make sure that the buffer is big enough. The 5 extra bytes
> 	 * are for the potential CoD field.
> 	 */
> @@ -7052,6 +7046,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 					      hdev->discovery.uuids);
> 			if (!match)
> 				return;
> +
> +			restart_le_scan(hdev);
> 		}
> 
> 		/* Copy EIR or advertising data into event */
> @@ -7080,6 +7076,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 						     hdev->discovery.uuid_count,
> 						     hdev->discovery.uuids))
> 				return;
> +
> +			restart_le_scan(hdev);

Both of these should have a comment on why this is done. You might also run into the problem that this gets run twice if the UUID has been found in advertising data already.

> 		}
> 
> 		/* Append scan response data to event */
> @@ -7093,6 +7091,18 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> 			return;
> 	}
> 
> +	/* When using service discovery with a RSSI threshold, then check
> +	 * if such a RSSI threshold is specified. If a RSSI threshold has
> +	 * been specified, then all results with a RSSI smaller than the
> +	 * RSSI threshold will be dropped.
> +	 *
> +	 * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
> +	 * the results are also dropped.
> +	 */
> +	if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
> +	    (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
> +		return;
> +
> 	ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> 	ev_size = sizeof(*ev) + eir_len + scan_rsp_len;

Regards

Marcel

Re: [RFC v4 2/2] Bluetooth: Add restarting to service discovery

[Jakub Pawlowski]

On Tue, Dec 9, 2014 at 7:32 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Jakub,
>
>> When using LE_SCAN_FILTER_DUP_ENABLE, controllers would send
>> advertising report from each LE device only once. That means that we
>> don't get any updates on RSSI value, and makes Service Discovery very
>> slow. This patch adds restarting scan when in Service Discovery, and
>> device with filtered uuid is found, but it's not in RSSI range to send
>> event yet. This way if device moves into range, we will quickly get RSSI
>> update.
>>
>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>> ---
>> include/net/bluetooth/hci_core.h |  1 +
>> net/bluetooth/mgmt.c             | 34 ++++++++++++++++++++++------------
>> 2 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 53e7975..96cf380 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1353,6 +1353,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
>> #define DISCOV_INTERLEAVED_TIMEOUT    5120    /* msec */
>> #define DISCOV_INTERLEAVED_INQUIRY_LEN        0x04
>> #define DISCOV_BREDR_INQUIRY_LEN      0x08
>> +#define DISCOV_LE_RESTART_DELAY              200     /* msec */
>>
>> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
>> int mgmt_new_settings(struct hci_dev *hdev);
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index a91e484..d40e253 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -6983,6 +6983,12 @@ static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
>>       return false;
>> }
>>
>> +static void restart_le_scan(struct hci_dev *hdev)
>> +{
>
> so here you should check if we are scanning or not and if this extra handling of restarts is actually needed.
>
> In addition you might really want to check that this is the LE scanning phase. I know that checking for LE_SCAN would do that, but this is a bit dangerous since this code path can also be entered from BR/EDR inquiry results.
>
>> +     queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
>> +                        msecs_to_jiffies(DISCOV_LE_RESTART_DELAY));
>> +}
>> +
>> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                      u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
>>                      u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
>> @@ -7003,18 +7009,6 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                       return;
>>       }
>>
>> -     /* When using service discovery with a RSSI threshold, then check
>> -      * if such a RSSI threshold is specified. If a RSSI threshold has
>> -      * been specified, then all results with a RSSI smaller than the
>> -      * RSSI threshold will be dropped.
>> -      *
>> -      * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
>> -      * the results are also dropped.
>> -      */
>> -     if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
>> -         (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
>> -             return;
>> -
>
> We are you moving this check around. There is no point in checking for the UUID when the RSSI threshold is not reached. This advertising event should be ignored.
>
> Especially if this is on BR/EDR or the controller is doing the multiple reporting for us, then there is no point in bothering with looking for the UUID.
>
> So I realize it is desired to only restart when the UUIDs we are looking for is actually found, but that does not mean we want to penalize all other paths or controllers that do not need a scan restart in the first place.

Yes, you're right, I'll fix that.

>
>>       /* Make sure that the buffer is big enough. The 5 extra bytes
>>        * are for the potential CoD field.
>>        */
>> @@ -7052,6 +7046,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                                             hdev->discovery.uuids);
>>                       if (!match)
>>                               return;
>> +
>> +                     restart_le_scan(hdev);
>>               }
>>
>>               /* Copy EIR or advertising data into event */
>> @@ -7080,6 +7076,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                                                    hdev->discovery.uuid_count,
>>                                                    hdev->discovery.uuids))
>>                               return;
>> +
>> +                     restart_le_scan(hdev);
>
> Both of these should have a comment on why this is done. You might also run into the problem that this gets run twice if the UUID has been found in advertising data already.

Yes, it's perfectly fine. You can also run on marvell controller (one
I used for tests) that doesn't honor deduplication and get multiple
reports for same device before the job is run:

internally it's only calling queue_delayed_work, and documentation says:

"The return value from these functions [queue_delayed_work] is 0 if
the work was successfully added to the queue; a nonzero result means
that this work_struct structure was already waiting in the queue, and
was not added a second time."

So this way I make sure, that:
- if device is advertising less often than every
DISCOV_LE_RESTART_DELAY (200ms), I'll get mgmt_device_found for each
advertisement (with updated RSSI value), because we restart often
enough,
- if device is advertising more frequently that
DISCOV_LE_RESTART_DELAY, you'll get only one mgmt_device_found every
DISCOV_LE_RESTART_DELAY for that device, because controller will
filter rest.

Also if there was no DISCOV_LE_RESTART_DELAY, you'll get device that
is advertising with big frequency, it would cause your controller to
restart all the time. That might cause only one device, most noisy one
to be discovered.
I might further tune DISCOV_LE_RESTART_DELAY in future when
implementing StartServiceDiscovery in D-BUS, to make sure it's working
nicely. Right now I just made sure that all controllers that I used
during tests would report RSSI change at least 3 times/sec for
frequently advertising device.


>
>>               }
>>
>>               /* Append scan response data to event */
>> @@ -7093,6 +7091,18 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>>                       return;
>>       }
>>
>> +     /* When using service discovery with a RSSI threshold, then check
>> +      * if such a RSSI threshold is specified. If a RSSI threshold has
>> +      * been specified, then all results with a RSSI smaller than the
>> +      * RSSI threshold will be dropped.
>> +      *
>> +      * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
>> +      * the results are also dropped.
>> +      */
>> +     if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
>> +         (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
>> +             return;
>> +
>>       ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
>>       ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>
> Regards
>
> Marcel
>

Re: [RFC v4 1/2] Bluetooth: Add le_scan_restart

[Marcel Holtmann]

Hi Jakub,

> Currently there is no way to restart le scan, and it's needed in
> service scan method. The way it work: it disable, and then enable le
> scan on controller. During this restart special flag is set to make
> sure we won't remove disable scan work from workqueue.
> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci_core.h |  9 +++++++++
> net/bluetooth/hci_core.c         | 42 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        |  9 ++++++---
> 3 files changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3c78270..53e7975 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -56,6 +56,13 @@ struct inquiry_entry {
> 	struct inquiry_data	data;
> };
> 
> +/* BR/EDR and/or LE discovery state flags: the flags defined here should
> + * represent state of discovery
> + */
> +enum {
> +	HCI_LE_SCAN_RESTARTING,
> +};
> +
> struct discovery_state {
> 	int			type;
> 	enum {
> @@ -79,6 +86,7 @@ struct discovery_state {
> 	s8			rssi;
> 	u16			uuid_count;
> 	u8			(*uuids)[16];
> +	unsigned long		flags;
> };
> 
> struct hci_conn_hash {
> @@ -343,6 +351,7 @@ struct hci_dev {
> 	unsigned long		dev_flags;
> 
> 	struct delayed_work	le_scan_disable;
> +	struct delayed_work	le_scan_restart;
> 
> 	__s8			adv_tx_power;
> 	__u8			adv_data[HCI_MAX_AD_LENGTH];
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 93f92a0..273311f 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2624,6 +2624,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> 		cancel_delayed_work(&hdev->service_cache);
> 
> 	cancel_delayed_work_sync(&hdev->le_scan_disable);
> +	cancel_delayed_work_sync(&hdev->le_scan_restart);
> 
> 	if (test_bit(HCI_MGMT, &hdev->dev_flags))
> 		cancel_delayed_work_sync(&hdev->rpa_expired);
> @@ -3892,6 +3893,8 @@ static void le_scan_disable_work(struct work_struct *work)
> 
> 	BT_DBG("%s", hdev->name);
> 
> +	cancel_delayed_work_sync(&hdev->le_scan_restart);
> +
> 	hci_req_init(&req, hdev);
> 
> 	hci_req_add_le_scan_disable(&req);
> @@ -3901,6 +3904,44 @@ static void le_scan_disable_work(struct work_struct *work)
> 		BT_ERR("Disable LE scanning request failed: err %d", err);
> }
> 
> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
> +{
> +	clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
> +
> +	if (status)
> +		BT_ERR("Failed to restart LE scan: status %d", status);

if this actually happens, don't we have to actually tell userspace that discovery stopped now? It seems we need some error handling here.

> +}
> +
> +static void le_scan_restart_work(struct work_struct *work)
> +{
> +	struct hci_dev *hdev = container_of(work, struct hci_dev,
> +					    le_scan_restart.work);
> +	struct hci_request req;
> +	struct hci_cp_le_set_scan_enable cp;
> +	int err;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	/* If controller is not scanning we are done. */
> +	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
> +		return;

Why are we queueing this work in the first place if we are not scanning. That should be done at the calling site already.

Same as we should check for HCI_QUIRK_STRICT_DUPLICATE_FILTER setting and only do the restart scan stuff if the driver told us that this controller is not providing multiple reports.

> +
> +	hci_req_init(&req, hdev);
> +
> +	hci_req_add_le_scan_disable(&req);
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.enable = LE_SCAN_ENABLE;
> +	cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
> +	hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +
> +	set_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
> +
> +	err = hci_req_run(&req, le_scan_restart_work_complete);
> +	if (err)
> +		BT_ERR("Restart LE scan request failed: err %d", err);
> +}
> +
> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
> {
> 	struct hci_dev *hdev = req->hdev;
> @@ -4078,6 +4119,7 @@ struct hci_dev *hci_alloc_dev(void)
> 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
> 	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
> 	INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
> +	INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
> 
> 	skb_queue_head_init(&hdev->rx_q);
> 	skb_queue_head_init(&hdev->cmd_q);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 322abbb..cefb35d 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> 					  d->last_adv_data_len, NULL, 0);
> 		}
> 
> -		/* Cancel this timer so that we don't try to disable scanning
> -		 * when it's already disabled.
> +		/* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
> +		 * because we're just restarting scan. Otherwise cancel it so
> +		 * that we don't try to disable scanning when it's already
> +		 * disabled.
> 		 */
> -		cancel_delayed_work(&hdev->le_scan_disable);
> +		if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags))
> +			cancel_delayed_work(&hdev->le_scan_disable);

This could be racy. We are restarting, but it also means that we will enable scanning again. So in case we are caught in the middle of this we might end up with scanning enabled.

> 
> 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);

Regards

Marcel

Re: [RFC v4 1/2] Bluetooth: Add le_scan_restart

[Jakub Pawlowski]

On Tue, Dec 9, 2014 at 7:32 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Jakub,
>
>> Currently there is no way to restart le scan, and it's needed in
>> service scan method. The way it work: it disable, and then enable le
>> scan on controller. During this restart special flag is set to make
>> sure we won't remove disable scan work from workqueue.
>>
>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>> ---
>> include/net/bluetooth/hci_core.h |  9 +++++++++
>> net/bluetooth/hci_core.c         | 42 ++++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_event.c        |  9 ++++++---
>> 3 files changed, 57 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 3c78270..53e7975 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -56,6 +56,13 @@ struct inquiry_entry {
>>       struct inquiry_data     data;
>> };
>>
>> +/* BR/EDR and/or LE discovery state flags: the flags defined here should
>> + * represent state of discovery
>> + */
>> +enum {
>> +     HCI_LE_SCAN_RESTARTING,
>> +};
>> +
>> struct discovery_state {
>>       int                     type;
>>       enum {
>> @@ -79,6 +86,7 @@ struct discovery_state {
>>       s8                      rssi;
>>       u16                     uuid_count;
>>       u8                      (*uuids)[16];
>> +     unsigned long           flags;
>> };
>>
>> struct hci_conn_hash {
>> @@ -343,6 +351,7 @@ struct hci_dev {
>>       unsigned long           dev_flags;
>>
>>       struct delayed_work     le_scan_disable;
>> +     struct delayed_work     le_scan_restart;
>>
>>       __s8                    adv_tx_power;
>>       __u8                    adv_data[HCI_MAX_AD_LENGTH];
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 93f92a0..273311f 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2624,6 +2624,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>>               cancel_delayed_work(&hdev->service_cache);
>>
>>       cancel_delayed_work_sync(&hdev->le_scan_disable);
>> +     cancel_delayed_work_sync(&hdev->le_scan_restart);
>>
>>       if (test_bit(HCI_MGMT, &hdev->dev_flags))
>>               cancel_delayed_work_sync(&hdev->rpa_expired);
>> @@ -3892,6 +3893,8 @@ static void le_scan_disable_work(struct work_struct *work)
>>
>>       BT_DBG("%s", hdev->name);
>>
>> +     cancel_delayed_work_sync(&hdev->le_scan_restart);
>> +
>>       hci_req_init(&req, hdev);
>>
>>       hci_req_add_le_scan_disable(&req);
>> @@ -3901,6 +3904,44 @@ static void le_scan_disable_work(struct work_struct *work)
>>               BT_ERR("Disable LE scanning request failed: err %d", err);
>> }
>>
>> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
>> +{
>> +     clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
>> +
>> +     if (status)
>> +             BT_ERR("Failed to restart LE scan: status %d", status);
>
> if this actually happens, don't we have to actually tell userspace that discovery stopped now? It seems we need some error handling here.

So the error should be sent when stopping succeed, but starting
failed? I'll check scan status here with  if (!test_bit(HCI_LE_SCAN,
&hdev->dev_flags)) and report error if we're not scanning.

>
>> +}
>> +
>> +static void le_scan_restart_work(struct work_struct *work)
>> +{
>> +     struct hci_dev *hdev = container_of(work, struct hci_dev,
>> +                                         le_scan_restart.work);
>> +     struct hci_request req;
>> +     struct hci_cp_le_set_scan_enable cp;
>> +     int err;
>> +
>> +     BT_DBG("%s", hdev->name);
>> +
>> +     /* If controller is not scanning we are done. */
>> +     if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
>> +             return;
>
> Why are we queueing this work in the first place if we are not scanning. That should be done at the calling site already.

We're queuing this work only if we are scanning, but between queuing
and execution scan might become disabled, so we have to check again
here.

>
> Same as we should check for HCI_QUIRK_STRICT_DUPLICATE_FILTER setting and only do the restart scan stuff if the driver told us that this controller is not providing multiple reports.

So I did some tests on 4 controllers I described in this email:
http://marc.info/?l=linux-bluetooth&m=141764424929887&w=2
I think that HCI_QUIRK_STRICT_DUPLICATE_FILTER wil be valid if we were
doing HCI_OP_INQUIRY, when I was using HCI_OP_LE_SET_SCAN_ENABLE with
LE_SCAN_FILTER_DUP_ENABLE none of the controllers I used reported
device twice with different RSSI except for marevell, which seems to
ignore
LE_SCAN_FILTER_DUP_ENABLE, and reports all advertisements.

The way I conducted this tests was to start the scan "tools/mgmt find
-l", and try to approach (15-20 meters) or move away from scanning
device, walk back and forth, or just keep the advertiser in place. I
was monitoring both btmgmt output and dmesg kernel logs (I added some
additional BT_INFO to make sure I won't miss anything)

Do you have different experience for LE scan ? Or maybe I just picked
wrong controllers for my tests ?

>
>> +
>> +     hci_req_init(&req, hdev);
>> +
>> +     hci_req_add_le_scan_disable(&req);
>> +
>> +     memset(&cp, 0, sizeof(cp));
>> +     cp.enable = LE_SCAN_ENABLE;
>> +     cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
>> +     hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> +
>> +     set_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
>> +
>> +     err = hci_req_run(&req, le_scan_restart_work_complete);
>> +     if (err)
>> +             BT_ERR("Restart LE scan request failed: err %d", err);
>> +}
>> +
>> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
>> {
>>       struct hci_dev *hdev = req->hdev;
>> @@ -4078,6 +4119,7 @@ struct hci_dev *hci_alloc_dev(void)
>>       INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>       INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
>>       INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
>> +     INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>>
>>       skb_queue_head_init(&hdev->rx_q);
>>       skb_queue_head_init(&hdev->cmd_q);
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 322abbb..cefb35d 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>                                         d->last_adv_data_len, NULL, 0);
>>               }
>>
>> -             /* Cancel this timer so that we don't try to disable scanning
>> -              * when it's already disabled.
>> +             /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
>> +              * because we're just restarting scan. Otherwise cancel it so
>> +              * that we don't try to disable scanning when it's already
>> +              * disabled.
>>                */
>> -             cancel_delayed_work(&hdev->le_scan_disable);
>> +             if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags))
>> +                     cancel_delayed_work(&hdev->le_scan_disable);
>
> This could be racy. We are restarting, but it also means that we will enable scanning again. So in case we are caught in the middle of this we might end up with scanning enabled.

So the proper solution would be to add new lock that would have to be
acquired before using HCI_OP_LE_SET_SCAN_ENABLE (for both enabling and
disabling), and modify all occurences to use this lock.

So all calls will do:

1. acquire LE scan lock
2. disable or enable LE scan
3. release LE scan lock

and restart le will do:

1. acquire LE scan lock
2. disable LE scan
3. enable LE scan
4. release LE scan lock

will that be ok ?

>
>>
>>               clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>
> Regards
>
> Marcel
>

Re: [RFC v4 1/2] Bluetooth: Add le_scan_restart

[Johan Hedberg]

Hi Jakub,

On Tue, Dec 09, 2014, Jakub Pawlowski wrote:
> > Same as we should check for HCI_QUIRK_STRICT_DUPLICATE_FILTER
> > setting and only do the restart scan stuff if the driver told us
> > that this controller is not providing multiple reports.
> 
> So I did some tests on 4 controllers I described in this email:
> http://marc.info/?l=linux-bluetooth&m=141764424929887&w=2
> I think that HCI_QUIRK_STRICT_DUPLICATE_FILTER wil be valid if we were
> doing HCI_OP_INQUIRY, when I was using HCI_OP_LE_SET_SCAN_ENABLE with
> LE_SCAN_FILTER_DUP_ENABLE none of the controllers I used reported
> device twice with different RSSI except for marevell, which seems to
> ignore
> LE_SCAN_FILTER_DUP_ENABLE, and reports all advertisements.
> 
> The way I conducted this tests was to start the scan "tools/mgmt find
> -l", and try to approach (15-20 meters) or move away from scanning
> device, walk back and forth, or just keep the advertiser in place. I
> was monitoring both btmgmt output and dmesg kernel logs (I added some
> additional BT_INFO to make sure I won't miss anything)
> 
> Do you have different experience for LE scan ? Or maybe I just picked
> wrong controllers for my tests ?

Please add CSR dongles to your mix. The most commonly available one is
probably the PTS dongle. They also report multiple advertisements as the
RSSI changes.

Johan

Re: [RFC v4 1/2] Bluetooth: Add le_scan_restart

[Jakub Pawlowski]

On Tue, Dec 9, 2014 at 10:01 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Jakub,
>
> On Tue, Dec 09, 2014, Jakub Pawlowski wrote:
>> > Same as we should check for HCI_QUIRK_STRICT_DUPLICATE_FILTER
>> > setting and only do the restart scan stuff if the driver told us
>> > that this controller is not providing multiple reports.
>>
>> So I did some tests on 4 controllers I described in this email:
>> http://marc.info/?l=linux-bluetooth&m=141764424929887&w=2
>> I think that HCI_QUIRK_STRICT_DUPLICATE_FILTER wil be valid if we were
>> doing HCI_OP_INQUIRY, when I was using HCI_OP_LE_SET_SCAN_ENABLE with
>> LE_SCAN_FILTER_DUP_ENABLE none of the controllers I used reported
>> device twice with different RSSI except for marevell, which seems to
>> ignore
>> LE_SCAN_FILTER_DUP_ENABLE, and reports all advertisements.
>>
>> The way I conducted this tests was to start the scan "tools/mgmt find
>> -l", and try to approach (15-20 meters) or move away from scanning
>> device, walk back and forth, or just keep the advertiser in place. I
>> was monitoring both btmgmt output and dmesg kernel logs (I added some
>> additional BT_INFO to make sure I won't miss anything)
>>
>> Do you have different experience for LE scan ? Or maybe I just picked
>> wrong controllers for my tests ?
>
> Please add CSR dongles to your mix. The most commonly available one is
> probably the PTS dongle. They also report multiple advertisements as the
> RSSI changes.

Thanks for info! I'll test on them, and modify my code to make sure
that they work ok.
>
> Johan