[PATCH 0/6] LE Set CIG Parameters / Create CIS fixes

[PATCH 0/6] LE Set CIG Parameters / Create CIS fixes

[Pauli Virtanen]

This patchset fixes a few issues with emission of HCI Remove CIG, Set
CIG Parameters, Create CIS when multiple ISO sockets in the same CIG are
closed and reconnected rapidly.

These were found when trying to make Samsung Galaxy Buds2 Pro stream
disabling and re-enabling work, and with a few additional emulator test
improvements.  In these cases, the user is closing and reconnecting ISO
sockets rapidly.  With this and the associated BlueZ patchset, disabling
and re-enabling streams on this device works correctly and the problem
in https://github.com/bluez/bluez/issues/516 is solved; this kernel
patchset contains nothing device specific though.

The last Create CIS patch probably could be done in different ways,
but seemed simplest to have hci_le_create_cis_sync be idempotent.

Pauli Virtanen (6):
  Bluetooth: ISO: fix maximum number of CIS in Set CIG Parameters
  Bluetooth: ISO: use hci_sync for setting CIG parameters
  Bluetooth: ISO: don't try to do Set CIG Parameters if CIG known busy
  Bluetooth: ISO: don't try to remove CIG if there are bound CIS left
  Bluetooth: ISO: use correct CIS order in Set CIG Parameters event
  Bluetooth: ISO: do not emit new LE Create CIS if previous is pending

 include/net/bluetooth/hci_core.h |   4 +-
 include/net/bluetooth/hci_sync.h |   2 +-
 net/bluetooth/hci_conn.c         | 139 +++++++++++++++++++------------
 net/bluetooth/hci_event.c        |  79 ++++++++++++++----
 net/bluetooth/hci_sync.c         |  90 ++++++++++++++------
 net/bluetooth/iso.c              |   2 +-
 6 files changed, 217 insertions(+), 99 deletions(-)

-- 
2.40.1

[PATCH 1/6] Bluetooth: ISO: fix maximum number of CIS in Set CIG Parameters

[Pauli Virtanen]

The maximum CIS_Count is 0x1f (Core v5.3 Vol 4 Part E Sec 7.8.97).

Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 1f906f8508bc..7b1a83ec50ae 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -788,7 +788,7 @@ struct iso_list_data {
 	int count;
 	struct {
 		struct hci_cp_le_set_cig_params cp;
-		struct hci_cis_params cis[0x11];
+		struct hci_cis_params cis[0x1f];
 	} pdu;
 };
 
@@ -1815,7 +1815,8 @@ static bool hci_le_set_cig_params(struct hci_conn *conn, struct bt_iso_qos *qos)
 	}
 
 	/* Reprogram all CIS(s) with the same CIG */
