[PATCH 0/7] Use HCI request framework to enable LE scanning

[PATCH 0/7] Use HCI request framework to enable LE scanning

[Andre Guedes]

Hi all,

The main goal of this patch set is to update hci_le_scan helper to use the new
HCI Request framework. Additionally, it also moves the LE scanning timeout
handling from LE scanning helpers to discovery code, as Marcel requested
during some discussion about LE connection improvements.

Below follows additional information about some patches of this patch set:

Patch 1 adds the hci_req_cleanup function so we are able to free HCI command
already queued on a given HCI request. This function must be used in case
something goes wrong during the HCI request creation. Otherwise, we will
face some memory leak issues. hci_req_cleanup is used in this patch set in
case something goes wrong with LE scanning HCI request creation.

Patches 6 and 7 do a very trivial changes in order to organize discovery
macros.

Regards,

Andre Guedes


Andre Guedes (7):
  Bluetooth: Add hci_req_cleanup function
  Bluetooth: Update hci_le_scan to use HCI request
  Bluetooth: Merge LE-only and interleaved cases
  Bluetooth: Remove timeout handling from hci_cancel_le_scan
  Bluetooth: Remove LE scan work
  Bluetooth: Change LE scanning timeout macros
  Bluetooth: Add LE scan type macros

 include/net/bluetooth/hci_core.h |  18 ++----
 net/bluetooth/hci_core.c         | 124 ++++++++++++---------------------------
 net/bluetooth/mgmt.c             |  65 +++++++++++++++-----
 3 files changed, 94 insertions(+), 113 deletions(-)

-- 
1.8.1.2

[PATCH 1/7] Bluetooth: Add hci_req_cleanup function

[Andre Guedes]

This patch adds the hci_req_cleanup function which basically frees
all HCI commands queued on a given request.

This function must be called in case something goes wrong during HCI
request creation. Otherwise, HCI commands already queued on req->cmd_q
will cause memory leaks.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3a9cbf2..494f8f5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1045,6 +1045,7 @@ struct hci_request {
 };
 
 void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
+void hci_req_cleanup(struct hci_request *req);
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
 int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param);
 void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index b6d44a2..7635c2e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2443,6 +2443,11 @@ void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
 	req->hdev = hdev;
 }
 
+void hci_req_cleanup(struct hci_request *req)
+{
+	skb_queue_purge(&req->cmd_q);
+}
+
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
 {
 	struct hci_dev *hdev = req->hdev;
-- 
1.8.1.2

[PATCH 2/7] Bluetooth: Update hci_le_scan to use HCI request

[Andre Guedes]

This patch changes hci_le_scan helper so it uses the HCI request
framework to enable LE scanning.

Also, the LE scanning disable timeout was removed from the helper
and it is now handled in mgmt start_discovery code.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 494f8f5..9efb571 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1169,8 +1169,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 							__u8 ltk[16]);
 int hci_do_inquiry(struct hci_dev *hdev, u8 length);
 int hci_cancel_inquiry(struct hci_dev *hdev);
-int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
-		int timeout);
+int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
 int hci_cancel_le_scan(struct hci_dev *hdev);
 
 u8 bdaddr_to_le(u8 bdaddr_type);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7635c2e..160bfe3 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1943,27 +1943,48 @@ static void le_scan_work(struct work_struct *work)
 		       param->timeout);
 }
 
-int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
-		int timeout)
+static int set_le_scan_param(struct hci_request *req, u8 type, u16 interval,
+			     u16 window)
 {
-	struct le_scan_params *param = &hdev->le_scan_params;
+	struct hci_cp_le_set_scan_param cp;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.type = type;
+	cp.interval = cpu_to_le16(interval);
+	cp.window = cpu_to_le16(window);
+
+	return hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
+}
+
+static int enable_le_scan(struct hci_request *req)
+{
+	struct hci_cp_le_set_scan_enable cp;
+
+	memset(&cp, 0, sizeof(cp));
+	cp.enable = 1;
+	cp.filter_dup = 1;
+
+	return hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+}
+
+int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window)
+{
+	struct hci_dev *hdev = req->hdev;
+	int err;
 
 	BT_DBG("%s", hdev->name);
 
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
 		return -ENOTSUPP;
 
-	if (work_busy(&hdev->le_scan))
-		return -EINPROGRESS;
-
-	param->type = type;
-	param->interval = interval;
-	param->window = window;
-	param->timeout = timeout;
+	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+		return -EALREADY;
 
-	queue_work(system_long_wq, &hdev->le_scan);
+	err = set_le_scan_param(req, type, interval, window);
+	if (err < 0)
+		return err;
 
-	return 0;
+	return enable_le_scan(req);
 }
 
 /* Alloc HCI device */
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 39395c7..b444b19 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2428,11 +2428,34 @@ int mgmt_interleaved_discovery(struct hci_dev *hdev)
 	return err;
 }
 
