[PATCH 0/4] Bluetooth: Add/Remove Device completion through hci_request

[PATCH 0/4] Bluetooth: Add/Remove Device completion through hci_request

[Johan Hedberg]

Hi,

So far the last couple of Remove Device tests in mgmt-tester have been
randomly failing some 50% of the time, mainly because the Add/Remove
mgmt command implementation wouldn't wait for HCI command completion
before returning. This would let the tests proceed from the setup to the
test phase when there are still setup phase HCI command uncompleted.

This set of patches updates the HCI scanning functions to take a
hci_request parameter so that the completion of any HCI commands can be
hooked up to sending the mgmt response to user space.

Johan

----------------------------------------------------------------
Johan Hedberg (4):
      Bluetooth: Move struct hci_request definition higher up in hci_core.h
      Bluetooth: Add hci_request parameter to hci_update_background_scan
      Bluetooth: Fix Remove Device to wait for HCI before sending cmd_complete
      Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

 include/net/bluetooth/hci_core.h |  24 +++----
 net/bluetooth/hci_conn.c         |   2 +-
 net/bluetooth/hci_core.c         |  35 ++++++-----
 net/bluetooth/hci_event.c        |   4 +-
 net/bluetooth/mgmt.c             | 119 ++++++++++++++++++++++++++++-------
 5 files changed, 131 insertions(+), 53 deletions(-)

[PATCH 1/4] Bluetooth: Move struct hci_request definition higher up in hci_core.h

[Johan Hedberg]

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

We'll soon start using struct hci_request as a parameter to several more
functions in hci_core.h. To avoid compiler warnings of an undefined
struct the definition needs to be moved higher up in the file. This
patch moves it to the end of the block where several other structs are
also defined.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3c7827005c25..75aaae07037f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -479,6 +479,16 @@ struct hci_conn_params {
 	struct hci_conn *conn;
 };
 
+struct hci_request {
+	struct hci_dev		*hdev;
+	struct sk_buff_head	cmd_q;
+
+	/* If something goes wrong when building the HCI request, the error
+	 * value is stored in this field.
+	 */
+	int			err;
+};
+
 extern struct list_head hci_dev_list;
 extern struct list_head hci_cb_list;
 extern rwlock_t hci_dev_list_lock;
