| Message ID | bcae59679f7387e6f5f3e6da79827da4cc341870.1685294131.git.pav@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | LE Set CIG Parameters / Create CIS 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 |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Pauli, On Sun, May 28, 2023 at 10:49 AM Pauli Virtanen <pav@iki.fi> wrote: > > The order of CIS handle array in Set CIG Parameters response shall match > the order of the CIS_ID array in the command (Core v5.3 Vol 4 Part E Sec > 7.8.97). We send CIS_IDs mainly in the order of increasing CIS_ID (but > with "last" CIS first if it has fixed CIG_ID). In handling of the > reply, we currently assume this is also the same as the order of > hci_conn in hdev->conn_hash, but that is not true. > > Match the correct hci_conn to the correct handle by matching them based > on the CIG+CIS combination. The CIG+CIS combination shall be unique for > ISO_LINK hci_conn at state >= BT_BOUND, which we maintain in > hci_le_set_cig_params. > > Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections") > Signed-off-by: Pauli Virtanen <pav@iki.fi> > --- > net/bluetooth/hci_event.c | 66 +++++++++++++++++++++++++++++---------- > 1 file changed, 50 insertions(+), 16 deletions(-) > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index d00ef6e3fc45..71d8f1442287 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -3804,43 +3804,77 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data, > struct sk_buff *skb) > { > struct hci_rp_le_set_cig_params *rp = data; > + struct hci_cp_le_set_cig_params *cp; > struct hci_conn *conn; > - int i = 0; > + u16 handles[0x1f]; > + int num_handles; > + u8 status = rp->status; > + int i; > > bt_dev_dbg(hdev, "status 0x%2.2x", rp->status); > > + cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_CIG_PARAMS); > + if (!cp || rp->num_handles != cp->num_cis || rp->cig_id != cp->cig_id || > + rp->num_handles > ARRAY_SIZE(handles)) { > + bt_dev_err(hdev, "unexpected Set CIG Parameters response data"); > + status = HCI_ERROR_UNSPECIFIED; > + } > + > hci_dev_lock(hdev); > > - if (rp->status) { > + if (status) { > while ((conn = hci_conn_hash_lookup_cig(hdev, rp->cig_id))) { > conn->state = BT_CLOSED; > - hci_connect_cfm(conn, rp->status); > + hci_connect_cfm(conn, status); > hci_conn_del(conn); > } > goto unlock; > } > > + /* BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E page 2553 > + * > + * If the Status return parameter is zero, then the Controller shall > + * set the Connection_Handle arrayed return parameter to the connection > + * handle(s) corresponding to the CIS configurations specified in > + * the CIS_IDs command parameter, in the same order. > + */ > + > + num_handles = rp->num_handles; > + for (i = 0; i < rp->num_handles; ++i) > + handles[i] = __le16_to_cpu(rp->handle[i]); Using the request is a good idea but the code below sounds a little too complicated, can we just lookup the hci_conn by cig/cis at this point using the request parameters and just assign the handle in a single loop? > rcu_read_lock(); > > list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) { > - if (conn->type != ISO_LINK || > - conn->iso_qos.ucast.cig != rp->cig_id || > - conn->state == BT_CONNECTED) > + if (conn->type != ISO_LINK || !bacmp(&conn->dst, BDADDR_ANY) || > + (conn->state != BT_BOUND && conn->state != BT_CONNECT) || > + conn->iso_qos.ucast.cig != rp->cig_id) > continue; > > - conn->handle = __le16_to_cpu(rp->handle[i++]); > + for (i = 0; i < rp->num_handles; ++i) { > + if (handles[i] == HCI_CONN_HANDLE_UNSET) > + continue; > + if (conn->iso_qos.ucast.cis != cp->cis[i].cis_id) > + continue; > > - bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", conn, > - conn->handle, conn->parent); > + conn->handle = handles[i]; > + handles[i] = HCI_CONN_HANDLE_UNSET; > + --num_handles; > > - /* Create CIS if LE is already connected */ > - if (conn->parent && conn->parent->state == BT_CONNECTED) { > - rcu_read_unlock(); > - hci_le_create_cis(conn); > - rcu_read_lock(); > + bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", > + conn, conn->handle, conn->parent); > + > + /* Create CIS if LE is already connected */ > + if (conn->parent && > + conn->parent->state == BT_CONNECTED) { > + rcu_read_unlock(); > + hci_le_create_cis(conn); > + rcu_read_lock(); > + } > + > + break; > } > - > - if (i == rp->num_handles) > + if (!num_handles) > break; > } > > -- > 2.40.1 >
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index d00ef6e3fc45..71d8f1442287 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -3804,43 +3804,77 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data, struct sk_buff *skb) { struct hci_rp_le_set_cig_params *rp = data; + struct hci_cp_le_set_cig_params *cp; struct hci_conn *conn; - int i = 0; + u16 handles[0x1f]; + int num_handles; + u8 status = rp->status; + int i; bt_dev_dbg(hdev, "status 0x%2.2x", rp->status); + cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_CIG_PARAMS); + if (!cp || rp->num_handles != cp->num_cis || rp->cig_id != cp->cig_id || + rp->num_handles > ARRAY_SIZE(handles)) { + bt_dev_err(hdev, "unexpected Set CIG Parameters response data"); + status = HCI_ERROR_UNSPECIFIED; + } + hci_dev_lock(hdev); - if (rp->status) { + if (status) { while ((conn = hci_conn_hash_lookup_cig(hdev, rp->cig_id))) { conn->state = BT_CLOSED; - hci_connect_cfm(conn, rp->status); + hci_connect_cfm(conn, status); hci_conn_del(conn); } goto unlock; } + /* BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E page 2553 + * + * If the Status return parameter is zero, then the Controller shall + * set the Connection_Handle arrayed return parameter to the connection + * handle(s) corresponding to the CIS configurations specified in + * the CIS_IDs command parameter, in the same order. + */ + + num_handles = rp->num_handles; + for (i = 0; i < rp->num_handles; ++i) + handles[i] = __le16_to_cpu(rp->handle[i]); + rcu_read_lock(); list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) { - if (conn->type != ISO_LINK || - conn->iso_qos.ucast.cig != rp->cig_id || - conn->state == BT_CONNECTED) + if (conn->type != ISO_LINK || !bacmp(&conn->dst, BDADDR_ANY) || + (conn->state != BT_BOUND && conn->state != BT_CONNECT) || + conn->iso_qos.ucast.cig != rp->cig_id) continue; - conn->handle = __le16_to_cpu(rp->handle[i++]); + for (i = 0; i < rp->num_handles; ++i) { + if (handles[i] == HCI_CONN_HANDLE_UNSET) + continue; + if (conn->iso_qos.ucast.cis != cp->cis[i].cis_id) + continue; - bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", conn, - conn->handle, conn->parent); + conn->handle = handles[i]; + handles[i] = HCI_CONN_HANDLE_UNSET; + --num_handles; - /* Create CIS if LE is already connected */ - if (conn->parent && conn->parent->state == BT_CONNECTED) { - rcu_read_unlock(); - hci_le_create_cis(conn); - rcu_read_lock(); + bt_dev_dbg(hdev, "%p handle 0x%4.4x parent %p", + conn, conn->handle, conn->parent); + + /* Create CIS if LE is already connected */ + if (conn->parent && + conn->parent->state == BT_CONNECTED) { + rcu_read_unlock(); + hci_le_create_cis(conn); + rcu_read_lock(); + } + + break; } - - if (i == rp->num_handles) + if (!num_handles) break; }
The order of CIS handle array in Set CIG Parameters response shall match the order of the CIS_ID array in the command (Core v5.3 Vol 4 Part E Sec 7.8.97). We send CIS_IDs mainly in the order of increasing CIS_ID (but with "last" CIS first if it has fixed CIG_ID). In handling of the reply, we currently assume this is also the same as the order of hci_conn in hdev->conn_hash, but that is not true. Match the correct hci_conn to the correct handle by matching them based on the CIG+CIS combination. The CIG+CIS combination shall be unique for ISO_LINK hci_conn at state >= BT_BOUND, which we maintain in hci_le_set_cig_params. Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections") Signed-off-by: Pauli Virtanen <pav@iki.fi> --- net/bluetooth/hci_event.c | 66 +++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 16 deletions(-)