+static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
+{
+	BT_DBG("status %d", status);
+
+	if (status)
+		return;
+
+	switch (hdev->discovery.type) {
+	case DISCOV_TYPE_LE:
+		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+				   msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
+		break;
+
+	case DISCOV_TYPE_INTERLEAVED:
+		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
+				   msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
+		break;
+	default:
+		BT_ERR("Invalid discovery type %d", hdev->discovery.type);
+	}
+}
+
 static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			   void *data, u16 len)
 {
 	struct mgmt_cp_start_discovery *cp = data;
 	struct pending_cmd *cmd;
+	struct hci_request req;
 	int err;
 
 	BT_DBG("%s", hdev->name);
@@ -2485,8 +2508,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
-		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
-				  LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
+		hci_req_init(&req, hdev);
+
+		err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+				  LE_SCAN_WIN);
+		if (err < 0) {
+			mgmt_pending_remove(cmd);
+			hci_req_cleanup(&req);
+			goto failed;
+		}
+
+		err = hci_req_run(&req, enable_le_scan_complete);
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
@@ -2497,8 +2529,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
-		err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT, LE_SCAN_WIN,
-				  LE_SCAN_TIMEOUT_BREDR_LE);
+		hci_req_init(&req, hdev);
+
+		err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+				  LE_SCAN_WIN);
+		if (err < 0) {
+			mgmt_pending_remove(cmd);
+			hci_req_cleanup(&req);
+			goto failed;
+		}
+
+		err = hci_req_run(&req, enable_le_scan_complete);
 		break;
 
 	default:
-- 
1.8.1.2

[PATCH 3/7] Bluetooth: Merge LE-only and interleaved cases

[Andre Guedes]

This patch merges the LE-only and interleaved discovery type cases.
Since hci_le_scan helper uses the HCI request framework, those cases
are pretty much the same now.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b444b19..7a4f8dd 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2501,6 +2501,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 		break;
 
 	case DISCOV_TYPE_LE:
+	case DISCOV_TYPE_INTERLEAVED:
 		if (!lmp_host_le_capable(hdev)) {
 			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
 					 MGMT_STATUS_NOT_SUPPORTED);
@@ -2508,21 +2509,8 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 			goto failed;
 		}
 
-		hci_req_init(&req, hdev);
-
-		err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
-				  LE_SCAN_WIN);
-		if (err < 0) {
-			mgmt_pending_remove(cmd);
-			hci_req_cleanup(&req);
-			goto failed;
-		}
-
-		err = hci_req_run(&req, enable_le_scan_complete);
-		break;
-
-	case DISCOV_TYPE_INTERLEAVED:
-		if (!lmp_host_le_capable(hdev) || !lmp_bredr_capable(hdev)) {
+		if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
+		    !lmp_bredr_capable(hdev)) {
 			err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
 					 MGMT_STATUS_NOT_SUPPORTED);
 			mgmt_pending_remove(cmd);
-- 
1.8.1.2

[PATCH 4/7] Bluetooth: Remove timeout handling from hci_cancel_le_scan

[Andre Guedes]

This patch moves the LE scanning timeout handling from hci_cancel_
le_scan helper to stop_discovery.