-	for (data.cig = qos->ucast.cig, data.cis = 0x00; data.cis < 0x11;
+	for (data.cig = qos->ucast.cig, data.cis = 0x00;
+	     data.cis < ARRAY_SIZE(data.pdu.cis);
 	     data.cis++) {
 		data.count = 0;
 
-- 
2.40.1

RE: LE Set CIG Parameters / Create CIS fixes

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=751684

---Test result---

Test Summary:
CheckPatch                    PASS      5.24 seconds
GitLint                       PASS      1.85 seconds
SubjectPrefix                 PASS      0.61 seconds
BuildKernel                   PASS      37.62 seconds
CheckAllWarning               PASS      40.98 seconds
CheckSparse                   WARNING   46.29 seconds
CheckSmatch                   WARNING   126.89 seconds
BuildKernel32                 PASS      36.43 seconds
TestRunnerSetup               PASS      517.97 seconds
TestRunner_l2cap-tester       PASS      19.01 seconds
TestRunner_iso-tester         PASS      25.59 seconds
TestRunner_bnep-tester        PASS      6.55 seconds
TestRunner_mgmt-tester        PASS      129.05 seconds
TestRunner_rfcomm-tester      PASS      10.34 seconds
TestRunner_sco-tester         PASS      9.55 seconds
TestRunner_ioctl-tester       PASS      11.24 seconds
TestRunner_mesh-tester        PASS      8.29 seconds
TestRunner_smp-tester         PASS      9.34 seconds
TestRunner_userchan-tester    PASS      6.83 seconds
IncrementalBuild              PASS      90.25 seconds

Details
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):


---
Regards,
Linux Bluetooth

Re: [PATCH 1/6] Bluetooth: ISO: fix maximum number of CIS in Set CIG Parameters

[Pauli Virtanen]

su, 2023-05-28 kello 17:44 +0000, Pauli Virtanen kirjoitti:
> The maximum CIS_Count is 0x1f (Core v5.3 Vol 4 Part E Sec 7.8.97).
> 
> Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections")
> Signed-off-by: Pauli Virtanen <pav@iki.fi>
> ---
>  net/bluetooth/hci_conn.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 1f906f8508bc..7b1a83ec50ae 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -788,7 +788,7 @@ struct iso_list_data {
>  	int count;
>  	struct {
>  		struct hci_cp_le_set_cig_params cp;
> -		struct hci_cis_params cis[0x11];
> +		struct hci_cis_params cis[0x1f];
>  	} pdu;
>  };
>  
> @@ -1815,7 +1815,8 @@ static bool hci_le_set_cig_params(struct hci_conn *conn, struct bt_iso_qos *qos)
>  	}
>  
>  	/* Reprogram all CIS(s) with the same CIG */
> -	for (data.cig = qos->ucast.cig, data.cis = 0x00; data.cis < 0x11;
> +	for (data.cig = qos->ucast.cig, data.cis = 0x00;
> +	     data.cis < ARRAY_SIZE(data.pdu.cis);
>  	     data.cis++) {
>  		data.count = 0;
>  

Probably should also have cleaned up this loop while at it, the command
takes 0x1f configurations at most, but CIS IDs are <= 0xef. For v2...

-- 
Pauli Virtanen

[PATCH 2/6] Bluetooth: ISO: use hci_sync for setting CIG parameters

[Pauli Virtanen]

When reconfiguring CIG after disconnection of the last CIS, LE Remove
CIG shall be sent before LE Set CIG Parameters.  Otherwise, it fails
because CIG is in the inactive state and not configurable (Core v5.3
Vol 6 Part B Sec. 4.5.14.3). This ordering is currently wrong under
suitable timing conditions, because LE Remove CIG is sent via the
hci_sync queue and may be delayed, but Set CIG Parameters is via
hci_send_cmd.

Make the ordering well-defined by sending also Set CIG Parameters via
hci_sync.

Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c | 47 +++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 7b1a83ec50ae..182dba4a19b5 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -775,6 +775,11 @@ static void le_conn_timeout(struct work_struct *work)
 	hci_abort_conn(conn, HCI_ERROR_REMOTE_USER_TERM);
 }
 
+struct iso_cig_params {
+	struct hci_cp_le_set_cig_params cp;
+	struct hci_cis_params cis[0x1f];
+};
+
 struct iso_list_data {
 	union {
 		u8  cig;
@@ -786,10 +791,7 @@ struct iso_list_data {
 		u16 sync_handle;
 	};
 	int count;
-	struct {
-		struct hci_cp_le_set_cig_params cp;
-		struct hci_cis_params cis[0x1f];
-	} pdu;
+	struct iso_cig_params pdu;
 };
 
 static void bis_list(struct hci_conn *conn, void *data)
@@ -1762,10 +1764,33 @@ static int hci_le_create_big(struct hci_conn *conn, struct bt_iso_qos *qos)
 	return hci_send_cmd(hdev, HCI_OP_LE_CREATE_BIG, sizeof(cp), &cp);
 }
 
+static void set_cig_params_complete(struct hci_dev *hdev, void *data, int err)
+{
+	struct iso_cig_params *pdu = data;
+
+	bt_dev_dbg(hdev, "");
+
+	if (err)
+		bt_dev_err(hdev, "Unable to set CIG parameters: %d", err);
+
+	kfree(pdu);
+}
+
+static int set_cig_params_sync(struct hci_dev *hdev, void *data)
+{
+	struct iso_cig_params *pdu = data;
+	u32 plen;
+
+	plen = sizeof(pdu->cp) + pdu->cp.num_cis * sizeof(pdu->cis[0]);
+	return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_CIG_PARAMS, plen, pdu,
+				     HCI_CMD_TIMEOUT);
+}
+
 static bool hci_le_set_cig_params(struct hci_conn *conn, struct bt_iso_qos *qos)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct iso_list_data data;
