Hi Jonas,
On Tue, Feb 13, 2024 at 6:47 PM Jonas Dreßler <verdre@xxxxxxx> wrote:
>
> 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().
Hmm but shall never be the case since we have the following check before it:
/* Since the controller supports only one LE connection attempt at a
* time, we return -EBUSY if there is any connection attempt running.
*/
if (hci_lookup_le_connect(hdev))
return ERR_PTR(-EBUSY);
>
> > - 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);
Not really following this code, do you want to be able to cancel
callbacks that are actually executing, rather than queued?
> 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;
> + }
It might be better to save the executing entry at hdev and then make
the lookup_entry return it if the parameters match so it can be
cancelled with cancel_entry, that said perhaps it would be better to
just cancel the work if -ECANCELED is received so we don't have to
keep checking if it is returned on every command the callback
generates.
> + 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;
> }
>
--
Luiz Augusto von Dentz
