[PATCH v1 1/5] Bluetooth: Introduce HCI_QUIRK_SIMULTAENOUS_DISCOVERY

[PATCH v1 1/5] Bluetooth: Introduce HCI_QUIRK_SIMULTAENOUS_DISCOVERY

[Jakub Pawlowski]

Some controllers allow both le and classic scan to run at the same
time, while others allow only one, le or classic 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 and classic discovery 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 8e54f82..d64851a 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, scanning for both le and classic devices
+	 * is done simoultaenously. If it's not set, the scan is
+	 * interleaved.
+	 *
+	 * This quirk can be set before hci_register_dev is called or
+	 * during the hdev->setup vendor callback.
+	 */
+	HCI_QUIRK_SIMULTAENOUS_DISCOVERY,
 };
 
 /* HCI device flags */
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v1 2/5] Bluetooth: Add simultaenous 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_SIMULTAENOUS_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>
---
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         | 26 ++++++++++++----
 net/bluetooth/hci_event.c        | 18 +++++++++--
 net/bluetooth/mgmt.c             | 66 ++++++++++++++++++++++++++++------------
 4 files changed, 84 insertions(+), 27 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index acec914..5cd9451 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1285,6 +1285,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
 #define DISCOV_LE_SCAN_WIN		0x12
 #define DISCOV_LE_SCAN_INT		0x12
 #define DISCOV_LE_TIMEOUT		10240	/* msec */
+#define DISCOV_LE_SIMULTAENOUS_SCAN_WIN	0x24
 #define DISCOV_INTERLEAVED_TIMEOUT	5120	/* msec */
 #define DISCOV_INTERLEAVED_INQUIRY_LEN	0x04
 #define DISCOV_BREDR_INQUIRY_LEN	0x08
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbd26bc..c4f2316 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2866,12 +2866,26 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status,
 
 		hci_dev_lock(hdev);
 
-		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);
+		if (!test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY,
+			      &hdev->quirks)) {
+			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);
+			}
+		} else {
+			/* If we were running le only scan, change discovery
+			 * state. If we were running both le and classic scans
+			 * simultaenously, 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);
 		}
 
 		hci_dev_unlock(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e9b17b5..d69de31 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2127,7 +2127,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 classic discovery change discovery state.
+		 * If we were running both le and classic scans simultaenously,
+		 * and le is already finished, change state, otherwise le
+		 * scan will stop discovery when finished.
+		 */
+		if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
+		    !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 		goto unlock;
 	}
 
@@ -2136,7 +2143,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 simultaenously,
+		 * and le is already finished, change state, otherwise le
+		 * scan will stop discovery when finished.
+		 */
+		if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
+		    !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
 	}
 
 unlock:
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1e4635a..9d95290 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3783,34 +3783,44 @@ done:
 	return err;
 }
 
-static bool trigger_discovery(struct hci_request *req, u8 *status)
+static bool trigger_classic_discovery(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 (test_bit(HCI_INQUIRY, &hdev->flags)) {
+		*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_discovery(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;
+	__le16 interval;
 	u8 own_addr_type;
 	int err;
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
-		*status = mgmt_bredr_support(hdev);
-		if (*status)
-			return false;
-
-		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
-			*status = MGMT_STATUS_BUSY;
+		if (!trigger_classic_discovery(req, status))
 			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);
 		break;
 
 	case DISCOV_TYPE_LE:
@@ -3858,8 +3868,17 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
 			return false;
 		}
 
+		/* During simultaenous scan, scan interval can't be equal to
+		 * scan window. We must leave some time to do BR/EDR scan.
+		 */
+		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+		    test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
+			interval = cpu_to_le16(DISCOV_LE_SIMULTAENOUS_SCAN_WIN);
+		else
+			interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
+
 		param_cp.type = LE_SCAN_ACTIVE;
-		param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
+		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),
@@ -3870,6 +3889,11 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
 		enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
 		hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
 			    &enable_cp);
+
+		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+		    test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
+			if (!trigger_classic_discovery(req, status))
+				return false;
 		break;
 
 	default:
@@ -3914,7 +3938,11 @@ 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 (test_bit(HCI_QUIRK_SIMULTAENOUS_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

[PATCH v1 3/5] Bluetooth: Set HCI_QUIRK_SIMULTAENOUS_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 3ca2e1b..119c77c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2777,6 +2777,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_SIMULTAENOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 	}
 
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v1 4/5] Bluetooth: Set HCI_QUIRK_SIMULTAENOUS_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 119c77c..90fbda3 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2754,6 +2754,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_SIMULTAENOUS_DISCOVERY, &hdev->quirks);
 	}
 
 	if (id->driver_info & BTUSB_INTEL_NEW) {
-- 
2.2.0.rc0.207.ga3a616c

[PATCH v1 5/5] Bluetooth: Set HCI_QUIRK_SIMULTAENOUS_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 90fbda3..9352c36 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2817,6 +2817,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_SIMULTAENOUS_DISCOVERY, &hdev->quirks);
 	}
 
 	if (id->driver_info & BTUSB_SNIFFER) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH v1 1/5] Bluetooth: Introduce HCI_QUIRK_SIMULTAENOUS_DISCOVERY

[Jakub Pawlowski]

On Mon, Mar 2, 2015 at 1:30 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>
> Some controllers allow both le and classic scan to run at the same
> time, while others allow only one, le or classic 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 and classic discovery 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 8e54f82..d64851a 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, scanning for both le and classic devices
> +        * is done simoultaenously. If it's not set, the scan is
> +        * interleaved.
> +        *
> +        * This quirk can be set before hci_register_dev is called or
> +        * during the hdev->setup vendor callback.
> +        */
> +       HCI_QUIRK_SIMULTAENOUS_DISCOVERY,
>  };
>
>  /* HCI device flags */
> --
> 2.2.0.rc0.207.ga3a616c
>

Ping?

Re: [PATCH v1 1/5] Bluetooth: Introduce HCI_QUIRK_SIMULTAENOUS_DISCOVERY

[Marcel Holtmann]

Hi Jakub,

> Some controllers allow both le and classic scan to run at the same
> time, while others allow only one, le or classic at given time.

lets call it BR/EDR inquiry and LE scan.

> 
> 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 and classic discovery 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 8e54f82..d64851a 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, scanning for both le and classic devices
> +	 * is done simoultaenously. If it's not set, the scan is
> +	 * interleaved.

	/* When this quirk is set, then the controller supports running
	 * BR/EDR inquiry and LE scanning simultaneously. If it is not
	 * set, then BR/EDR inquiry and LE scanning will be interleaved.
	 *
	 * ..

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

Regards

Marcel

Re: [PATCH v1 2/5] Bluetooth: Add simultaenous 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_SIMULTAENOUS_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>
> ---
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_core.c         | 26 ++++++++++++----
> net/bluetooth/hci_event.c        | 18 +++++++++--
> net/bluetooth/mgmt.c             | 66 ++++++++++++++++++++++++++++------------
> 4 files changed, 84 insertions(+), 27 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index acec914..5cd9451 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1285,6 +1285,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> #define DISCOV_LE_SCAN_WIN		0x12
> #define DISCOV_LE_SCAN_INT		0x12
> #define DISCOV_LE_TIMEOUT		10240	/* msec */
> +#define DISCOV_LE_SIMULTAENOUS_SCAN_WIN	0x24

actually to keep the number of constants to a minimum, I would just make sure the code uses DISCOV_LE_SCAN_WIN * 2 with a comment on top of explaining why. And that comment should be above the code that widens the scan window.

> #define DISCOV_INTERLEAVED_TIMEOUT	5120	/* msec */
> #define DISCOV_INTERLEAVED_INQUIRY_LEN	0x04
> #define DISCOV_BREDR_INQUIRY_LEN	0x08
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index dbd26bc..c4f2316 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2866,12 +2866,26 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status,
> 
> 		hci_dev_lock(hdev);
> 
> -		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);
> +		if (!test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY,
> +			      &hdev->quirks)) {
> +			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);
> +			}
> +		} else {
> +			/* If we were running le only scan, change discovery
> +			 * state. If we were running both le and classic scans
> +			 * simultaenously, 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);
> 		}

I we have an else branch anyway and they are both roughly the same size, then there is no good reason to start the negative one. Just flip these around.

> 
> 		hci_dev_unlock(hdev);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index e9b17b5..d69de31 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2127,7 +2127,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 classic discovery change discovery state.
> +		 * If we were running both le and classic scans simultaenously,

The capitalization is off here a bit. Also spelling of simultaneously seems wrong.

> +		 * and le is already finished, change state, otherwise le
> +		 * scan will stop discovery when finished.

Personally I prefer to use LE and BR/EDR terms in clear uppercase in comments. We want to be a bit consistent in our comments. Classic discovery is actually BR/EDR inquiry and we have LE scan. We should use these terms.

> +		 */
> +		if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
> +		    !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
> +			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 		goto unlock;
> 	}
> 
> @@ -2136,7 +2143,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 simultaenously,
> +		 * and le is already finished, change state, otherwise le
> +		 * scan will stop discovery when finished.
> +		 */
> +		if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
> +		    !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
> +			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 	}