+	struct iso_cig_params *pdu;
 
 	memset(&data, 0, sizeof(data));
 
@@ -1836,12 +1861,18 @@ static bool hci_le_set_cig_params(struct hci_conn *conn, struct bt_iso_qos *qos)
 	if (qos->ucast.cis == BT_ISO_QOS_CIS_UNSET || !data.pdu.cp.num_cis)
 		return false;
 
-	if (hci_send_cmd(hdev, HCI_OP_LE_SET_CIG_PARAMS,
-			 sizeof(data.pdu.cp) +
-			 (data.pdu.cp.num_cis * sizeof(*data.pdu.cis)),
-			 &data.pdu) < 0)
+	pdu = kzalloc(sizeof(*pdu), GFP_KERNEL);
+	if (!pdu)
 		return false;
 
+	memcpy(pdu, &data.pdu, sizeof(*pdu));
+
+	if (hci_cmd_sync_queue(hdev, set_cig_params_sync, pdu,
+			       set_cig_params_complete) < 0) {
+		kfree(pdu);
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.40.1

[PATCH 3/6] Bluetooth: ISO: don't try to do Set CIG Parameters if CIG known busy

[Pauli Virtanen]

Don't try to do set CIG parameters if we know the CIG is busy, because
we drop all connections on command failure in hci_cc_le_set_cig_params.
Trying to connect new CIS while CIG is not configurable shall not cause
other active CIS connections to fail.

Check the CIG is not busy before trying to emit the Set CIG Parameters
command.

Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 182dba4a19b5..47a8ec796d97 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1815,6 +1815,19 @@ static bool hci_le_set_cig_params(struct hci_conn *conn, struct bt_iso_qos *qos)
 
 		/* Update CIG */
 		qos->ucast.cig = data.cig;
+	} else {
+		/* If CIG is busy, fail early because cmd error closes conns */
+		data.cig = qos->ucast.cig;
+		data.count = 0;
+
+		hci_conn_hash_list_state(hdev, find_cis, ISO_LINK,
+					 BT_CONNECT, &data);
+		if (data.count)
+			return false;
+		hci_conn_hash_list_state(hdev, find_cis, ISO_LINK,
+					 BT_CONNECTED, &data);
+		if (data.count)
+			return false;
 	}
 
 	data.pdu.cp.cig_id = qos->ucast.cig;
-- 
2.40.1

[PATCH 4/6] Bluetooth: ISO: don't try to remove CIG if there are bound CIS left

[Pauli Virtanen]

Consider existing BOUND & CONNECT state CIS to block CIG removal.
Otherwise, under suitable timing conditions we may attempt to remove CIG
while Create CIS is pending, which fails.

Fixes: 26afbd826ee3 ("Bluetooth: Add initial implementation of CIS connections")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 net/bluetooth/hci_conn.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 47a8ec796d97..e8a4f2825201 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -974,6 +974,8 @@ static void cis_cleanup(struct hci_conn *conn)
 	/* Check if ISO connection is a CIS and remove CIG if there are
 	 * no other connections using it.
 	 */
