linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect
@ 2023-05-28 17:39 Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 2/4] btdev: fix inactive CIG configurable status Pauli Virtanen
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Pauli Virtanen @ 2023-05-28 17:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Add test for reconnecting a CIG with two CIS, either both with same peer
or with different peers.

ISO Reconnect AC 6(i) - Success
ISO Reconnect AC 6(ii) - Success
---

Notes:
    The intent here was to trigger the kernel to send Remove CIG and Set CIG
    Parameters in the wrong order, but it doesn't hit the right timing in
    the emulator. It occurs on real hardware when BlueZ recreates multiple
    CIS at the same time.
    
    This can occur because Remove CIG is sent via hci_sync queue and
    queueing its request may be delayed until hci_cmd_sync_work runs,
    whereas Set CIG Parameters request is queued directly via hci_send_cmd.
    
    However, these tests were useful to have, as they hit Create CIS timing
    issues if Set CIG Parameters is moved to hci_sync.

 tools/iso-tester.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/tools/iso-tester.c b/tools/iso-tester.c
index 63c37bd52..776d87fc1 100644
--- a/tools/iso-tester.c
+++ b/tools/iso-tester.c
@@ -839,6 +839,15 @@ static const struct iso_client_data connect_ac_6i = {
 	.defer = true,
 };
 
+static const struct iso_client_data reconnect_ac_6i = {
+	.qos = AC_6i_1,
+	.qos_2 = AC_6i_2,
+	.expect_err = 0,
+	.mcis = true,
+	.defer = true,
+	.disconnect = true,
+};
+
 static const struct iso_client_data connect_ac_6ii = {
 	.qos = AC_6ii_1,
 	.qos_2 = AC_6ii_2,
@@ -847,6 +856,15 @@ static const struct iso_client_data connect_ac_6ii = {
 	.defer = true,
 };
 
+static const struct iso_client_data reconnect_ac_6ii = {
+	.qos = AC_6ii_1,
+	.qos_2 = AC_6ii_2,
+	.expect_err = 0,
+	.mcis = true,
+	.defer = true,
+	.disconnect = true,
+};
+
 static const struct iso_client_data connect_ac_7i = {
 	.qos = AC_7i_1,
 	.qos_2 = AC_7i_2,
@@ -1626,7 +1644,7 @@ static void iso_send(struct test_data *data, GIOChannel *io)
 		iso_recv(data, io);
 }
 
-static void setup_connect(struct test_data *data, uint8_t num, GIOFunc func);
+static void test_connect(const void *test_data);
 static gboolean iso_connect_cb(GIOChannel *io, GIOCondition cond,
 							gpointer user_data);
 
@@ -1642,7 +1660,7 @@ static gboolean iso_disconnected(GIOChannel *io, GIOCondition cond,
 
 		if (data->reconnect) {
 			data->reconnect = false;
-			setup_connect(data, 0, iso_connect_cb);
+			test_connect(data->test_data);
 			return FALSE;
 		}
 
@@ -1885,17 +1903,12 @@ static void test_connect(const void *test_data)
 	setup_connect_many(data, n, num, func);
 }
 
-static void setup_reconnect(struct test_data *data, uint8_t num, GIOFunc func)
-{
-	data->reconnect = true;
-	setup_connect(data, num, func);
-}
-
 static void test_reconnect(const void *test_data)
 {
 	struct test_data *data = tester_get_data();
 
-	setup_reconnect(data, 0, iso_connect_cb);
+	data->reconnect = true;
+	test_connect(test_data);
 }
 
 static void test_defer(const void *test_data)
@@ -2410,6 +2423,14 @@ int main(int argc, char *argv[])
 							setup_powered,
 							test_connect2_seq);
 
+	test_iso2("ISO Reconnect AC 6(i) - Success", &reconnect_ac_6i,
+							setup_powered,
+							test_reconnect);
+
+	test_iso2("ISO Reconnect AC 6(ii) - Success", &reconnect_ac_6ii,
+							setup_powered,
+							test_reconnect);
+
 	test_iso("ISO Broadcaster - Success", &bcast_16_2_1_send, setup_powered,
 							test_bcast);
 	test_iso("ISO Broadcaster Encrypted - Success", &bcast_enc_16_2_1_send,
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH BlueZ 2/4] btdev: fix inactive CIG configurable status
  2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
