Hi Iulia,
On Wed, Jun 28, 2023 at 8:06 AM Iulia Tanasescu <iulia.tanasescu@xxxxxxx> wrote:
>
> Some use cases require the user to be informed if BIG synchronization
> fails. This commit makes it so that even if the BIG sync established
> event arrives with error status, a new hconn is added for each BIS,
> and the iso layer is notified about the failed connections.
>
> Unsuccesful bis connections will be marked using the
> HCI_CONN_BIG_SYNC_FAILED flag. From the iso layer, the POLLERR event
> is triggered on the newly allocated bis sockets, before adding them
> to the accept list of the parent socket.
>
> From user space, a new fd for each failed bis connection will be
> obtained by calling accept. The user should check for the POLLERR
> event on the new socket, to determine if the connection was successful
> or not.
>
> The HCI_CONN_BIG_SYNC flag has been added to mark whether the BIG sync
> has been successfully established. This flag is checked at bis cleanup,
> so the HCI LE BIG Terminate Sync command is only issued if needed.
>
> The BT_SK_BIG_SYNC flag indicates if BIG create sync has been called
> for a listening socket, to avoid issuing the command everytime a BIGInfo
> advertising report is received.
>
> Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> ---
> include/net/bluetooth/hci_core.h | 25 ++++++++++++++++
> net/bluetooth/hci_conn.c | 50 +++++++++++++++++---------------
> net/bluetooth/hci_event.c | 21 +++++++++++---
> net/bluetooth/iso.c | 37 ++++++++++++++++-------
> 4 files changed, 95 insertions(+), 38 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 05a9b3ab3f56..f068a578ff59 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -978,6 +978,8 @@ enum {
> HCI_CONN_PER_ADV,
> HCI_CONN_BIG_CREATED,
> HCI_CONN_CREATE_CIS,
> + HCI_CONN_BIG_SYNC,
> + HCI_CONN_BIG_SYNC_FAILED,
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> @@ -1288,6 +1290,29 @@ static inline struct hci_conn *hci_conn_hash_lookup_big(struct hci_dev *hdev,
> return NULL;
> }
>
> +static inline struct hci_conn *hci_conn_hash_lookup_big_any_dst(struct hci_dev *hdev,
> + __u8 handle)
> +{
> + 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->type != ISO_LINK)
> + continue;
> +
> + if (handle == c->iso_qos.bcast.big) {
> + rcu_read_unlock();
> + return c;
> + }
> + }
> +
> + rcu_read_unlock();
> +
> + return NULL;
> +}
> +
> static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
> __u8 type, __u16 state)
> {
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 47e7aa4d63a9..491ca8e876f0 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -686,6 +686,19 @@ static void hci_conn_timeout(struct work_struct *work)
> return;
> }
>
> + /* Cleanup bises that failed to be established */
> + if (test_and_clear_bit(HCI_CONN_BIG_SYNC_FAILED, &conn->flags)) {
> + conn->state = BT_CLOSED;
> + hci_disconn_cfm(conn, hci_proto_disconn_ind(conn));
> + hci_conn_hash_del(conn->hdev, conn);
> +
> + if (conn->cleanup)
> + conn->cleanup(conn);
> +
> + hci_conn_put(conn);
> + return;
> + }
I will be pushing some changes to how hci_abort_conn works, the code
above shall probably be done inside the likes of hci_conn_failed
though but first we need to add support for BIS into
hci_abort_conn_sync.
> hci_abort_conn(conn, hci_proto_disconn_ind(conn));
> }
>
> @@ -793,6 +806,7 @@ struct iso_list_data {
> int count;
> struct iso_cig_params pdu;
> bool big_term;
> + bool big_sync_term;
> };
>
> static void bis_list(struct hci_conn *conn, void *data)
> @@ -810,17 +824,6 @@ static void bis_list(struct hci_conn *conn, void *data)
> d->count++;
> }
>
> -static void find_bis(struct hci_conn *conn, void *data)
> -{
> - struct iso_list_data *d = data;
> -
> - /* Ignore unicast */
> - if (bacmp(&conn->dst, BDADDR_ANY))
> - return;
> -
> - d->count++;
> -}
> -
> static int terminate_big_sync(struct hci_dev *hdev, void *data)
> {
> struct iso_list_data *d = data;
> @@ -873,31 +876,26 @@ static int big_terminate_sync(struct hci_dev *hdev, void *data)
> bt_dev_dbg(hdev, "big 0x%2.2x sync_handle 0x%4.4x", d->big,
> d->sync_handle);
>
> - /* Check if ISO connection is a BIS and terminate BIG if there are
> - * no other connections using it.
> - */
> - hci_conn_hash_list_state(hdev, find_bis, ISO_LINK, BT_CONNECTED, d);
> - if (d->count)
> - return 0;
> -
> - hci_le_big_terminate_sync(hdev, d->big);
> + if (d->big_sync_term)
> + hci_le_big_terminate_sync(hdev, d->big);
>
> return hci_le_pa_terminate_sync(hdev, d->sync_handle);
> }
>
> -static int hci_le_big_terminate(struct hci_dev *hdev, u8 big, u16 sync_handle)
> +static int hci_le_big_terminate(struct hci_dev *hdev, u8 big, struct hci_conn *conn)
> {
> struct iso_list_data *d;
> int ret;
>
> - bt_dev_dbg(hdev, "big 0x%2.2x sync_handle 0x%4.4x", big, sync_handle);
> + bt_dev_dbg(hdev, "big 0x%2.2x sync_handle 0x%4.4x", big, conn->sync_handle);
>
> d = kzalloc(sizeof(*d), GFP_KERNEL);
> if (!d)
> return -ENOMEM;
>
> d->big = big;
> - d->sync_handle = sync_handle;
> + d->sync_handle = conn->sync_handle;
> + d->big_sync_term = test_and_clear_bit(HCI_CONN_BIG_SYNC, &conn->flags);
>
> ret = hci_cmd_sync_queue(hdev, big_terminate_sync, d,
> terminate_big_destroy);
> @@ -933,8 +931,14 @@ static void bis_cleanup(struct hci_conn *conn)
>
> hci_le_terminate_big(hdev, conn);
> } else {
> + bis = hci_conn_hash_lookup_big_any_dst(hdev,
> + conn->iso_qos.bcast.big);
> +
> + if (bis)
> + return;
> +
> hci_le_big_terminate(hdev, conn->iso_qos.bcast.big,
> - conn->sync_handle);
> + conn);
> }
> }
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 77cbf13037b3..30d0a6631e17 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -7039,9 +7039,6 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data,
> flex_array_size(ev, bis, ev->num_bis)))
> return;
>
> - if (ev->status)
> - return;
> -
> hci_dev_lock(hdev);
>
> for (i = 0; i < ev->num_bis; i++) {
> @@ -7065,9 +7062,25 @@ static void hci_le_big_sync_established_evt(struct hci_dev *hdev, void *data,
> bis->iso_qos.bcast.in.latency = le16_to_cpu(ev->interval) * 125 / 100;
> bis->iso_qos.bcast.in.sdu = le16_to_cpu(ev->max_pdu);
>
> - hci_iso_setup_path(bis);
> + if (!ev->status) {
> + set_bit(HCI_CONN_BIG_SYNC, &bis->flags);
> + hci_iso_setup_path(bis);
> + }
> }
>
> + /* In case BIG sync failed, notify each failed connection to
> + * the user after all hci connections have been added
> + */
> + if (ev->status)
> + for (i = 0; i < ev->num_bis; i++) {
> + u16 handle = le16_to_cpu(ev->bis[i]);
> +
> + bis = hci_conn_hash_lookup_handle(hdev, handle);
> +
> + set_bit(HCI_CONN_BIG_SYNC_FAILED, &bis->flags);
> + hci_connect_cfm(bis, ev->status);
> + }
> +
> hci_dev_unlock(hdev);
> }
>
> diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
> index 84d238d0639a..e0386b12ea4e 100644
> --- a/net/bluetooth/iso.c
> +++ b/net/bluetooth/iso.c
> @@ -48,6 +48,11 @@ static void iso_sock_kill(struct sock *sk);
> #define EIR_SERVICE_DATA_LENGTH 4
> #define BASE_MAX_LENGTH (HCI_MAX_PER_AD_LENGTH - EIR_SERVICE_DATA_LENGTH)
>
> +/* iso_pinfo flags values */
> +enum {
> + BT_SK_BIG_SYNC,
> +};
> +
> struct iso_pinfo {
> struct bt_sock bt;
> bdaddr_t src;
> @@ -58,7 +63,7 @@ struct iso_pinfo {
> __u8 bc_num_bis;
> __u8 bc_bis[ISO_MAX_NUM_BIS];
> __u16 sync_handle;
> - __u32 flags;
> + unsigned long flags;
> struct bt_iso_qos qos;
> bool qos_user_set;
> __u8 base_len;
> @@ -1569,6 +1574,12 @@ static void iso_conn_ready(struct iso_conn *conn)
> hci_conn_hold(hcon);
> iso_chan_add(conn, sk, parent);
>
> + if (ev && ((struct hci_evt_le_big_sync_estabilished *)ev)->status) {
> + /* Trigger error signal on child socket */
> + sk->sk_err = ECONNREFUSED;
> + sk->sk_error_report(sk);
> + }
> +
> if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(parent)->flags))
> sk->sk_state = BT_CONNECT2;
> else
> @@ -1637,15 +1648,19 @@ int iso_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 *flags)
> if (ev2->num_bis < iso_pi(sk)->bc_num_bis)
> iso_pi(sk)->bc_num_bis = ev2->num_bis;
>
> - err = hci_le_big_create_sync(hdev,
> - &iso_pi(sk)->qos,
> - iso_pi(sk)->sync_handle,
> - iso_pi(sk)->bc_num_bis,
> - iso_pi(sk)->bc_bis);
> - if (err) {
> - bt_dev_err(hdev, "hci_le_big_create_sync: %d",
> - err);
> - sk = NULL;
> + if (!test_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags)) {
> + set_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags);
You can probably use test_and_set_bit instead above.
> +
> + err = hci_le_big_create_sync(hdev,
> + &iso_pi(sk)->qos,
> + iso_pi(sk)->sync_handle,
> + iso_pi(sk)->bc_num_bis,
> + iso_pi(sk)->bc_bis);
> + if (err) {
> + bt_dev_err(hdev, "hci_le_big_create_sync: %d",
> + err);
> + sk = NULL;
> + }
> }
> }
> } else {
> @@ -1688,7 +1703,7 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
>
> BT_DBG("hcon %p bdaddr %pMR status %d", hcon, &hcon->dst, status);
>
> - if (!status) {
> + if (!status || test_bit(HCI_CONN_BIG_SYNC_FAILED, &hcon->flags)) {
Shouldn't it be !test_bit above?
> struct iso_conn *conn;
>
> conn = iso_conn_add(hcon);
> --
> 2.34.1
>
--
Luiz Augusto von Dentz