+	hci_conn_hash_list_state(hdev, find_cis, ISO_LINK, BT_BOUND, &d);
+	hci_conn_hash_list_state(hdev, find_cis, ISO_LINK, BT_CONNECT, &d);
 	hci_conn_hash_list_state(hdev, find_cis, ISO_LINK, BT_CONNECTED, &d);
 	if (d.count)
 		return;
-- 
2.40.1

[PATCH 5/6] Bluetooth: ISO: use correct CIS order in Set CIG Parameters event

[Pauli Virtanen]

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]);
+
 	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

Re: [PATCH 5/6] Bluetooth: ISO: use correct CIS order in Set CIG Parameters event

[Luiz Augusto von Dentz]

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
>


-- 
Luiz Augusto von Dentz

[PATCH 6/6] Bluetooth: ISO: do not emit new LE Create CIS if previous is pending

[Pauli Virtanen]

LE Create CIS command shall not be sent before all CIS Established
events from its previous invocation have been processed. Currently it is
sent via hci_sync but that only waits for the first event, but there can
be multiple.

Make it wait for all events, and simplify the CIS creation as follows:

Add new flag HCI_CONN_CREATE_CIS, which is set if Create CIS has been
sent for the connection but it is not yet completed.

Make BT_CONNECT state to mean the connection wants Create CIS.

On events after which new Create CIS may need to be sent, send it if
possible and some connections need it. These events are:
hci_connect_cis, iso_connect_cfm, hci_cs_le_create_cis,
hci_le_cis_estabilished_evt.

The Create CIS status/completion events shall queue new Create CIS only
if at least one of the connections transitions away from BT_CONNECT, so
that we don't loop if controller is sending bogus events.

This fixes sending multiple CIS Create for the same CIS in the
"ISO AC 6(i) - Success" BlueZ test case:

< HCI Command: LE Create Co.. (0x08|0x0064) plen 9  #129 [hci0]
        Number of CIS: 2
        CIS Handle: 257
        ACL Handle: 42
        CIS Handle: 258
        ACL Handle: 42
> HCI Event: Command Status (0x0f) plen 4           #130 [hci0]
      LE Create Connected Isochronous Stream (0x08|0x0064) ncmd 1
        Status: Success (0x00)
> HCI Event: LE Meta Event (0x3e) plen 29           #131 [hci0]
      LE Connected Isochronous Stream Established (0x19)
        Status: Success (0x00)
        Connection Handle: 257
        ...
< HCI Command: LE Setup Is.. (0x08|0x006e) plen 13  #132 [hci0]
        ...
> HCI Event: Command Complete (0x0e) plen 6         #133 [hci0]
      LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1
        ...
< HCI Command: LE Create Co.. (0x08|0x0064) plen 5  #134 [hci0]
        Number of CIS: 1
        CIS Handle: 258
        ACL Handle: 42
> HCI Event: Command Status (0x0f) plen 4           #135 [hci0]
      LE Create Connected Isochronous Stream (0x08|0x0064) ncmd 1
        Status: ACL Connection Already Exists (0x0b)
> HCI Event: LE Meta Event (0x3e) plen 29           #136 [hci0]
      LE Connected Isochronous Stream Established (0x19)
        Status: Success (0x00)
        Connection Handle: 258
        ...

Fixes: c09b80be6ffc ("Bluetooth: hci_conn: Fix not waiting for HCI_EVT_LE_CIS_ESTABLISHED")
Signed-off-by: Pauli Virtanen <pav@iki.fi>
---
 include/net/bluetooth/hci_core.h |  4 +-
 include/net/bluetooth/hci_sync.h |  2 +-
 net/bluetooth/hci_conn.c         | 74 +++++++++++---------------
 net/bluetooth/hci_event.c        | 27 +++++++---
 net/bluetooth/hci_sync.c         | 90 ++++++++++++++++++++++----------
 net/bluetooth/iso.c              |  2 +-
 6 files changed, 118 insertions(+), 81 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9f37326c1c05..e3d7a18a9c04 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -975,6 +975,7 @@ enum {
 	HCI_CONN_SCANNING,
 	HCI_CONN_AUTH_FAILURE,
 	HCI_CONN_PER_ADV,
+	HCI_CONN_CREATE_CIS,
 };
 
 static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
