Hi Pauli,
> -----Original Message-----
> From: Pauli Virtanen <pav@xxxxxx>
> Sent: Tuesday, October 31, 2023 12:13 AM
> To: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>; linux-
> bluetooth@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/1] Bluetooth: ISO: Reassociate a socket with an
> active BIS
>
> Hi,
>
> ma, 2023-10-30 kello 17:43 +0200, Iulia Tanasescu kirjoitti:
> > For ISO Broadcast, all BISes from a BIG have the same lifespan - they
> > cannot be created or terminated independently from each other.
> >
> > This links together all BIS hcons that are part of the same BIG, so
> > all hcons are kept alive as long as the BIG is active.
> >
> > If multiple BIS sockets are opened for a BIG handle, and only part of
> > them are closed at some point, the associated hcons will be marked as
> > open. If new sockets will later be opened for the same BIG, they will
> > be reassociated with the open BIS hcons.
>
> Can you explain here a bit the new BIS hci_conn refcounting scheme, it's
> not straightforward to follow? Who owns their refcounts?
>
> >
> > All BIS hcons will be cleaned up and the BIG will be terminated when
> > the last BIS socket is closed from userspace.
> >
> > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> > ---
> > include/net/bluetooth/hci_core.h | 24 +++++++++++++
> > net/bluetooth/hci_conn.c | 27 ++++++++++++++
> > net/bluetooth/iso.c | 60 ++++++++++++++++++++++++++++++++
> > 3 files changed, 111 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h
> > b/include/net/bluetooth/hci_core.h
> > index 20988623c5cc..201c0809540a 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1292,6 +1292,30 @@ 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_state(struct hci_dev *hdev, __u8 handle,
> > +__u16 state) {
> > + 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 (bacmp(&c->dst, BDADDR_ANY) || c->type != ISO_LINK ||
> > + c->state != state)
> > + 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_pa_sync_big_handle(struct hci_dev *hdev, __u8
> > big) { diff --git a/net/bluetooth/hci_conn.c
> > b/net/bluetooth/hci_conn.c index 2cee330188ce..b8ab5c0cd48e 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -2228,7 +2228,17 @@ struct hci_conn *hci_bind_bis(struct hci_dev
> *hdev, bdaddr_t *dst,
> > __u8 base_len, __u8 *base) {
> > struct hci_conn *conn;
> > + struct hci_conn *parent;
> > __u8 eir[HCI_MAX_PER_AD_LENGTH];
> > + struct hci_link *link;
> > +
> > + /* Look for any BIS that is open for rebinding */
> > + conn = hci_conn_hash_lookup_big_state(hdev, qos->bcast.big,
> BT_OPEN);
> > + if (conn) {
> > + memcpy(qos, &conn->iso_qos, sizeof(*qos));
> > + conn->state = BT_CONNECTED;
> > + return conn;
> > + }
> >
> > if (base_len && base)
> > base_len = eir_append_service_data(eir, 0, 0x1851, @@
> > -2256,6 +2266,20 @@ struct hci_conn *hci_bind_bis(struct hci_dev *hdev,
> bdaddr_t *dst,
> > conn->iso_qos = *qos;
> > conn->state = BT_BOUND;
> >
> > + /* Link BISes together */
> > + parent = hci_conn_hash_lookup_big(hdev,
> > + conn->iso_qos.bcast.big);
> > + if (parent && parent != conn) {
> > + link = hci_conn_link(parent, conn);
> > + if (!link) {
> > + hci_conn_drop(conn);
> > + return ERR_PTR(-ENOLINK);
> > + }
> > +
> > + /* Link takes the refcount */
> > + hci_conn_drop(conn);
> > + }
>
> So the first hci_conn added in a BIG is assigned as "parent". How does the
> refcounting here work?
>
> hci_conn_link takes refcount of the parent (cf. hci_conn_unlink), but it's not
> incremented here so I guess it may go negative.
When adding a new BIS hcon, hci_add_bis will take the refcount.
The first BIS in a BIG will be assigned as parent. When new BISes are added,
they will be linked to the parent and hci_conn_link takes an extra refcount
for children:
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/hci_conn.c#n1724
This is why hci_conn_drop needs to be called for children BISes, to remove
the duplicate refcount.
>
> hci_conn_unlink does not drop child connections, and they're not dropped
> by the socket closing below unless last, so I'm not sure if they are then
> cleaned up if closing sockets in any order.
If hci_conn_unlink is called for a child hcon, the child will be unlinked
and hci_conn_drop will be called on the parent:
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/net/bluetooth/hci_conn.c#n1130
hci_conn_unlink will then be called for the parent, and all other children
hcons will be cleaned up with hci_conn_cleanup_child.
So regardless if the parent is closed first or any child, all hcons will get
to be cleaned up.
>
> This change will also expose BIS hci_conns to hci_conn_cleanup_child,
> which was written only for unicast, and might not be right for the use case
> here.
That is right, from the tests I performed I noticed the conditions in
hci_conn_cleanup_child are also met for BIS so it will reach
hci_conn_failed, but I submitted an updated patch to add a dedicated
condition for broadcast also, to cover all cases.
>
> > +
> > return conn;
> > }
> >
> > @@ -2287,6 +2311,9 @@ struct hci_conn *hci_connect_bis(struct hci_dev
> *hdev, bdaddr_t *dst,
> > if (IS_ERR(conn))
> > return conn;
> >
> > + if (conn->state == BT_CONNECTED)
> > + return conn;
> > +
> > data.big = qos->bcast.big;
> > data.bis = qos->bcast.bis;
> >
> > diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index
> > e01b6abe36fb..13353d7dc4b1 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -587,6 +587,44 @@ static struct sock *iso_get_sock_listen(bdaddr_t
> *src, bdaddr_t *dst,
> > return sk ? sk : sk1;
> > }
> >
> > +static struct sock *iso_get_sock_big(struct sock *match_sk, bdaddr_t *src,
> > + bdaddr_t *dst, uint8_t big) {
> > + struct sock *sk = NULL;
> > +
> > + read_lock(&iso_sk_list.lock);
> > +
> > + sk_for_each(sk, &iso_sk_list.head) {
> > + if (match_sk == sk)
> > + continue;
> > +
> > + /* Look for sockets that have already been
> > + * connected to the BIG
> > + */
> > + if (sk->sk_state != BT_CONNECTED &&
> > + sk->sk_state != BT_CONNECT)
> > + continue;
> > +
> > + /* Match Broadcast destination */
> > + if (bacmp(&iso_pi(sk)->dst, dst))
> > + continue;
> > +
> > + /* Match BIG handle */
> > + if (iso_pi(sk)->qos.bcast.big != big)
> > + continue;
> > +
> > + /* Match source address */
> > + if (bacmp(&iso_pi(sk)->src, src))
> > + continue;
> > +
> > + break;
> > + }
> > +
> > + read_unlock(&iso_sk_list.lock);
> > +
> > + return sk;
>
> What keeps sk alive?
>
> This pattern is used also in iso_get_sock_listen, but I don't understand why
> it's OK. In unix/ & netrom/ & ax25/ I see sock_hold before lock release in
> similar constructs.
It seems like it might be necessary to add sock_hold to make sure the
socket is kept alive while it is being processed, so I added this update
in the new patch version.
>
> > +}
> > +
> > static void iso_sock_destruct(struct sock *sk) {
> > BT_DBG("sk %p", sk);
> > @@ -639,6 +677,28 @@ static void iso_sock_kill(struct sock *sk)
> >
> > static void iso_sock_disconn(struct sock *sk) {
> > + struct sock *bis_sk;
> > + struct hci_conn *hcon = iso_pi(sk)->conn->hcon;
> > +
> > + if (test_bit(HCI_CONN_BIG_CREATED, &hcon->flags)) {
> > + bis_sk = iso_get_sock_big(sk, &iso_pi(sk)->src,
> > + &iso_pi(sk)->dst,
> > + iso_pi(sk)->qos.bcast.big);
> > +
> > + /* If there are any other connected sockets for the
> > + * same BIG, just delete the sk and leave the bis
> > + * hcon active, in case later rebinding is needed.
> > + */
> > + if (bis_sk) {
> > + hcon->state = BT_OPEN;
> > + iso_pi(sk)->conn->hcon = NULL;
> > + release_sock(sk);
> > + iso_conn_del(hcon,
> > + bt_to_errno(hcon->abort_reason));
>
> iso_conn_del must be called with hdev->lock held, that's assumed in
> iso_connect*.
>
> Locks must be taken in order hci_dev_lock > lock_sock > iso_conn_lock.
>
> Could this use iso_chan_del instead (plus maybe iso_sock_clear_timer)?
> Lifetime of iso_conn currently follows hcon, not sure if that needs to be
> changed?
I agree, I could use iso_chan_del here, so only the socket is cleaned up,
and the ISO conn will remain allocated as long as hcon is alive. I updated
in the new patch.
>
> > + lock_sock(sk);
> > + return;
> > + }
> > + }
> > +
> > sk->sk_state = BT_DISCONN;
> > iso_sock_set_timer(sk, ISO_DISCONN_TIMEOUT);
> > iso_conn_lock(iso_pi(sk)->conn);
>
> Not related to this patch precisely, but I suspect the disconnect timeout is
> something that is not useful for ISO sockets and maybe we should remove it,
> since closing sockets is used for CIG/BIG management.
I think the disconnect timeout makes more sense for CIS management, because
when a CIS socket is closed, HCI_OP_DISCONNECT is first sent to controller
to disconnect the CIS, and only after HCI_EV_DISCONN_COMPLETE is received,
the hcon will be cleaned up and the CIG will be removed.
>
> --
> Pauli Virtanen
Regards,
Iulia
