[PATCH 0/3] Bluetooth: Cleanups to syncronous request functions

[PATCH 0/3] Bluetooth: Cleanups to syncronous request functions

[Johan Hedberg]

Hi,

Here's a couple of initial cleanups to the new code in hci_request.c.
The patches are essentially removing the *_complete() functions that
are now unecessary.

Johan

-----------------
Johan Hedberg (3):
      Bluetooth: Pass inquiry length to bredr_inquiry()
      Bluetooth: Simplify le_scan_disable_work()
      Bluetooth: Remove unnecessary le_scan_restart_work_complete() function

 net/bluetooth/hci_request.c | 204 +++++++++++++++++--------------------------
 1 file changed, 80 insertions(+), 124 deletions(-)

[PATCH 1/3] Bluetooth: Pass inquiry length to bredr_inquiry()

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Passing the needed inquiry length to bredr_inquiry() makes it possible
to also use this helper for interleaved discovery where the controller
doesn't support simultaneous Inquiry & LE scan.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_request.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 3219ee66faad..98827e7631ca 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1044,6 +1044,7 @@ static void le_scan_restart_work(struct work_struct *work)
 
 static int bredr_inquiry(struct hci_request *req, unsigned long opt)
 {
+	u8 length = opt;
 	struct hci_cp_inquiry cp;
 	/* General inquiry access code (GIAC) */
 	u8 lap[3] = { 0x33, 0x8b, 0x9e };
@@ -1056,7 +1057,7 @@ static int bredr_inquiry(struct hci_request *req, unsigned long opt)
 
 	memset(&cp, 0, sizeof(cp));
 	memcpy(&cp.lap, lap, sizeof(cp.lap));
-	cp.length = DISCOV_BREDR_INQUIRY_LEN;
+	cp.length = length;
 
 	hci_req_add(req, HCI_OP_INQUIRY, sizeof(cp), &cp);
 
@@ -1150,7 +1151,7 @@ static int interleaved_discov(struct hci_request *req, unsigned long opt)
 	if (err)
 		return err;
 
-	return bredr_inquiry(req, opt);
+	return bredr_inquiry(req, DISCOV_BREDR_INQUIRY_LEN);
 }
 
 static void start_discovery(struct hci_dev *hdev, u8 *status)
@@ -1162,7 +1163,8 @@ static void start_discovery(struct hci_dev *hdev, u8 *status)
 	switch (hdev->discovery.type) {
 	case DISCOV_TYPE_BREDR:
 		if (!hci_dev_test_flag(hdev, HCI_INQUIRY))
-			hci_req_sync(hdev, bredr_inquiry, 0, HCI_CMD_TIMEOUT,
+			hci_req_sync(hdev, bredr_inquiry,
+				     DISCOV_BREDR_INQUIRY_LEN, HCI_CMD_TIMEOUT,
 				     status);
 		return;
 	case DISCOV_TYPE_INTERLEAVED:
-- 
2.5.0

[PATCH 2/3] Bluetooth: Simplify le_scan_disable_work()

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

Merge le_scan_disable_work_complete into the main le_scan_disable_work
function and take advantage of the updated bredr_inquiry() to run the
Inquiry through hci_req_sync().

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_request.c | 148 +++++++++++++++++---------------------------
 1 file changed, 57 insertions(+), 91 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 98827e7631ca..04c3357b1e1c 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -864,83 +864,31 @@ static void bg_scan_update(struct work_struct *work)
 	hci_dev_unlock(hdev);
 }
 
-static void inquiry_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+static int le_scan_disable(struct hci_request *req, unsigned long opt)
 {
-	if (status) {
-		BT_ERR("Failed to start inquiry: status %d", status);
-
-		hci_dev_lock(hdev);
-		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
-		hci_dev_unlock(hdev);
-		return;
-	}
+	hci_req_add_le_scan_disable(req);
+	return 0;
 }
 
-static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status)
+static int bredr_inquiry(struct hci_request *req, unsigned long opt)
 {
+	u8 length = opt;
 	/* General inquiry access code (GIAC) */
 	u8 lap[3] = { 0x33, 0x8b, 0x9e };
 	struct hci_cp_inquiry cp;
-	int err;
-
-	if (status) {
-		BT_ERR("Failed to disable LE scanning: status %d", status);
-		return;
-	}
-
-	hdev->discovery.scan_start = 0;
-
-	switch (hdev->discovery.type) {
-	case DISCOV_TYPE_LE:
-		hci_dev_lock(hdev);
-		hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
-		hci_dev_unlock(hdev);
-		break;
 
-	case DISCOV_TYPE_INTERLEAVED:
-		hci_dev_lock(hdev);
-
-		if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY,
-			     &hdev->quirks)) {
-			/* If we were running LE only scan, change discovery
-			 * state. If we were running both LE and BR/EDR inquiry
-			 * simultaneously, and BR/EDR inquiry is already
-			 * finished, stop discovery, otherwise BR/EDR inquiry
-			 * will stop discovery when finished. If we will resolve
-			 * remote device name, do not change discovery state.
-			 */
-			if (!test_bit(HCI_INQUIRY, &hdev->flags) &&
-			    hdev->discovery.state != DISCOVERY_RESOLVING)
-				hci_discovery_set_state(hdev,
-							DISCOVERY_STOPPED);
-		} else {
-			struct hci_request req;
-
-			hci_inquiry_cache_flush(hdev);
-
-			hci_req_init(&req, hdev);
+	BT_DBG("%s", req->hdev->name);
 
-			memset(&cp, 0, sizeof(cp));
-			memcpy(&cp.lap, lap, sizeof(cp.lap));
-			cp.length = DISCOV_INTERLEAVED_INQUIRY_LEN;
-			hci_req_add(&req, HCI_OP_INQUIRY, sizeof(cp), &cp);
+	hci_dev_lock(req->hdev);
+	hci_inquiry_cache_flush(req->hdev);
+	hci_dev_unlock(req->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);
-			}
-		}
+	memset(&cp, 0, sizeof(cp));
+	memcpy(&cp.lap, lap, sizeof(cp.lap));
+	cp.length = length;
 