@@ -1324,7 +1325,8 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason);
 bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
 void hci_sco_setup(struct hci_conn *conn, __u8 status);
 bool hci_iso_setup_path(struct hci_conn *conn);
-int hci_le_create_cis(struct hci_conn *conn);
+int hci_le_create_cis_pending(struct hci_dev *hdev);
+int hci_conn_check_create_cis(struct hci_conn *conn);
 
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
 			      u8 role);
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index 2495be4d8b82..b516a0f4a55b 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -124,7 +124,7 @@ int hci_abort_conn_sync(struct hci_dev *hdev, struct hci_conn *conn, u8 reason);
 
 int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn);
 
-int hci_le_create_cis_sync(struct hci_dev *hdev, struct hci_conn *conn);
+int hci_le_create_cis_sync(struct hci_dev *hdev);
 
 int hci_le_remove_cig_sync(struct hci_dev *hdev, u8 handle);
 
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e8a4f2825201..086b9e128bf9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1985,59 +1985,47 @@ bool hci_iso_setup_path(struct hci_conn *conn)
 	return true;
 }
 
+int hci_conn_check_create_cis(struct hci_conn *conn)
+{
+	if (conn->type != ISO_LINK || !bacmp(&conn->dst, BDADDR_ANY))
+		return -EINVAL;
+
+	if (!conn->parent || conn->parent->state != BT_CONNECTED ||
+	    conn->state != BT_CONNECT || conn->handle == HCI_CONN_HANDLE_UNSET)
+		return 1;
+
+	return 0;
+}
+
 static int hci_create_cis_sync(struct hci_dev *hdev, void *data)
 {
-	return hci_le_create_cis_sync(hdev, data);
+	return hci_le_create_cis_sync(hdev);
 }
 
-int hci_le_create_cis(struct hci_conn *conn)
+int hci_le_create_cis_pending(struct hci_dev *hdev)
 {
-	struct hci_conn *cis;
-	struct hci_link *link, *t;
-	struct hci_dev *hdev = conn->hdev;
-	int err;
+	struct hci_conn *conn;
+	bool pending = false;
 
-	bt_dev_dbg(hdev, "hcon %p", conn);
+	rcu_read_lock();
 
-	switch (conn->type) {
-	case LE_LINK:
-		if (conn->state != BT_CONNECTED || list_empty(&conn->link_list))
-			return -EINVAL;
-
-		cis = NULL;
-
-		/* hci_conn_link uses list_add_tail_rcu so the list is in
-		 * the same order as the connections are requested.
-		 */
-		list_for_each_entry_safe(link, t, &conn->link_list, list) {
-			if (link->conn->state == BT_BOUND) {
-				err = hci_le_create_cis(link->conn);
-				if (err)
-					return err;
-
-				cis = link->conn;
-			}
+	list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) {
+		if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags)) {
+			rcu_read_unlock();
+			return -EBUSY;
 		}
 
-		return cis ? 0 : -EINVAL;
-	case ISO_LINK:
-		cis = conn;
-		break;
-	default:
-		return -EINVAL;
+		if (!hci_conn_check_create_cis(conn))
+			pending = true;
 	}
 
-	if (cis->state == BT_CONNECT)
+	rcu_read_unlock();
+
+	if (!pending)
 		return 0;
 
 	/* Queue Create CIS */
-	err = hci_cmd_sync_queue(hdev, hci_create_cis_sync, cis, NULL);
-	if (err)
-		return err;
-
-	cis->state = BT_CONNECT;
-
-	return 0;
+	return hci_cmd_sync_queue(hdev, hci_create_cis_sync, NULL, NULL);
 }
 
 static void hci_iso_qos_setup(struct hci_dev *hdev, struct hci_conn *conn,
@@ -2273,11 +2261,9 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
 		return NULL;
 	}
 