@@ -1284,16 +1294,6 @@ static inline int hci_check_conn_params(u16 min, u16 max, u16 latency,
 int hci_register_cb(struct hci_cb *hcb);
 int hci_unregister_cb(struct hci_cb *hcb);
 
-struct hci_request {
-	struct hci_dev		*hdev;
-	struct sk_buff_head	cmd_q;
-
-	/* If something goes wrong when building the HCI request, the error
-	 * value is stored in this field.
-	 */
-	int			err;
-};
-
 void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
 int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
 void hci_req_add(struct hci_request *req, u16 opcode, u32 plen,
-- 
2.1.0

[PATCH 2/4] Bluetooth: Add hci_request parameter to hci_update_background_scan

[Johan Hedberg]

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

Many places using hci_update_background_scan() try to synchronize
whatever they're doing with the help of hci_request callbacks. However,
since the hci_update_background_scan() function hasn't so far accepted a
hci_request pointer any commands triggered by it have been left out by
the synchronization. This patch an extra parameter to the function to
fix the issue. This there are also many places where there is no
existing hci_request context, the parameter is made optional, i.e. by
passing NULL hci_update_background_scan() uses its own request and a
dummy callback.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  2 +-
 net/bluetooth/hci_conn.c         |  2 +-
 net/bluetooth/hci_core.c         | 33 ++++++++++++++++++++-------------
 net/bluetooth/hci_event.c        |  4 ++--
 net/bluetooth/mgmt.c             | 19 +++++++++----------
 5 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 75aaae07037f..d89a359c83ea 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -940,7 +940,7 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
 						  bdaddr_t *addr,
 						  u8 addr_type);
 
-void hci_update_background_scan(struct hci_dev *hdev);
+void hci_update_background_scan(struct hci_dev *hdev, struct hci_request *req);
 
 void hci_uuids_clear(struct hci_dev *hdev);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fe18825cc8a4..2d3268bcd745 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -621,7 +621,7 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
 	/* Since we may have temporarily stopped the background scanning in
 	 * favor of connection establishment, we should restart it.
 	 */
-	hci_update_background_scan(hdev);
+	hci_update_background_scan(hdev, NULL);
 
 	/* Re-enable advertising in case this was a failed connection
 	 * attempt as a peripheral.
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5dcacf9607e4..4e54f6e0d248 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2072,7 +2072,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
 
 	switch (state) {
 	case DISCOVERY_STOPPED:
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 
 		if (old_state != DISCOVERY_STARTING)
 			mgmt_discovering(hdev, 0);
@@ -3749,17 +3749,17 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 	switch (auto_connect) {
 	case HCI_AUTO_CONN_DISABLED:
 	case HCI_AUTO_CONN_LINK_LOSS:
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 		break;
 	case HCI_AUTO_CONN_REPORT:
 		list_add(&params->action, &hdev->pend_le_reports);
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 		break;
 	case HCI_AUTO_CONN_DIRECT:
 	case HCI_AUTO_CONN_ALWAYS:
 		if (!is_connected(hdev, addr, addr_type)) {
 			list_add(&params->action, &hdev->pend_le_conns);
-			hci_update_background_scan(hdev);
+			hci_update_background_scan(hdev, NULL);
 		}
 		break;
 	}
@@ -3795,7 +3795,7 @@ void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
 
 	hci_conn_params_free(params);
 
-	hci_update_background_scan(hdev);
+	hci_update_background_scan(hdev, NULL);
 
 	BT_DBG("addr %pMR (type %u)", addr, addr_type);
 }
@@ -3823,7 +3823,7 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
 	list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list)
 		hci_conn_params_free(params);
 
-	hci_update_background_scan(hdev);
+	hci_update_background_scan(hdev, NULL);
 
 	BT_DBG("All LE connection parameters were removed");
 }
@@ -5693,9 +5693,9 @@ static void update_background_scan_complete(struct hci_dev *hdev, u8 status)
  *
  * This function requires the caller holds hdev->lock.
  */
-void hci_update_background_scan(struct hci_dev *hdev)
+void hci_update_background_scan(struct hci_dev *hdev, struct hci_request *req)
 {
-	struct hci_request req;
+	struct hci_request local_req;
 	struct hci_conn *conn;
 	int err;
 
@@ -5724,7 +5724,10 @@ void hci_update_background_scan(struct hci_dev *hdev)
 	 */
 	hci_discovery_filter_clear(hdev);
 
-	hci_req_init(&req, hdev);
+	if (!req) {
+		req = &local_req;
+		hci_req_init(req, hdev);
+	}
 
 	if (list_empty(&hdev->pend_le_conns) &&
 	    list_empty(&hdev->pend_le_reports)) {
@@ -5737,7 +5740,7 @@ void hci_update_background_scan(struct hci_dev *hdev)
 		if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
 			return;
 
-		hci_req_add_le_scan_disable(&req);
+		hci_req_add_le_scan_disable(req);
 
 		BT_DBG("%s stopping background scanning", hdev->name);
 	} else {
@@ -5757,14 +5760,18 @@ void hci_update_background_scan(struct hci_dev *hdev)
 		 * don't miss any advertising (due to duplicates filter).
 		 */
 		if (test_bit(HCI_LE_SCAN, &hdev->dev_flags))
-			hci_req_add_le_scan_disable(&req);
+			hci_req_add_le_scan_disable(req);
 
-		hci_req_add_le_passive_scan(&req);
+		hci_req_add_le_passive_scan(req);
 
 		BT_DBG("%s starting background scanning", hdev->name);
 	}
 
-	err = hci_req_run(&req, update_background_scan_complete);
+	/* Callers providing a req are responsible for calling req_run */
+	if (req != &local_req)
+		return;
+
+	err = hci_req_run(req, update_background_scan_complete);
 	if (err)
 		BT_ERR("Failed to run HCI request: err %d", err);
 }
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 39a5c8a01726..80ccafcde477 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2323,7 +2323,7 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		case HCI_AUTO_CONN_ALWAYS:
 			list_del_init(&params->action);
 			list_add(&params->action, &hdev->pend_le_conns);
-			hci_update_background_scan(hdev);
+			hci_update_background_scan(hdev, NULL);
 			break;
 
 		default:
@@ -4338,7 +4338,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 
 unlock:
-	hci_update_background_scan(hdev);
+	hci_update_background_scan(hdev, NULL);
 	hci_dev_unlock(hdev);
 }
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 693ce8bcd06e..7efdeb3cf224 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1816,7 +1816,7 @@ static void set_connectable_complete(struct hci_dev *hdev, u8 status)
 		hci_update_page_scan(hdev, NULL);
 		if (discov_changed)
 			mgmt_update_adv_data(hdev);
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 	}
 
 remove_cmd:
@@ -1848,7 +1848,7 @@ static int set_connectable_update_settings(struct hci_dev *hdev,
 
 	if (changed) {
 		hci_update_page_scan(hdev, NULL);
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 		return new_settings(hdev, sk);
 	}
 
@@ -2227,9 +2227,8 @@ static void le_enable_complete(struct hci_dev *hdev, u8 status)
 		hci_req_init(&req, hdev);
 		update_adv_data(&req);
 		update_scan_rsp_data(&req);
+		hci_update_background_scan(hdev, &req);
 		hci_req_run(&req, NULL);
-
-		hci_update_background_scan(hdev);
 	}
 
 unlock:
@@ -5587,7 +5586,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 		list_del(&params->action);
 		list_del(&params->list);
 		kfree(params);
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 
 		device_removed(sk, hdev, &cp->addr.bdaddr, cp->addr.type);
 	} else {
@@ -5620,7 +5619,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 
 		BT_DBG("All LE connection parameters were removed");
 
-		hci_update_background_scan(hdev);
+		hci_update_background_scan(hdev, NULL);
 	}
 
 complete:
@@ -6037,7 +6036,7 @@ void mgmt_index_removed(struct hci_dev *hdev)
 }
 
 /* This function requires the caller holds hdev->lock */
-static void restart_le_actions(struct hci_dev *hdev)
+static void restart_le_actions(struct hci_dev *hdev, struct hci_request *req)
 {
 	struct hci_conn_params *p;
 
@@ -6060,7 +6059,7 @@ static void restart_le_actions(struct hci_dev *hdev)
 		}
 	}
 
-	hci_update_background_scan(hdev);
+	hci_update_background_scan(hdev, req);
 }
 
 static void powered_complete(struct hci_dev *hdev, u8 status)
@@ -6071,8 +6070,6 @@ static void powered_complete(struct hci_dev *hdev, u8 status)
 
 	hci_dev_lock(hdev);
 
-	restart_le_actions(hdev);
-
 	mgmt_pending_foreach(MGMT_OP_SET_POWERED, hdev, settings_rsp, &match);
 
 	new_settings(hdev, match.sk);
