[RFC 0/4] MGMT Start Discovery interleaved support

[RFC 0/4] MGMT Start Discovery interleaved support

[Andre Guedes]

Hi all,

This RFC series adds interleaved discovery support to MGMT Start Discovery
command.

As previously discussed (see [PATCH] Don't set LE flags in mgmt_start_
discovery), if an interleaved Start Discovery command is issued but the
device is not dual mode, we perform a regular BR/EDR or LE-only discovery
(according to device capabilities) instead of returning error.

BR,

Andre


Andre Guedes (4):
  Bluetooth: Prepare start_discovery
  Bluetooth: Track discovery type
  Bluetooth: Merge INQUIRY and LE_SCAN discovery states
  Bluetooth: Interleaved discovery support

 include/net/bluetooth/hci_core.h |   10 +++++-
 net/bluetooth/hci_core.c         |    8 ++--
 net/bluetooth/hci_event.c        |   19 +++++++----
 net/bluetooth/mgmt.c             |   64 ++++++++++++++++++++++++++++++++++---
 4 files changed, 82 insertions(+), 19 deletions(-)

-- 
1.7.9.1

[RFC 1/4] Bluetooth: Prepare start_discovery

[Andre Guedes]

This patch does some code refactoring in start_discovery function
in order to prepare it for interleaved discovery support.

The discovery type macros were defined according to mgmt-api.txt
specification:

Possible values for the Type parameter are a bit-wise or of the
following bits:

	1	BR/EDR
	2	LE Public
	3	LE Random

By combining these e.g. the following values are possible:

	1	BR/EDR
	6	LE (public & random)
	7	BR/EDR/LE (interleaved discovery)

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    4 ++++
 net/bluetooth/mgmt.c             |   15 ++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b20d990..9982179 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -28,6 +28,10 @@
 #include <linux/interrupt.h>
 #include <net/bluetooth/hci.h>
 
+#define DISCOV_TYPE_BREDR		(BIT(0))
+#define DISCOV_TYPE_LE			(BIT(1) | BIT(2))
+#define DISCOV_TYPE_INTERLEAVED		(BIT(0) | BIT(1) | BIT(2))
+
 /* HCI priority */
 #define HCI_PRIO_MAX	7
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 066d338..410392e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2037,7 +2037,6 @@ static int start_discovery(struct sock *sk, u16 index,
 						void *data, u16 len)
 {
 	struct mgmt_cp_start_discovery *cp = data;
-	unsigned long discov_type = cp->type;
 	struct pending_cmd *cmd;
 	struct hci_dev *hdev;
 	int err;
@@ -2073,14 +2072,20 @@ static int start_discovery(struct sock *sk, u16 index,
 		goto failed;
 	}
 
-	if (test_bit(MGMT_ADDR_BREDR, &discov_type))
+	switch (cp->type) {
+	case DISCOV_TYPE_BREDR:
+	case DISCOV_TYPE_INTERLEAVED:
 		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
-	else if (test_bit(MGMT_ADDR_LE_PUBLIC, &discov_type) &&
-				test_bit(MGMT_ADDR_LE_RANDOM, &discov_type))
+		break;
+
+	case DISCOV_TYPE_LE:
 		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
 					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
-	else
+		break;
+
+	default:
 		err = -EINVAL;
+	}
 
 	if (err < 0)
 		mgmt_pending_remove(cmd);
-- 
1.7.9.1

Re: [RFC 1/4] Bluetooth: Prepare start_discovery

[Johan Hedberg]

Hi Andre,

On Thu, Feb 16, 2012, Andre Guedes wrote:
> +#define DISCOV_TYPE_BREDR		(BIT(0))
> +#define DISCOV_TYPE_LE			(BIT(1) | BIT(2))
> +#define DISCOV_TYPE_INTERLEAVED		(BIT(0) | BIT(1) | BIT(2))

Defining these like this looks a bit (no pun intended) odd. The previous
code was at least using the mgmt defs instead of magic numbers (which is
how these values have been originally chosen too).

Johan

Re: [RFC 1/4] Bluetooth: Prepare start_discovery

[Andre Guedes]

Hi Johan,

On Thu, Feb 16, 2012 at 7:15 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Thu, Feb 16, 2012, Andre Guedes wrote:
>> +#define DISCOV_TYPE_BREDR            (BIT(0))
>> +#define DISCOV_TYPE_LE                       (BIT(1) | BIT(2))
>> +#define DISCOV_TYPE_INTERLEAVED              (BIT(0) | BIT(1) | BIT(2))
>
> Defining these like this looks a bit (no pun intended) odd. The previous
> code was at least using the mgmt defs instead of magic numbers (which is
> how these values have been originally chosen too).

Do you think this would be better?

+#define DISCOV_TYPE_BREDR            (BIT(MGMT_ADDR_BREDR))
+#define DISCOV_TYPE_LE               (BIT(MGMT_ADDR_LE_PUBLIC) |
BIT(MGMT_ADDR_LE_RANDOM))
+#define DISCOV_TYPE_INTERLEAVED      (BIT(MGMT_ADDR_BREDR) |
BIT(MGMT_ADDR_LE_PUBLIC) | \
+
BIT(MGMT_ADDR_LE_RANDOM))