Since we want discovery code handling LE scanning timeout, we should
handle it in stop_discovery instead of hci_cancel_le_scan.

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

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 160bfe3..73487f0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1903,20 +1903,16 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
 
 int hci_cancel_le_scan(struct hci_dev *hdev)
 {
+	struct hci_cp_le_set_scan_enable cp;
+
 	BT_DBG("%s", hdev->name);
 
 	if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
 		return -EALREADY;
 
-	if (cancel_delayed_work(&hdev->le_scan_disable)) {
-		struct hci_cp_le_set_scan_enable cp;
-
-		/* Send HCI command to disable LE Scan */
-		memset(&cp, 0, sizeof(cp));
-		hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
-	}
-
-	return 0;
+	/* Send HCI command to disable LE Scan */
+	memset(&cp, 0, sizeof(cp));
+	return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
 }
 
 static void le_scan_disable_work(struct work_struct *work)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7a4f8dd..de1d2f4 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2582,10 +2582,19 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	switch (hdev->discovery.state) {
 	case DISCOVERY_FINDING:
-		if (test_bit(HCI_INQUIRY, &hdev->flags))
+		if (test_bit(HCI_INQUIRY, &hdev->flags)) {
 			err = hci_cancel_inquiry(hdev);
-		else
-			err = hci_cancel_le_scan(hdev);
+		} else {
+			/*
+			 * If delayed work could not be canceled, it means the
+			 * work is running. Therefore, there is no need to send
+			 * another HCI command to disable LE scanning.
+			 */
+			if (!cancel_delayed_work(&hdev->le_scan_disable))
+				err = 0;
+			else
+				err = hci_cancel_le_scan(hdev);
+		}
 
 		break;
 
-- 
1.8.1.2

[PATCH 5/7] Bluetooth: Remove LE scan work

[Andre Guedes]

This patch removes the LE scan work and all code related to it. We
now use the HCI request framework to enable LE scanning so the LE
scan work is no longer needed.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h | 10 ------
 net/bluetooth/hci_core.c         | 72 ----------------------------------------
 2 files changed, 82 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9efb571..2f95e4d 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -117,13 +117,6 @@ struct oob_data {
 	u8 randomizer[16];
 };
 
-struct le_scan_params {
-	u8 type;
-	u16 interval;
-	u16 window;
-	int timeout;
-};
-
 #define HCI_MAX_SHORT_NAME_LENGTH	10
 
 struct amp_assoc {
@@ -278,9 +271,6 @@ struct hci_dev {
 
 	struct delayed_work	le_scan_disable;
 
-	struct work_struct	le_scan;
-	struct le_scan_params	le_scan_params;
-
 	__s8			adv_tx_power;
 	__u8			adv_data[HCI_MAX_AD_LENGTH];
 	__u8			adv_data_len;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 73487f0..6bdab9b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1061,8 +1061,6 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 {
 	BT_DBG("%s %p", hdev->name, hdev);
 
-	cancel_work_sync(&hdev->le_scan);
-
 	cancel_delayed_work(&hdev->power_off);
 
 	hci_req_cancel(hdev, ENODEV);
@@ -1843,64 +1841,6 @@ int hci_blacklist_del(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type)
 	return mgmt_device_unblocked(hdev, bdaddr, type);
 }
 
-static void le_scan_param_req(struct hci_request *req, unsigned long opt)
-{
-	struct le_scan_params *param =  (struct le_scan_params *) opt;
-	struct hci_cp_le_set_scan_param cp;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.type = param->type;
-	cp.interval = cpu_to_le16(param->interval);
-	cp.window = cpu_to_le16(param->window);
-
-	hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
-}
-
-static void le_scan_enable_req(struct hci_request *req, unsigned long opt)
-{
-	struct hci_cp_le_set_scan_enable cp;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.enable = 1;
-	cp.filter_dup = 1;
-
-	hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
-}
-
-static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
-			  u16 window, int timeout)
-{
-	long timeo = msecs_to_jiffies(3000);
-	struct le_scan_params param;
-	int err;
-
-	BT_DBG("%s", hdev->name);
-
-	if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
-		return -EINPROGRESS;
-
-	param.type = type;
-	param.interval = interval;
-	param.window = window;
-
-	hci_req_lock(hdev);
-
-	err = __hci_req_sync(hdev, le_scan_param_req, (unsigned long) &param,
-			     timeo);
-	if (!err)
-		err = __hci_req_sync(hdev, le_scan_enable_req, 0, timeo);
-
-	hci_req_unlock(hdev);
-
-	if (err < 0)
-		return err;
-
-	queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
-			   msecs_to_jiffies(timeout));
-
-	return 0;
-}
-
 int hci_cancel_le_scan(struct hci_dev *hdev)
 {
 	struct hci_cp_le_set_scan_enable cp;
@@ -1928,17 +1868,6 @@ static void le_scan_disable_work(struct work_struct *work)
 	hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
 }
 
-static void le_scan_work(struct work_struct *work)
-{
-	struct hci_dev *hdev = container_of(work, struct hci_dev, le_scan);
-	struct le_scan_params *param = &hdev->le_scan_params;
-
-	BT_DBG("%s", hdev->name);
-
-	hci_do_le_scan(hdev, param->type, param->interval, param->window,
-		       param->timeout);
-}
-
 static int set_le_scan_param(struct hci_request *req, u8 type, u16 interval,
 			     u16 window)
 {
@@ -2017,7 +1946,6 @@ struct hci_dev *hci_alloc_dev(void)
 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);
 	INIT_WORK(&hdev->tx_work, hci_tx_work);
 	INIT_WORK(&hdev->power_on, hci_power_on);
-	INIT_WORK(&hdev->le_scan, le_scan_work);
 
 	INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
 	INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
-- 
1.8.1.2

[PATCH 6/7] Bluetooth: Change LE scanning timeout macros

[Andre Guedes]

Define LE scanning timeout macros in jiffies just like we do for
others timeout macros.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/mgmt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index de1d2f4..ef22261 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -109,8 +109,8 @@ static const u16 mgmt_events[] = {
 #define LE_SCAN_TYPE			0x01
 #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 LE_SCAN_TIMEOUT_LE_ONLY		msecs_to_jiffies(10240)
+#define LE_SCAN_TIMEOUT_BREDR_LE	msecs_to_jiffies(5120)
 
 #define INQUIRY_LEN_BREDR		0x08	/* TGAP(100) */
 #define INQUIRY_LEN_BREDR_LE		0x04	/* TGAP(100)/2 */
@@ -2438,12 +2438,12 @@ static void enable_le_scan_complete(struct hci_dev *hdev, u8 status)
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_LE:
 		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
-				   msecs_to_jiffies(LE_SCAN_TIMEOUT_LE_ONLY));
+				   LE_SCAN_TIMEOUT_LE_ONLY);
 		break;
 
 	case DISCOV_TYPE_INTERLEAVED:
 		queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