@@ -6130,6 +6127,8 @@ static int powered_update_hci(struct hci_dev *hdev)
 
 		if (test_bit(HCI_ADVERTISING, &hdev->dev_flags))
 			enable_advertising(&req);
+
+		restart_le_actions(hdev, &req);
 	}
 
 	link_sec = test_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
-- 
2.1.0

[PATCH 3/4] Bluetooth: Fix Remove Device to wait for HCI before sending cmd_complete

[Johan Hedberg]

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

This patch updates the Remove Device mgmt command handler to use a
hci_request to wait for HCI command completion before notifying user
space of the mgmt command completion. This way we ensure that once the
mgmt command returns all HCI commands triggered by it have also
completed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/mgmt.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7efdeb3cf224..ab3dae3076d7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5522,16 +5522,47 @@ static void device_removed(struct sock *sk, struct hci_dev *hdev,
 	mgmt_event(MGMT_EV_DEVICE_REMOVED, hdev, &ev, sizeof(ev), sk);
 }
 
+static void remove_device_complete(struct hci_dev *hdev, u8 status)
+{
+	struct pending_cmd *cmd;
+
+	BT_DBG("status 0x%02x", status);
+
+	hci_dev_lock(hdev);
+
+	cmd = mgmt_pending_find(MGMT_OP_REMOVE_DEVICE, hdev);
+	if (!cmd)
+		goto unlock;
+
+	cmd->cmd_complete(cmd, mgmt_status(status));
+	mgmt_pending_remove(cmd);
+
+unlock:
+	hci_dev_unlock(hdev);
+}
+
 static int remove_device(struct sock *sk, struct hci_dev *hdev,
 			 void *data, u16 len)
 {
 	struct mgmt_cp_remove_device *cp = data;
+	struct pending_cmd *cmd;
+	struct hci_request req;
 	int err;
 
 	BT_DBG("%s", hdev->name);
 
+	hci_req_init(&req, hdev);
+
 	hci_dev_lock(hdev);
 
+	cmd = mgmt_pending_add(sk, MGMT_OP_REMOVE_DEVICE, hdev, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+
+	cmd->cmd_complete = addr_cmd_complete;
+
 	if (bacmp(&cp->addr.bdaddr, BDADDR_ANY)) {
 		struct hci_conn_params *params;
 		u8 addr_type;
@@ -5555,7 +5586,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 				goto unlock;
 			}
 
-			hci_update_page_scan(hdev, NULL);
+			hci_update_page_scan(hdev, &req);
 
 			device_removed(sk, hdev, &cp->addr.bdaddr,
 				       cp->addr.type);
@@ -5586,7 +5617,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 		list_del(&params->action);
 		list_del(&params->list);
 		kfree(params);
-		hci_update_background_scan(hdev, NULL);
+		hci_update_background_scan(hdev, &req);
 
 		device_removed(sk, hdev, &cp->addr.bdaddr, cp->addr.type);
 	} else {
@@ -5606,7 +5637,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 			kfree(b);
 		}
 
-		hci_update_page_scan(hdev, NULL);
+		hci_update_page_scan(hdev, &req);
 
 		list_for_each_entry_safe(p, tmp, &hdev->le_conn_params, list) {
 			if (p->auto_connect == HCI_AUTO_CONN_DISABLED)
@@ -5619,12 +5650,18 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
 
 		BT_DBG("All LE connection parameters were removed");
 
-		hci_update_background_scan(hdev, NULL);
+		hci_update_background_scan(hdev, &req);
 	}
 
 complete:
-	err = cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_DEVICE,
-			   MGMT_STATUS_SUCCESS, &cp->addr, sizeof(cp->addr));
+	err = hci_req_run(&req, remove_device_complete);
+	if (err < 0) {
+		if (err == -ENODATA) {
+			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
+			err = 0;
+		}
+		mgmt_pending_remove(cmd);
+	}
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.1.0

[PATCH 4/4] Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

[Johan Hedberg]

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

This patch updates the Add Device mgmt command handler to use a
hci_request to wait for HCI command completion before notifying user
space of the mgmt command completion. To do this we need to add an extra
hci_request parameter to the hci_conn_params_set function. This way we
ensure that once the mgmt command returns all HCI commands triggered by
it have also completed.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |  2 +-
 net/bluetooth/hci_core.c         |  8 +++---
 net/bluetooth/mgmt.c             | 55 ++++++++++++++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d89a359c83ea..3cd121856b1e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -931,7 +931,7 @@ struct hci_conn_params *hci_conn_params_lookup(struct hci_dev *hdev,
 struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
 					    bdaddr_t *addr, u8 addr_type);
 int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