Andre

Re: [RFC 1/4] Bluetooth: Prepare start_discovery

[Marcel Holtmann]

Hi Andre,

> > On Thu, Feb 16, 2012, Andre Guedes wrote:
> >> +#define DISCOV_TYPE_BREDR            (BIT(0))
> >> +#define DISCOV_TYPE_LE                       (BIT(1) | BIT(2))
> >> +#define DISCOV_TYPE_INTERLEAVED              (BIT(0) | BIT(1) | BIT(2))
> >
> > Defining these like this looks a bit (no pun intended) odd. The previous
> > code was at least using the mgmt defs instead of magic numbers (which is
> > how these values have been originally chosen too).
> 
> Do you think this would be better?
> 
> +#define DISCOV_TYPE_BREDR            (BIT(MGMT_ADDR_BREDR))
> +#define DISCOV_TYPE_LE               (BIT(MGMT_ADDR_LE_PUBLIC) |
> BIT(MGMT_ADDR_LE_RANDOM))
> +#define DISCOV_TYPE_INTERLEAVED      (BIT(MGMT_ADDR_BREDR) |
> BIT(MGMT_ADDR_LE_PUBLIC) | \
> +
> BIT(MGMT_ADDR_LE_RANDOM))

even while I already acked the previous patch, this is a bit better.

Regards

Marcel

Re: [RFC 1/4] Bluetooth: Prepare start_discovery

[Marcel Holtmann]

Hi Andre,

> This patch does some code refactoring in start_discovery function
> in order to prepare it for interleaved discovery support.
> 
> The discovery type macros were defined according to mgmt-api.txt
> specification:
> 
> Possible values for the Type parameter are a bit-wise or of the
> following bits:
> 
> 	1	BR/EDR
> 	2	LE Public
> 	3	LE Random
> 
> By combining these e.g. the following values are possible:
> 
> 	1	BR/EDR
> 	6	LE (public & random)
> 	7	BR/EDR/LE (interleaved discovery)
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    4 ++++
>  net/bluetooth/mgmt.c             |   15 ++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[RFC 2/4] Bluetooth: Track discovery type

[Andre Guedes]

