diff mbox series

[BlueZ,1/5] iso-tester: always use DEFER_SETUP for multiple CIS in same CIG

Message ID 024df2d86c14fc811701ba27bfa576476bc9c0d6.1684682575.git.pav@iki.fi
State New
Headers show
Series [BlueZ,1/5] iso-tester: always use DEFER_SETUP for multiple CIS in same CIG | expand

Commit Message

Pauli Virtanen May 21, 2023, 3:27 p.m. UTC 
There is a race between multiple connect() for CIS in the same CIG.
connect() will both reconfigure the CIG and connect the CIS, but CIG
cannot be reconfigured once one CIS has already connected.  That these
tests pass currently relies on some timing/event ordering in the
emulator.

Connecting multiple CIS in same CIG is meant to be done using
DEFER_SETUP, so change the tests to use it, so we test the intended
usage.
---
 tools/iso-tester.c | 150 +++++++++++++++++++++++++++++++--------------
 1 file changed, 104 insertions(+), 46 deletions(-)

Comments

bluez.test.bot@gmail.com May 21, 2023, 6:11 p.m. UTC | #1 
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=749544

---Test result---

Test Summary:
CheckPatch                    FAIL      3.27 seconds
GitLint                       FAIL      2.14 seconds
BuildEll                      PASS      35.87 seconds
BluezMake                     PASS      1238.75 seconds
MakeCheck                     PASS      13.36 seconds
MakeDistcheck                 PASS      202.49 seconds
CheckValgrind                 PASS      329.93 seconds
CheckSmatch                   WARNING   459.17 seconds
bluezmakeextell               PASS      136.68 seconds
IncrementalBuild              PASS      5329.21 seconds
ScanBuild                     WARNING   1454.33 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,3/5] btdev: support multiple CIG
WARNING:PREFER_DEFINED_ATTRIBUTE_MACRO: Prefer __packed over __attribute__((packed))
#74: FILE: emulator/btdev.c:108:
+} __attribute__ ((packed));

/github/workspace/src/src/13249428.patch total: 0 errors, 1 warnings, 279 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13249428.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,5/5] iso-tester: add tests for multiple simultaneous CIG

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
17: B2 Line has trailing whitespace: "    "
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
emulator/btdev.c:416:29: warning: Variable length array is used.emulator/btdev.c:416:29: warning: Variable length array is used.
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
emulator/btdev.c:1079: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:1391: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
patchwork-bot+bluetooth@kernel.org May 22, 2023, 8:20 p.m. UTC | #2 
Hello:

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

On Sun, 21 May 2023 15:27:34 +0000 you wrote:
> There is a race between multiple connect() for CIS in the same CIG.
> connect() will both reconfigure the CIG and connect the CIS, but CIG
> cannot be reconfigured once one CIS has already connected.  That these
> tests pass currently relies on some timing/event ordering in the
> emulator.
> 
> Connecting multiple CIS in same CIG is meant to be done using
> DEFER_SETUP, so change the tests to use it, so we test the intended
> usage.
> 
> [...]

Here is the summary with links:
  - [BlueZ,1/5] iso-tester: always use DEFER_SETUP for multiple CIS in same CIG
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=a88c74e29b6e
  - [BlueZ,2/5] shared/tester: retain test failure status
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=195b9abbae0e
  - [BlueZ,3/5] btdev: support multiple CIG
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=678265f37c28
  - [BlueZ,4/5] btdev: report right reason for local Disconnect complete
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0c32cfdf9462
  - [BlueZ,5/5] iso-tester: add tests for multiple simultaneous CIG
    (no matching commit)

You are awesome, thank you!
Luiz Augusto von Dentz May 22, 2023, 8:27 p.m. UTC | #3 
Hi Pauli,

On Mon, May 22, 2023 at 1:20 PM <patchwork-bot+bluetooth@kernel.org> wrote:
>
> Hello:
>
> This series was applied to bluetooth/bluez.git (master)
> by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:
>
> On Sun, 21 May 2023 15:27:34 +0000 you wrote:
> > There is a race between multiple connect() for CIS in the same CIG.
> > connect() will both reconfigure the CIG and connect the CIS, but CIG
> > cannot be reconfigured once one CIS has already connected.  That these
> > tests pass currently relies on some timing/event ordering in the
> > emulator.
> >
> > Connecting multiple CIS in same CIG is meant to be done using
> > DEFER_SETUP, so change the tests to use it, so we test the intended
> > usage.
> >
> > [...]
>
> Here is the summary with links:
>   - [BlueZ,1/5] iso-tester: always use DEFER_SETUP for multiple CIS in same CIG
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=a88c74e29b6e
>   - [BlueZ,2/5] shared/tester: retain test failure status
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=195b9abbae0e
>   - [BlueZ,3/5] btdev: support multiple CIG
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=678265f37c28
>   - [BlueZ,4/5] btdev: report right reason for local Disconnect complete
>     https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=0c32cfdf9462
>   - [BlueZ,5/5] iso-tester: add tests for multiple simultaneous CIG
>     (no matching commit)

