[PATCH v4 1/7] Bluetooth: Introduce HCI_QUIRK_SIMULTANEOUS_DISCOVERY

[PATCH v4 1/7] Bluetooth: Introduce HCI_QUIRK_SIMULTANEOUS_DISCOVERY

[Jakub Pawlowski]

Some controllers allow both LE scan and BR/EDR inquiry to run at
the same time, while others allow only one, LE SCAN or BR/EDR
inquiry at given time.

Since this is specific to each controller, add a new quirk setting
that allows drivers to tell the core wether given controller can
do both LE scan and BR/EDR inquiry at same time.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 include/net/bluetooth/hci.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ce75730..7091601 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -160,6 +160,15 @@ enum {
 	 * during the hdev->setup vendor callback.
 	 */
 	HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+
+	/* When this quirk is set, LE scan and BR/EDR inquiry is done
+	 * simultaneously, otherwise it's interleaved.
+	 * .
+	 *
+	 * This quirk can be set before hci_register_dev is called or
+	 * during the hdev->setup vendor callback.
+	 */
+	HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
 };
 
 /* HCI device flags */
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v4 2/7] Bluetooth: Refactor BREDR inquiry and LE scan triggering.

[Jakub Pawlowski]

This patch refactor BREDR inquiry and LE scan triggering logic into
separate methods.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 net/bluetooth/mgmt.c | 145 +++++++++++++++++++++++++++++----------------------
 1 file changed, 82 insertions(+), 63 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 32c2c75..92fa8e88 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3913,93 +3913,112 @@ done:
 	return err;
 }
 
-static bool trigger_discovery(struct hci_request *req, u8 *status)
+static bool trigger_bredr_inquiry(struct hci_request *req, u8 *status)
 {
 	struct hci_dev *hdev = req->hdev;
-	struct hci_cp_le_set_scan_param param_cp;
-	struct hci_cp_le_set_scan_enable enable_cp;
 	struct hci_cp_inquiry inq_cp;
 	/* General inquiry access code (GIAC) */
 	u8 lap[3] = { 0x33, 0x8b, 0x9e };
+
+	*status = mgmt_bredr_support(hdev);
+	if (*status)
+		return false;
+
+	if (hci_dev_test_flag(hdev, HCI_INQUIRY)) {
+		*status = MGMT_STATUS_BUSY;
+		return false;
+	}
+
+	hci_inquiry_cache_flush(hdev);
+
+	memset(&inq_cp, 0, sizeof(inq_cp));
+	memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
+	inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
+	hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
+	return true;
+}
+
+static bool trigger_le_scan(struct hci_request *req, u8 *status,
+			    __le16 interval)
+{
+	struct hci_dev *hdev = req->hdev;
+	struct hci_cp_le_set_scan_param param_cp;
+	struct hci_cp_le_set_scan_enable enable_cp;
 	u8 own_addr_type;
 	int err;
 
-	switch (hdev->discovery.type) {
-	case DISCOV_TYPE_BREDR:
-		*status = mgmt_bredr_support(hdev);
-		if (*status)
-			return false;
+	*status = mgmt_le_support(hdev);
+	if (*status)
+		return false;
 
-		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
-			*status = MGMT_STATUS_BUSY;
+	if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
+		/* Don't let discovery abort an outgoing connection attempt
+		 * that's using directed advertising.
+		 */
+		if (hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
+			*status = MGMT_STATUS_REJECTED;
 			return false;
 		}
 
-		hci_inquiry_cache_flush(hdev);
+		disable_advertising(req);
+	}
+
+	/* If controller is scanning, it means the background scanning is
+	 * running. Thus, we should temporarily stop it in order to set the
+	 * discovery scanning parameters.
+	 */
+	if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
+		hci_req_add_le_scan_disable(req);
 
-		memset(&inq_cp, 0, sizeof(inq_cp));
-		memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
-		inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
-		hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
+	memset(&param_cp, 0, sizeof(param_cp));
+
+	/* All active scans will be done with either a resolvable private
+	 * address (when privacy feature has been enabled) or non-resolvable
+	 * private address.
+	 */
+	err = hci_update_random_address(req, true, &own_addr_type);
+	if (err < 0) {
+		*status = MGMT_STATUS_FAILED;
+		return false;
+	}
+
+	param_cp.type = LE_SCAN_ACTIVE;
+	param_cp.interval = interval;
+	param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
+	param_cp.own_address_type = own_addr_type;
+	hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
+		    &param_cp);
+
+	memset(&enable_cp, 0, sizeof(enable_cp));
+	enable_cp.enable = LE_SCAN_ENABLE;
+	enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
+	hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
+		    &enable_cp);
+
+	return true;
+}
+
+static bool trigger_discovery(struct hci_request *req, u8 *status)
+{
+	struct hci_dev *hdev = req->hdev;
+
+	switch (hdev->discovery.type) {
+	case DISCOV_TYPE_BREDR:
+		if (!trigger_bredr_inquiry(req, status))
+			return false;
 		break;
 
 	case DISCOV_TYPE_LE:
 	case DISCOV_TYPE_INTERLEAVED:
-		*status = mgmt_le_support(hdev);
-		if (*status)
-			return false;
-
 		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
 		    !hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
 			*status = MGMT_STATUS_NOT_SUPPORTED;
 			return false;
 		}
 
-		if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
-			/* Don't let discovery abort an outgoing
-			 * connection attempt that's using directed
-			 * advertising.
-			 */
-			if (hci_conn_hash_lookup_state(hdev, LE_LINK,
-						       BT_CONNECT)) {
-				*status = MGMT_STATUS_REJECTED;
-				return false;
-			}
-
-			disable_advertising(req);
-		}
-
-		/* If controller is scanning, it means the background scanning
-		 * is running. Thus, we should temporarily stop it in order to
-		 * set the discovery scanning parameters.
-		 */
-		if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
-			hci_req_add_le_scan_disable(req);
-
-		memset(&param_cp, 0, sizeof(param_cp));
-
-		/* All active scans will be done with either a resolvable
-		 * private address (when privacy feature has been enabled)
-		 * or non-resolvable private address.
-		 */
-		err = hci_update_random_address(req, true, &own_addr_type);
-		if (err < 0) {
-			*status = MGMT_STATUS_FAILED;
+		if (!trigger_le_scan(req, status,
+				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
 			return false;
-		}
-
-		param_cp.type = LE_SCAN_ACTIVE;
-		param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
-		param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
-		param_cp.own_address_type = own_addr_type;
-		hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
-			    &param_cp);
-
-		memset(&enable_cp, 0, sizeof(enable_cp));
-		enable_cp.enable = LE_SCAN_ENABLE;
-		enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
-		hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
-			    &enable_cp);
 		break;
 
 	default:
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 2/7] Bluetooth: Refactor BREDR inquiry and LE scan triggering.