-			u8 auto_connect);
+			u8 auto_connect, struct hci_request *req);
 void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
 void hci_conn_params_clear_all(struct hci_dev *hdev);
 void hci_conn_params_clear_disabled(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4e54f6e0d248..d4358dc40375 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3733,7 +3733,7 @@ struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
 
 /* This function requires the caller holds hdev->lock */
 int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
-			u8 auto_connect)
+			u8 auto_connect, struct hci_request *req)
 {
 	struct hci_conn_params *params;
 
@@ -3749,17 +3749,17 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
 	switch (auto_connect) {
 	case HCI_AUTO_CONN_DISABLED:
 	case HCI_AUTO_CONN_LINK_LOSS:
-		hci_update_background_scan(hdev, NULL);
+		hci_update_background_scan(hdev, req);
 		break;
 	case HCI_AUTO_CONN_REPORT:
 		list_add(&params->action, &hdev->pend_le_reports);
-		hci_update_background_scan(hdev, NULL);
+		hci_update_background_scan(hdev, req);
 		break;
 	case HCI_AUTO_CONN_DIRECT:
 	case HCI_AUTO_CONN_ALWAYS:
 		if (!is_connected(hdev, addr, addr_type)) {
 			list_add(&params->action, &hdev->pend_le_conns);
-			hci_update_background_scan(hdev, NULL);
+			hci_update_background_scan(hdev, req);
 		}
 		break;
 	}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index ab3dae3076d7..9c44f4c3c2c0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -5436,10 +5436,31 @@ static void device_added(struct sock *sk, struct hci_dev *hdev,
 	mgmt_event(MGMT_EV_DEVICE_ADDED, hdev, &ev, sizeof(ev), sk);
 }
 
+static void add_device_complete(struct hci_dev *hdev, u8 status)
+{
+	struct pending_cmd *cmd;
+
+	BT_DBG("status 0x%02x", status);
+
+	hci_dev_lock(hdev);
+
+	cmd = mgmt_pending_find(MGMT_OP_ADD_DEVICE, hdev);
+	if (!cmd)
+		goto unlock;
+
+	cmd->cmd_complete(cmd, mgmt_status(status));
+	mgmt_pending_remove(cmd);
+
+unlock:
+	hci_dev_unlock(hdev);
+}
+
 static int add_device(struct sock *sk, struct hci_dev *hdev,
 		      void *data, u16 len)
 {
 	struct mgmt_cp_add_device *cp = data;
+	struct pending_cmd *cmd;
+	struct hci_request req;
 	u8 auto_conn, addr_type;
 	int err;
 
@@ -5456,14 +5477,23 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 				    MGMT_STATUS_INVALID_PARAMS,
 				    &cp->addr, sizeof(cp->addr));
 
+	hci_req_init(&req, hdev);
+
 	hci_dev_lock(hdev);
 
+	cmd = mgmt_pending_add(sk, MGMT_OP_ADD_DEVICE, hdev, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto unlock;
+	}
+
+	cmd->cmd_complete = addr_cmd_complete;
+
 	if (cp->addr.type == BDADDR_BREDR) {
 		/* Only incoming connections action is supported for now */
 		if (cp->action != 0x01) {
-			err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
-					   MGMT_STATUS_INVALID_PARAMS,
-					   &cp->addr, sizeof(cp->addr));
+			cmd->cmd_complete(cmd, MGMT_STATUS_INVALID_PARAMS);
+			mgmt_pending_remove(cmd);
 			goto unlock;
 		}
 
@@ -5472,7 +5502,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 		if (err)
 			goto unlock;
 
-		hci_update_page_scan(hdev, NULL);
+		hci_update_page_scan(hdev, &req);
 
 		goto added;
 	}
@@ -5493,18 +5523,23 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
 	 * they will be created and configured with defaults.
 	 */
 	if (hci_conn_params_set(hdev, &cp->addr.bdaddr, addr_type,
-				auto_conn) < 0) {
-		err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
-				   MGMT_STATUS_FAILED,
-				   &cp->addr, sizeof(cp->addr));
+				auto_conn, &req) < 0) {
+		cmd->cmd_complete(cmd, MGMT_STATUS_FAILED);
+		mgmt_pending_remove(cmd);
 		goto unlock;
 	}
 
 added:
 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
 
-	err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
-			   MGMT_STATUS_SUCCESS, &cp->addr, sizeof(cp->addr));
+	err = hci_req_run(&req, add_device_complete);
+	if (err < 0) {
+		if (err == -ENODATA) {
+			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
+			err = 0;
+		}
+		mgmt_pending_remove(cmd);
+	}
 
 unlock:
 	hci_dev_unlock(hdev);
-- 
2.1.0

Re: [PATCH 1/4] Bluetooth: Move struct hci_request definition higher up in hci_core.h

[Marcel Holtmann]

Hi Johan,

