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.
> 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