-		hci_dev_unlock(hdev);
-		break;
-	}
-}
+	hci_req_add(req, HCI_OP_INQUIRY, sizeof(cp), &cp);
 
-static int le_scan_disable(struct hci_request *req, unsigned long opt)
-{
-	hci_req_add_le_scan_disable(req);
 	return 0;
 }
 
@@ -949,17 +897,57 @@ static void le_scan_disable_work(struct work_struct *work)
 	struct hci_dev *hdev = container_of(work, struct hci_dev,
 					    le_scan_disable.work);
 	u8 status;
-	int err;
 
 	BT_DBG("%s", hdev->name);
 
+	if (!hci_dev_test_flag(hdev, HCI_LE_SCAN))
+		return;
+
 	cancel_delayed_work(&hdev->le_scan_restart);
 
-	err = hci_req_sync(hdev, le_scan_disable, 0, HCI_CMD_TIMEOUT, &status);
-	if (err)
+	hci_req_sync(hdev, le_scan_disable, 0, HCI_CMD_TIMEOUT, &status);
+	if (status) {
+		BT_ERR("Failed to disable LE scan: status 0x%02x", status);
+		return;
+	}
+
+	hdev->discovery.scan_start = 0;
+
+	/* If we were running LE only scan, change discovery state. If
+	 * we were running both LE and BR/EDR inquiry simultaneously,
+	 * and BR/EDR inquiry is already finished, stop discovery,
+	 * otherwise BR/EDR inquiry will stop discovery when finished.
+	 * If we will resolve remote device name, do not change
+	 * discovery state.
+	 */
+
+	if (hdev->discovery.type == DISCOV_TYPE_LE)
+		goto discov_stopped;
+
+	if (hdev->discovery.type != DISCOV_TYPE_INTERLEAVED)
+		return;
+
+	if (test_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks)) {
+		if (!test_bit(HCI_INQUIRY, &hdev->flags) &&
+		    hdev->discovery.state != DISCOVERY_RESOLVING)
+			goto discov_stopped;
+
 		return;
+	}
+
+	hci_req_sync(hdev, bredr_inquiry, DISCOV_INTERLEAVED_INQUIRY_LEN,
+		     HCI_CMD_TIMEOUT, &status);
+	if (status) {
+		BT_ERR("Inquiry failed: status 0x%02x", status);
+		goto discov_stopped;
+	}
+
+	return;
 
-	le_scan_disable_work_complete(hdev, status);
+discov_stopped:
+	hci_dev_lock(hdev);
+	hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
+	hci_dev_unlock(hdev);
 }
 
 static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
@@ -1042,28 +1030,6 @@ static void le_scan_restart_work(struct work_struct *work)
 	le_scan_restart_work_complete(hdev, status);
 }
 