> We'll soon start using struct hci_request as a parameter to several more
> functions in hci_core.h. To avoid compiler warnings of an undefined
> struct the definition needs to be moved higher up in the file. This
> patch moves it to the end of the block where several other structs are
> also defined.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 3c7827005c25..75aaae07037f 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -479,6 +479,16 @@ struct hci_conn_params {
> 	struct hci_conn *conn;
> };
> 
> +struct hci_request {
> +	struct hci_dev		*hdev;
> +	struct sk_buff_head	cmd_q;
> +
> +	/* If something goes wrong when building the HCI request, the error
> +	 * value is stored in this field.
> +	 */
> +	int			err;
> +};
> +
> extern struct list_head hci_dev_list;
> extern struct list_head hci_cb_list;
> extern rwlock_t hci_dev_list_lock;
> @@ -1284,16 +1294,6 @@ static inline int hci_check_conn_params(u16 min, u16 max, u16 latency,
> int hci_register_cb(struct hci_cb *hcb);
> int hci_unregister_cb(struct hci_cb *hcb);
> 
> -struct hci_request {
> -	struct hci_dev		*hdev;
> -	struct sk_buff_head	cmd_q;
> -
> -	/* If something goes wrong when building the HCI request, the error
> -	 * value is stored in this field.
> -	 */
> -	int			err;
> -};
> -
> void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
> int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
> void hci_req_add(struct hci_request *req, u16 opcode, u32 plen,

any reason to not move these along with the data structure? They actually do belong together.

Regards

Marcel

Re: [PATCH 2/4] Bluetooth: Add hci_request parameter to hci_update_background_scan

[Marcel Holtmann]

Hi Johan,

> Many places using hci_update_background_scan() try to synchronize
> whatever they're doing with the help of hci_request callbacks. However,
> since the hci_update_background_scan() function hasn't so far accepted a
> hci_request pointer any commands triggered by it have been left out by
> the synchronization. This patch an extra parameter to the function to
> fix the issue. This there are also many places where there is no
> existing hci_request context, the parameter is made optional, i.e. by
> passing NULL hci_update_background_scan() uses its own request and a
> dummy callback.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  2 +-
> net/bluetooth/hci_conn.c         |  2 +-
> net/bluetooth/hci_core.c         | 33 ++++++++++++++++++++-------------
> net/bluetooth/hci_event.c        |  4 ++--
> net/bluetooth/mgmt.c             | 19 +++++++++----------
> 5 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 75aaae07037f..d89a359c83ea 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -940,7 +940,7 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
> 						  bdaddr_t *addr,
> 						  u8 addr_type);
> 
> -void hci_update_background_scan(struct hci_dev *hdev);
> +void hci_update_background_scan(struct hci_dev *hdev, struct hci_request *req);

this goes a bit against what we done otherwise in the code base. We normally only give the hci_request pointer since that always provides the hci_dev as well.

I have the feeling that we might better split this into two functions. One just taking hci_dev and the other just taking hci_request. Or we have to make all callers create a hci_request context in the first place.

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

[Marcel Holtmann]

Hi Johan,

> This patch updates the Add Device mgmt command handler to use a
> hci_request to wait for HCI command completion before notifying user
> space of the mgmt command completion. To do this we need to add an extra
> hci_request parameter to the hci_conn_params_set function. This way we
> ensure that once the mgmt command returns all HCI commands triggered by
> it have also completed.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci_core.h |  2 +-
> net/bluetooth/hci_core.c         |  8 +++---
> net/bluetooth/mgmt.c             | 55 ++++++++++++++++++++++++++++++++--------
> 3 files changed, 50 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d89a359c83ea..3cd121856b1e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -931,7 +931,7 @@ struct hci_conn_params *hci_conn_params_lookup(struct hci_dev *hdev,
> struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
> 					    bdaddr_t *addr, u8 addr_type);
> int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> -			u8 auto_connect);
> +			u8 auto_connect, struct hci_request *req);

same as the other one. Either hci_dev or hci_request, but not both.

