2023-07-26 21:43:16

by Pauli Virtanen

[permalink] [raw]
Subject: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

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 <[email protected]>
---
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 the BIG
* which is done later on connection cleanup.
*/
@@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct 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->flags))
+ /* Cleanup failed CIS, and BIS that failed to be established */
+ if (bacmp(&conn->dst, BDADDR_ANY) ||
+ test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
+ hci_conn_failed(conn, reason);
+ break;
+ case BT_BOUND:
+ /* Bound CIS should be cleaned up */
+ if (conn->type == ISO_LINK && bacmp(&conn->dst, BDADDR_ANY))
hci_conn_failed(conn, reason);
+ else
+ conn->state = BT_CLOSED;
break;
default:
conn->state = BT_CLOSED;
--
2.41.0



2023-07-27 21:38:58

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

Hi Pauli,

On Wed, Jul 26, 2023 at 2:43 PM Pauli Virtanen <[email protected]> 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 <[email protected]>
> ---
> 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 the BIG
> * which is done later on connection cleanup.
> */
> @@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct 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->flags))
> + /* Cleanup failed CIS, and BIS that failed to be established */
> + if (bacmp(&conn->dst, BDADDR_ANY) ||
> + test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))

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 == ISO_LINK && bacmp(&conn->dst, BDADDR_ANY))
> hci_conn_failed(conn, reason);
> + else
> + conn->state = BT_CLOSED;
> break;
> default:
> conn->state = BT_CLOSED;
> --
> 2.41.0
>


--
Luiz Augusto von Dentz

2023-07-27 21:53:51

by Pauli Virtanen

[permalink] [raw]
Subject: Re: [PATCH RFC 5/6] Bluetooth: hci_sync: delete CIS in BT_OPEN/CONNECT/BOUND when aborting

to, 2023-07-27 kello 14:23 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
>
> On Wed, Jul 26, 2023 at 2:43 PM Pauli Virtanen <[email protected]> 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 <[email protected]>
> > ---
> > 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 the BIG
> > * which is done later on connection cleanup.
> > */
> > @@ -5426,9 +5430,17 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct 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->flags))
> > + /* Cleanup failed CIS, and BIS that failed to be established */
> > + if (bacmp(&conn->dst, BDADDR_ANY) ||
> > + test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags))
>
> 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.

Ack, this was supposed to only do it for ISO_LINK. Whether it makes
sense for all conn types would need more careful look.

Earlier, for BT_OPEN we only call hci_conn_failed if that BIS flag was
set.

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