[Marcel Holtmann]

Hi Jakub,

> This patch refactor BREDR inquiry and LE scan triggering logic into
> separate methods.

please write it as BR/EDR which makes it generally more consistent.

> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> net/bluetooth/mgmt.c | 145 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 82 insertions(+), 63 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 32c2c75..92fa8e88 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3913,93 +3913,112 @@ done:
> 	return err;
> }
> 
> -static bool trigger_discovery(struct hci_request *req, u8 *status)
> +static bool trigger_bredr_inquiry(struct hci_request *req, u8 *status)
> {
> 	struct hci_dev *hdev = req->hdev;
> -	struct hci_cp_le_set_scan_param param_cp;
> -	struct hci_cp_le_set_scan_enable enable_cp;
> 	struct hci_cp_inquiry inq_cp;

I think you can rename this into just cp now. There is only now.

> 	/* General inquiry access code (GIAC) */
> 	u8 lap[3] = { 0x33, 0x8b, 0x9e };
> +
> +	*status = mgmt_bredr_support(hdev);
> +	if (*status)
> +		return false;
> +
> +	if (hci_dev_test_flag(hdev, HCI_INQUIRY)) {
> +		*status = MGMT_STATUS_BUSY;
> +		return false;
> +	}
> +
> +	hci_inquiry_cache_flush(hdev);
> +
> +	memset(&inq_cp, 0, sizeof(inq_cp));
> +	memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
> +	inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;

I think we normally have an empty line between filling the structs and then calling hci_req_add.

> +	hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);

Extra empty line here.

