[RFC v2 0/4] MGMT Start Discovery interleaved support

[RFC v2 0/4] MGMT Start Discovery interleaved support

[Andre Guedes]

Hi all,

This RFCv2 series implements Marcel's and Johan's comments from previous
version. The changes are:
1. Use MGMT_ADDR_* macros instead of hard-coded values to define discovery
type macros;
2. Fix locking in mgmt_interleaved_discovery

Marcel, I removed your ack from patch 1/4 because I changed it a little
bit. MGMT_ADDR_* macros were moved to hci_core.h since they are needed to
define discovery type macros.

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 |   18 +++++++++-
 include/net/bluetooth/mgmt.h     |    5 ---
 net/bluetooth/hci_core.c         |    8 ++--
 net/bluetooth/hci_event.c        |   19 +++++++----
 net/bluetooth/mgmt.c             |   64 ++++++++++++++++++++++++++++++++++---
 5 files changed, 90 insertions(+), 24 deletions(-)

-- 
1.7.9.1

[RFC v2 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.

MGMT_ADDR_* macros were moved to hci_core.h since they are now used
to define discovery type macros.

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 |   12 ++++++++++++
 include/net/bluetooth/mgmt.h     |    5 -----
 net/bluetooth/mgmt.c             |   15 ++++++++++-----
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 43e0b1e..be8da5d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -956,6 +956,18 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb,
 							struct sock *skip_sk);
 
 /* Management interface */
+#define MGMT_ADDR_BREDR			0x00
+#define MGMT_ADDR_LE_PUBLIC		0x01
+#define MGMT_ADDR_LE_RANDOM		0x02
+#define MGMT_ADDR_INVALID		0xff
+
+#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))
+
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
 int mgmt_index_added(struct hci_dev *hdev);
 int mgmt_index_removed(struct hci_dev *hdev);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index ee625a6..ad54b5f 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -48,11 +48,6 @@ struct mgmt_hdr {
 	__le16 len;
 } __packed;
 
-#define MGMT_ADDR_BREDR			0x00
-#define MGMT_ADDR_LE_PUBLIC		0x01
-#define MGMT_ADDR_LE_RANDOM		0x02
-#define MGMT_ADDR_INVALID		0xff
-
 struct mgmt_addr_info {
 	bdaddr_t bdaddr;
 	__u8 type;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f9f3e4c..196215c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2157,7 +2157,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;
@@ -2193,14 +2192,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 v2 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.
> 
> MGMT_ADDR_* macros were moved to hci_core.h since they are now used
> to define discovery type macros.
> 
> 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 |   12 ++++++++++++
>  include/net/bluetooth/mgmt.h     |    5 -----
>  net/bluetooth/mgmt.c             |   15 ++++++++++-----
>  3 files changed, 22 insertions(+), 10 deletions(-)

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

Regards

Marcel

[RFC v2 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>
Acked-by: Marcel Holtmann <marcel@holtmann.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 be8da5d..d7c79b5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -57,6 +57,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 196215c..9d98382 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2192,7 +2192,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

[RFC v2 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>
Acked-by: Marcel Holtmann <marcel@holtmann.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 d7c79b5..942de77 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -61,8 +61,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 179d127..9aea7b8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1080,7 +1080,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;
 
@@ -1159,7 +1159,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);
 }
 
@@ -1645,7 +1645,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 9d98382..a9cd38d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2250,7 +2250,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

[RFC v2 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>
Acked-by: Marcel Holtmann <marcel@holtmann.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 942de77..2aafeb3 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -716,6 +716,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)
@@ -1019,6 +1020,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 9aea7b8..04fb1f0 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1090,11 +1090,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 a9cd38d..89754bb 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -108,8 +108,10 @@ 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)
 
@@ -2153,6 +2155,46 @@ 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);
+
+	hci_dev_lock(hdev);
+
+	err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR_LE);
+	if (err < 0)
+		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)
 {
@@ -2196,7 +2238,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;
 
@@ -2205,6 +2246,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 v2 0/4] MGMT Start Discovery interleaved support

[Johan Hedberg]

Hi Andre,

On Fri, Feb 17, 2012, Andre Guedes wrote:
> This RFCv2 series implements Marcel's and Johan's comments from previous
> version. The changes are:
> 1. Use MGMT_ADDR_* macros instead of hard-coded values to define discovery
> type macros;
> 2. Fix locking in mgmt_interleaved_discovery
> 
> Marcel, I removed your ack from patch 1/4 because I changed it a little
> bit. MGMT_ADDR_* macros were moved to hci_core.h since they are needed to
> define discovery type macros.

All four patches have been applied to my bluetooth-next tree. Thanks.

I'd still like to have consistency in the locking since now your
function is the only one handling the hdev lock by itself and that's
something that can easily cause bugs in the long run. I.e. probably all
mgmt_* functions should be converted to take hdev unlocked and handle
the locking by themselves.

Johan

Re: [RFC v2 0/4] MGMT Start Discovery interleaved support

[Ulisses Furquim]

Hi Johan,

On Sun, Feb 19, 2012 at 7:51 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Andre,
>
> On Fri, Feb 17, 2012, Andre Guedes wrote:
>> This RFCv2 series implements Marcel's and Johan's comments from previous
>> version. The changes are:
>> 1. Use MGMT_ADDR_* macros instead of hard-coded values to define discovery
>> type macros;
>> 2. Fix locking in mgmt_interleaved_discovery
>>
>> Marcel, I removed your ack from patch 1/4 because I changed it a little
>> bit. MGMT_ADDR_* macros were moved to hci_core.h since they are needed to
>> define discovery type macros.
>
> All four patches have been applied to my bluetooth-next tree. Thanks.
>
> I'd still like to have consistency in the locking since now your
> function is the only one handling the hdev lock by itself and that's
> something that can easily cause bugs in the long run. I.e. probably all
> mgmt_* functions should be converted to take hdev unlocked and handle
> the locking by themselves.

Do you think such a change is worth? I'm not sure if we have just one
function that deals with locking on its own.

Regards,

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