Hi Luiz,
If connection is still queued/pending in the cmd_sync queue it means no
command has been generated and it should be safe to just dequeue the
callback when it is being aborted.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
---
include/net/bluetooth/hci_core.h | 19 ++++++++
include/net/bluetooth/hci_sync.h | 10 +++--
net/bluetooth/hci_conn.c | 70 ++++++------------------------
net/bluetooth/hci_sync.c | 74 ++++++++++++++++++++++++++++----
4 files changed, 102 insertions(+), 71 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2bdea85b7c44..317d495cfcf5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1083,6 +1083,24 @@ static inline unsigned int hci_conn_count(struct hci_dev *hdev)
return c->acl_num + c->amp_num + c->sco_num + c->le_num + c->iso_num;
}
+static inline bool hci_conn_valid(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ struct hci_conn_hash *h = &hdev->conn_hash;
+ struct hci_conn *c;
+
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(c, &h->list, list) {
+ if (c == conn) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
static inline __u8 hci_conn_lookup_type(struct hci_dev *hdev, __u16 handle)
{
struct hci_conn_hash *h = &hdev->conn_hash;
@@ -1493,6 +1511,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, bool dst_resolved, u8 sec_level,
u16 conn_timeout, u8 role);
+void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
u8 sec_level, u8 auth_type,
enum conn_reasons conn_reason, u16 timeout);
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 4ff4aa68ee19..6a9d063e9f47 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -48,11 +48,11 @@ int hci_cmd_sync_submit(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
+int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data, hci_cmd_sync_work_destroy_t destroy);
struct hci_cmd_sync_work_entry *
hci_cmd_sync_lookup_entry(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
void *data, hci_cmd_sync_work_destroy_t destroy);
-int hci_cmd_sync_queue_once(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
- void *data, hci_cmd_sync_work_destroy_t destroy);
void hci_cmd_sync_cancel_entry(struct hci_dev *hdev,
struct hci_cmd_sync_work_entry *entry);
bool hci_cmd_sync_dequeue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
@@ -139,8 +139,6 @@ struct hci_conn;
int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
-int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
-
int hci_le_create_cis_sync(struct hci_dev *hdev);
int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
@@ -152,3 +150,7 @@ int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn);
+
+int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn);
+
+int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 587eb27f374c..21e0b4064d05 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -68,7 +68,7 @@ static const struct sco_param esco_param_msbc[] = {
};
/* This function requires the caller holds hdev->lock */
-static void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
+void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status)
{
struct hci_conn_params *params;
struct hci_dev *hdev = conn->hdev;
@@ -1124,6 +1124,9 @@ void hci_conn_del(struct hci_conn *conn)
* rest of hci_conn_del.
*/
hci_conn_cleanup(conn);
+
+ /* Dequeue callbacks using connection pointer as data */
+ hci_cmd_sync_dequeue(hdev, NULL, conn, NULL);
}
struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src, uint8_t src_type)
@@ -1258,53 +1261,6 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
return 0;
}
-static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
-{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return;
-
- bt_dev_dbg(hdev, "err %d", err);
-
- hci_dev_lock(hdev);
-
- if (!err) {
- hci_connect_le_scan_cleanup(conn, 0x00);
- goto done;
- }
-
- /* Check if connection is still pending */
- if (conn != hci_lookup_le_connect(hdev))
- goto done;
-
- /* Flush to make sure we send create conn cancel command if needed */
- flush_delayed_work(&conn->le_conn_timeout);
- hci_conn_failed(conn, bt_status(err));
-
-done:
- hci_dev_unlock(hdev);
-}
-
-static int hci_connect_le_sync(struct hci_dev *hdev, void *data)
-{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
-
- bt_dev_dbg(hdev, "conn %p", conn);
-
- clear_bit(HCI_CONN_SCANNING, &conn->flags);
- conn->state = BT_CONNECT;
-
- return hci_le_create_conn_sync(hdev, conn);
-}
-
struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, bool dst_resolved, u8 sec_level,
u16 conn_timeout, u8 role)
@@ -1371,9 +1327,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
conn->sec_level = BT_SECURITY_LOW;
conn->conn_timeout = conn_timeout;
Might want to add a
+ if (conn->state != BT_OPEN && conn->state != BT_CLOSED)
+ return conn;
before setting the conn->dst_type while at it, similar to how it's
in hci_connect_acl().
- err = hci_cmd_sync_queue(hdev, hci_connect_le_sync,
- UINT_PTR(conn->handle),
- create_le_conn_complete);
+ err = hci_connect_le_sync(hdev, conn);
if (err) {
hci_conn_del(conn);
return ERR_PTR(err);
@@ -2909,12 +2863,10 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
static int abort_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
+ struct hci_conn *conn = data;
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
+ if (!hci_conn_valid(hdev, conn))
+ return -ECANCELED;
return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
}
@@ -2949,8 +2901,10 @@ int hci_abort_conn(struct hci_conn *conn, u8 reason)
hci_cmd_sync_cancel(hdev, -ECANCELED);
break;
}
+ /* Cancel connect attempt if still queued/pending */
+ } else if (!hci_cancel_connect_sync(hdev, conn)) {
+ return 0;
}
- return hci_cmd_sync_queue(hdev, abort_conn_sync, UINT_PTR(conn->handle),
- NULL);
+ return hci_cmd_sync_queue_once(hdev, abort_conn_sync, conn, NULL);
}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 5b314bf844f8..b7d8e99e2a30 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6285,12 +6285,21 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
conn->conn_timeout, NULL);
}
-int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
+static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data)
{
struct hci_cp_le_create_conn cp;
struct hci_conn_params *params;
u8 own_addr_type;
int err;
+ struct hci_conn *conn = data;
+
+ if (!hci_conn_valid(hdev, conn))
+ return -ECANCELED;
+
+ bt_dev_dbg(hdev, "conn %p", conn);
+
+ clear_bit(HCI_CONN_SCANNING, &conn->flags);
+ conn->state = BT_CONNECT;
/* If requested to connect as peripheral use directed advertising */
if (conn->role == HCI_ROLE_SLAVE) {
@@ -6611,16 +6620,11 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
{
- struct hci_conn *conn;
- u16 handle = PTR_UINT(data);
+ struct hci_conn *conn = data;
struct inquiry_entry *ie;
struct hci_cp_create_conn cp;
int err;
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
-
/* Many controllers disallow HCI Create Connection while it is doing
* HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
* Connection. This may cause the MGMT discovering state to become false
@@ -6679,6 +6683,58 @@ static int hci_acl_create_conn_sync(struct hci_dev *hdev, void *data)
int hci_connect_acl_sync(struct hci_dev *hdev, struct hci_conn *conn)
{
- return hci_cmd_sync_queue(hdev, hci_acl_create_conn_sync,
- UINT_PTR(conn->handle), NULL);
+ return hci_cmd_sync_queue_once(hdev, hci_acl_create_conn_sync, conn,
+ NULL);
+}
+
+static void create_le_conn_complete(struct hci_dev *hdev, void *data, int err)
+{
+ struct hci_conn *conn = data;
+
+ bt_dev_dbg(hdev, "err %d", err);
+
+ if (err == -ECANCELED)
+ return;
+
+ hci_dev_lock(hdev);
+
+ if (!err) {
+ hci_connect_le_scan_cleanup(conn, 0x00);
+ goto done;
+ }
+
+ /* Check if connection is still pending */
+ if (conn != hci_lookup_le_connect(hdev))
+ goto done;
+
+ /* Flush to make sure we send create conn cancel command if needed */
+ flush_delayed_work(&conn->le_conn_timeout);
+ hci_conn_failed(conn, bt_status(err));
+
+done:
+ hci_dev_unlock(hdev);
+}
+
+int hci_connect_le_sync(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ return hci_cmd_sync_queue_once(hdev, hci_le_create_conn_sync, conn,
+ create_le_conn_complete);
+}
+
+int hci_cancel_connect_sync(struct hci_dev *hdev, struct hci_conn *conn)
+{
+ if (conn->state != BT_OPEN)
+ return -EINVAL;
+
+ switch (conn->type) {
+ case ACL_LINK:
+ return !hci_cmd_sync_dequeue_once(hdev,
+ hci_acl_create_conn_sync,
+ conn, NULL);
+ case LE_LINK:
+ return !hci_cmd_sync_dequeue_once(hdev, hci_le_create_conn_sync,
+ conn, create_le_conn_complete);
+ }
+
+ return -ENOENT;
}
--
2.43.0
Thanks for sending those patches, this is pretty much exactly what I had in mind!
I came up with a slightly different cancel function for the queued work, one that
also cancels the ongoing work. I'm not sure if it makes too much sense, because it
means adding careful -ECANCELED handling to those sync_work callbacks.
The nice thing about it is that it should also allow getting rid of the
hci_cmd_sync_cancel(hdev, -ECANCELED) in hci_abort_conn().
Adding the patch below, feel free to reuse whatever you like!
Cheers,
Jonas
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 067d445701..a2b14f6be1 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -154,6 +154,13 @@ struct sk_buff *__hci_cmd_sync_sk(struct hci_dev *hdev, u16 opcode, u32 plen,
bt_dev_dbg(hdev, "Opcode 0x%4x", opcode);
+ if (hdev->cur_hci_sync_work_cancelled) {
+ hdev->cur_hci_sync_work_cancelled = false;
+
+ return ERR_PTR(-ECANCELED);
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
hci_req_init(&req, hdev);
hci_cmd_sync_add(&req, opcode, plen, param, event, sk);
@@ -303,9 +310,29 @@ static void hci_cmd_sync_work(struct work_struct *work)
int err;
hci_req_sync_lock(hdev);
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ hdev->cur_hci_sync_work_func = entry->func;
+ hdev->cur_hci_sync_work_data = entry->data;
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
err = entry->func(hdev, entry->data);
if (entry->destroy)
entry->destroy(hdev, entry->data, err);
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ hdev->cur_hci_sync_work_func = NULL;
+ hdev->cur_hci_sync_work_data = NULL;
+
+ if (hdev->cur_hci_sync_work_cancelled) {
+ /* If cur_hci_sync_work_cancelled is still set at this point,
+ * no more request was sent and the work func has never been
+ * notified of our cancellation.
+ */
+ hdev->cur_hci_sync_work_cancelled = false;
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
hci_req_sync_unlock(hdev);
}
@@ -735,6 +762,87 @@ int hci_cmd_sync_queue(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
}
EXPORT_SYMBOL(hci_cmd_sync_queue);
+bool hci_cmd_sync_has_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data)
+{
+ struct hci_cmd_sync_work_entry *entry;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ if (hdev->cur_hci_sync_work_func == func &&
+ hdev->cur_hci_sync_work_data == data) {
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+ return true;
+ }
+
+ list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
+ if (entry->func == func && entry->data == data) {
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+ return true;
+ }
+ }
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return false;
+}
+
+void hci_cmd_sync_cancel_cmd(struct hci_dev *hdev, hci_cmd_sync_work_func_t func,
+ void *data)
+{
+ struct hci_cmd_sync_work_entry *entry;
+ bool work_already_ongoing;
+
+ mutex_lock(&hdev->cmd_sync_work_lock);
+ if (hdev->cur_hci_sync_work_func == func &&
+ hdev->cur_hci_sync_work_data == data) {
+ /* If the command is already ongoing, we check if we're currently
+ * sending a async HCI request. If we are, we can cancel that
+ * and hope that the hci_cmd_sync_work_func is handling -ECANCELED.
+ */
+
+ if (hdev->req_status == HCI_REQ_PEND) {
+ /* If we're already executing a request, cancel that request.
+ * This will signal cancellation to the work func which sent
+ * the request in the first place.
+ */
+ __hci_cmd_sync_cancel(hdev, -ECANCELED);
+ } else {
+ /* Otherwise, just set a flag which will cancel the next
+ * request. Just as above, this will then signal cancellation
+ * to the work func.
+ */
+ hdev->cur_hci_sync_work_cancelled = true;
+ }
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ return;
+ } else {
+ /* Or is it still queued? Then we remove it from the queue and
+ * execute the destroy callback.
+ */
+ list_for_each_entry(entry, &hdev->cmd_sync_work_list, list) {
+ if (entry->func == func && entry->data == data) {
+ if (entry->destroy)
+ entry->destroy(hdev, entry->data, -ECANCELED);
+ list_del(&entry->list);
+ kfree(entry);
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+
+ if (list_empty(&hdev->cmd_sync_work_list)) {
+ cancel_work_sync(&hdev->cmd_sync_work);
+ cancel_work_sync(&hdev->reenable_adv_work);
+ }
+
+ return;
+ }
+ }
+
+ }
+
+ mutex_unlock(&hdev->cmd_sync_work_lock);
+}
+
int hci_update_eir_sync(struct hci_dev *hdev)
{
struct hci_cp_write_eir cp;
@@ -6262,15 +6370,20 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
}
/* Pause advertising while doing directed advertising. */
- hci_pause_advertising_sync(hdev);
+ err = hci_pause_advertising_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
err = hci_le_directed_advertising_sync(hdev, conn);
goto done;
}
/* Disable advertising if simultaneous roles is not in use. */
- if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES))
- hci_pause_advertising_sync(hdev);
+ if (!hci_dev_test_flag(hdev, HCI_LE_SIMULTANEOUS_ROLES)) {
+ err = hci_pause_advertising_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
+ }
params = hci_conn_params_lookup(hdev, &conn->dst, conn->dst_type);
if (params) {
@@ -6292,7 +6405,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
* state.
*/
if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
- hci_scan_disable_sync(hdev);
+ err = hci_scan_disable_sync(hdev);
+ if (err == -ECANCELED)
+ goto done;
+
hci_dev_set_flag(hdev, HCI_LE_SCAN_INTERRUPTED);
}
@@ -6336,11 +6452,10 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
HCI_EV_LE_ENHANCED_CONN_COMPLETE :
HCI_EV_LE_CONN_COMPLETE,
conn->conn_timeout, NULL);
+ if (err == -ECANCELED || err == -ETIMEDOUT)
+ hci_le_connect_cancel_sync(hdev, conn, conn->abort_reason || 0x00);
done:
- if (err == -ETIMEDOUT)
- hci_le_connect_cancel_sync(hdev, conn, 0x00);
-
/* Re-enable advertising after the connection attempt is finished. */
hci_resume_advertising_sync(hdev);
return err;
@@ -6586,7 +6701,9 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
if (test_bit(HCI_INQUIRY, &hdev->flags)) {
err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
NULL, HCI_CMD_TIMEOUT);
- if (err)
+ if (err == -ECANCELED)
+ return -ECANCELED;
+ else if (err)
bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
}
@@ -6625,15 +6742,10 @@ static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
HCI_EV_CONN_COMPLETE,
HCI_ACL_CONN_TIMEOUT, NULL);
- if (err == -ETIMEDOUT)
- hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM);
+ if (err == -ECANCELED || err == -ETIMEDOUT) {
+ hci_connect_cancel_sync(hdev, conn, conn->abort_reason || HCI_ERROR_LOCAL_HOST_TERM);
+ return err;
+ }
return err;
}