This patch adds to struct discovery_state the field 'type' so that
we can track the discovery type the device is performing.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    2 ++
 net/bluetooth/mgmt.c             |    4 +++-
 3 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9982179..3a62f93 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -61,6 +61,7 @@ struct inquiry_entry {
 };
 
 struct discovery_state {
+	int type;
 	enum {
 		DISCOVERY_STOPPED,
 		DISCOVERY_STARTING,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dc31e7d..29a9b01 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -380,6 +380,8 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
 
 	switch (state) {
 	case DISCOVERY_STOPPED:
+		hdev->discovery.type = 0;
+
 		if (hdev->discovery.state != DISCOVERY_STARTING)
 			mgmt_discovering(hdev, 0);
 		break;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 410392e..e71564e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2072,7 +2072,9 @@ static int start_discovery(struct sock *sk, u16 index,
 		goto failed;
 	}
 
-	switch (cp->type) {
+	hdev->discovery.type = cp->type;
+
+	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
 	case DISCOV_TYPE_INTERLEAVED:
 		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
-- 
1.7.9.1

Re: [RFC 2/4] Bluetooth: Track discovery type

[Marcel Holtmann]

Hi Andre,

> This patch adds to struct discovery_state the field 'type' so that
> we can track the discovery type the device is performing.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |    2 ++
>  net/bluetooth/mgmt.c             |    4 +++-
>  3 files changed, 6 insertions(+), 1 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[RFC 3/4] Bluetooth: Merge INQUIRY and LE_SCAN discovery states

[Andre Guedes]

This patch merges DISCOVERY_INQUIRY and DISCOVERY_LE_SCAN states
into a new state called DISCOVERY_FINDING.

>From the discovery perspective, we are pretty much worried about
to know just if we are finding devices than what exactly phase of
"finding devices" (inquiry or LE scan) we are currently running.
Besides, to know if the controller is performing inquiry or LE scan
we should check HCI_INQUIRY or HCI_LE_SCAN bits in hdev flags.

Moreover, merging this two states will simplify the discovery state
machine and will keep interleaved discovery implementation simpler.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    3 +--
 net/bluetooth/hci_core.c         |    6 ++----
 net/bluetooth/hci_event.c        |    6 +++---
 net/bluetooth/mgmt.c             |    2 +-
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3a62f93..ee4dba5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -65,8 +65,7 @@ struct discovery_state {
 	enum {
 		DISCOVERY_STOPPED,
 		DISCOVERY_STARTING,
-		DISCOVERY_INQUIRY,
-		DISCOVERY_LE_SCAN,
+		DISCOVERY_FINDING,
 		DISCOVERY_RESOLVING,
 		DISCOVERY_STOPPING,
 	} state;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 29a9b01..fabca08 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -361,8 +361,7 @@ bool hci_discovery_active(struct hci_dev *hdev)
 	struct discovery_state *discov = &hdev->discovery;
 
 	switch (discov->state) {
-	case DISCOVERY_INQUIRY:
-	case DISCOVERY_LE_SCAN:
+	case DISCOVERY_FINDING:
 	case DISCOVERY_RESOLVING:
 		return true;
 
@@ -387,8 +386,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
 		break;
 	case DISCOVERY_STARTING:
 		break;
-	case DISCOVERY_INQUIRY:
-	case DISCOVERY_LE_SCAN:
+	case DISCOVERY_FINDING:
 		mgmt_discovering(hdev, 1);
 		break;
 	case DISCOVERY_RESOLVING:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b0784ee..d68e4cc 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1073,7 +1073,7 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 		hci_dev_lock(hdev);
 		hci_adv_entries_clear(hdev);
-		hci_discovery_set_state(hdev, DISCOVERY_LE_SCAN);
+		hci_discovery_set_state(hdev, DISCOVERY_FINDING);
 		hci_dev_unlock(hdev);
 		break;
 
@@ -1152,7 +1152,7 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 	set_bit(HCI_INQUIRY, &hdev->flags);
 
 	hci_dev_lock(hdev);
-	hci_discovery_set_state(hdev, DISCOVERY_INQUIRY);
+	hci_discovery_set_state(hdev, DISCOVERY_FINDING);
 	hci_dev_unlock(hdev);
 }
 
@@ -1638,7 +1638,7 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
 
 	hci_dev_lock(hdev);
 
-	if (discov->state != DISCOVERY_INQUIRY)
+	if (discov->state != DISCOVERY_FINDING)
 		goto unlock;
 
 	if (list_empty(&discov->resolve)) {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e71564e..7165d66 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2130,7 +2130,7 @@ static int stop_discovery(struct sock *sk, u16 index)
 		goto unlock;
 	}
 
-	if (hdev->discovery.state == DISCOVERY_INQUIRY) {
+	if (hdev->discovery.state == DISCOVERY_FINDING) {
 		err = hci_cancel_inquiry(hdev);
 		if (err < 0)
 			mgmt_pending_remove(cmd);
-- 
1.7.9.1

Re: [RFC 3/4] Bluetooth: Merge INQUIRY and LE_SCAN discovery states

[Marcel Holtmann]

Hi Andre,

> This patch merges DISCOVERY_INQUIRY and DISCOVERY_LE_SCAN states
> into a new state called DISCOVERY_FINDING.
> 
> From the discovery perspective, we are pretty much worried about
> to know just if we are finding devices than what exactly phase of
> "finding devices" (inquiry or LE scan) we are currently running.
> Besides, to know if the controller is performing inquiry or LE scan
> we should check HCI_INQUIRY or HCI_LE_SCAN bits in hdev flags.
> 
> Moreover, merging this two states will simplify the discovery state
> machine and will keep interleaved discovery implementation simpler.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    3 +--
>  net/bluetooth/hci_core.c         |    6 ++----
>  net/bluetooth/hci_event.c        |    6 +++---
>  net/bluetooth/mgmt.c             |    2 +-
>  4 files changed, 7 insertions(+), 10 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

[RFC 4/4] Bluetooth: Interleaved discovery support

[Andre Guedes]

This patch adds interleaved discovery support to MGMT Start
Discovery command.

In case interleaved discovery is not supported (not a dual mode
device), we perform BR/EDR or LE-only discovery according to the
device capabilities.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_event.c        |   13 +++++++---
 net/bluetooth/mgmt.c             |   47 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ee4dba5..26666f0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -720,6 +720,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
 #define lmp_ssp_capable(dev)       ((dev)->features[6] & LMP_SIMPLE_PAIR)
 #define lmp_no_flush_capable(dev)  ((dev)->features[6] & LMP_NO_FLUSH)
 #define lmp_le_capable(dev)        ((dev)->features[4] & LMP_LE)
+#define lmp_bredr_capable(dev)     (!((dev)->features[4] & LMP_NO_BREDR))
 
 /* ----- Extended LMP capabilities ----- */
 #define lmp_host_le_capable(dev)   ((dev)->host_features[0] & LMP_HOST_LE)
@@ -1009,6 +1010,7 @@ int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
 int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status);
 int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status);
 int mgmt_discovering(struct hci_dev *hdev, u8 discovering);
+int mgmt_interleaved_discovery(struct hci_dev *hdev);
 int mgmt_device_blocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 int mgmt_device_unblocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d68e4cc..5bfe8c0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1083,11 +1083,16 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 
 		clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
 
-		hci_dev_lock(hdev);
-		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
-		hci_dev_unlock(hdev);
-
 		schedule_delayed_work(&hdev->adv_work, ADV_CLEAR_TIMEOUT);
+
+		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
+			mgmt_interleaved_discovery(hdev);
+		} else {
+			hci_dev_lock(hdev);
+			hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+			hci_dev_unlock(hdev);
+		}
+
 		break;
 
 	default:
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7165d66..3d24dad 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -106,8 +106,11 @@ static const u16 mgmt_events[] = {
 #define LE_SCAN_WIN			0x12
 #define LE_SCAN_INT			0x12
 #define LE_SCAN_TIMEOUT_LE_ONLY		10240	/* TGAP(gen_disc_scan_min) */
+#define LE_SCAN_TIMEOUT_BREDR_LE	5120	/* TGAP(100)/2 */
 
 #define INQUIRY_LEN_BREDR		0x08	/* TGAP(100) */
+#define INQUIRY_LEN_BREDR_LE		0x04	/* TGAP(100)/2 */
+
 
 #define SERVICE_CACHE_TIMEOUT (5 * 1000)
 
@@ -2033,6 +2036,45 @@ static int remove_remote_oob_data(struct sock *sk, u16 index,
 	return err;
 }
 
+static int discovery(struct hci_dev *hdev)
+{
+	int err;
+
+	if (lmp_host_le_capable(hdev)) {
+		if (lmp_bredr_capable(hdev)) {
+			err = hci_le_scan(hdev, LE_SCAN_TYPE,
+						LE_SCAN_INT, LE_SCAN_WIN,
+						LE_SCAN_TIMEOUT_BREDR_LE);
+		} else {
+			hdev->discovery.type = DISCOV_TYPE_LE;
+			err = hci_le_scan(hdev, LE_SCAN_TYPE,
+						LE_SCAN_INT, LE_SCAN_WIN,
+						LE_SCAN_TIMEOUT_LE_ONLY);
+		}
+	} else {
+		hdev->discovery.type = DISCOV_TYPE_BREDR;
+		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
+	}
+
+	return err;
+}
+
+int mgmt_interleaved_discovery(struct hci_dev *hdev)
+{
+	int err;
+
+	BT_DBG("%s", hdev->name);
+
+	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
+	if (err < 0) {
+		hci_dev_lock(hdev);
+		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+		hci_dev_unlock(hdev);
+	}
+
+	return err;
+}
+
 static int start_discovery(struct sock *sk, u16 index,
 						void *data, u16 len)
 {
@@ -2076,7 +2118,6 @@ static int start_discovery(struct sock *sk, u16 index,
 
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
-	case DISCOV_TYPE_INTERLEAVED:
 		err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
 		break;
 
@@ -2085,6 +2126,10 @@ static int start_discovery(struct sock *sk, u16 index,
 					LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
 		break;
 
+	case DISCOV_TYPE_INTERLEAVED:
+		err = discovery(hdev);
+		break;
+
 	default:
 		err = -EINVAL;
 	}
-- 
1.7.9.1

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Johan Hedberg]

Hi Andre,

On Thu, Feb 16, 2012, Andre Guedes wrote:
>  #define INQUIRY_LEN_BREDR		0x08	/* TGAP(100) */
> +#define INQUIRY_LEN_BREDR_LE		0x04	/* TGAP(100)/2 */
> +
>  
>  #define SERVICE_CACHE_TIMEOUT (5 * 1000)

One unnecessary empty line added above.

> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
> +{
> +	int err;
> +
> +	BT_DBG("%s", hdev->name);
> +
> +	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
> +	if (err < 0) {
> +		hci_dev_lock(hdev);
> +		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +		hci_dev_unlock(hdev);
> +	}
> +
> +	return err;
> +}

The locking doesn't look right to me above. hci_do_inquiry should be
called with the lock held. I think it might be simpler if you make
mgmt_interleaved_discovery() require the caller to hold the lock.

Johan

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Andre Guedes]

Hi Johan,

On Thu, Feb 16, 2012 at 7:12 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Thu, Feb 16, 2012, Andre Guedes wrote:
>>  #define INQUIRY_LEN_BREDR            0x08    /* TGAP(100) */
>> +#define INQUIRY_LEN_BREDR_LE         0x04    /* TGAP(100)/2 */
>> +
>>
>>  #define SERVICE_CACHE_TIMEOUT (5 * 1000)
>
> One unnecessary empty line added above.

Thanks, I'll fix it.

>> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
>> +{
>> +     int err;
>> +
>> +     BT_DBG("%s", hdev->name);
>> +
>> +     err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
>> +     if (err < 0) {
>> +             hci_dev_lock(hdev);
>> +             hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> +             hci_dev_unlock(hdev);
>> +     }
>> +
>> +     return err;
>> +}
>
> The locking doesn't look right to me above. hci_do_inquiry should be
> called with the lock held. I think it might be simpler if you make
> mgmt_interleaved_discovery() require the caller to hold the lock.

Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
cache_flush which requires hdev->lock held. I'll fix this too. Thanks.

BR,

Andre

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Marcel Holtmann]

Hi Andre,

> > On Thu, Feb 16, 2012, Andre Guedes wrote:
> >>  #define INQUIRY_LEN_BREDR            0x08    /* TGAP(100) */
> >> +#define INQUIRY_LEN_BREDR_LE         0x04    /* TGAP(100)/2 */
> >> +
> >>
> >>  #define SERVICE_CACHE_TIMEOUT (5 * 1000)
> >
> > One unnecessary empty line added above.
> 
> Thanks, I'll fix it.
> 
> >> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
> >> +{
> >> +     int err;
> >> +
> >> +     BT_DBG("%s", hdev->name);
> >> +
> >> +     err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
> >> +     if (err < 0) {
> >> +             hci_dev_lock(hdev);
> >> +             hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> >> +             hci_dev_unlock(hdev);
> >> +     }
> >> +
> >> +     return err;
> >> +}
> >
> > The locking doesn't look right to me above. hci_do_inquiry should be
> > called with the lock held. I think it might be simpler if you make
> > mgmt_interleaved_discovery() require the caller to hold the lock.
> 
> Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> cache_flush which requires hdev->lock held. I'll fix this too. Thanks.

please keep the lock inside mgmt_interleaved_discovery() for now. We
have enough locking crazy. I don't wanna add to it by making the caller
deal with it right now.

Otherwise looks fine to me.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Johan Hedberg]

Hi Marcel,

On Fri, Feb 17, 2012, Marcel Holtmann wrote:
> > >> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
> > >> +{
> > >> +     int err;
> > >> +
> > >> +     BT_DBG("%s", hdev->name);
> > >> +
> > >> +     err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
> > >> +     if (err < 0) {
> > >> +             hci_dev_lock(hdev);
> > >> +             hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > >> +             hci_dev_unlock(hdev);
> > >> +     }
> > >> +
> > >> +     return err;
> > >> +}
> > >
> > > The locking doesn't look right to me above. hci_do_inquiry should be
> > > called with the lock held. I think it might be simpler if you make
> > > mgmt_interleaved_discovery() require the caller to hold the lock.
> > 
> > Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> > cache_flush which requires hdev->lock held. I'll fix this too. Thanks.
> 
> please keep the lock inside mgmt_interleaved_discovery() for now. We
> have enough locking crazy. I don't wanna add to it by making the caller
> deal with it right now.

The calling code looks like this:

+
+               if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
+                       mgmt_interleaved_discovery(hdev);
+               } else {
+                       hci_dev_lock(hdev);
+                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+                       hci_dev_unlock(hdev);
+               }

I was thinking that if mgmt_interleaved_discovery required the lock to
be held this code would become simpler:

	hci_dev_lock(hdev);

	if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED)
		mgmt_interleaved_discovery(hdev);
	else
		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);

        hci_dev_unlock(hdev);