> void hci_conn_params_del(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
> void hci_conn_params_clear_all(struct hci_dev *hdev);
> void hci_conn_params_clear_disabled(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4e54f6e0d248..d4358dc40375 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3733,7 +3733,7 @@ struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
> 
> /* This function requires the caller holds hdev->lock */
> int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> -			u8 auto_connect)
> +			u8 auto_connect, struct hci_request *req)
> {
> 	struct hci_conn_params *params;
> 
> @@ -3749,17 +3749,17 @@ int hci_conn_params_set(struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type,
> 	switch (auto_connect) {
> 	case HCI_AUTO_CONN_DISABLED:
> 	case HCI_AUTO_CONN_LINK_LOSS:
> -		hci_update_background_scan(hdev, NULL);
> +		hci_update_background_scan(hdev, req);
> 		break;
> 	case HCI_AUTO_CONN_REPORT:
> 		list_add(&params->action, &hdev->pend_le_reports);
> -		hci_update_background_scan(hdev, NULL);
> +		hci_update_background_scan(hdev, req);
> 		break;
> 	case HCI_AUTO_CONN_DIRECT:
> 	case HCI_AUTO_CONN_ALWAYS:
> 		if (!is_connected(hdev, addr, addr_type)) {
> 			list_add(&params->action, &hdev->pend_le_conns);
> -			hci_update_background_scan(hdev, NULL);
> +			hci_update_background_scan(hdev, req);
> 		}
> 		break;
> 	}
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index ab3dae3076d7..9c44f4c3c2c0 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5436,10 +5436,31 @@ static void device_added(struct sock *sk, struct hci_dev *hdev,
> 	mgmt_event(MGMT_EV_DEVICE_ADDED, hdev, &ev, sizeof(ev), sk);
> }
> 
> +static void add_device_complete(struct hci_dev *hdev, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	hci_dev_lock(hdev);
> +
> +	cmd = mgmt_pending_find(MGMT_OP_ADD_DEVICE, hdev);
> +	if (!cmd)
> +		goto unlock;
> +
> +	cmd->cmd_complete(cmd, mgmt_status(status));
> +	mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +}
> +
> static int add_device(struct sock *sk, struct hci_dev *hdev,
> 		      void *data, u16 len)
> {
> 	struct mgmt_cp_add_device *cp = data;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> 	u8 auto_conn, addr_type;
> 	int err;
> 
> @@ -5456,14 +5477,23 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 				    MGMT_STATUS_INVALID_PARAMS,
> 				    &cp->addr, sizeof(cp->addr));
> 
> +	hci_req_init(&req, hdev);
> +
> 	hci_dev_lock(hdev);
> 
> +	cmd = mgmt_pending_add(sk, MGMT_OP_ADD_DEVICE, hdev, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	cmd->cmd_complete = addr_cmd_complete;
> +
> 	if (cp->addr.type == BDADDR_BREDR) {
> 		/* Only incoming connections action is supported for now */
> 		if (cp->action != 0x01) {
> -			err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
> -					   MGMT_STATUS_INVALID_PARAMS,
> -					   &cp->addr, sizeof(cp->addr));
> +			cmd->cmd_complete(cmd, MGMT_STATUS_INVALID_PARAMS);
> +			mgmt_pending_remove(cmd);
> 			goto unlock;
> 		}
> 
> @@ -5472,7 +5502,7 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 		if (err)
> 			goto unlock;
> 
> -		hci_update_page_scan(hdev, NULL);
> +		hci_update_page_scan(hdev, &req);
> 
> 		goto added;
> 	}
> @@ -5493,18 +5523,23 @@ static int add_device(struct sock *sk, struct hci_dev *hdev,
> 	 * they will be created and configured with defaults.
> 	 */
> 	if (hci_conn_params_set(hdev, &cp->addr.bdaddr, addr_type,
> -				auto_conn) < 0) {
> -		err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
> -				   MGMT_STATUS_FAILED,
> -				   &cp->addr, sizeof(cp->addr));
> +				auto_conn, &req) < 0) {
> +		cmd->cmd_complete(cmd, MGMT_STATUS_FAILED);
> +		mgmt_pending_remove(cmd);
> 		goto unlock;
> 	}
> 
> added:
> 	device_added(sk, hdev, &cp->addr.bdaddr, cp->addr.type, cp->action);
> 
> -	err = cmd_complete(sk, hdev->id, MGMT_OP_ADD_DEVICE,
> -			   MGMT_STATUS_SUCCESS, &cp->addr, sizeof(cp->addr));
> +	err = hci_req_run(&req, add_device_complete);
> +	if (err < 0) {
> +		if (err == -ENODATA) {
> +			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
> +			err = 0;
> +		}
> +		mgmt_pending_remove(cmd);
> +	}

But this command should also work when the controller is powered down. Who is doing these checks now?

Regards

Marcel

Re: [PATCH 3/4] Bluetooth: Fix Remove Device to wait for HCI before sending cmd_complete

[Marcel Holtmann]

Hi Johan,

> This patch updates the Remove Device mgmt command handler to use a
> hci_request to wait for HCI command completion before notifying user
> space of the mgmt command completion. This way we ensure that once the
> mgmt command returns all HCI commands triggered by it have also
> completed.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 49 +++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 7efdeb3cf224..ab3dae3076d7 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -5522,16 +5522,47 @@ static void device_removed(struct sock *sk, struct hci_dev *hdev,
> 	mgmt_event(MGMT_EV_DEVICE_REMOVED, hdev, &ev, sizeof(ev), sk);
> }
> 
> +static void remove_device_complete(struct hci_dev *hdev, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +
> +	BT_DBG("status 0x%02x", status);
> +
> +	hci_dev_lock(hdev);
> +
> +	cmd = mgmt_pending_find(MGMT_OP_REMOVE_DEVICE, hdev);
> +	if (!cmd)
> +		goto unlock;
> +
> +	cmd->cmd_complete(cmd, mgmt_status(status));
> +	mgmt_pending_remove(cmd);
> +
> +unlock:
> +	hci_dev_unlock(hdev);
> +}
> +
> static int remove_device(struct sock *sk, struct hci_dev *hdev,
> 			 void *data, u16 len)
> {
> 	struct mgmt_cp_remove_device *cp = data;
> +	struct pending_cmd *cmd;
> +	struct hci_request req;
> 	int err;
> 
> 	BT_DBG("%s", hdev->name);
> 
> +	hci_req_init(&req, hdev);
> +
> 	hci_dev_lock(hdev);
> 
> +	cmd = mgmt_pending_add(sk, MGMT_OP_REMOVE_DEVICE, hdev, data, len);
> +	if (!cmd) {
> +		err = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	cmd->cmd_complete = addr_cmd_complete;
> +
> 	if (bacmp(&cp->addr.bdaddr, BDADDR_ANY)) {
> 		struct hci_conn_params *params;
> 		u8 addr_type;
> @@ -5555,7 +5586,7 @@ static int remove_device(struct sock *sk, struct hci_dev *hdev,
> 				goto unlock;
> 			}
> 
> -			hci_update_page_scan(hdev, NULL);
> +			hci_update_page_scan(hdev, &req);

so it seems we have done this at least once before. This does not mean that I start to like it now. Especially since now we are adding hci_request pointers to almost all of them now.

I have the feeling we need to fix hci_update_page_scan first to only take the hci_request pointer. We really could just do these two:

	hci_update_page_scan(struct hci_dev *);
	__hci_update_page_scan(struct hci_request *);

And then of course similar things with the other ones.

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

[Johan Hedberg]

Hi Marcel,

On Fri, Dec 19, 2014, Marcel Holtmann wrote:
> > +	err = hci_req_run(&req, add_device_complete);
> > +	if (err < 0) {
> > +		if (err == -ENODATA) {
> > +			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
> > +			err = 0;
> > +		}
> > +		mgmt_pending_remove(cmd);
> > +	}
> 
> But this command should also work when the controller is powered down.
> Who is doing these checks now?

That's covered by the ENODATA check. hci_update_background_scan() bails
out without doing anything if the hdev is powered off, leaving the
hci_request empty.

Johan

Re: [PATCH 1/4] Bluetooth: Move struct hci_request definition higher up in hci_core.h

[Johan Hedberg]

Hi Marcel,

On Fri, Dec 19, 2014, Marcel Holtmann wrote:
> > We'll soon start using struct hci_request as a parameter to several more
> > functions in hci_core.h. To avoid compiler warnings of an undefined
> > struct the definition needs to be moved higher up in the file. This
> > patch moves it to the end of the block where several other structs are
> > also defined.
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 3c7827005c25..75aaae07037f 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -479,6 +479,16 @@ struct hci_conn_params {
> > 	struct hci_conn *conn;
> > };
> > 
> > +struct hci_request {
> > +	struct hci_dev		*hdev;
> > +	struct sk_buff_head	cmd_q;
> > +
> > +	/* If something goes wrong when building the HCI request, the error
> > +	 * value is stored in this field.
> > +	 */
> > +	int			err;
> > +};
> > +
> > extern struct list_head hci_dev_list;
> > extern struct list_head hci_cb_list;
> > extern rwlock_t hci_dev_list_lock;
> > @@ -1284,16 +1294,6 @@ static inline int hci_check_conn_params(u16 min, u16 max, u16 latency,
> > int hci_register_cb(struct hci_cb *hcb);
> > int hci_unregister_cb(struct hci_cb *hcb);
> > 
> > -struct hci_request {
> > -	struct hci_dev		*hdev;
> > -	struct sk_buff_head	cmd_q;
> > -
> > -	/* If something goes wrong when building the HCI request, the error
> > -	 * value is stored in this field.
> > -	 */
> > -	int			err;
> > -};
> > -
> > void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
> > int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
> > void hci_req_add(struct hci_request *req, u16 opcode, u32 plen,
> 
> any reason to not move these along with the data structure? They
> actually do belong together.

Nothing else except that hci_core.h didn't seem to have a very well
established order to begin with and there seemed to be a general section
for structs where I moved this to. I can move the functions along with
it.

Johan

Re: [PATCH 2/4] Bluetooth: Add hci_request parameter to hci_update_background_scan

[Johan Hedberg]

Hi Marcel,

On Fri, Dec 19, 2014, Marcel Holtmann wrote:
> > Many places using hci_update_background_scan() try to synchronize
> > whatever they're doing with the help of hci_request callbacks. However,
> > since the hci_update_background_scan() function hasn't so far accepted a
> > hci_request pointer any commands triggered by it have been left out by
> > the synchronization. This patch an extra parameter to the function to
> > fix the issue. This there are also many places where there is no
> > existing hci_request context, the parameter is made optional, i.e. by
> > passing NULL hci_update_background_scan() uses its own request and a
> > dummy callback.
> > 
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > include/net/bluetooth/hci_core.h |  2 +-
> > net/bluetooth/hci_conn.c         |  2 +-
> > net/bluetooth/hci_core.c         | 33 ++++++++++++++++++++-------------
> > net/bluetooth/hci_event.c        |  4 ++--
> > net/bluetooth/mgmt.c             | 19 +++++++++----------
> > 5 files changed, 33 insertions(+), 27 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 75aaae07037f..d89a359c83ea 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -940,7 +940,7 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
> > 						  bdaddr_t *addr,
> > 						  u8 addr_type);
> > 
> > -void hci_update_background_scan(struct hci_dev *hdev);
> > +void hci_update_background_scan(struct hci_dev *hdev, struct hci_request *req);
> 
> this goes a bit against what we done otherwise in the code base. We
> normally only give the hci_request pointer since that always provides
> the hci_dev as well.
> 
> I have the feeling that we might better split this into two functions.
> One just taking hci_dev and the other just taking hci_request. Or we
> have to make all callers create a hci_request context in the first
> place.

My change is consistent with hci_update_page_scan() so I suppose that
needs fixing too.

I still feel like this "optional parameter" approach isn't too
convoluted, but if you insist I think I'd rather split this into two
functions. Requiring all places to initialize a hci_request and do
hci_req_run would be quite a lot of duplicated boiler-plate code for
little benefit.

Johan

Re: [PATCH 2/4] Bluetooth: Add hci_request parameter to hci_update_background_scan

[Marcel Holtmann]

Hi Johan,

>>> Many places using hci_update_background_scan() try to synchronize
>>> whatever they're doing with the help of hci_request callbacks. However,
>>> since the hci_update_background_scan() function hasn't so far accepted a
>>> hci_request pointer any commands triggered by it have been left out by
>>> the synchronization. This patch an extra parameter to the function to
>>> fix the issue. This there are also many places where there is no
>>> existing hci_request context, the parameter is made optional, i.e. by
>>> passing NULL hci_update_background_scan() uses its own request and a
>>> dummy callback.
>>> 
>>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>> ---
>>> include/net/bluetooth/hci_core.h |  2 +-
>>> net/bluetooth/hci_conn.c         |  2 +-
>>> net/bluetooth/hci_core.c         | 33 ++++++++++++++++++++-------------
>>> net/bluetooth/hci_event.c        |  4 ++--
>>> net/bluetooth/mgmt.c             | 19 +++++++++----------
>>> 5 files changed, 33 insertions(+), 27 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 75aaae07037f..d89a359c83ea 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -940,7 +940,7 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>>> 						  bdaddr_t *addr,
>>> 						  u8 addr_type);
>>> 
>>> -void hci_update_background_scan(struct hci_dev *hdev);
>>> +void hci_update_background_scan(struct hci_dev *hdev, struct hci_request *req);
>> 
>> this goes a bit against what we done otherwise in the code base. We
>> normally only give the hci_request pointer since that always provides
>> the hci_dev as well.
>> 
>> I have the feeling that we might better split this into two functions.
>> One just taking hci_dev and the other just taking hci_request. Or we
>> have to make all callers create a hci_request context in the first
>> place.
> 
> My change is consistent with hci_update_page_scan() so I suppose that
> needs fixing too.

you are correct on that. However you are also tipping the balance on that one with the other patch. Where it used to be 1 of them took the parameter, now all but 2 take not the parameter. So I get the feeling it warrants a course of direction now.

> I still feel like this "optional parameter" approach isn't too
> convoluted, but if you insist I think I'd rather split this into two
> functions. Requiring all places to initialize a hci_request and do
> hci_req_run would be quite a lot of duplicated boiler-plate code for
> little benefit.

I think that using two functions as outlined in the other patch might be a good idea.

The advantage then is really that if ever one becomes no longer needed, we can easily see that and remove it then. Because I have the slight feeling that slowly we will have more and more proper hci_request context anyway.

Regards

Marcel

Re: [PATCH 1/4] Bluetooth: Move struct hci_request definition higher up in hci_core.h

[Marcel Holtmann]

Hi Johan,

>>> We'll soon start using struct hci_request as a parameter to several more
>>> functions in hci_core.h. To avoid compiler warnings of an undefined
>>> struct the definition needs to be moved higher up in the file. This
>>> patch moves it to the end of the block where several other structs are
>>> also defined.
>>> 
>>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>>> ---
>>> include/net/bluetooth/hci_core.h | 20 ++++++++++----------
>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 3c7827005c25..75aaae07037f 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -479,6 +479,16 @@ struct hci_conn_params {
>>> 	struct hci_conn *conn;
>>> };
>>> 
>>> +struct hci_request {
>>> +	struct hci_dev		*hdev;
>>> +	struct sk_buff_head	cmd_q;
>>> +
>>> +	/* If something goes wrong when building the HCI request, the error
>>> +	 * value is stored in this field.
>>> +	 */
>>> +	int			err;
>>> +};
>>> +
>>> extern struct list_head hci_dev_list;
>>> extern struct list_head hci_cb_list;
>>> extern rwlock_t hci_dev_list_lock;
>>> @@ -1284,16 +1294,6 @@ static inline int hci_check_conn_params(u16 min, u16 max, u16 latency,
>>> int hci_register_cb(struct hci_cb *hcb);
>>> int hci_unregister_cb(struct hci_cb *hcb);
>>> 
>>> -struct hci_request {
>>> -	struct hci_dev		*hdev;
>>> -	struct sk_buff_head	cmd_q;
>>> -
>>> -	/* If something goes wrong when building the HCI request, the error
>>> -	 * value is stored in this field.
>>> -	 */
>>> -	int			err;
>>> -};
>>> -
>>> void hci_req_init(struct hci_request *req, struct hci_dev *hdev);
>>> int hci_req_run(struct hci_request *req, hci_req_complete_t complete);
>>> void hci_req_add(struct hci_request *req, u16 opcode, u32 plen,
>> 
>> any reason to not move these along with the data structure? They
>> actually do belong together.
> 
> Nothing else except that hci_core.h didn't seem to have a very well
> established order to begin with and there seemed to be a general section
> for structs where I moved this to. I can move the functions along with
> it.

I think that at some point I tried to split hci_core.h into smaller pieces and because of its dependencies ended up giving up on it.

What might be worth while trying is to create a net/bluetooth/hci_request.[ch] and move the whole hci_request stuff locally into net/bluetooth/. This is at least one of the things that is limited to bluetooth.ko core module.

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

[Marcel Holtmann]

Hi Johan,

>>> +	err = hci_req_run(&req, add_device_complete);
>>> +	if (err < 0) {
>>> +		if (err == -ENODATA) {
>>> +			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
>>> +			err = 0;
>>> +		}
>>> +		mgmt_pending_remove(cmd);
>>> +	}
>> 
>> But this command should also work when the controller is powered down.
>> Who is doing these checks now?
> 
> That's covered by the ENODATA check. hci_update_background_scan() bails
> out without doing anything if the hdev is powered off, leaving the
> hci_request empty.

we have never done it like that in mgmt commands, or have we?

I think a shortcut check with is_powered and leaving the function might be a bit more clearer to understand. Or at least a command that explain this. Since I clearly missed it.

Regards

Marcel

Re: [PATCH 4/4] Bluetooth: Fix Add Device to wait for HCI before sending cmd_complete

[Johan Hedberg]

Hi Marcel,

On Fri, Dec 19, 2014, Marcel Holtmann wrote:
> >>> +	err = hci_req_run(&req, add_device_complete);
> >>> +	if (err < 0) {
> >>> +		if (err == -ENODATA) {
> >>> +			cmd->cmd_complete(cmd, MGMT_STATUS_SUCCESS);
> >>> +			err = 0;
> >>> +		}
> >>> +		mgmt_pending_remove(cmd);
> >>> +	}
> >> 
> >> But this command should also work when the controller is powered down.
> >> Who is doing these checks now?
> > 
> > That's covered by the ENODATA check. hci_update_background_scan() bails
> > out without doing anything if the hdev is powered off, leaving the
> > hci_request empty.
> 
> we have never done it like that in mgmt commands, or have we?
> 
> I think a shortcut check with is_powered and leaving the function
> might be a bit more clearer to understand. Or at least a command that
> explain this. Since I clearly missed it.

It'd need to be a code comment since we do still have to call
hci_conn_params_set() which may or may not end up adding something to
the hci_request. Or then we need to completely redesign these functions
to clearly split the various list updates from code that may send HCI
commands (but the amount of needed patches is already exploding so I'd
rather not go there yet).

Johan