Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230502145737.140856-1-lrh2000@pku.edu.cn> <20230502145737.140856-5-lrh2000@pku.edu.cn>
In-Reply-To: <20230502145737.140856-5-lrh2000@pku.edu.cn>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 2 May 2023 09:22:03 -0700
Message-ID: <CABBYNZK=N6kwhpa=v35T8ReyR6-1BEy-yEFtxGbYPJxeOB1S0w@mail.gmail.com>
Subject: Re: [PATCH v3 4/6] Bluetooth: Perform hci_conn_drop in hci_conn_unlink
To:     Ruihan Li <lrh2000@pku.edu.cn>
Cc:     linux-bluetooth@vger.kernel.org,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Ruihan,

On Tue, May 2, 2023 at 7:57=E2=80=AFAM Ruihan Li <lrh2000@pku.edu.cn> 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 =3D=3D NULL]
>                 -> hci_conn_unlink(child)  [child->parent =3D=3D 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@pku.edu.cn>
> ---
>  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 =3D 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 +=3D conn->sent;
>         } else {
> -               struct hci_conn *acl =3D conn->parent;
> -
> -               if (acl) {
> -                       hci_conn_unlink(conn);
> -                       hci_conn_drop(acl);
> -               }
> +               hci_conn_unlink(conn);
>
>                 /* Unacked ISO frames */
>                 if (conn->type =3D=3D 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
>


--=20
Luiz Augusto von Dentz