Johan

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Johan Hedberg]

Hi,

On Fri, Feb 17, 2012, Johan Hedberg wrote:
> > > > The locking doesn't look right to me above. hci_do_inquiry should be
> > > > called with the lock held. I think it might be simpler if you make
> > > > mgmt_interleaved_discovery() require the caller to hold the lock.
> > > 
> > > Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> > > cache_flush which requires hdev->lock held. I'll fix this too. Thanks.
> > 
> > please keep the lock inside mgmt_interleaved_discovery() for now. We
> > have enough locking crazy. I don't wanna add to it by making the caller
> > deal with it right now.
> 
> The calling code looks like this:
> 
> +
> +               if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
> +                       mgmt_interleaved_discovery(hdev);
> +               } else {
> +                       hci_dev_lock(hdev);
> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +                       hci_dev_unlock(hdev);
> +               }
> 
> I was thinking that if mgmt_interleaved_discovery required the lock to
> be held this code would become simpler:
> 
> 	hci_dev_lock(hdev);
> 
> 	if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED)
> 		mgmt_interleaved_discovery(hdev);
> 	else
> 		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 
> 	hci_dev_unlock(hdev);

Furthermore, almost all calls from hci_core.c or hci_event.c into mgmt.c
(that pass hdev as a parameter) have hdev locked so this would just be
maintaining consistency.