> +	return true;
> +}
> +
> +static bool trigger_le_scan(struct hci_request *req, u8 *status,
> +			    __le16 interval)
> +{

Lets make the status the last parameter. Having a return value in the middle is odd.

> +	struct hci_dev *hdev = req->hdev;
> +	struct hci_cp_le_set_scan_param param_cp;
> +	struct hci_cp_le_set_scan_enable enable_cp;
> 	u8 own_addr_type;
> 	int err;
> 
> -	switch (hdev->discovery.type) {
> -	case DISCOV_TYPE_BREDR:
> -		*status = mgmt_bredr_support(hdev);
> -		if (*status)
> -			return false;
> +	*status = mgmt_le_support(hdev);
> +	if (*status)
> +		return false;
> 
> -		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> -			*status = MGMT_STATUS_BUSY;
> +	if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
> +		/* Don't let discovery abort an outgoing connection attempt
> +		 * that's using directed advertising.
> +		 */
> +		if (hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
> +			*status = MGMT_STATUS_REJECTED;
> 			return false;
> 		}
> 
> -		hci_inquiry_cache_flush(hdev);
> +		disable_advertising(req);
> +	}
> +
> +	/* If controller is scanning, it means the background scanning is
> +	 * running. Thus, we should temporarily stop it in order to set the
> +	 * discovery scanning parameters.
> +	 */
> +	if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
> +		hci_req_add_le_scan_disable(req);
> 
> -		memset(&inq_cp, 0, sizeof(inq_cp));
> -		memcpy(&inq_cp.lap, lap, sizeof(inq_cp.lap));
> -		inq_cp.length = DISCOV_BREDR_INQUIRY_LEN;
> -		hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
> +	memset(&param_cp, 0, sizeof(param_cp));

This memset should go above the code that fills the param_cp.

> +
> +	/* All active scans will be done with either a resolvable private
> +	 * address (when privacy feature has been enabled) or non-resolvable
> +	 * private address.
> +	 */
> +	err = hci_update_random_address(req, true, &own_addr_type);
> +	if (err < 0) {
> +		*status = MGMT_STATUS_FAILED;
> +		return false;
> +	}
> +

Put the memset from above right here.

> +	param_cp.type = LE_SCAN_ACTIVE;
> +	param_cp.interval = interval;
> +	param_cp.window = cpu_to_le16(DISCOV_LE_SCAN_WIN);
> +	param_cp.own_address_type = own_addr_type;
> +	hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp),
> +		    &param_cp);
> +
> +	memset(&enable_cp, 0, sizeof(enable_cp));
> +	enable_cp.enable = LE_SCAN_ENABLE;
> +	enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
> +	hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> +		    &enable_cp);
> +
> +	return true;
> +}
> +
> +static bool trigger_discovery(struct hci_request *req, u8 *status)
> +{
> +	struct hci_dev *hdev = req->hdev;
> +
> +	switch (hdev->discovery.type) {
> +	case DISCOV_TYPE_BREDR:
> +		if (!trigger_bredr_inquiry(req, status))
> +			return false;
> 		break;
> 
> 	case DISCOV_TYPE_LE:
> 	case DISCOV_TYPE_INTERLEAVED:
> -		*status = mgmt_le_support(hdev);
> -		if (*status)
> -			return false;
> -
> 		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> 		    !hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> 			*status = MGMT_STATUS_NOT_SUPPORTED;
> 			return false;
> 		}
> 
> -		if (hci_dev_test_flag(hdev, HCI_LE_ADV)) {
> -			/* Don't let discovery abort an outgoing
> -			 * connection attempt that's using directed
> -			 * advertising.
> -			 */
> -			if (hci_conn_hash_lookup_state(hdev, LE_LINK,
> -						       BT_CONNECT)) {
> -				*status = MGMT_STATUS_REJECTED;
> -				return false;
> -			}
> -
> -			disable_advertising(req);
> -		}
> -
> -		/* If controller is scanning, it means the background scanning
> -		 * is running. Thus, we should temporarily stop it in order to
> -		 * set the discovery scanning parameters.
> -		 */
> -		if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
> -			hci_req_add_le_scan_disable(req);
> -
> -		memset(&param_cp, 0, sizeof(param_cp));
> -
> -		/* All active scans will be done with either a resolvable
> -		 * private address (when privacy feature has been enabled)
> -		 * or non-resolvable private address.
> -		 */
> -		err = hci_update_random_address(req, true, &own_addr_type);
> -		if (err < 0) {
> -			*status = MGMT_STATUS_FAILED;
> +		if (!trigger_le_scan(req, status,
> +				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
> 			return false;

I had to apply the patch see the resulting code, but I prefer if we turn this actually into nice code while we are at it. I pretty much dislike entering a case block and then checking the same value again.

        switch (hdev->discovery.type) {
        case DISCOV_TYPE_BREDR:
                if (!trigger_bredr_inquiry(req, status))
                        return false;
                break;

        case DISCOV_TYPE_INTERLEAVED:
                if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
                        *status = MGMT_STATUS_NOT_SUPPORTED;
                        return false;
                }
                /* fall through */

        case DISCOV_TYPE_LE:
               if (!trigger_le_scan(req, status,
                                     cpu_to_le16(DISCOV_LE_SCAN_INT)))
                        return false;
                break;

        default:
                *status = MGMT_STATUS_INVALID_PARAMS;
                return false;
        }

Regards

Marcel

[PATCH v4 3/7] Bluetooth: Split triggering le and interleaved scan into separate cases

[Jakub Pawlowski]

This patch splits triggering of le and interleaved scan into
separate cases.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 net/bluetooth/mgmt.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 92fa8e88..9d3c7a1 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4009,9 +4009,13 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
 		break;
 
 	case DISCOV_TYPE_LE:
+		if (!trigger_le_scan(req, status,
+				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
+			return false;
+		break;
+
 	case DISCOV_TYPE_INTERLEAVED:
-		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
-		    !hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
+		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
 			*status = MGMT_STATUS_NOT_SUPPORTED;
 			return false;
 		}
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 3/7] Bluetooth: Split triggering le and interleaved scan into separate cases

[Marcel Holtmann]

Hi Jakub,

> This patch splits triggering of le and interleaved scan into
> separate cases.
> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> net/bluetooth/mgmt.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 92fa8e88..9d3c7a1 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4009,9 +4009,13 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
> 		break;
> 
> 	case DISCOV_TYPE_LE:
> +		if (!trigger_le_scan(req, status,
> +				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
> +			return false;
> +		break;
> +
> 	case DISCOV_TYPE_INTERLEAVED:
> -		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> -		    !hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> +		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> 			*status = MGMT_STATUS_NOT_SUPPORTED;
> 			return false;
> 		}

see my re-ordering with a fall through statement from my previous comment. Lets split them out properly first and then the sequence on how things evolve will make more sense. And I think this patch is then no longer needed.

Regards

Marcel

[PATCH v4 4/7] Bluetooth: Add simultaneous dual mode scan

[Jakub Pawlowski]

When doing scan through mgmt api, some controllers can do both le and
classic scan at same time. They can be distinguished by
HCI_QUIRK_SIMULTANEOUS_DISCOVERY set.

This patch enables them to use this feature when doing dual mode scan.
Instead of doing le, then classic scan, both scans are run at once.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 net/bluetooth/hci_core.c  | 24 +++++++++++++++++++-----
 net/bluetooth/hci_event.c | 18 ++++++++++++++++--
 net/bluetooth/mgmt.c      | 36 ++++++++++++++++++++++++++++++------
 3 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 750d344..3d51bf7 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2902,12 +2902,26 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status,
 
 		hci_dev_lock(hdev);
 
-		hci_inquiry_cache_flush(hdev);
+		if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
+			     &hdev->quirks)) {
+			/* If we were running le only scan, change discovery
+			 * state. If we were running both le and classic scans
+			 * simultaneously, and classic is already finished,
+			 * stop discovery, otherwise  classic scan will stop
+			 * discovery when finished.
+			 */
+			if (!test_bit(HCI_INQUIRY, &hdev->flags))
+				hci_discovery_set_state(hdev,
+							DISCOVERY_STOPPED);
+		} else {
+			hci_inquiry_cache_flush(hdev);
 
-		err = hci_req_run(&req, inquiry_complete);
-		if (err) {
-			BT_ERR("Inquiry request failed: err %d", err);
-			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+			err = hci_req_run(&req, inquiry_complete);
+			if (err) {
+				BT_ERR("Inquiry request failed: err %d", err);
+				hci_discovery_set_state(hdev,
+							DISCOVERY_STOPPED);
+			}
 		}
 
 		hci_dev_unlock(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d800f0c..4a3cc58 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2126,7 +2126,14 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		goto unlock;
 
 	if (list_empty(&discov->resolve)) {
-		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		/* if we were running BR/EDR inquiry change discovery state.
+		 * If we were running both BR/EDR inquiry and LE scan
+		 * simultaneously, and LE is already finished, change state,
+		 * otherwise LE scan will stop discovery when finished.
+		 */
+		if (!hci_dev_test_flag(hdev, HCI_LE_SCAN) ||
+		    !test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		goto unlock;
 	}
 
@@ -2135,7 +2142,14 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		e->name_state = NAME_PENDING;
 		hci_discovery_set_state(hdev, DISCOVERY_RESOLVING);
 	} else {
-		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		/* if we were running classic discovery change discovery state.
+		 * If we were running both le and classic scans simultaneously,
+		 * and le is already finished, change state, otherwise le
+		 * scan will stop discovery when finished.
+		 */
+		if (!hci_dev_test_flag(hdev, HCI_LE_SCAN) ||
+		    !test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 	}
 
 unlock:
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 9d3c7a1..ef4db8a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1402,7 +1402,9 @@ static bool hci_stop_discovery(struct hci_request *req)
 	case DISCOVERY_FINDING:
 		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
 			hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
-		} else {
+		}
+
+		if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
 			cancel_delayed_work(&hdev->le_scan_disable);
 			hci_req_add_le_scan_disable(req);
 		}
@@ -4015,13 +4017,27 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
-		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
-			*status = MGMT_STATUS_NOT_SUPPORTED;
-			return false;
+		if (!test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
+			      &hdev->quirks)) {
+			if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
+				*status = MGMT_STATUS_NOT_SUPPORTED;
+				return false;
+			}
+
+			if (!trigger_le_scan(req, status,
+					     cpu_to_le16(DISCOV_LE_SCAN_INT)))
+				return false;
+			return true;
 		}
 
+		/* During simultaneous scan, we double scan interval. We must
+		 * leave some time to do BR/EDR scan.
+		 */
 		if (!trigger_le_scan(req, status,
-				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
+				     cpu_to_le16(DISCOV_LE_SCAN_INT * 2)))
+			return false;
+
+		if (!trigger_bredr_inquiry(req, status))
 			return false;
 		break;
 
@@ -4067,7 +4083,15 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status,
 		timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
 		break;
 	case DISCOV_TYPE_INTERLEAVED:
-		timeout = msecs_to_jiffies(hdev->discov_interleaved_timeout);
+		/* if we're doing simultaneous discovery, LE scan should last
+		 * whole time. If we do interleaved scan, LE scan last only half
+		 * of that (BR/EDR inquiry takes rest of time).
+		 */
+		if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
+			timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
+		else
+			timeout = msecs_to_jiffies(
+					     hdev->discov_interleaved_timeout);
 		break;
 	case DISCOV_TYPE_BREDR:
 		timeout = 0;
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 4/7] Bluetooth: Add simultaneous dual mode scan