I wonder now if it would not be better if hci_discovery_set_state handle this for us. Seems we are calling that function in all cases. So it could just do the right thing. Any reason you did it otherwise here?

> 
> unlock:
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1e4635a..9d95290 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3783,34 +3783,44 @@ done:
> 	return err;
> }
> 
> -static bool trigger_discovery(struct hci_request *req, u8 *status)
> +static bool trigger_classic_discovery(struct hci_request *req, u8 *status)

Please do not call this classic discovery. It is inquiry or bredr_inquiry.

> {
> 	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 (test_bit(HCI_INQUIRY, &hdev->flags)) {
> +		*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_discovery(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;
> +	__le16 interval;
> 	u8 own_addr_type;
> 	int err;
> 
> 	switch (hdev->discovery.type) {
> 	case DISCOV_TYPE_BREDR:
> -		*status = mgmt_bredr_support(hdev);
> -		if (*status)
> -			return false;
> -
> -		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> -			*status = MGMT_STATUS_BUSY;
> +		if (!trigger_classic_discovery(req, status))
> 			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);
> 		break;
> 
> 	case DISCOV_TYPE_LE:
> @@ -3858,8 +3868,17 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
> 			return false;
> 		}
> 
> +		/* During simultaenous scan, scan interval can't be equal to
> +		 * scan window. We must leave some time to do BR/EDR scan.
> +		 */
> +		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> +		    test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
> +			interval = cpu_to_le16(DISCOV_LE_SIMULTAENOUS_SCAN_WIN);
> +		else
> +			interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
> +

so I think we might need a bit of refactoring of the code now. Previously this was combining the LE and interleaved case statements into one since both start with LE scan. However now that is no longer the case.

I really dislike hacking if case in here checking for discovery.type since that is what the switch statement is for in the first place. So we need to split the inquiry start and le scan start out into nice little helpers and then call them accordingly. However it is clear the DISCOV_TYPE_INTERLEAVED now gets its own case statement.

> 		param_cp.type = LE_SCAN_ACTIVE;
> -		param_cp.interval = cpu_to_le16(DISCOV_LE_SCAN_INT);
> +		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),
> @@ -3870,6 +3889,11 @@ static bool trigger_discovery(struct hci_request *req, u8 *status)
> 		enable_cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
> 		hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> 			    &enable_cp);
> +
> +		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> +		    test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
> +			if (!trigger_classic_discovery(req, status))
> +				return false;
> 		break;
> 
> 	default:
> @@ -3914,7 +3938,11 @@ 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 (test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quirks))
> +			timeout = msecs_to_jiffies(DISCOV_LE_TIMEOUT);
> +		else
> +			timeout = msecs_to_jiffies(
> +					     hdev->discov_interleaved_timeout);

This one you need explain to me. I do not understand this change.

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

Regards

Marcel

Re: [PATCH v1 2/5] Bluetooth: Add simultaenous dual mode scan

[Jakub Pawlowski]