-				   msecs_to_jiffies(LE_SCAN_TIMEOUT_BREDR_LE));
+				   LE_SCAN_TIMEOUT_BREDR_LE);
 		break;
 	default:
 		BT_ERR("Invalid discovery type %d", hdev->discovery.type);
-- 
1.8.1.2

[PATCH 7/7] Bluetooth: Add LE scan type macros

[Andre Guedes]

This patch adds macros for active and passive LE scan type values. It
also removes the LE_SCAN_TYPE macro since it is not used anymore.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2f95e4d..53a4bee 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1159,6 +1159,10 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 							__u8 ltk[16]);
 int hci_do_inquiry(struct hci_dev *hdev, u8 length);
 int hci_cancel_inquiry(struct hci_dev *hdev);
+
+#define LE_SCAN_PASSIVE		0x00
+#define LE_SCAN_ACTIVE		0x01
+
 int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
 int hci_cancel_le_scan(struct hci_dev *hdev);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ef22261..60e0212 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -106,7 +106,6 @@ static const u16 mgmt_events[] = {
  * These LE scan and inquiry parameters were chosen according to LE General
  * Discovery Procedure specification.
  */
-#define LE_SCAN_TYPE			0x01
 #define LE_SCAN_WIN			0x12
 #define LE_SCAN_INT			0x12
 #define LE_SCAN_TIMEOUT_LE_ONLY		msecs_to_jiffies(10240)
@@ -2519,7 +2518,7 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
 
 		hci_req_init(&req, hdev);
 
-		err = hci_le_scan(&req, LE_SCAN_TYPE, LE_SCAN_INT,
+		err = hci_le_scan(&req, LE_SCAN_ACTIVE, LE_SCAN_INT,
 				  LE_SCAN_WIN);
 		if (err < 0) {
 			mgmt_pending_remove(cmd);
-- 
1.8.1.2

Re: [PATCH 2/7] Bluetooth: Update hci_le_scan to use HCI request

[Marcel Holtmann]

Hi Andre,

> This patch changes hci_le_scan helper so it uses the HCI request
> framework to enable LE scanning.
> 
> Also, the LE scanning disable timeout was removed from the helper
> and it is now handled in mgmt start_discovery code.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  3 +--
> net/bluetooth/hci_core.c         | 45 ++++++++++++++++++++++++++----------
> net/bluetooth/mgmt.c             | 49 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 79 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 494f8f5..9efb571 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1169,8 +1169,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 							__u8 ltk[16]);
> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
> int hci_cancel_inquiry(struct hci_dev *hdev);
> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> -		int timeout);
> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
> int hci_cancel_le_scan(struct hci_dev *hdev);
> 
> u8 bdaddr_to_le(u8 bdaddr_type);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7635c2e..160bfe3 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1943,27 +1943,48 @@ static void le_scan_work(struct work_struct *work)
> 		       param->timeout);
> }
> 
> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> -		int timeout)
> +static int set_le_scan_param(struct hci_request *req, u8 type, u16 interval,
> +			     u16 window)
> {
> -	struct le_scan_params *param = &hdev->le_scan_params;
> +	struct hci_cp_le_set_scan_param cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.type = type;
> +	cp.interval = cpu_to_le16(interval);
> +	cp.window = cpu_to_le16(window);
> +
> +	return hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
> +}
> +
> +static int enable_le_scan(struct hci_request *req)
> +{
> +	struct hci_cp_le_set_scan_enable cp;
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.enable = 1;
> +	cp.filter_dup = 1;
> +
> +	return hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +}

