| Message ID | 20241108160723.3399-3-iulia.tanasescu@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 >
Hi Luiz, > -----Original Message----- > From: Luiz Augusto von Dentz <luiz.dentz@gmail.com> > Sent: Friday, November 8, 2024 8:01 PM > To: Iulia Tanasescu <iulia.tanasescu@nxp.com> > Cc: linux-bluetooth@vger.kernel.org; Claudia Cristina Draghicescu > <claudia.rosu@nxp.com>; Mihai-Octavian Urzica <mihai- > octavian.urzica@nxp.com>; Andrei Istodorescu > <andrei.istodorescu@nxp.com> > 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@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. 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
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(-)