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: <cover.1690399379.git.pav@iki.fi> <6d9672c9a1e97b87e823e05ff07576013683979d.1690399379.git.pav@iki.fi>
In-Reply-To: <6d9672c9a1e97b87e823e05ff07576013683979d.1690399379.git.pav@iki.fi>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 27 Jul 2023 14:23:29 -0700
Message-ID: <CABBYNZLfL0BFoctaCuxp8xwbhk9kWa=FRC7w1Gh7GnTURa1_jg@mail.gmail.com>
Subject: Re: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in
 BT_OPEN/CONNECT/BOUND when aborting
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Pauli,

On Wed, Jul 26, 2023 at 2:43=E2=80=AFPM Pauli Virtanen <pav@iki.fi> wrote:
>
> Dropped CIS that are in state BT_OPEN/BT_BOUND, and in state BT_CONNECT
> with HCI_CONN_CREATE_CIS unset, should be cleaned up immediately.
> Closing CIS ISO sockets should result to the hci_conn be deleted, so
> that potentially pending CIG removal can run.
>
> hci_abort_conn cannot refer to them by handle, since their handle is
> still unset if Set CIG Parameters has not yet completed.
>
> This fixes CIS not being terminated if the socket is shut down
> immediately after connection, so that the hci_abort_conn runs before Set
> CIG Parameters completes. See new BlueZ test "ISO Connect Close - Success=
"
>
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  net/bluetooth/hci_sync.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 101548fe81da..3926213c29e6 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -5339,6 +5339,10 @@ static int hci_connect_cancel_sync(struct hci_dev =
*hdev, struct hci_conn *conn,
>                 if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
>                         return hci_disconnect_sync(hdev, conn, reason);
>
> +               /* CIS with no Create CIS sent have nothing to cancel */
> +               if (bacmp(&conn->dst, BDADDR_ANY))
> +                       return HCI_ERROR_LOCAL_HOST_TERM;
> +
>                 /* There is no way to cancel a BIS without terminating th=
e BIG
>                  * which is done later on connection cleanup.
>                  */
> @@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, stru=
ct hci_conn *conn, u8 reason)
>         case BT_CONNECT2:
>                 return hci_reject_conn_sync(hdev, conn, reason);
>         case BT_OPEN:
> -               /* Cleanup bises that failed to be established */
> -               if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->f=
lags))
> +               /* Cleanup failed CIS, and BIS that failed to be establis=
hed */
> +               if (bacmp(&conn->dst, BDADDR_ANY) ||
> +                   test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->f=
lags))

bacmp(&conn->dst, BDADDR_ANY) will match connections other than
ISO_LINK as well so I wonder if this is intentional? If it is then we
need to update the commands to reflect that we are going to call
hci_conn_failed, it seems we didn't call it before but perhaps this is
a side effect of the other changes.

> +                       hci_conn_failed(conn, reason);
> +               break;
> +       case BT_BOUND:
> +               /* Bound CIS should be cleaned up */
> +               if (conn->type =3D=3D ISO_LINK && bacmp(&conn->dst, BDADD=
R_ANY))
>                         hci_conn_failed(conn, reason);
> +               else
> +                       conn->state =3D BT_CLOSED;
>                 break;
>         default:
>                 conn->state =3D BT_CLOSED;
> --
> 2.41.0
>


--=20
Luiz Augusto von Dentz