since we have this framework now. I do not want to see another 20 functions that are doing something obvious like this.

Please put this all in one function. And as I mentioned to Johan, use 0x01 for values out of the HCI parameters.

Can we please also check in what cases hci_req_add can actually fail. This seems be a pretty silly think to keep checking on it all the time.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: Update hci_le_scan to use HCI request

[Andre Guedes]

Hi Marcel,

On Wed, Mar 6, 2013 at 4:22 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch changes hci_le_scan helper so it uses the HCI request
>> framework to enable LE scanning.
>>
>> Also, the LE scanning disable timeout was removed from the helper
>> and it is now handled in mgmt start_discovery code.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |  3 +--
>> net/bluetooth/hci_core.c         | 45 ++++++++++++++++++++++++++----------
>> net/bluetooth/mgmt.c             | 49 ++++++++++++++++++++++++++++++++++++----
>> 3 files changed, 79 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 494f8f5..9efb571 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1169,8 +1169,7 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
>>                                                       __u8 ltk[16]);
>> int hci_do_inquiry(struct hci_dev *hdev, u8 length);
>> int hci_cancel_inquiry(struct hci_dev *hdev);
>> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>> -             int timeout);
>> +int hci_le_scan(struct hci_request *req, u8 type, u16 interval, u16 window);
>> int hci_cancel_le_scan(struct hci_dev *hdev);
>>
>> u8 bdaddr_to_le(u8 bdaddr_type);
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 7635c2e..160bfe3 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1943,27 +1943,48 @@ static void le_scan_work(struct work_struct *work)
>>                      param->timeout);
>> }
>>
>> -int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>> -             int timeout)
>> +static int set_le_scan_param(struct hci_request *req, u8 type, u16 interval,
>> +                          u16 window)
>> {
>> -     struct le_scan_params *param = &hdev->le_scan_params;
>> +     struct hci_cp_le_set_scan_param cp;
>> +
>> +     memset(&cp, 0, sizeof(cp));
>> +     cp.type = type;
>> +     cp.interval = cpu_to_le16(interval);
>> +     cp.window = cpu_to_le16(window);
>> +
>> +     return hci_req_add(req, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
>> +}
>> +
>> +static int enable_le_scan(struct hci_request *req)
>> +{
>> +     struct hci_cp_le_set_scan_enable cp;
>> +
>> +     memset(&cp, 0, sizeof(cp));
>> +     cp.enable = 1;
>> +     cp.filter_dup = 1;
>> +
>> +     return hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> +}
>
> since we have this framework now. I do not want to see another 20 functions that are doing something obvious like this.
>
> Please put this all in one function. And as I mentioned to Johan, use 0x01 for values out of the HCI parameters.

Sure, I'll fix it.

> Can we please also check in what cases hci_req_add can actually fail. This seems be a pretty silly think to keep checking on it all the time.

Today, hci_req_add fails only if we can't allocate a skb. So I'll
remove this error checking.

Regards,

Andre

Re: [PATCH 1/7] Bluetooth: Add hci_req_cleanup function

[Marcel Holtmann]

Hi Andre,

> This patch adds the hci_req_cleanup function which basically frees
> all HCI commands queued on a given request.
> 
> This function must be called in case something goes wrong during HCI
> request creation. Otherwise, HCI commands already queued on req->cmd_q
> will cause memory leaks.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/hci_core.c         | 5 +++++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3a9cbf2..494f8f5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1045,6 +1045,7 @@ struct hci_request {
> };
> 
> void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
> +void hci_req_cleanup(struct hci_request *req);
> int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
> int hci_req_add(struct hci_request *req, u16 opcode, u32 plen, void *param);
> void hci_req_cmd_complete(struct hci_dev *hdev, u16 opcode, u8 status);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index b6d44a2..7635c2e 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2443,6 +2443,11 @@ void hci_req_init(struct hci_request *req, struct hci_dev *hdev)
> 	req->hdev = hdev;
> }
> 
> +void hci_req_cleanup(struct hci_request *req)
> +{
> +	skb_queue_purge(&req->cmd_q);
> +}
> +
> int hci_req_run(struct hci_request *req, hci_req_complete_t complete)
> {
> 	struct hci_dev *hdev = req->hdev;

since the other one is called hci_req_run, then this should be named hci_req_abort or hci_req_cancel.

Regards

Marcel