Hi Ruihan,
On Tue, May 2, 2023 at 7:57 AM Ruihan Li <lrh2000@xxxxxxxxxx> wrote:
>
> Since hci_conn_link invokes both hci_conn_get and hci_conn_hold,
> hci_conn_unlink should perform both hci_conn_put and hci_conn_drop as
> well. However, currently it performs only hci_conn_put.
>
> This patch makes hci_conn_unlink call hci_conn_drop as well, which
> simplifies the logic in hci_conn_del a bit and may benefit future users
> of hci_conn_unlink. But it is noted that this change additionally
> implies that hci_conn_unlink can queue disc_work on conn itself, with
> the following call stack:
>
> hci_conn_unlink(conn) [conn->parent == NULL]
> -> hci_conn_unlink(child) [child->parent == conn]
> -> hci_conn_drop(child->parent)
> -> queue_delayed_work(&conn->disc_work)
>
> Queued disc_work after hci_conn_del can be spurious, so during the
> process of hci_conn_del, it is necessary to make the call to
> cancel_delayed_work(&conn->disc_work) after invoking hci_conn_unlink.
>
> Signed-off-by: Ruihan Li <lrh2000@xxxxxxxxxx>
> ---
> net/bluetooth/hci_conn.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index eef148291..e76ebb50d 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -1105,6 +1105,7 @@ static void hci_conn_unlink(struct hci_conn *conn)
> list_del_rcu(&conn->link->list);
> synchronize_rcu();
>
> + hci_conn_drop(conn->parent);
> hci_conn_put(conn->parent);
> conn->parent = NULL;
>
> @@ -1118,7 +1119,6 @@ void hci_conn_del(struct hci_conn *conn)
>
> BT_DBG("%s hcon %p handle %d", hdev->name, conn, conn->handle);
>
> - cancel_delayed_work_sync(&conn->disc_work);
> cancel_delayed_work_sync(&conn->auto_accept_work);
> cancel_delayed_work_sync(&conn->idle_work);
>
> @@ -1134,12 +1134,7 @@ void hci_conn_del(struct hci_conn *conn)
> else
> hdev->acl_cnt += conn->sent;
> } else {
> - struct hci_conn *acl = conn->parent;
> -
> - if (acl) {
> - hci_conn_unlink(conn);
> - hci_conn_drop(acl);
> - }
> + hci_conn_unlink(conn);
>
> /* Unacked ISO frames */
> if (conn->type == ISO_LINK) {
> @@ -1152,6 +1147,11 @@ void hci_conn_del(struct hci_conn *conn)
> }
> }
>
> + /* hci_conn_unlink may trigger additional disc_work, so
> + * ensure to perform cancelling after that.
> + */
> + cancel_delayed_work_sync(&conn->disc_work);
Just merge the change where hci_conn_del calls hci_conn_unlink
unconditionally so we don't have to do this change independently just
to revert later.
> if (conn->amp_mgr)
> amp_mgr_put(conn->amp_mgr);
>
> --
> 2.40.0
>
--
Luiz Augusto von Dentz
