Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Friday, November 8, 2024 8:01 PM
> To: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Claudia Cristina Draghicescu
> <claudia.rosu@xxxxxxx>; Mihai-Octavian Urzica <mihai-
> octavian.urzica@xxxxxxx>; Andrei Istodorescu
> <andrei.istodorescu@xxxxxxx>
> Subject: Re: [PATCH 2/2] Bluetooth: ISO: Send BIG Create Sync via
> hci_sync
>
> Hi Iulia,
>
> On Fri, Nov 8, 2024 at 11:07 AM Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> wrote:
> >
> > Before issuing the LE BIG Create Sync command, an available BIG handle
> > is chosen by iterating through the conn_hash list and finding the
> > first unused value.
> >
> > If a BIG is terminated, the associated hcons are removed from the list
> > and the LE BIG Terminate Sync command is sent via hci_sync queue.
> > However, a new LE BIG Create sync command might be issued via
> > hci_send_cmd, before the previous BIG sync was terminated. This can
> > cause the same BIG handle to be reused and the LE BIG Create Sync to
> > fail with Command Disallowed.
>
> We should be using hci_send_cmd anymore, so
> hci_le_big_create_sync_pending shall be used whenever we want to queue LE
> BIG Create Sync command, we shall also make it safe to be called multiple
> times by using hci_cmd_sync_queue_once so it doesn't get queued multiple
> times.
>
> > < HCI Command: LE Broadcast Isochronous Group Create Sync
> (0x08|0x006b)
> > BIG Handle: 0x00
> > BIG Sync Handle: 0x0002
> > Encryption: Unencrypted (0x00)
> > Broadcast Code[16]: 00000000000000000000000000000000
> > Maximum Number Subevents: 0x00
> > Timeout: 20000 ms (0x07d0)
> > Number of BIS: 1
> > BIS ID: 0x01
> > > HCI Event: Command Status (0x0f) plen 4
> > LE Broadcast Isochronous Group Create Sync (0x08|0x006b) ncmd 1
> > Status: Command Disallowed (0x0c) < HCI Command: LE Broadcast
> > Isochronous Group Terminate Sync (0x08|0x006c)
> > BIG Handle: 0x00
> >
> > This commit fixes the ordering of the LE BIG Create Sync/LE BIG
> > Terminate Sync commands, to make sure that either the previous BIG
> > sync is terminated before reusing the handle, or that a new handle is
> > chosen for a new sync.
>
> If it is a fix of a previous change then we need to add the Fixes tag.
> Regarding the order, hci_cmd_sync_queue also fix that since then the
> command shall be executed in the order and in the situation where it needs to
> be canceled we can do so with hci_cmd_sync_dequeue.
>
> > Signed-off-by: Iulia Tanasescu <iulia.tanasescu@xxxxxxx>
> > ---
> > net/bluetooth/hci_conn.c | 19 ++++++++++++++++++-
> > net/bluetooth/iso.c | 4 ++++
> > 2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index
> > b5b78d469d54..ba74fac823c5 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -2180,11 +2180,20 @@ static bool
> hci_conn_check_create_big_sync(struct hci_conn *conn)
> > return true;
> > }
> >
> > -int hci_le_big_create_sync_pending(struct hci_dev *hdev)
> > +static void big_create_sync_complete(struct hci_dev *hdev, void
> > +*data, int err) {
> > + bt_dev_dbg(hdev, "");
> > +
> > + if (err)
> > + bt_dev_err(hdev, "Unable to create BIG sync: %d",
> > +err); }
> > +
> > +static int big_create_sync(struct hci_dev *hdev, void *data)
> > {
> > DEFINE_FLEX(struct hci_cp_le_big_create_sync, pdu, bis, num_bis,
> 0x11);
> > struct hci_conn *conn;
> >
> > + hci_dev_lock(hdev);
>
> I'd add a comment why we are acquiring the dev lock here, sync functions
> typically aren't allowed to do that except for minor updates but never when
> you can sleep/yield waiting for something to complete.
I added hci_dev_lock here thinking that it might be necessary to protect
iterating through hdev->conn_hash.list, but I think rcu_read_lock and
list_for_each_entry_rcu already offer the required protection, so I
removed hci_dev_lock.
However, I think hci_dev_lock is necessary in iso.c, to protect
hci_le_big_create_sync_pending, since hci_cmd_sync_queue_once checks
hdev flags which might change. I added a comment to explain this.
>
> > rcu_read_lock();
> >
> > pdu->num_bis = 0;
> > @@ -2229,6 +2238,7 @@ int hci_le_big_create_sync_pending(struct
> > hci_dev *hdev)
> >
> > unlock:
> > rcu_read_unlock();
> > + hci_dev_unlock(hdev);
> >
> > if (!pdu->num_bis)
> > return 0;
> > @@ -2237,6 +2247,13 @@ int hci_le_big_create_sync_pending(struct
> hci_dev *hdev)
> > struct_size(pdu, bis, pdu->num_bis), pdu);
> > }
> >
> > +int hci_le_big_create_sync_pending(struct hci_dev *hdev) {
> > + /* Queue big_create_sync */
> > + return hci_cmd_sync_queue(hdev, big_create_sync,
> > + NULL, big_create_sync_complete); }
> > +
> > int hci_le_big_create_sync(struct hci_dev *hdev, struct hci_conn *hcon,
> > struct bt_iso_qos *qos,
> > __u16 sync_handle, __u8 num_bis, __u8
> > bis[]) diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c index
> > 9e119da43147..ac1598b1e1b6 100644
> > --- a/net/bluetooth/iso.c
> > +++ b/net/bluetooth/iso.c
> > @@ -1338,6 +1338,8 @@ static void iso_conn_big_sync(struct sock *sk)
> > if (!hdev)
> > return;
> >
> > + hci_dev_lock(hdev);
>
> Same thing here, typically we don't use hci_dev_lock at the socket layer as
> there is the likes of iso_conn to interface with HCI layer, so we need to put
> some comments when there really is no other way and hci_dev_lock must be
> used.
>
> > if (!test_and_set_bit(BT_SK_BIG_SYNC, &iso_pi(sk)->flags)) {
> > err = hci_le_big_create_sync(hdev, iso_pi(sk)->conn->hcon,
> > &iso_pi(sk)->qos, @@
> > -1348,6 +1350,8 @@ static void iso_conn_big_sync(struct sock *sk)
> > bt_dev_err(hdev, "hci_le_big_create_sync: %d",
> > err);
> > }
> > +
> > + hci_dev_unlock(hdev);
> > }
> >
> > static int iso_sock_recvmsg(struct socket *sock, struct msghdr *msg,
> > --
> > 2.43.0
> >
>
>
> --
> Luiz Augusto von Dentz
Regards,
Iulia