@ 2023-05-28 17:39 ` Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 3/4] btdev: check LE Create CIS error conditions Pauli Virtanen
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Pauli Virtanen @ 2023-05-28 17:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

CIG shall not be configurable after the first Create CIS until Remove
CIG is issued (Core v5.3 Vol 6 Part B Sec. 4.5.14.3).  We currently have
it configurable in the inactive state (Create CIS done and all CIS
closed), which is incorrect.

Track CIG state and allow reconfigure only in nonexistent/configured
state, i.e., when no CIS have been created yet.
---
 emulator/btdev.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/emulator/btdev.c b/emulator/btdev.c
index f9260511a..98d7af99e 100644
--- a/emulator/btdev.c
+++ b/emulator/btdev.c
@@ -105,6 +105,7 @@ struct le_ext_adv {
 struct le_cig {
 	struct bt_hci_cmd_le_set_cig_params params;
 	struct bt_hci_cis_params cis[CIS_SIZE];
+	bool activated;
 } __attribute__ ((packed));
 
 struct btdev {
@@ -5864,29 +5865,24 @@ static int cmd_set_cig_params(struct btdev *dev, const void *data,
 		goto done;
 	}
 
+	/* BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
+	 * page 2553
+	 *
+	 * If the Host issues this command when the CIG is not in the
+	 * configurable state, the Controller shall return the error
+	 * code Command Disallowed (0x0C).
+	 */
+	if (dev->le_cig[cig_idx].activated) {
+		rsp.params.status = BT_HCI_ERR_COMMAND_DISALLOWED;
+		goto done;
+	}
+
 	rsp.params.status = BT_HCI_ERR_SUCCESS;
 	rsp.params.cig_id = cmd->cig_id;
 
 	for (i = 0; i < cmd->num_cis; i++) {
-		struct btdev_conn *iso;
-
 		rsp.params.num_handles++;
 		rsp.handle[i] = cpu_to_le16(make_cis_handle(cig_idx, i));
-
-		/* BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
-		 * page 2553
-		 *
-		 * If the Host issues this command when the CIG is not in the
-		 * configurable state, the Controller shall return the error
-		 * code Command Disallowed (0x0C).
-		 */
-		iso = queue_find(dev->conns, match_handle,
-				UINT_TO_PTR(le16_to_cpu(rsp.handle[i])));
-		if (iso) {
-			rsp.params.status = BT_HCI_ERR_COMMAND_DISALLOWED;
-			i = 0;
-			goto done;
-		}
 	}
 
 	memcpy(&dev->le_cig[cig_idx], data, len);
@@ -6006,6 +6002,8 @@ static int cmd_create_cis_complete(struct btdev *dev, const void *data,
 		evt.cig_id = le_cig->params.cig_id;
 		evt.cis_id = le_cig->cis[cis_idx].cis_id;
 
+		le_cig->activated = true;
+
 		le_meta_event(iso->link->dev, BT_HCI_EVT_LE_CIS_REQ, &evt,
 					sizeof(evt));
 	}
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH BlueZ 3/4] btdev: check LE Create CIS error conditions
  2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 2/4] btdev: fix inactive CIG configurable status Pauli Virtanen
@ 2023-05-28 17:39 ` Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 4/4] test-runner: enable no_hash_pointers=1 Pauli Virtanen
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Pauli Virtanen @ 2023-05-28 17:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Check LE Create CIS input parameter are valid and return correct status
codes (Core v5.3 Vol 4 Part E Sec. 7.8.99).

On current bluetooth-next kernel, this results to

ISO AC 6(i) - Success                                Failed
ISO AC 7(i) - Success                                Failed
ISO AC 8(i) - Success                                Failed
ISO AC 9(i) - Success                                Failed
ISO AC 11(i) - Success                               Failed

as in these tests the kernel is sending new Create CIS commands before
it has seen all events from the previous, which is not allowed:

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

The emulator uses Already Exists error code here, not Command
Disallowed, since the Established events are logically generated
immediately after the first status event, even though the kernel hasn't
yet processed them.
---
 emulator/btdev.c | 38 ++++++++++++++++++++++++++++++++++++++
 monitor/bt.h     |  1 +
 2 files changed, 39 insertions(+)