-	/* If LE is already connected and CIS handle is already set proceed to
-	 * Create CIS immediately.
-	 */
-	if (le->state == BT_CONNECTED && cis->handle != HCI_CONN_HANDLE_UNSET)
-		hci_le_create_cis(cis);
+	cis->state = BT_CONNECT;
+
+	hci_le_create_cis_pending(hdev);
 
 	return cis;
 }
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 71d8f1442287..4a9ef9610dc8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3809,6 +3809,7 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
 	u16 handles[0x1f];
 	int num_handles;
 	u8 status = rp->status;
+	bool pending = false;
 	int i;
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", rp->status);
@@ -3864,13 +3865,8 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
 			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();
-			}
+			if (conn->state == BT_CONNECT)
+				pending = true;
 
 			break;
 		}
@@ -3881,6 +3877,9 @@ static u8 hci_cc_le_set_cig_params(struct hci_dev *hdev, void *data,
 	rcu_read_unlock();
 
 unlock:
+	if (pending)
+		hci_le_create_cis_pending(hdev);
+
 	hci_dev_unlock(hdev);
 
 	return rp->status;
@@ -4246,6 +4245,7 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, void *data,
 static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status)
 {
 	struct hci_cp_le_create_cis *cp;
+	bool pending = false;
 	int i;
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", status);
@@ -4268,12 +4268,18 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status)
 
 		conn = hci_conn_hash_lookup_handle(hdev, handle);
 		if (conn) {
+			if (test_and_clear_bit(HCI_CONN_CREATE_CIS,
+					       &conn->flags))
+				pending = true;
 			conn->state = BT_CLOSED;
 			hci_connect_cfm(conn, status);
 			hci_conn_del(conn);
 		}
 	}
 
+	if (pending)
+		hci_le_create_cis_pending(hdev);
+
 	hci_dev_unlock(hdev);
 }
 
@@ -6816,6 +6822,7 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
 {
 	struct hci_evt_le_cis_established *ev = data;
 	struct hci_conn *conn;
+	bool pending = false;
 	u16 handle = __le16_to_cpu(ev->handle);
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
@@ -6837,6 +6844,8 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	pending = test_and_clear_bit(HCI_CONN_CREATE_CIS, &conn->flags);
+
 	if (conn->role == HCI_ROLE_SLAVE) {
 		__le32 interval;
 
@@ -6862,10 +6871,14 @@ static void hci_le_cis_estabilished_evt(struct hci_dev *hdev, void *data,
 		goto unlock;
 	}
 
+	conn->state = BT_CLOSED;
 	hci_connect_cfm(conn, ev->status);
 	hci_conn_del(conn);
 
 unlock:
+	if (pending)
+		hci_le_create_cis_pending(hdev);
+
 	hci_dev_unlock(hdev);
 }
 
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a59695f04c25..dd011ddc1aff 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6170,56 +6170,92 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
 	return err;
 }
 