[Marcel Holtmann]

Hi Jakub,

> When doing scan through mgmt api, some controllers can do both le and
> classic scan at same time. They can be distinguished by
> HCI_QUIRK_SIMULTANEOUS_DISCOVERY set.
> 
> This patch enables them to use this feature when doing dual mode scan.
> Instead of doing le, then classic scan, both scans are run at once.
> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> net/bluetooth/hci_core.c  | 24 +++++++++++++++++++-----
> net/bluetooth/hci_event.c | 18 ++++++++++++++++--
> net/bluetooth/mgmt.c      | 36 ++++++++++++++++++++++++++++++------
> 3 files changed, 65 insertions(+), 13 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 750d344..3d51bf7 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2902,12 +2902,26 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status,
> 
> 		hci_dev_lock(hdev);
> 
> -		hci_inquiry_cache_flush(hdev);
> +		if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
> +			     &hdev->quirks)) {
> +			/* If we were running le only scan, change discovery

Please name it LE only scan.

> +			 * state. If we were running both le and classic scans

LE scan and BR/EDR inquiry.

> +			 * simultaneously, and classic is already finished,

BR/EDR inquiry.

> +			 * stop discovery, otherwise  classic scan will stop

Single spaces are fine. And please also call it BR/EDR inquiry.

> +			 * discovery when finished.
> +			 */

			/* When only LE scanning is running, then change discovery
			 * state to indicate that it completed. In case LE scanning
			 * and BR/EDR inquiry is active and the inquiry completed
			 * already, then also change the discovery state.
			 *
			 * In case only BR/EDR inquiry is running, then it will
			 * change the discovery state when it finished. Nothing to
			 * do here in that case.
			 */

So I would have written it something like this to make why we are doing it this way. If I understood you correctly.

> +			if (!test_bit(HCI_INQUIRY, &hdev->flags))
> +				hci_discovery_set_state(hdev,
> +							DISCOVERY_STOPPED);
> +		} else {
> +			hci_inquiry_cache_flush(hdev);
> 
> -		err = hci_req_run(&req, inquiry_complete);
> -		if (err) {
> -			BT_ERR("Inquiry request failed: err %d", err);
> -			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +			err = hci_req_run(&req, inquiry_complete);
> +			if (err) {
> +				BT_ERR("Inquiry request failed: err %d", err);
> +				hci_discovery_set_state(hdev,
> +							DISCOVERY_STOPPED);
> +			}
> 		}
> 
> 		hci_dev_unlock(hdev);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d800f0c..4a3cc58 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2126,7 +2126,14 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		goto unlock;
> 
> 	if (list_empty(&discov->resolve)) {
> -		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +		/* if we were running BR/EDR inquiry change discovery state.

> +		 * If we were running both BR/EDR inquiry and LE scan
> +		 * simultaneously, and LE is already finished, change state,
> +		 * otherwise LE scan will stop discovery when finished.
> +		 */

		/* When BR/EDR inquiry is active and not LE scanning is in
		 * progress, then change discovery state to indicate completion.
		 *
		 * When running LE scanning and BR/EDR inquiry simultaneously
		 * and the LE scan already finished, then change the discovery
		 * state to indicate completion.
		 */

> +		if (!hci_dev_test_flag(hdev, HCI_LE_SCAN) ||
> +		    !test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
> +			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 		goto unlock;
> 	}
> 
> @@ -2135,7 +2142,14 @@ static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		e->name_state = NAME_PENDING;
> 		hci_discovery_set_state(hdev, DISCOVERY_RESOLVING);
> 	} else {
> -		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +		/* if we were running classic discovery change discovery state.
> +		 * If we were running both le and classic scans simultaneously,
> +		 * and le is already finished, change state, otherwise le
> +		 * scan will stop discovery when finished.
> +		 */

I leave this as an exercise to reword it similar to the two other statements.

> +		if (!hci_dev_test_flag(hdev, HCI_LE_SCAN) ||
> +		    !test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
> +			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 	}
> 
> unlock:
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 9d3c7a1..ef4db8a 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1402,7 +1402,9 @@ static bool hci_stop_discovery(struct hci_request *req)
> 	case DISCOVERY_FINDING:
> 		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> 			hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> -		} else {
> +		}
> +

If this is correct, then the { } can be removed now.

> +		if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
> 			cancel_delayed_work(&hdev->le_scan_disable);
> 			hci_req_add_le_scan_disable(req);
> 		}
> @@ -4015,13 +4017,27 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
> 		break;
> 
> 	case DISCOV_TYPE_INTERLEAVED:
> -		if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> -			*status = MGMT_STATUS_NOT_SUPPORTED;
> -			return false;
> +		if (!test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
> +			      &hdev->quirks)) {
> +			if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED)) {
> +				*status = MGMT_STATUS_NOT_SUPPORTED;
> +				return false;
> +			}
> +
> +			if (!trigger_le_scan(req, status,
> +					     cpu_to_le16(DISCOV_LE_SCAN_INT)))
> +				return false;