On Tue, Mar 10, 2015 at 7:55 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> 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_SIMULTAENOUS_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>
>> ---
>> include/net/bluetooth/hci_core.h |  1 +
>> net/bluetooth/hci_core.c         | 26 ++++++++++++----
>> net/bluetooth/hci_event.c        | 18 +++++++++--
>> net/bluetooth/mgmt.c             | 66 ++++++++++++++++++++++++++++------=
------
>> 4 files changed, 84 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hc=
i_core.h
>> index acec914..5cd9451 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1285,6 +1285,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int =
event);
>> #define DISCOV_LE_SCAN_WIN            0x12
>> #define DISCOV_LE_SCAN_INT            0x12
>> #define DISCOV_LE_TIMEOUT             10240   /* msec */
>> +#define DISCOV_LE_SIMULTAENOUS_SCAN_WIN      0x24
>
> actually to keep the number of constants to a minimum, I would just make =
sure the code uses DISCOV_LE_SCAN_WIN * 2 with a comment on top of explaini=
ng why. And that comment should be above the code that widens the scan wind=
ow.
>
>> #define DISCOV_INTERLEAVED_TIMEOUT    5120    /* msec */
>> #define DISCOV_INTERLEAVED_INQUIRY_LEN        0x04
>> #define DISCOV_BREDR_INQUIRY_LEN      0x08
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index dbd26bc..c4f2316 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2866,12 +2866,26 @@ static void le_scan_disable_work_complete(struct=
 hci_dev *hdev, u8 status,
>>
>>               hci_dev_lock(hdev);
>>
>> -             hci_inquiry_cache_flush(hdev);
>> -
>> -             err =3D hci_req_run(&req, inquiry_complete);
>> -             if (err) {
>> -                     BT_ERR("Inquiry request failed: err %d", err);
>> -                     hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +             if (!test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY,
>> +                           &hdev->quirks)) {
>> +                     hci_inquiry_cache_flush(hdev);
>> +
>> +                     err =3D hci_req_run(&req, inquiry_complete);
>> +                     if (err) {
>> +                             BT_ERR("Inquiry request failed: err %d", e=
rr);
>> +                             hci_discovery_set_state(hdev,
>> +                                                     DISCOVERY_STOPPED)=
;
>> +                     }
>> +             } else {
>> +                     /* If we were running le only scan, change discove=
ry
>> +                      * state. If we were running both le and classic s=
cans
>> +                      * simultaenously, and classic is already finished=
,
>> +                      * stop discovery, otherwise  classic scan will st=
op
>> +                      * discovery when finished.
>> +                      */
>> +                     if (!test_bit(HCI_INQUIRY, &hdev->flags))
>> +                             hci_discovery_set_state(hdev,
>> +                                                     DISCOVERY_STOPPED)=
;
>>               }
>
> I we have an else branch anyway and they are both roughly the same size, =
then there is no good reason to start the negative one. Just flip these aro=
und.
>
>>
>>               hci_dev_unlock(hdev);
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index e9b17b5..d69de31 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -2127,7 +2127,14 @@ static void hci_inquiry_complete_evt(struct hci_d=
ev *hdev, struct sk_buff *skb)
>>               goto unlock;
>>
>>       if (list_empty(&discov->resolve)) {
>> -             hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +             /* if we were running classic discovery change discovery s=
tate.
>> +              * If we were running both le and classic scans simultaeno=
usly,
>
> The capitalization is off here a bit. Also spelling of simultaneously see=
ms wrong.
>
>> +              * and le is already finished, change state, otherwise le
>> +              * scan will stop discovery when finished.
>
> Personally I prefer to use LE and BR/EDR terms in clear uppercase in comm=
ents. We want to be a bit consistent in our comments. Classic discovery is =
actually BR/EDR inquiry and we have LE scan. We should use these terms.
>
>> +              */
>> +             if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
>> +                 !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->qui=
rks))
>> +                     hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>               goto unlock;
>>       }
>>
>> @@ -2136,7 +2143,14 @@ static void hci_inquiry_complete_evt(struct hci_d=
ev *hdev, struct sk_buff *skb)
>>               e->name_state =3D 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 s=
tate.
>> +              * If we were running both le and classic scans simultaeno=
usly,
>> +              * and le is already finished, change state, otherwise le
>> +              * scan will stop discovery when finished.
>> +              */
>> +             if (!test_bit(HCI_LE_SCAN, &hdev->flags) ||
>> +                 !test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->qui=
rks))
>> +                     hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>       }
>
> I wonder now if it would not be better if hci_discovery_set_state handle =
this for us. Seems we are calling that function in all cases. So it could j=
ust do the right thing. Any reason you did it otherwise here?
>
>>
>> unlock:
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 1e4635a..9d95290 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -3783,34 +3783,44 @@ done:
>>       return err;
>> }
>>
>> -static bool trigger_discovery(struct hci_request *req, u8 *status)
>> +static bool trigger_classic_discovery(struct hci_request *req, u8 *stat=
us)
>
> Please do not call this classic discovery. It is inquiry or bredr_inquiry=
.
>
>> {
>>       struct hci_dev *hdev =3D 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] =3D { 0x33, 0x8b, 0x9e };
>> +
>> +     *status =3D mgmt_bredr_support(hdev);
>> +     if (*status)
>> +             return false;
>> +
>> +     if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>> +             *status =3D 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 =3D DISCOV_BREDR_INQUIRY_LEN;
>> +     hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
>> +     return true;
>> +}
>> +
>> +static bool trigger_discovery(struct hci_request *req, u8 *status)
>> +{
>> +     struct hci_dev *hdev =3D req->hdev;
>> +     struct hci_cp_le_set_scan_param param_cp;
>> +     struct hci_cp_le_set_scan_enable enable_cp;
>> +     __le16 interval;
>>       u8 own_addr_type;
>>       int err;
>>
>>       switch (hdev->discovery.type) {
>>       case DISCOV_TYPE_BREDR:
>> -             *status =3D mgmt_bredr_support(hdev);
>> -             if (*status)
>> -                     return false;
>> -
>> -             if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>> -                     *status =3D MGMT_STATUS_BUSY;
>> +             if (!trigger_classic_discovery(req, status))
>>                       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 =3D DISCOV_BREDR_INQUIRY_LEN;
>> -             hci_req_add(req, HCI_OP_INQUIRY, sizeof(inq_cp), &inq_cp);
>>               break;
>>
>>       case DISCOV_TYPE_LE:
>> @@ -3858,8 +3868,17 @@ static bool trigger_discovery(struct hci_request =
*req, u8 *status)
>>                       return false;
>>               }
>>
>> +             /* During simultaenous scan, scan interval can't be equal =
to
>> +              * scan window. We must leave some time to do BR/EDR scan.
>> +              */
>> +             if (hdev->discovery.type =3D=3D DISCOV_TYPE_INTERLEAVED &&
>> +                 test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quir=
ks))
>> +                     interval =3D cpu_to_le16(DISCOV_LE_SIMULTAENOUS_SC=
AN_WIN);
>> +             else
>> +                     interval =3D cpu_to_le16(DISCOV_LE_SCAN_INT);
>> +
>
> so I think we might need a bit of refactoring of the code now. Previously=
 this was combining the LE and interleaved case statements into one since b=
