| Message ID | 20241108160723.3399-3-iulia.tanasescu@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: ISO: PA/BIG sync fixes | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
Hi Iulia, On Fri, Nov 8, 2024 at 11:07 AM Iulia Tanasescu <iulia.tanasescu@nxp.com> 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@nxp.com> > --- > 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 >
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); 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); + 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,
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. < 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. Signed-off-by: Iulia Tanasescu <iulia.tanasescu@nxp.com> --- net/bluetooth/hci_conn.c | 19 ++++++++++++++++++- net/bluetooth/iso.c | 4 ++++ 2 files changed, 22 insertions(+), 1 deletion(-)