Extra empty space here.

> +			return true;
> 		}
> 
> +		/* During simultaneous scan, we double scan interval. We must

Lets call it discovery and not scan. It is also LE scan interval.

> +		 * leave some time to do BR/EDR scan.

some time for the controller to execute BR/EDR inquiry.

> +		 */
> 		if (!trigger_le_scan(req, status,
> -				     cpu_to_le16(DISCOV_LE_SCAN_INT)))
> +				     cpu_to_le16(DISCOV_LE_SCAN_INT * 2)))
> +			return false;
> +
> +		if (!trigger_bredr_inquiry(req, status))
> 			return false;
> 		break;
> 
> @@ -4067,7 +4083,15 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status,
> 		timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
> 		break;
> 	case DISCOV_TYPE_INTERLEAVED:
> -		timeout = msecs_to_jiffies(hdev->discov_interleaved_timeout);
> +		/* if we're doing simultaneous discovery, LE scan should last
> +		 * whole time. If we do interleaved scan, LE scan last only half
> +		 * of that (BR/EDR inquiry takes rest of time).
> +		 */

		/* When running simultaneous discovery, the LE scanning time
		 * should occupy the whole discovery time sine BR/EDR inquiry
		 * and LE scanning are scheduled by the controller.
		 *
		 * For interleaving discovery in comparison, BR/EDR inquiry
		 * and LE scanning are done sequentially with separate
		 * timeouts.
		 */