Johan

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Ulisses Furquim]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

Hi,

On Fri, Feb 17, 2012 at 1:00 PM, Johan Hedberg <johan.hedberg@gmail.com>wrote:

> Hi,
>
> On Fri, Feb 17, 2012, Johan Hedberg wrote:
> > > > > The locking doesn't look right to me above. hci_do_inquiry should
> be
> > > > > called with the lock held. I think it might be simpler if you make
> > > > > mgmt_interleaved_discovery() require the caller to hold the lock.
> > > >
> > > > Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> > > > cache_flush which requires hdev->lock held. I'll fix this too.
> Thanks.
> > >
> > > please keep the lock inside mgmt_interleaved_discovery() for now. We
> > > have enough locking crazy. I don't wanna add to it by making the caller
> > > deal with it right now.
> >
> > The calling code looks like this:
> >
> > +
> > +               if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
> > +                       mgmt_interleaved_discovery(hdev);
> > +               } else {
> > +                       hci_dev_lock(hdev);
> > +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > +                       hci_dev_unlock(hdev);
> > +               }
> >
> > I was thinking that if mgmt_interleaved_discovery required the lock to
> > be held this code would become simpler:
> >
> >       hci_dev_lock(hdev);
> >
> >       if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED)
> >               mgmt_interleaved_discovery(hdev);
> >       else
> >               hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> >
> >       hci_dev_unlock(hdev);
>
> Furthermore, almost all calls from hci_core.c or hci_event.c into mgmt.c
> (that pass hdev as a parameter) have hdev locked so this would just be
> maintaining consistency.


