From: Luiz Augusto von Dentz <[email protected]>
This consolidates code for aborting connections using
hci_cmd_sync_queue so it is synchronized with other threads, but
because of the fact that some commands may block the cmd_sync_queue
while waiting specific events this attempt to cancel those requests by
using hci_cmd_sync_cancel.
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 160 +++++++------------------------
net/bluetooth/hci_sync.c | 20 ++--
net/bluetooth/mgmt.c | 15 +--
4 files changed, 48 insertions(+), 149 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05a9b3ab3f56..094ca3aca15e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -741,6 +741,7 @@ struct hci_conn {
unsigned long flags;
enum conn_reasons conn_reason;
+ __u8 abort_reason;
__u32 clock;
__u16 clock_accuracy;
@@ -760,7 +761,6 @@ struct hci_conn {
struct delayed_work auto_accept_work;
struct delayed_work idle_work;
struct delayed_work le_conn_timeout;
- struct work_struct le_scan_cleanup;
struct device dev;
struct dentry *debugfs;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 47e7aa4d63a9..88f18f375684 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -178,57 +178,6 @@ static void hci_conn_cleanup(struct hci_conn *conn)
hci_conn_put(conn);
}
-static void le_scan_cleanup(struct work_struct *work)
-{
- struct hci_conn *conn = container_of(work, struct hci_conn,
- le_scan_cleanup);
- struct hci_dev *hdev = conn->hdev;
- struct hci_conn *c = NULL;
-
- BT_DBG("%s hcon %p", hdev->name, conn);
-
- hci_dev_lock(hdev);
-
- /* Check that the hci_conn is still around */
- rcu_read_lock();
- list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
- if (c == conn)
- break;
- }
- rcu_read_unlock();
-
- if (c == conn) {
- hci_connect_le_scan_cleanup(conn, 0x00);
- hci_conn_cleanup(conn);
- }
-
- hci_dev_unlock(hdev);
- hci_dev_put(hdev);
- hci_conn_put(conn);
-}
-
-static void hci_connect_le_scan_remove(struct hci_conn *conn)
-{
- BT_DBG("%s hcon %p", conn->hdev->name, conn);
-
- /* We can't call hci_conn_del/hci_conn_cleanup here since that
- * could deadlock with another hci_conn_del() call that's holding
- * hci_dev_lock and doing cancel_delayed_work_sync(&conn->disc_work).
- * Instead, grab temporary extra references to the hci_dev and
- * hci_conn and perform the necessary cleanup in a separate work
- * callback.
- */
-
- hci_dev_hold(conn->hdev);
- hci_conn_get(conn);
-
- /* Even though we hold a reference to the hdev, many other
- * things might get cleaned up meanwhile, including the hdev's
- * own workqueue, so we can't use that for scheduling.
- */
- schedule_work(&conn->le_scan_cleanup);
-}
-
static void hci_acl_create_connection(struct hci_conn *conn)
{
struct hci_dev *hdev = conn->hdev;
@@ -679,13 +628,6 @@ static void hci_conn_timeout(struct work_struct *work)
if (refcnt > 0)
return;
- /* LE connections in scanning state need special handling */
- if (conn->state == BT_CONNECT && conn->type == LE_LINK &&
- test_bit(HCI_CONN_SCANNING, &conn->flags)) {
- hci_connect_le_scan_remove(conn);
- return;
- }
-
hci_abort_conn(conn, hci_proto_disconn_ind(conn));
}
@@ -1066,7 +1008,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
INIT_DELAYED_WORK(&conn->auto_accept_work, hci_conn_auto_accept);
INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
- INIT_WORK(&conn->le_scan_cleanup, le_scan_cleanup);
atomic_set(&conn->refcnt, 0);
@@ -2888,81 +2829,46 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
return phys;
}
-int hci_abort_conn(struct hci_conn *conn, u8 reason)
+static int abort_conn_sync(struct hci_dev *hdev, void *data)
{
- int r = 0;
+ struct hci_conn *conn;
+ u16 handle = PTR_ERR(data);
- if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
+ conn = hci_conn_hash_lookup_handle(hdev, handle);
+ if (!conn)
return 0;
- switch (conn->state) {
- case BT_CONNECTED:
- case BT_CONFIG:
- if (conn->type == AMP_LINK) {
- struct hci_cp_disconn_phy_link cp;
+ return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
+}
- cp.phy_handle = HCI_PHY_HANDLE(conn->handle);
- cp.reason = reason;
- r = hci_send_cmd(conn->hdev, HCI_OP_DISCONN_PHY_LINK,
- sizeof(cp), &cp);
- } else {
- struct hci_cp_disconnect dc;
+int hci_abort_conn(struct hci_conn *conn, u8 reason)
+{
+ struct hci_dev *hdev = conn->hdev;
- dc.handle = cpu_to_le16(conn->handle);
- dc.reason = reason;
- r = hci_send_cmd(conn->hdev, HCI_OP_DISCONNECT,
- sizeof(dc), &dc);
+ /* If abort_reason has already been set it means the connection is
+ * already being aborted so don't attempt to overwrite it.
+ */
+ if (conn->abort_reason)
+ return 0;
+
+ bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
+
+ conn->abort_reason = reason;
+
+ /* If the connection is pending check the command opcode since that
+ * might be blocking on hci_cmd_sync_work while waiting its respective
+ * event so we need to hci_cmd_sync_cancel to cancel it.
+ */
+ if (conn->state == BT_CONNECT && hdev->req_status == HCI_REQ_PEND) {
+ switch (hci_skb_event(hdev->sent_cmd)) {
+ case HCI_EV_LE_CONN_COMPLETE:
+ case HCI_EV_LE_ENHANCED_CONN_COMPLETE:
+ case HCI_EVT_LE_CIS_ESTABLISHED:
+ hci_cmd_sync_cancel(hdev, -ECANCELED);
+ break;
}
-
- conn->state = BT_DISCONN;
-
- break;
- case BT_CONNECT:
- if (conn->type == LE_LINK) {
- if (test_bit(HCI_CONN_SCANNING, &conn->flags))
- break;
- r = hci_send_cmd(conn->hdev,
- HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
- } else if (conn->type == ACL_LINK) {
- if (conn->hdev->hci_ver < BLUETOOTH_VER_1_2)
- break;
- r = hci_send_cmd(conn->hdev,
- HCI_OP_CREATE_CONN_CANCEL,
- 6, &conn->dst);
- }
- break;
- case BT_CONNECT2:
- if (conn->type == ACL_LINK) {
- struct hci_cp_reject_conn_req rej;
-
- bacpy(&rej.bdaddr, &conn->dst);
- rej.reason = reason;
-
- r = hci_send_cmd(conn->hdev,
- HCI_OP_REJECT_CONN_REQ,
- sizeof(rej), &rej);
- } else if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
- struct hci_cp_reject_sync_conn_req rej;
-
- bacpy(&rej.bdaddr, &conn->dst);
-
- /* SCO rejection has its own limited set of
- * allowed error values (0x0D-0x0F) which isn't
- * compatible with most values passed to this
- * function. To be safe hard-code one of the
- * values that's suitable for SCO.
- */
- rej.reason = HCI_ERROR_REJ_LIMITED_RESOURCES;
-
- r = hci_send_cmd(conn->hdev,
- HCI_OP_REJECT_SYNC_CONN_REQ,
- sizeof(rej), &rej);
- }
- break;
- default:
- conn->state = BT_CLOSED;
- break;
}
- return r;
+ return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
+ NULL);
}
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index afb8e970e62c..ef8297d04a8c 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5271,10 +5271,13 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
}
static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
- struct hci_conn *conn)
+ struct hci_conn *conn, u8 reason)
{
+ /* Return reason if scanning since the connection shall probably be
+ * cleanup directly.
+ */
if (test_bit(HCI_CONN_SCANNING, &conn->flags))
- return 0;
+ return reason;
if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
return 0;
@@ -5283,10 +5286,11 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
0, NULL, HCI_CMD_TIMEOUT);
}
-static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn)
+static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
+ u8 reason)
{
if (conn->type == LE_LINK)
- return hci_le_connect_cancel_sync(hdev, conn);
+ return hci_le_connect_cancel_sync(hdev, conn, reason);
if (hdev->hci_ver < BLUETOOTH_VER_1_2)
return 0;
@@ -5339,9 +5343,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
case BT_CONFIG:
return hci_disconnect_sync(hdev, conn, reason);
case BT_CONNECT:
- err = hci_connect_cancel_sync(hdev, conn);
+ err = hci_connect_cancel_sync(hdev, conn, reason);
/* Cleanup hci_conn object if it cannot be cancelled as it
- * likelly means the controller and host stack are out of sync.
+ * likelly means the controller and host stack are out of sync
+ * or in case of LE it was still scanning so it can be cleanup
+ * safely.
*/
if (err) {
hci_dev_lock(hdev);
@@ -6255,7 +6261,7 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
done:
if (err == -ETIMEDOUT)
- hci_le_connect_cancel_sync(hdev, conn);
+ hci_le_connect_cancel_sync(hdev, conn, 0x00);
/* Re-enable advertising after the connection attempt is finished. */
hci_resume_advertising_sync(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0f5ba618ceb1..3156bc27088e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3586,18 +3586,6 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
return err;
}
-static int abort_conn_sync(struct hci_dev *hdev, void *data)
-{
- struct hci_conn *conn;
- u16 handle = PTR_ERR(data);
-
- conn = hci_conn_hash_lookup_handle(hdev, handle);
- if (!conn)
- return 0;
-
- return hci_abort_conn_sync(hdev, conn, HCI_ERROR_REMOTE_USER_TERM);
-}
-
static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
u16 len)
{
@@ -3648,8 +3636,7 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
le_addr_type(addr->type));
if (conn->conn_reason == CONN_REASON_PAIR_DEVICE)
- hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
- NULL);
+ hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
unlock:
hci_dev_unlock(hdev);
--
2.40.1
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=760750
---Test result---
Test Summary:
CheckPatch PASS 1.18 seconds
GitLint PASS 0.29 seconds
SubjectPrefix PASS 0.10 seconds
BuildKernel PASS 32.11 seconds
CheckAllWarning PASS 34.85 seconds
CheckSparse PASS 39.32 seconds
CheckSmatch PASS 110.88 seconds
BuildKernel32 PASS 31.18 seconds
TestRunnerSetup PASS 441.19 seconds
TestRunner_l2cap-tester PASS 16.34 seconds
TestRunner_iso-tester PASS 22.12 seconds
TestRunner_bnep-tester PASS 5.21 seconds
TestRunner_mgmt-tester PASS 125.89 seconds
TestRunner_rfcomm-tester PASS 8.33 seconds
TestRunner_sco-tester PASS 9.77 seconds
TestRunner_ioctl-tester PASS 8.92 seconds
TestRunner_mesh-tester PASS 6.57 seconds
TestRunner_smp-tester PASS 7.69 seconds
TestRunner_userchan-tester PASS 5.47 seconds
IncrementalBuild PASS 29.16 seconds
---
Regards,
Linux Bluetooth
Hi Luiz,
ti, 2023-06-27 kello 14:53 -0700, Luiz Augusto von Dentz kirjoitti:
> From: Luiz Augusto von Dentz <[email protected]>
>
> This consolidates code for aborting connections using
> hci_cmd_sync_queue so it is synchronized with other threads, but
> because of the fact that some commands may block the cmd_sync_queue
> while waiting specific events this attempt to cancel those requests by
> using hci_cmd_sync_cancel.
>
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_conn.c | 160 +++++++------------------------
> net/bluetooth/hci_sync.c | 20 ++--
> net/bluetooth/mgmt.c | 15 +--
> 4 files changed, 48 insertions(+), 149 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05a9b3ab3f56..094ca3aca15e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -741,6 +741,7 @@ struct hci_conn {
> unsigned long flags;
>
> enum conn_reasons conn_reason;
> + __u8 abort_reason;
>
> __u32 clock;
> __u16 clock_accuracy;
> @@ -760,7 +761,6 @@ struct hci_conn {
> struct delayed_work auto_accept_work;
> struct delayed_work idle_work;
> struct delayed_work le_conn_timeout;
> - struct work_struct le_scan_cleanup;
>
> struct device dev;
> struct dentry *debugfs;
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 47e7aa4d63a9..88f18f375684 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -178,57 +178,6 @@ static void hci_conn_cleanup(struct hci_conn *conn)
> hci_conn_put(conn);
> }
>
> -static void le_scan_cleanup(struct work_struct *work)
> -{
> - struct hci_conn *conn = container_of(work, struct hci_conn,
> - le_scan_cleanup);
> - struct hci_dev *hdev = conn->hdev;
> - struct hci_conn *c = NULL;
> -
> - BT_DBG("%s hcon %p", hdev->name, conn);
> -
> - hci_dev_lock(hdev);
> -
> - /* Check that the hci_conn is still around */
> - rcu_read_lock();
> - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
> - if (c == conn)
> - break;
> - }
> - rcu_read_unlock();
> -
> - if (c == conn) {
> - hci_connect_le_scan_cleanup(conn, 0x00);
> - hci_conn_cleanup(conn);
> - }
> -
> - hci_dev_unlock(hdev);
> - hci_dev_put(hdev);
> - hci_conn_put(conn);
> -}
> -
> -static void hci_connect_le_scan_remove(struct hci_conn *conn)
> -{
> - BT_DBG("%s hcon %p", conn->hdev->name, conn);
> -
> - /* We can't call hci_conn_del/hci_conn_cleanup here since that
> - * could deadlock with another hci_conn_del() call that's holding
> - * hci_dev_lock and doing cancel_delayed_work_sync(&conn->disc_work).
> - * Instead, grab temporary extra references to the hci_dev and
> - * hci_conn and perform the necessary cleanup in a separate work
> - * callback.
> - */
> -
> - hci_dev_hold(conn->hdev);
> - hci_conn_get(conn);
> -
> - /* Even though we hold a reference to the hdev, many other
> - * things might get cleaned up meanwhile, including the hdev's
> - * own workqueue, so we can't use that for scheduling.
> - */
> - schedule_work(&conn->le_scan_cleanup);
> -}
> -
> static void hci_acl_create_connection(struct hci_conn *conn)
> {
> struct hci_dev *hdev = conn->hdev;
> @@ -679,13 +628,6 @@ static void hci_conn_timeout(struct work_struct *work)
> if (refcnt > 0)
> return;
>
> - /* LE connections in scanning state need special handling */
> - if (conn->state == BT_CONNECT && conn->type == LE_LINK &&
> - test_bit(HCI_CONN_SCANNING, &conn->flags)) {
> - hci_connect_le_scan_remove(conn);
> - return;
> - }
> -
> hci_abort_conn(conn, hci_proto_disconn_ind(conn));
> }
>
> @@ -1066,7 +1008,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
> INIT_DELAYED_WORK(&conn->auto_accept_work, hci_conn_auto_accept);
> INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
> INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
> - INIT_WORK(&conn->le_scan_cleanup, le_scan_cleanup);
>
> atomic_set(&conn->refcnt, 0);
>
> @@ -2888,81 +2829,46 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
> return phys;
> }
>
> -int hci_abort_conn(struct hci_conn *conn, u8 reason)
> +static int abort_conn_sync(struct hci_dev *hdev, void *data)
> {
> - int r = 0;
> + struct hci_conn *conn;
> + u16 handle = PTR_ERR(data);
>
> - if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
> + conn = hci_conn_hash_lookup_handle(hdev, handle);
> + if (!conn)
> return 0;
>
> - switch (conn->state) {
> - case BT_CONNECTED:
> - case BT_CONFIG:
> - if (conn->type == AMP_LINK) {
> - struct hci_cp_disconn_phy_link cp;
> + return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> +}
>
> - cp.phy_handle = HCI_PHY_HANDLE(conn->handle);
> - cp.reason = reason;
> - r = hci_send_cmd(conn->hdev, HCI_OP_DISCONN_PHY_LINK,
> - sizeof(cp), &cp);
> - } else {
> - struct hci_cp_disconnect dc;
> +int hci_abort_conn(struct hci_conn *conn, u8 reason)
> +{
> + struct hci_dev *hdev = conn->hdev;
>
> - dc.handle = cpu_to_le16(conn->handle);
> - dc.reason = reason;
> - r = hci_send_cmd(conn->hdev, HCI_OP_DISCONNECT,
> - sizeof(dc), &dc);
> + /* If abort_reason has already been set it means the connection is
> + * already being aborted so don't attempt to overwrite it.
> + */
> + if (conn->abort_reason)
> + return 0;
> +
> + bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
> +
> + conn->abort_reason = reason;
> +
> + /* If the connection is pending check the command opcode since that
> + * might be blocking on hci_cmd_sync_work while waiting its respective
> + * event so we need to hci_cmd_sync_cancel to cancel it.
> + */
> + if (conn->state == BT_CONNECT && hdev->req_status == HCI_REQ_PEND) {
> + switch (hci_skb_event(hdev->sent_cmd)) {
> + case HCI_EV_LE_CONN_COMPLETE:
> + case HCI_EV_LE_ENHANCED_CONN_COMPLETE:
> + case HCI_EVT_LE_CIS_ESTABLISHED:
> + hci_cmd_sync_cancel(hdev, -ECANCELED);
> + break;
Can this also accidentally cancel connection attempt for a different
conn?
Eg. start Create CIS conn1, then queue LE Connect conn2, then conn1
starts waiting for the events, then hci_conn_abort conn2, then here
conn1 gets canceled with error.
> }
> -
> - conn->state = BT_DISCONN;
> -
> - break;
> - case BT_CONNECT:
> - if (conn->type == LE_LINK) {
> - if (test_bit(HCI_CONN_SCANNING, &conn->flags))
> - break;
> - r = hci_send_cmd(conn->hdev,
> - HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
> - } else if (conn->type == ACL_LINK) {
> - if (conn->hdev->hci_ver < BLUETOOTH_VER_1_2)
> - break;
> - r = hci_send_cmd(conn->hdev,
> - HCI_OP_CREATE_CONN_CANCEL,
> - 6, &conn->dst);
> - }
> - break;
> - case BT_CONNECT2:
> - if (conn->type == ACL_LINK) {
> - struct hci_cp_reject_conn_req rej;
> -
> - bacpy(&rej.bdaddr, &conn->dst);
> - rej.reason = reason;
> -
> - r = hci_send_cmd(conn->hdev,
> - HCI_OP_REJECT_CONN_REQ,
> - sizeof(rej), &rej);
> - } else if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
> - struct hci_cp_reject_sync_conn_req rej;
> -
> - bacpy(&rej.bdaddr, &conn->dst);
> -
> - /* SCO rejection has its own limited set of
> - * allowed error values (0x0D-0x0F) which isn't
> - * compatible with most values passed to this
> - * function. To be safe hard-code one of the
> - * values that's suitable for SCO.
> - */
> - rej.reason = HCI_ERROR_REJ_LIMITED_RESOURCES;
> -
> - r = hci_send_cmd(conn->hdev,
> - HCI_OP_REJECT_SYNC_CONN_REQ,
> - sizeof(rej), &rej);
> - }
> - break;
> - default:
> - conn->state = BT_CLOSED;
> - break;
> }
>
> - return r;
> + return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
> + NULL);
> }
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index afb8e970e62c..ef8297d04a8c 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5271,10 +5271,13 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
> }
>
> static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
> - struct hci_conn *conn)
> + struct hci_conn *conn, u8 reason)
> {
> + /* Return reason if scanning since the connection shall probably be
> + * cleanup directly.
> + */
> if (test_bit(HCI_CONN_SCANNING, &conn->flags))
> - return 0;
> + return reason;
>
> if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
> return 0;
> @@ -5283,10 +5286,11 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
> 0, NULL, HCI_CMD_TIMEOUT);
> }
>
> -static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn)
> +static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
> + u8 reason)
> {
> if (conn->type == LE_LINK)
> - return hci_le_connect_cancel_sync(hdev, conn);
> + return hci_le_connect_cancel_sync(hdev, conn, reason);
>
> if (hdev->hci_ver < BLUETOOTH_VER_1_2)
> return 0;
> @@ -5339,9 +5343,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
> case BT_CONFIG:
> return hci_disconnect_sync(hdev, conn, reason);
> case BT_CONNECT:
> - err = hci_connect_cancel_sync(hdev, conn);
> + err = hci_connect_cancel_sync(hdev, conn, reason);
> /* Cleanup hci_conn object if it cannot be cancelled as it
> - * likelly means the controller and host stack are out of sync.
> + * likelly means the controller and host stack are out of sync
> + * or in case of LE it was still scanning so it can be cleanup
> + * safely.
> */
> if (err) {
> hci_dev_lock(hdev);
> @@ -6255,7 +6261,7 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
>
> done:
> if (err == -ETIMEDOUT)
> - hci_le_connect_cancel_sync(hdev, conn);
> + hci_le_connect_cancel_sync(hdev, conn, 0x00);
>
> /* Re-enable advertising after the connection attempt is finished. */
> hci_resume_advertising_sync(hdev);
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 0f5ba618ceb1..3156bc27088e 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3586,18 +3586,6 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> return err;
> }
>
> -static int abort_conn_sync(struct hci_dev *hdev, void *data)
> -{
> - struct hci_conn *conn;
> - u16 handle = PTR_ERR(data);
> -
> - conn = hci_conn_hash_lookup_handle(hdev, handle);
> - if (!conn)
> - return 0;
> -
> - return hci_abort_conn_sync(hdev, conn, HCI_ERROR_REMOTE_USER_TERM);
> -}
> -
> static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> u16 len)
> {
> @@ -3648,8 +3636,7 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> le_addr_type(addr->type));
>
> if (conn->conn_reason == CONN_REASON_PAIR_DEVICE)
> - hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
> - NULL);
> + hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
>
> unlock:
> hci_dev_unlock(hdev);
Hi Pauli,
On Wed, Jun 28, 2023 at 7:49 AM Pauli Virtanen <[email protected]> wrote:
>
> Hi Luiz,
>
> ti, 2023-06-27 kello 14:53 -0700, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <[email protected]>
> >
> > This consolidates code for aborting connections using
> > hci_cmd_sync_queue so it is synchronized with other threads, but
> > because of the fact that some commands may block the cmd_sync_queue
> > while waiting specific events this attempt to cancel those requests by
> > using hci_cmd_sync_cancel.
> >
> > Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> > ---
> > include/net/bluetooth/hci_core.h | 2 +-
> > net/bluetooth/hci_conn.c | 160 +++++++------------------------
> > net/bluetooth/hci_sync.c | 20 ++--
> > net/bluetooth/mgmt.c | 15 +--
> > 4 files changed, 48 insertions(+), 149 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 05a9b3ab3f56..094ca3aca15e 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -741,6 +741,7 @@ struct hci_conn {
> > unsigned long flags;
> >
> > enum conn_reasons conn_reason;
> > + __u8 abort_reason;
> >
> > __u32 clock;
> > __u16 clock_accuracy;
> > @@ -760,7 +761,6 @@ struct hci_conn {
> > struct delayed_work auto_accept_work;
> > struct delayed_work idle_work;
> > struct delayed_work le_conn_timeout;
> > - struct work_struct le_scan_cleanup;
> >
> > struct device dev;
> > struct dentry *debugfs;
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 47e7aa4d63a9..88f18f375684 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -178,57 +178,6 @@ static void hci_conn_cleanup(struct hci_conn *conn)
> > hci_conn_put(conn);
> > }
> >
> > -static void le_scan_cleanup(struct work_struct *work)
> > -{
> > - struct hci_conn *conn = container_of(work, struct hci_conn,
> > - le_scan_cleanup);
> > - struct hci_dev *hdev = conn->hdev;
> > - struct hci_conn *c = NULL;
> > -
> > - BT_DBG("%s hcon %p", hdev->name, conn);
> > -
> > - hci_dev_lock(hdev);
> > -
> > - /* Check that the hci_conn is still around */
> > - rcu_read_lock();
> > - list_for_each_entry_rcu(c, &hdev->conn_hash.list, list) {
> > - if (c == conn)
> > - break;
> > - }
> > - rcu_read_unlock();
> > -
> > - if (c == conn) {
> > - hci_connect_le_scan_cleanup(conn, 0x00);
> > - hci_conn_cleanup(conn);
> > - }
> > -
> > - hci_dev_unlock(hdev);
> > - hci_dev_put(hdev);
> > - hci_conn_put(conn);
> > -}
> > -
> > -static void hci_connect_le_scan_remove(struct hci_conn *conn)
> > -{
> > - BT_DBG("%s hcon %p", conn->hdev->name, conn);
> > -
> > - /* We can't call hci_conn_del/hci_conn_cleanup here since that
> > - * could deadlock with another hci_conn_del() call that's holding
> > - * hci_dev_lock and doing cancel_delayed_work_sync(&conn->disc_work).
> > - * Instead, grab temporary extra references to the hci_dev and
> > - * hci_conn and perform the necessary cleanup in a separate work
> > - * callback.
> > - */
> > -
> > - hci_dev_hold(conn->hdev);
> > - hci_conn_get(conn);
> > -
> > - /* Even though we hold a reference to the hdev, many other
> > - * things might get cleaned up meanwhile, including the hdev's
> > - * own workqueue, so we can't use that for scheduling.
> > - */
> > - schedule_work(&conn->le_scan_cleanup);
> > -}
> > -
> > static void hci_acl_create_connection(struct hci_conn *conn)
> > {
> > struct hci_dev *hdev = conn->hdev;
> > @@ -679,13 +628,6 @@ static void hci_conn_timeout(struct work_struct *work)
> > if (refcnt > 0)
> > return;
> >
> > - /* LE connections in scanning state need special handling */
> > - if (conn->state == BT_CONNECT && conn->type == LE_LINK &&
> > - test_bit(HCI_CONN_SCANNING, &conn->flags)) {
> > - hci_connect_le_scan_remove(conn);
> > - return;
> > - }
> > -
> > hci_abort_conn(conn, hci_proto_disconn_ind(conn));
> > }
> >
> > @@ -1066,7 +1008,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
> > INIT_DELAYED_WORK(&conn->auto_accept_work, hci_conn_auto_accept);
> > INIT_DELAYED_WORK(&conn->idle_work, hci_conn_idle);
> > INIT_DELAYED_WORK(&conn->le_conn_timeout, le_conn_timeout);
> > - INIT_WORK(&conn->le_scan_cleanup, le_scan_cleanup);
> >
> > atomic_set(&conn->refcnt, 0);
> >
> > @@ -2888,81 +2829,46 @@ u32 hci_conn_get_phy(struct hci_conn *conn)
> > return phys;
> > }
> >
> > -int hci_abort_conn(struct hci_conn *conn, u8 reason)
> > +static int abort_conn_sync(struct hci_dev *hdev, void *data)
> > {
> > - int r = 0;
> > + struct hci_conn *conn;
> > + u16 handle = PTR_ERR(data);
> >
> > - if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
> > + conn = hci_conn_hash_lookup_handle(hdev, handle);
> > + if (!conn)
> > return 0;
> >
> > - switch (conn->state) {
> > - case BT_CONNECTED:
> > - case BT_CONFIG:
> > - if (conn->type == AMP_LINK) {
> > - struct hci_cp_disconn_phy_link cp;
> > + return hci_abort_conn_sync(hdev, conn, conn->abort_reason);
> > +}
> >
> > - cp.phy_handle = HCI_PHY_HANDLE(conn->handle);
> > - cp.reason = reason;
> > - r = hci_send_cmd(conn->hdev, HCI_OP_DISCONN_PHY_LINK,
> > - sizeof(cp), &cp);
> > - } else {
> > - struct hci_cp_disconnect dc;
> > +int hci_abort_conn(struct hci_conn *conn, u8 reason)
> > +{
> > + struct hci_dev *hdev = conn->hdev;
> >
> > - dc.handle = cpu_to_le16(conn->handle);
> > - dc.reason = reason;
> > - r = hci_send_cmd(conn->hdev, HCI_OP_DISCONNECT,
> > - sizeof(dc), &dc);
> > + /* If abort_reason has already been set it means the connection is
> > + * already being aborted so don't attempt to overwrite it.
> > + */
> > + if (conn->abort_reason)
> > + return 0;
> > +
> > + bt_dev_dbg(hdev, "handle 0x%2.2x reason 0x%2.2x", conn->handle, reason);
> > +
> > + conn->abort_reason = reason;
> > +
> > + /* If the connection is pending check the command opcode since that
> > + * might be blocking on hci_cmd_sync_work while waiting its respective
> > + * event so we need to hci_cmd_sync_cancel to cancel it.
> > + */
> > + if (conn->state == BT_CONNECT && hdev->req_status == HCI_REQ_PEND) {
> > + switch (hci_skb_event(hdev->sent_cmd)) {
> > + case HCI_EV_LE_CONN_COMPLETE:
> > + case HCI_EV_LE_ENHANCED_CONN_COMPLETE:
> > + case HCI_EVT_LE_CIS_ESTABLISHED:
> > + hci_cmd_sync_cancel(hdev, -ECANCELED);
> > + break;
>
> Can this also accidentally cancel connection attempt for a different
> conn?
>
> Eg. start Create CIS conn1, then queue LE Connect conn2, then conn1
> starts waiting for the events, then hci_conn_abort conn2, then here
> conn1 gets canceled with error.
The handles would be different though since in case of CIS we do get
then assigned in CIG response, that said it maybe possible for ACL/LE
where the handles are not assigned until the connection is complete so
we may need to assign unique handles for them as well to prevent this
to happen.
> > }
> > -
> > - conn->state = BT_DISCONN;
> > -
> > - break;
> > - case BT_CONNECT:
> > - if (conn->type == LE_LINK) {
> > - if (test_bit(HCI_CONN_SCANNING, &conn->flags))
> > - break;
> > - r = hci_send_cmd(conn->hdev,
> > - HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
> > - } else if (conn->type == ACL_LINK) {
> > - if (conn->hdev->hci_ver < BLUETOOTH_VER_1_2)
> > - break;
> > - r = hci_send_cmd(conn->hdev,
> > - HCI_OP_CREATE_CONN_CANCEL,
> > - 6, &conn->dst);
> > - }
> > - break;
> > - case BT_CONNECT2:
> > - if (conn->type == ACL_LINK) {
> > - struct hci_cp_reject_conn_req rej;
> > -
> > - bacpy(&rej.bdaddr, &conn->dst);
> > - rej.reason = reason;
> > -
> > - r = hci_send_cmd(conn->hdev,
> > - HCI_OP_REJECT_CONN_REQ,
> > - sizeof(rej), &rej);
> > - } else if (conn->type == SCO_LINK || conn->type == ESCO_LINK) {
> > - struct hci_cp_reject_sync_conn_req rej;
> > -
> > - bacpy(&rej.bdaddr, &conn->dst);
> > -
> > - /* SCO rejection has its own limited set of
> > - * allowed error values (0x0D-0x0F) which isn't
> > - * compatible with most values passed to this
> > - * function. To be safe hard-code one of the
> > - * values that's suitable for SCO.
> > - */
> > - rej.reason = HCI_ERROR_REJ_LIMITED_RESOURCES;
> > -
> > - r = hci_send_cmd(conn->hdev,
> > - HCI_OP_REJECT_SYNC_CONN_REQ,
> > - sizeof(rej), &rej);
> > - }
> > - break;
> > - default:
> > - conn->state = BT_CLOSED;
> > - break;
> > }
> >
> > - return r;
> > + return hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
> > + NULL);
> > }
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index afb8e970e62c..ef8297d04a8c 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -5271,10 +5271,13 @@ static int hci_disconnect_sync(struct hci_dev *hdev, struct hci_conn *conn,
> > }
> >
> > static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
> > - struct hci_conn *conn)
> > + struct hci_conn *conn, u8 reason)
> > {
> > + /* Return reason if scanning since the connection shall probably be
> > + * cleanup directly.
> > + */
> > if (test_bit(HCI_CONN_SCANNING, &conn->flags))
> > - return 0;
> > + return reason;
> >
> > if (test_and_set_bit(HCI_CONN_CANCEL, &conn->flags))
> > return 0;
> > @@ -5283,10 +5286,11 @@ static int hci_le_connect_cancel_sync(struct hci_dev *hdev,
> > 0, NULL, HCI_CMD_TIMEOUT);
> > }
> >
> > -static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > +static int hci_connect_cancel_sync(struct hci_dev *hdev, struct hci_conn *conn,
> > + u8 reason)
> > {
> > if (conn->type == LE_LINK)
> > - return hci_le_connect_cancel_sync(hdev, conn);
> > + return hci_le_connect_cancel_sync(hdev, conn, reason);
> >
> > if (hdev->hci_ver < BLUETOOTH_VER_1_2)
> > return 0;
> > @@ -5339,9 +5343,11 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason)
> > case BT_CONFIG:
> > return hci_disconnect_sync(hdev, conn, reason);
> > case BT_CONNECT:
> > - err = hci_connect_cancel_sync(hdev, conn);
> > + err = hci_connect_cancel_sync(hdev, conn, reason);
> > /* Cleanup hci_conn object if it cannot be cancelled as it
> > - * likelly means the controller and host stack are out of sync.
> > + * likelly means the controller and host stack are out of sync
> > + * or in case of LE it was still scanning so it can be cleanup
> > + * safely.
> > */
> > if (err) {
> > hci_dev_lock(hdev);
> > @@ -6255,7 +6261,7 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> >
> > done:
> > if (err == -ETIMEDOUT)
> > - hci_le_connect_cancel_sync(hdev, conn);
> > + hci_le_connect_cancel_sync(hdev, conn, 0x00);
> >
> > /* Re-enable advertising after the connection attempt is finished. */
> > hci_resume_advertising_sync(hdev);
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 0f5ba618ceb1..3156bc27088e 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -3586,18 +3586,6 @@ static int pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> > return err;
> > }
> >
> > -static int abort_conn_sync(struct hci_dev *hdev, void *data)
> > -{
> > - struct hci_conn *conn;
> > - u16 handle = PTR_ERR(data);
> > -
> > - conn = hci_conn_hash_lookup_handle(hdev, handle);
> > - if (!conn)
> > - return 0;
> > -
> > - return hci_abort_conn_sync(hdev, conn, HCI_ERROR_REMOTE_USER_TERM);
> > -}
> > -
> > static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> > u16 len)
> > {
> > @@ -3648,8 +3636,7 @@ static int cancel_pair_device(struct sock *sk, struct hci_dev *hdev, void *data,
> > le_addr_type(addr->type));
> >
> > if (conn->conn_reason == CONN_REASON_PAIR_DEVICE)
> > - hci_cmd_sync_queue(hdev, abort_conn_sync, ERR_PTR(conn->handle),
> > - NULL);
> > + hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
> >
> > unlock:
> > hci_dev_unlock(hdev);
>
--
Luiz Augusto von Dentz