-int hci_le_create_cis_sync(struct hci_dev *hdev, struct hci_conn *conn)
+int hci_le_create_cis_sync(struct hci_dev *hdev)
 {
 	struct {
 		struct hci_cp_le_create_cis cp;
 		struct hci_cis cis[0x1f];
 	} cmd;
-	u8 cig;
-	struct hci_conn *hcon = conn;
+	struct hci_conn *conn;
+	u8 cig = BT_ISO_QOS_CIG_UNSET;
+
+	/* The spec allows only one pending LE Create CIS command at a time. If
+	 * the command is pending now, don't do anything. We check for pending
+	 * connections after each CIS Established event.
+	 *
+	 * BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
+	 * page 2566:
+	 *
+	 * If the Host issues this command before all the
+	 * HCI_LE_CIS_Established events from the previous use of the
+	 * command have been generated, the Controller shall return the
+	 * error code Command Disallowed (0x0C).
+	 *
+	 * BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
+	 * page 2567:
+	 *
+	 * When the Controller receives the HCI_LE_Create_CIS command, the
+	 * Controller sends the HCI_Command_Status event to the Host. An
+	 * HCI_LE_CIS_Established event will be generated for each CIS when it
+	 * is established or if it is disconnected or considered lost before
+	 * being established; until all the events are generated, the command
+	 * remains pending.
+	 */
 
 	memset(&cmd, 0, sizeof(cmd));
-	cmd.cis[0].acl_handle = cpu_to_le16(conn->parent->handle);
-	cmd.cis[0].cis_handle = cpu_to_le16(conn->handle);
-	cmd.cp.num_cis++;
-	cig = conn->iso_qos.ucast.cig;
 
 	hci_dev_lock(hdev);
 
 	rcu_read_lock();
 
+	/* Wait until previous Create CIS has completed */
+	list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) {
+		if (test_bit(HCI_CONN_CREATE_CIS, &conn->flags))
+			goto done;
+	}
+
+	/* Find CIG with all CIS ready */
+	list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) {
+		struct hci_conn *link;
+
+		if (hci_conn_check_create_cis(conn))
+			continue;
+
+		cig = conn->iso_qos.ucast.cig;
+
+		list_for_each_entry_rcu(link, &hdev->conn_hash.list, list) {
+			if (hci_conn_check_create_cis(link) > 0 &&
+			    link->iso_qos.ucast.cig == cig &&
+			    link->state != BT_CONNECTED) {
+				cig = BT_ISO_QOS_CIG_UNSET;
+				break;
+			}
+		}
+
+		if (cig != BT_ISO_QOS_CIG_UNSET)
+			break;
+	}
+
+	if (cig == BT_ISO_QOS_CIG_UNSET)
+		goto done;
+
 	list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) {
 		struct hci_cis *cis = &cmd.cis[cmd.cp.num_cis];
 
-		if (conn == hcon || conn->type != ISO_LINK ||
-		    conn->state == BT_CONNECTED ||
+		if (hci_conn_check_create_cis(conn) ||
 		    conn->iso_qos.ucast.cig != cig)
 			continue;
 
-		/* Check if all CIS(s) belonging to a CIG are ready */
-		if (!conn->parent || conn->parent->state != BT_CONNECTED ||
-		    conn->state != BT_CONNECT) {
-			cmd.cp.num_cis = 0;
-			break;
-		}
-
-		/* Group all CIS with state BT_CONNECT since the spec don't
-		 * allow to send them individually:
-		 *
-		 * BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
-		 * page 2566:
-		 *
-		 * If the Host issues this command before all the
-		 * HCI_LE_CIS_Established events from the previous use of the
-		 * command have been generated, the Controller shall return the
-		 * error code Command Disallowed (0x0C).
-		 */
+		set_bit(HCI_CONN_CREATE_CIS, &conn->flags);
 		cis->acl_handle = cpu_to_le16(conn->parent->handle);
 		cis->cis_handle = cpu_to_le16(conn->handle);
 		cmd.cp.num_cis++;
+
+		if (cmd.cp.num_cis >= ARRAY_SIZE(cmd.cis))
+			break;
 	}
 
+done:
 	rcu_read_unlock();
 
 	hci_dev_unlock(hdev);
diff --git a/net/bluetooth/iso.c b/net/bluetooth/iso.c
index 34d55a85d8f6..c8510aba64fc 100644
--- a/net/bluetooth/iso.c
+++ b/net/bluetooth/iso.c
@@ -1667,7 +1667,7 @@ static void iso_connect_cfm(struct hci_conn *hcon, __u8 status)
 		}
 
 		/* Create CIS if pending */
-		hci_le_create_cis(hcon);
+		hci_le_create_cis_pending(hcon->hdev);
 		return;
 	}
 
-- 
2.40.1