Makes sense to me. Marcel, any reason not to be this way?

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

[-- Attachment #2: Type: text/html, Size: 2591 bytes --]

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Ulisses Furquim]

Hi,

On Fri, Feb 17, 2012 at 1:00 PM, Johan Hedberg <johan.hedberg@gmail.com> wr=
ote:
>
> Hi,
>
> On Fri, Feb 17, 2012, Johan Hedberg wrote:
> > > > > The locking doesn't look right to me above. hci_do_inquiry should=
 be
> > > > > called with the lock held. I think it might be simpler if you mak=
e
> > > > > mgmt_interleaved_discovery() require the caller to hold the lock.
> > > >
> > > > Yes, you're right. I just realized hci_do_inquiry now calls inquiry=
_
> > > > cache_flush which requires hdev->lock held. I'll fix this too. Than=
ks.
> > >
> > > please keep the lock inside mgmt_interleaved_discovery() for now. We
> > > have enough locking crazy. I don't wanna add to it by making the call=
er
> > > deal with it right now.
> >
> > The calling code looks like this:
> >
> > +
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (hdev->discovery.type =3D=3D DISCOV_TY=
PE_INTERLEAVED) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mgmt_interleaved_discover=
y(hdev);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_dev_lock(hdev);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_discovery_set_state(h=
dev, DISCOVERY_STOPPED);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_dev_unlock(hdev);
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> >
> > I was thinking that if mgmt_interleaved_discovery required the lock to
> > be held this code would become simpler:
> >
> > =A0 =A0 =A0 hci_dev_lock(hdev);
> >
> > =A0 =A0 =A0 if (hdev->discovery.type =3D=3D DISCOV_TYPE_INTERLEAVED)
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 mgmt_interleaved_discovery(hdev);
> > =A0 =A0 =A0 else
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 hci_discovery_set_state(hdev, DISCOVERY_STO=
PPED);
> >
> > =A0 =A0 =A0 hci_dev_unlock(hdev);
>
> Furthermore, almost all calls from hci_core.c or hci_event.c into mgmt.c
> (that pass hdev as a parameter) have hdev locked so this would just be
> maintaining consistency.