-static int bredr_inquiry(struct hci_request *req, unsigned long opt)
-{
-	u8 length = opt;
-	struct hci_cp_inquiry cp;
-	/* General inquiry access code (GIAC) */
-	u8 lap[3] = { 0x33, 0x8b, 0x9e };
-
-	BT_DBG("%s", req->hdev->name);
-
-	hci_dev_lock(req->hdev);
-	hci_inquiry_cache_flush(req->hdev);
-	hci_dev_unlock(req->hdev);
-
-	memset(&cp, 0, sizeof(cp));
-	memcpy(&cp.lap, lap, sizeof(cp.lap));
-	cp.length = length;
-
-	hci_req_add(req, HCI_OP_INQUIRY, sizeof(cp), &cp);
-
-	return 0;
-}
-
 static void cancel_adv_timeout(struct hci_dev *hdev)
 {
 	if (hdev->adv_instance_timeout) {
-- 
2.5.0

[PATCH 3/3] Bluetooth: Remove unnecessary le_scan_restart_work_complete() function

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The only user of this, le_scan_restart_work(), is so short and simple
that it makes sense to just merge the code there.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/hci_request.c | 60 ++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 04c3357b1e1c..e8345d8106b5 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -950,12 +950,35 @@ discov_stopped:
 	hci_dev_unlock(hdev);
 }
 
-static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
+static int le_scan_restart(struct hci_request *req, unsigned long opt)
+{
+	struct hci_dev *hdev = req->hdev;
+	struct hci_cp_le_set_scan_enable cp;
+
+	/* If controller is not scanning we are done. */
+	if (!hci_dev_test_flag(hdev, HCI_LE_SCAN))
+		return 0;
+
+	hci_req_add_le_scan_disable(req);
+
+	memset(&cp, 0, sizeof(cp));
+	cp.enable = LE_SCAN_ENABLE;
+	cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
+	hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+	return 0;
+}
+
+static void le_scan_restart_work(struct work_struct *work)
 {
+	struct hci_dev *hdev = container_of(work, struct hci_dev,
+					    le_scan_restart.work);
 	unsigned long timeout, duration, scan_start, now;
+	u8 status;
 
 	BT_DBG("%s", hdev->name);
 
+	hci_req_sync(hdev, le_scan_restart, 0, HCI_CMD_TIMEOUT, &status);
 	if (status) {
 		BT_ERR("Failed to restart LE scan: status %d", status);
 		return;
@@ -995,41 +1018,6 @@ unlock:
 	hci_dev_unlock(hdev);
 }
 
-static int le_scan_restart(struct hci_request *req, unsigned long opt)
-{
-	struct hci_dev *hdev = req->hdev;
-	struct hci_cp_le_set_scan_enable cp;
-
-	/* If controller is not scanning we are done. */
-	if (!hci_dev_test_flag(hdev, HCI_LE_SCAN))
-		return 0;
-
-	hci_req_add_le_scan_disable(req);
-
-	memset(&cp, 0, sizeof(cp));
-	cp.enable = LE_SCAN_ENABLE;
-	cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
-	hci_req_add(req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
-
-	return 0;
-}
-
-static void le_scan_restart_work(struct work_struct *work)
-{
-	struct hci_dev *hdev = container_of(work, struct hci_dev,
-					    le_scan_restart.work);
-	u8 status;
-	int err;
-
-	BT_DBG("%s", hdev->name);
-
-	err = hci_req_sync(hdev, le_scan_restart, 0, HCI_CMD_TIMEOUT, &status);
-	if (err)
-		return;
-
-	le_scan_restart_work_complete(hdev, status);
-}
-
 static void cancel_adv_timeout(struct hci_dev *hdev)
 {
 	if (hdev->adv_instance_timeout) {
-- 
2.5.0

Re: [PATCH 0/3] Bluetooth: Cleanups to syncronous request functions

[Marcel Holtmann]

Hi Johan,

> Here's a couple of initial cleanups to the new code in hci_request.c.
> The patches are essentially removing the *_complete() functions that
> are now unecessary.
> 
> Johan
> 
> -----------------
> Johan Hedberg (3):
>      Bluetooth: Pass inquiry length to bredr_inquiry()
>      Bluetooth: Simplify le_scan_disable_work()
>      Bluetooth: Remove unnecessary le_scan_restart_work_complete() function
> 
> net/bluetooth/hci_request.c | 204 +++++++++++++++++--------------------------
> 1 file changed, 80 insertions(+), 124 deletions(-)

all 3 patches have been applied to bluetooth-next tree.

Regards

Marcel