diff --git a/emulator/btdev.c b/emulator/btdev.c
index 98d7af99e..08506c66e 100644
--- a/emulator/btdev.c
+++ b/emulator/btdev.c
@@ -5903,6 +5903,38 @@ static int cmd_set_cig_params_test(struct btdev *dev, const void *data,
 
 static int cmd_create_cis(struct btdev *dev, const void *data, uint8_t len)
 {
+	const struct bt_hci_cmd_le_create_cis *cmd = data;
+	int i, j;
+
+	for (i = 0; i < cmd->num_cis; i++) {
+		const struct bt_hci_cis *cis = &cmd->cis[i];
+		struct btdev_conn *acl;
+		struct btdev_conn *iso;
+		int cig_idx, cis_idx;
+
+		/* Check for errors (Core v5.3 Vol 4 Part E Sec. 7.8.99) */
+		for (j = 0; j < i; j++)
+			if (cis->cis_handle == cmd->cis[j].cis_handle)
+				return -EINVAL;
+
+		cig_idx = parse_cis_handle(le16_to_cpu(cis->cis_handle),
+								&cis_idx);
+		if (cig_idx < 0)
+			return -ENOENT;
+		if (cis_idx >= dev->le_cig[cig_idx].params.num_cis)
+			return -ENOENT;
+
+		acl = queue_find(dev->conns, match_handle,
+				 UINT_TO_PTR(le16_to_cpu(cis->acl_handle)));
+		if (!acl)
+			return -ENOENT;
+
+		iso = queue_find(dev->conns, match_handle,
+				 UINT_TO_PTR(le16_to_cpu(cis->cis_handle)));
+		if (iso)
+			return -EEXIST;
+	}
+
 	cmd_status(dev, BT_HCI_ERR_SUCCESS, BT_HCI_CMD_LE_CREATE_CIS);
 
 	return 0;
@@ -7142,6 +7174,12 @@ static const struct btdev_cmd *run_cmd(struct btdev *btdev,
 	case -EPERM:
 		status = BT_HCI_ERR_COMMAND_DISALLOWED;
 		break;
+	case -EEXIST:
+		status = BT_HCI_ERR_CONN_ALREADY_EXISTS;
+		break;
+	case -ENOENT:
+		status = BT_HCI_ERR_UNKNOWN_CONN_ID;
+		break;
 	default:
 		status = BT_HCI_ERR_UNSPECIFIED_ERROR;
 		break;
diff --git a/monitor/bt.h b/monitor/bt.h
index b99ada0b2..37fcdaeaa 100644
--- a/monitor/bt.h
+++ b/monitor/bt.h
@@ -3713,6 +3713,7 @@ struct bt_hci_evt_le_big_info_adv_report {
 #define BT_HCI_ERR_AUTH_FAILURE			0x05
 #define BT_HCI_ERR_PIN_OR_KEY_MISSING		0x06
 #define BT_HCI_ERR_MEM_CAPACITY_EXCEEDED	0x07
+#define BT_HCI_ERR_CONN_ALREADY_EXISTS		0x0b
 #define BT_HCI_ERR_COMMAND_DISALLOWED		0x0c
 #define BT_HCI_ERR_UNSUPPORTED_FEATURE		0x11
 #define BT_HCI_ERR_INVALID_PARAMETERS		0x12
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH BlueZ 4/4] test-runner: enable no_hash_pointers=1
  2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 2/4] btdev: fix inactive CIG configurable status Pauli Virtanen
  2023-05-28 17:39 ` [PATCH BlueZ 3/4] btdev: check LE Create CIS error conditions Pauli Virtanen
@ 2023-05-28 17:39 ` Pauli Virtanen
  2023-05-28 20:07 ` [BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect bluez.test.bot
  2023-05-30 18:40 ` [PATCH BlueZ 1/4] " patchwork-bot+bluetooth
  4 siblings, 0 replies; 6+ messages in thread
From: Pauli Virtanen @ 2023-05-28 17:39 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Set no_hash_pointers=1 to avoid printk printing "___ptrval___" for %p.
This happens with test-runner since the kernel is not running long
enough to gather entropy and so refuses to print addresses, and makes
debugging harder.  As test-runner is only used for testing and
debugging, pointer hashing is not needed.
---
 tools/test-runner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/test-runner.c b/tools/test-runner.c
index 8b18f848a..d74bb1087 100644
--- a/tools/test-runner.c
+++ b/tools/test-runner.c
@@ -247,7 +247,7 @@ static void start_qemu(void)
 
 	snprintf(cmdline, sizeof(cmdline),
 				"console=ttyS0,115200n8 earlyprintk=serial "
-				"rootfstype=9p "
+				"no_hash_pointers=1 rootfstype=9p "
 				"rootflags=trans=virtio,version=9p2000.u "
 				"acpi=off pci=noacpi noapic quiet ro init=%s "
 				"TESTHOME=%s TESTDBUS=%u TESTDAEMON=%u "
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* RE: [BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect
  2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
                   ` (2 preceding siblings ...)
  2023-05-28 17:39 ` [PATCH BlueZ 4/4] test-runner: enable no_hash_pointers=1 Pauli Virtanen
@ 2023-05-28 20:07 ` bluez.test.bot
  2023-05-30 18:40 ` [PATCH BlueZ 1/4] " patchwork-bot+bluetooth
  4 siblings, 0 replies; 6+ messages in thread
From: bluez.test.bot @ 2023-05-28 20:07 UTC (permalink / raw)
  To: linux-bluetooth, pav

[-- Attachment #1: Type: text/plain, Size: 2394 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=751682

---Test result---

Test Summary:
CheckPatch                    PASS      1.63 seconds
GitLint                       FAIL      1.35 seconds
BuildEll                      PASS      33.10 seconds
BluezMake                     PASS      1035.93 seconds
MakeCheck                     PASS      13.28 seconds
MakeDistcheck                 PASS      193.40 seconds
CheckValgrind                 PASS      305.90 seconds
CheckSmatch                   WARNING   407.90 seconds
bluezmakeextell               PASS      124.24 seconds
IncrementalBuild              PASS      3302.71 seconds
ScanBuild                     WARNING   1262.65 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
15: B2 Line has trailing whitespace: "    "
19: B2 Line has trailing whitespace: "    "
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
emulator/btdev.c:417:29: warning: Variable length array is used.emulator/btdev.c:417:29: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
emulator/btdev.c:1080:10: warning: Although the value stored to 'conn' is used in the enclosing expression, the value is never actually read from 'conn'
        while ((conn = queue_find(dev->conns, match_handle,
                ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
emulator/btdev.c:1392:13: warning: Access to field 'dev' results in a dereference of a null pointer (loaded from variable 'conn')
        send_event(conn->dev, BT_HCI_EVT_AUTH_COMPLETE, &ev, sizeof(ev));
                   ^~~~~~~~~
2 warnings generated.



---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect
  2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
                   ` (3 preceding siblings ...)
  2023-05-28 20:07 ` [BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect bluez.test.bot
@ 2023-05-30 18:40 ` patchwork-bot+bluetooth
  4 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+bluetooth @ 2023-05-30 18:40 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hello:

This series was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Sun, 28 May 2023 17:39:13 +0000 you wrote:
> Add test for reconnecting a CIG with two CIS, either both with same peer
> or with different peers.
> 
> ISO Reconnect AC 6(i) - Success
> ISO Reconnect AC 6(ii) - Success
> ---
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=000c2012f38a
  - [BlueZ,2/4] btdev: fix inactive CIG configurable status
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=a8b927e34733
  - [BlueZ,3/4] btdev: check LE Create CIS error conditions
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=d214fe5f7522
  - [BlueZ,4/4] test-runner: enable no_hash_pointers=1
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=15eb57049b19

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-05-30 18:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-28 17:39 [PATCH BlueZ 1/4] iso-tester: Add tests for AC configuration reconnect Pauli Virtanen
2023-05-28 17:39 ` [PATCH BlueZ 2/4] btdev: fix inactive CIG configurable status Pauli Virtanen
2023-05-28 17:39 ` [PATCH BlueZ 3/4] btdev: check LE Create CIS error conditions Pauli Virtanen
2023-05-28 17:39 ` [PATCH BlueZ 4/4] test-runner: enable no_hash_pointers=1 Pauli Virtanen
2023-05-28 20:07 ` [BlueZ,1/4] iso-tester: Add tests for AC configuration reconnect bluez.test.bot
2023-05-30 18:40 ` [PATCH BlueZ 1/4] " patchwork-bot+bluetooth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).