> +		if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks))
> +			timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
> +		else
> +			timeout = msecs_to_jiffies(
> +					     hdev->discov_interleaved_timeout);

This indentation is odd. Just go over 80 characters here.

> 		break;
> 	case DISCOV_TYPE_BREDR:
> 		timeout = 0;

Regards

Marcel

[PATCH v4 5/7] Bluetooth: Set HCI_QUIRK_SIMULTANEOUS_DISCOVERY for BTUSB_ATH3012

[Jakub Pawlowski]

Atheros controllers can do both LE and classic scan at once.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 drivers/bluetooth/btusb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 6fa9338..7c3c2a1 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3042,6 +3042,7 @@ static int btusb_probe(struct usb_interface *intf,
 
 	if (id->driver_info & BTUSB_ATH3012) {
 		hdev->set_bdaddr = btusb_set_bdaddr_ath3012;
+		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 	}
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v4 6/7] Bluetooth: Set HCI_QUIRK_SIMULTANEOUS_DISCOVERY for BTUSB_INTEL

[Jakub Pawlowski]

Intel controllers can do both LE and classic scan at once.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 drivers/bluetooth/btusb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7c3c2a1..879b55e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3019,6 +3019,7 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->shutdown = btusb_shutdown_intel;
 		hdev->set_bdaddr = btusb_set_bdaddr_intel;
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
+		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 	}
 
 	if (id->driver_info & BTUSB_INTEL_NEW) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 6/7] Bluetooth: Set HCI_QUIRK_SIMULTANEOUS_DISCOVERY for BTUSB_INTEL