oth start with LE scan. However now that is no longer the case.
>
> I really dislike hacking if case in here checking for discovery.type sinc=
e that is what the switch statement is for in the first place. So we need t=
o split the inquiry start and le scan start out into nice little helpers an=
d then call them accordingly. However it is clear the DISCOV_TYPE_INTERLEAV=
ED now gets its own case statement.
>
>>               param_cp.type =3D LE_SCAN_ACTIVE;
>> -             param_cp.interval =3D cpu_to_le16(DISCOV_LE_SCAN_INT);
>> +             param_cp.interval =3D interval;
>>               param_cp.window =3D cpu_to_le16(DISCOV_LE_SCAN_WIN);
>>               param_cp.own_address_type =3D own_addr_type;
>>               hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(param_cp=
),
>> @@ -3870,6 +3889,11 @@ static bool trigger_discovery(struct hci_request =
*req, u8 *status)
>>               enable_cp.filter_dup =3D LE_SCAN_FILTER_DUP_ENABLE;
>>               hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_=
cp),
>>                           &enable_cp);
>> +
>> +             if (hdev->discovery.type =3D=3D DISCOV_TYPE_INTERLEAVED &&
>> +                 test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quir=
ks))
>> +                     if (!trigger_classic_discovery(req, status))
>> +                             return false;
>>               break;
>>
>>       default:
>> @@ -3914,7 +3938,11 @@ static void start_discovery_complete(struct hci_d=
ev *hdev, u8 status,
>>               timeout =3D msecs_to_jiffies(DISCOV_LE_TIMEOUT);
>>               break;
>>       case DISCOV_TYPE_INTERLEAVED:
>> -             timeout =3D msecs_to_jiffies(hdev->discov_interleaved_time=
out);
>> +             if (test_bit(HCI_QUIRK_SIMULTAENOUS_DISCOVERY, &hdev->quir=
ks))
>> +                     timeout =3D msecs_to_jiffies(DISCOV_LE_TIMEOUT);
>> +             else
>> +                     timeout =3D msecs_to_jiffies(
>> +                                          hdev->discov_interleaved_time=
out);
>
> This one you need explain to me. I do not understand this change.

If you use simultaenous discovery, then stop LE sacn after 10 seconds,
if doing interleaved, then stop after 5 seconds, BR/EDR inquiry will
take another 5 seconds.

I'll fix all other places
>
>>               break;
>>       case DISCOV_TYPE_BREDR:
>>               timeout =3D 0;
>
> Regards
>
> Marcel
>