Ive ended up making some changes to patch 5/5 so it combines AC 1 with
AC 2, we can probably extend these tests to have more combinations of
different AC, etc, just to make sure the code can handle them
properly.
diff mbox series

Patch

diff --git a/tools/iso-tester.c b/tools/iso-tester.c
index c5c6f0aec..164cb465f 100644
--- a/tools/iso-tester.c
+++ b/tools/iso-tester.c
@@ -740,6 +740,12 @@  static const struct iso_client_data defer_16_2_1 = {
 	.defer = true,
 };
 
+static const struct iso_client_data defer_1_16_2_1 = {
+	.qos = QOS_1_16_2_1,
+	.expect_err = 0,
+	.defer = true,
+};
+
 static const struct iso_client_data connect_16_2_1_defer_send = {
 	.qos = QOS_16_2_1,
 	.expect_err = 0,
@@ -817,6 +823,7 @@  static const struct iso_client_data connect_ac_6i = {
 	.qos_2 = AC_6i_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_6ii = {
@@ -824,6 +831,7 @@  static const struct iso_client_data connect_ac_6ii = {
 	.qos_2 = AC_6ii_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_7i = {
@@ -831,6 +839,7 @@  static const struct iso_client_data connect_ac_7i = {
 	.qos_2 = AC_7i_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_7ii = {
@@ -838,6 +847,7 @@  static const struct iso_client_data connect_ac_7ii = {
 	.qos_2 = AC_7ii_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_8i = {
@@ -845,6 +855,7 @@  static const struct iso_client_data connect_ac_8i = {
 	.qos_2 = AC_8i_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_8ii = {
@@ -852,6 +863,7 @@  static const struct iso_client_data connect_ac_8ii = {
 	.qos_2 = AC_8ii_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_9i = {
@@ -859,6 +871,7 @@  static const struct iso_client_data connect_ac_9i = {
 	.qos_2 = AC_9i_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_9ii = {
@@ -866,6 +879,7 @@  static const struct iso_client_data connect_ac_9ii = {
 	.qos_2 = AC_9ii_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_11i = {
@@ -873,6 +887,7 @@  static const struct iso_client_data connect_ac_11i = {
 	.qos_2 = AC_11i_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data connect_ac_11ii = {
@@ -880,6 +895,7 @@  static const struct iso_client_data connect_ac_11ii = {
 	.qos_2 = AC_11ii_2,
 	.expect_err = 0,
 	.mcis = true,
+	.defer = true,
 };
 
 static const struct iso_client_data bcast_16_2_1_send = {
@@ -1715,13 +1731,9 @@  static gboolean iso_connect2_cb(GIOChannel *io, GIOCondition cond,
 	return iso_connect(io, cond, user_data);
 }
 
-static void setup_connect(struct test_data *data, uint8_t num, GIOFunc func)
+static int setup_sock(struct test_data *data, uint8_t num)
 {
-	const struct iso_client_data *isodata = data->test_data;
-	GIOChannel *io;
 	int sk, err;
-	char c;
-	struct pollfd pfd;
 
 	sk = create_iso_sock(data);
 	if (sk < 0) {
@@ -1729,7 +1741,8 @@  static void setup_connect(struct test_data *data, uint8_t num, GIOFunc func)
 			tester_test_abort();
 		else
 			tester_test_failed();
-		return;
+
+		return sk;
 	}
 
 	err = connect_iso_sock(data, num, sk);
@@ -1743,65 +1756,106 @@  static void setup_connect(struct test_data *data, uint8_t num, GIOFunc func)
 		else
 			tester_test_failed();
 
-		return;
+		return err;
 	}
 
-	if (isodata->defer) {
-		int defer;
-		socklen_t len;
-
-		/* Check if socket has DEFER_SETUP set */
-		len = sizeof(defer);
-		if (getsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP, &defer,
-				&len) < 0) {
-			tester_warn("getsockopt: %s (%d)", strerror(errno),
-								errno);
+	return sk;
+}
+
+static int connect_deferred(int sk)
+{
+	int defer;
+	socklen_t len;
+	struct pollfd pfd;
+	char c;
+
+	/* Check if socket has DEFER_SETUP set */
+	len = sizeof(defer);
+	if (getsockopt(sk, SOL_BLUETOOTH, BT_DEFER_SETUP, &defer,
+					&len) < 0) {
+		tester_warn("getsockopt: %s (%d)", strerror(errno),
+				errno);
+		tester_test_failed();
+		return 0;
+	}
+
+	memset(&pfd, 0, sizeof(pfd));
+	pfd.fd = sk;
+	pfd.events = POLLOUT;
+
+	if (poll(&pfd, 1, 0) < 0) {
+		tester_warn("poll: %s (%d)", strerror(errno), errno);
+		tester_test_failed();
+		return -EIO;
+	}
+
+	if (!(pfd.revents & POLLOUT)) {
+		if (read(sk, &c, 1) < 0) {
+			tester_warn("read: %s (%d)", strerror(errno),
+					errno);
 			tester_test_failed();
-			return;
+			return -EIO;
 		}
+	}
+
+	return 0;
+}
 
-		memset(&pfd, 0, sizeof(pfd));
-		pfd.fd = sk;
-		pfd.events = POLLOUT;
+static void setup_connect_many(struct test_data *data, uint8_t n, uint8_t *num,
+								GIOFunc *func)
+{
+	const struct iso_client_data *isodata = data->test_data;
+	int sk[256];
+	GIOChannel *io;
+	unsigned int i;
 
-		if (poll(&pfd, 1, 0) < 0) {
-			tester_warn("poll: %s (%d)", strerror(errno), errno);
-			tester_test_failed();
+	for (i = 0; i < n; ++i) {
+		sk[i] = setup_sock(data, num[i]);
+		if (sk[i] < 0)
 			return;
-		}
+	}
 
-		if (!(pfd.revents & POLLOUT)) {
-			if (read(sk, &c, 1) < 0) {
-				tester_warn("read: %s (%d)", strerror(errno),
-								errno);
-				tester_test_failed();
+	if (isodata->defer) {
+		for (i = 0; i < n; ++i)
+			if (connect_deferred(sk[i]) < 0)
 				return;
-			}
-		}
 	}
 
-	io = g_io_channel_unix_new(sk);
-	g_io_channel_set_close_on_unref(io, TRUE);
+	for (i = 0; i < n; ++i) {
+		io = g_io_channel_unix_new(sk[i]);
+		g_io_channel_set_close_on_unref(io, TRUE);
 
-	data->io_id[num] = g_io_add_watch(io, G_IO_OUT, func, NULL);
+		data->io_id[num[i]] = g_io_add_watch(io, G_IO_OUT, func[i],
+									NULL);
 
-	g_io_channel_unref(io);
+		g_io_channel_unref(io);
 
-	tester_print("Connect in progress");
+		tester_print("Connect %d in progress", num[i]);
 
-	data->step++;
+		data->step++;
+	}
+}
+
+static void setup_connect(struct test_data *data, uint8_t num, GIOFunc func)
+{
+	return setup_connect_many(data, 1, &num, &func);
 }
 
 static void test_connect(const void *test_data)
 {
 	struct test_data *data = tester_get_data();
 	const struct iso_client_data *isodata = test_data;
+	uint8_t n = 0;
+	GIOFunc func[2];
+	uint8_t num[2] = {0, 1};
 
-	setup_connect(data, 0, iso_connect_cb);
+	func[n++] = iso_connect_cb;
 
 	/* Check if configuration requires multiple CIS setup */
 	if (!isodata->bcast && isodata->mcis)
-		setup_connect(data, 1, iso_connect2_cb);
+		func[n++] = iso_connect2_cb;
+
+	setup_connect_many(data, n, num, func);
 }
 
 static void setup_reconnect(struct test_data *data, uint8_t num, GIOFunc func)
@@ -2066,9 +2120,10 @@  static void test_listen(const void *test_data)
 static void test_connect2(const void *test_data)
 {
 	struct test_data *data = tester_get_data();
+	uint8_t num[2] = {0, 1};
+	GIOFunc funcs[2] = {iso_connect_cb, iso_connect2_cb};
 
-	setup_connect(data, 0, iso_connect_cb);
-	setup_connect(data, 1, iso_connect2_cb);
+	setup_connect_many(data, 2, num, funcs);
 }
 
 static void test_bcast(const void *test_data)
@@ -2212,10 +2267,6 @@  int main(int argc, char *argv[])
 	test_iso_rej("ISO Connect - Reject", &connect_reject, setup_powered,
 			test_connect, BT_HCI_ERR_CONN_FAILED_TO_ESTABLISH);
 
-	test_iso2("ISO Connect2 CIG 0x01 - Success", &connect_1_16_2_1,
-							setup_powered,
-							test_connect2);
-
 	test_iso("ISO Send - Success", &connect_16_2_1_send, setup_powered,
 							test_connect);
 
@@ -2229,6 +2280,13 @@  int main(int argc, char *argv[])
 	test_iso("ISO Defer - Success", &defer_16_2_1, setup_powered,
 							test_defer);
 
+	test_iso("ISO Defer Connect - Success", &defer_16_2_1, setup_powered,
+							test_connect);
+
+	test_iso2("ISO Defer Connect2 CIG 0x01 - Success", &defer_1_16_2_1,
+							setup_powered,
+							test_connect2);
+
 	test_iso("ISO Defer Send - Success", &connect_16_2_1_defer_send,
 							setup_powered,
 							test_connect);