[Marcel Holtmann]

Hi Jakub,

> Intel controllers can do both LE and classic scan at once.

please name it LE scanning and BR/EDR inquiry. The classic term is not really defined.

Regards

Marcel

[PATCH v4 7/7] Bluetooth: Set HCI_QUIRK_SIMULTANEOUS_DISCOVERY for BTUSB_CSR

[Jakub Pawlowski]

CSR controllers can do both LE and classic scan at once.

Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
 drivers/bluetooth/btusb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 879b55e..a2a7310 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3087,6 +3087,8 @@ static int btusb_probe(struct usb_interface *intf,
 		/* Fake CSR devices with broken commands */
 		if (bcdDevice <= 0x100)
 			hdev->setup = btusb_setup_csr;
+
+		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 	}
 
 	if (id->driver_info & BTUSB_SNIFFER) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v4 1/7] Bluetooth: Introduce HCI_QUIRK_SIMULTANEOUS_DISCOVERY

[Marcel Holtmann]

Hi Jakub,

> Some controllers allow both LE scan and BR/EDR inquiry to run at
> the same time, while others allow only one, LE SCAN or BR/EDR
> inquiry at given time.
> 
> Since this is specific to each controller, add a new quirk setting
> that allows drivers to tell the core wether given controller can
> do both LE scan and BR/EDR inquiry at same time.
> 
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ce75730..7091601 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -160,6 +160,15 @@ enum {
> 	 * during the hdev->setup vendor callback.
> 	 */
> 	HCI_QUIRK_STRICT_DUPLICATE_FILTER,
> +
> +	/* When this quirk is set, LE scan and BR/EDR inquiry is done
> +	 * simultaneously, otherwise it's interleaved.
> +	 * .

you might to remove this line with the lonely dot. Looks like a leftover.

> +	 *
> +	 * This quirk can be set before hci_register_dev is called or
> +	 * during the hdev->setup vendor callback.
> +	 */
> +	HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
> };

Regards

Marcel