Makes sense to me. Any reason not be this way, Marcel?

Regards,

--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFC 4/4] Bluetooth: Interleaved discovery support

[Marcel Holtmann]

Hi Johan,

> > > >> +int mgmt_interleaved_discovery(struct hci_dev *hdev)
> > > >> +{
> > > >> +     int err;
> > > >> +
> > > >> +     BT_DBG("%s", hdev->name);
> > > >> +
> > > >> +     err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
> > > >> +     if (err < 0) {
> > > >> +             hci_dev_lock(hdev);
> > > >> +             hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > > >> +             hci_dev_unlock(hdev);
> > > >> +     }
> > > >> +
> > > >> +     return err;
> > > >> +}
> > > >
> > > > The locking doesn't look right to me above. hci_do_inquiry should be
> > > > called with the lock held. I think it might be simpler if you make
> > > > mgmt_interleaved_discovery() require the caller to hold the lock.
> > > 
> > > Yes, you're right. I just realized hci_do_inquiry now calls inquiry_
> > > cache_flush which requires hdev->lock held. I'll fix this too. Thanks.
> > 
> > please keep the lock inside mgmt_interleaved_discovery() for now. We
> > have enough locking crazy. I don't wanna add to it by making the caller
> > deal with it right now.
> 
> The calling code looks like this:
> 
> +
> +               if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED) {
> +                       mgmt_interleaved_discovery(hdev);
> +               } else {
> +                       hci_dev_lock(hdev);
> +                       hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> +                       hci_dev_unlock(hdev);
> +               }
> 
> I was thinking that if mgmt_interleaved_discovery required the lock to
> be held this code would become simpler:
> 
> 	hci_dev_lock(hdev);
> 
> 	if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED)
> 		mgmt_interleaved_discovery(hdev);
> 	else
> 		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> 
>         hci_dev_unlock(hdev);

this is ugly no matter what since we are intermixing things here.

However I think that hci_discovery_set_state() should better take the
look by itself or get a __ prefix for indicating it is unlocked.

Regards

Marcel