diff mbox series

[BlueZ,v2] btdev: Broadcast EXT_ADV packets every 200 ms

Message ID 20250212175427.131356-1-arkadiusz.bokowy@gmail.com (mailing list archive)
State New
Headers show
Series [BlueZ,v2] btdev: Broadcast EXT_ADV packets every 200 ms | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch warning CheckSparse WARNING emulator/btdev.c:449:29: warning: Variable length array is used.
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Arkadiusz Bokowy Feb. 12, 2025, 5:54 p.m. UTC 
Real BLE devices transmit LE advertisement report packages in given
intervals (typically in range between 20 ms and 10.24 s). With current
kernel module Bluetooth stack implementation it is possible that the
first LE meta packet just after enabling scanning will be lost. It is
not an issue for real devices, because more advertisement reports will
be delivered later (in given interval time).

This patch changes optimistic implementation of sending only one LE
meta packets just after enabling scanning to sending LE meta packets
in 200 ms intervals. Such behavior will better emulate real HCI and
will work around the issue of dropping the very first LE meta packet
by the kernel.
---
 emulator/btdev.c | 105 ++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 66 deletions(-)

Comments

Arkadiusz Bokowy Feb. 12, 2025, 6:10 p.m. UTC | #1 
> > No, it's not an optimization for my particular setup, but more
> > generally for CPU load. I thought that it might be better not to run
> > advertisement code too frequently. But I guess that lower values
> > should also be OK, e.g. 100 ms or 50 ms. There is one "issue" with
> > that, though.... Now, the advertisement packet will be sent one
> > interval after the advertisement was enabled. If that's indeed an
> > issue, it can be fixed by calling the callback function in the moment
> > when the timer is armed.
>
> I'd keep the initial logic of sending the advertisement immediately

I've fixed that in the v2 patch.

> Btw, make sure you run the testers to confirm you are not breaking any
> kernel testers.

I've run the test-runner to verify everything (--auto). The result
seems to be no worse than without my patch. What do I mean by that?
The case is that I was not able to run the test-runner on the vanilla
master branch with 100% success rate. There is always some failure
with my "setup". However, when checking only the tools/mgmt-tester
test, the success rate seems to be higher than without it (but it
might be just a fluke). I run it like this: "tools/test-runner -k
~/linux/arch/x86/boot/bzImage -- tools/mgmt-tester". Anyway, I can see
one test always failing - "Read Exp Feature - Success (Index None)" -
it happens with and without my patch. Also, I've verified that the
advertising broadcast code was executed (by adding some prints to that
function).

To conclude that, I'm afraid that I don't know how to run the test
suite properly... So, my verification might miss something. I would
advise you to verify that patch with your setup as well (if you can).

Regards,
Arek
bluez.test.bot@gmail.com Feb. 12, 2025, 7:17 p.m. UTC | #2 
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=933306

---Test result---

Test Summary:
CheckPatch                    PENDING   0.24 seconds
GitLint                       PENDING   0.20 seconds
BuildEll                      PASS      20.43 seconds
BluezMake                     PASS      1590.06 seconds
MakeCheck                     PASS      13.60 seconds
MakeDistcheck                 PASS      157.66 seconds
CheckValgrind                 PASS      213.77 seconds
CheckSmatch                   WARNING   285.74 seconds
bluezmakeextell               PASS      98.26 seconds
IncrementalBuild              PENDING   0.28 seconds
ScanBuild                     PASS      862.34 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
emulator/btdev.c:449:29: warning: Variable length array is used.
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Feb. 12, 2025, 10:49 p.m. UTC | #3 
Hi Arkadiusz,

On Wed, Feb 12, 2025 at 1:10 PM Arkadiusz Bokowy
<arkadiusz.bokowy@gmail.com> wrote:
>
> > > No, it's not an optimization for my particular setup, but more
> > > generally for CPU load. I thought that it might be better not to run
> > > advertisement code too frequently. But I guess that lower values
> > > should also be OK, e.g. 100 ms or 50 ms. There is one "issue" with
> > > that, though.... Now, the advertisement packet will be sent one
> > > interval after the advertisement was enabled. If that's indeed an
> > > issue, it can be fixed by calling the callback function in the moment
> > > when the timer is armed.
> >
> > I'd keep the initial logic of sending the advertisement immediately
>
> I've fixed that in the v2 patch.

Ive sent a v3 with a couple of updates so it should now use the
advertisement interval rather than just assuming 200ms would cover all
intervals.

> > Btw, make sure you run the testers to confirm you are not breaking any
> > kernel testers.
>
> I've run the test-runner to verify everything (--auto). The result
> seems to be no worse than without my patch. What do I mean by that?
> The case is that I was not able to run the test-runner on the vanilla
> master branch with 100% success rate. There is always some failure
> with my "setup". However, when checking only the tools/mgmt-tester
> test, the success rate seems to be higher than without it (but it
> might be just a fluke). I run it like this: "tools/test-runner -k
> ~/linux/arch/x86/boot/bzImage -- tools/mgmt-tester". Anyway, I can see
> one test always failing - "Read Exp Feature - Success (Index None)" -
> it happens with and without my patch. Also, I've verified that the
> advertising broadcast code was executed (by adding some prints to that
> function).
>
> To conclude that, I'm afraid that I don't know how to run the test
> suite properly... So, my verification might miss something. I would
> advise you to verify that patch with your setup as well (if you can).

On v3 there actually seems to be some regression with the following test:

Ext Device Found - Advertising data - Zero padded - test passed
  Client set connectable: Success (0x00)
  Test setup condition complete, -1 left
  Client set connectable: Success (0x00)
  Test setup condition complete, -2 left
qqemu-system-x86_64: terminating on signal 2

So we will probably need to fix that before we can apply it, I suspect
we are getting more events than anticipated causing the steps to go
into negative which probably is why it hangs, anyway it should have
stopped when it reached the end.

> Regards,
> Arek
Arkadiusz Bokowy Feb. 13, 2025, 1:29 p.m. UTC | #4 
> I've sent a v3 with a couple of updates so it should now use the
> advertisement interval rather than just assuming 200ms would cover all
> intervals.

Awesome!

> So we will probably need to fix that before we can apply it, I suspect
> we are getting more events than anticipated causing the steps to go
> into negative which probably is why it hangs, anyway it should have
> stopped when it reached the end.

I think that I've fixed that in v4. The case is that sometimes the ext
adv parameters are set with min/max advertisement intervals set to 0
(which is not valid I think ?). Because of that, the broadcast timer
(with timeout 0) blocks everything and the test hangs. I've fixed it
by providing a default advertising interval (according to the
Bluetooth Core Spec), and checking for invalid values. Also, in case
of high duty cycle advertising (direct advertising), the timeout is
set to non-zero (10ms) in order not to hang everything. I've verified
that with mgmt-tested and everything passes (except one test which
always fails with my setup).

Regards,
Arek
diff mbox series

Patch

diff --git a/emulator/btdev.c b/emulator/btdev.c
index 60901ba56..fb2f2f41d 100644
--- a/emulator/btdev.c
+++ b/emulator/btdev.c
@@ -102,7 +102,8 @@  struct le_ext_adv {
 	uint8_t adv_data_len;
 	uint8_t scan_data[252];
 	uint8_t scan_data_len;
-	unsigned int id;
+	unsigned int broadcast_id;
+	unsigned int timeout_id;
 };
 
 struct le_per_adv {
@@ -575,8 +576,10 @@  static void le_ext_adv_free(void *data)
 	/* Remove to queue */
 	queue_remove(ext_adv->dev->le_ext_adv, ext_adv);
 
-	if (ext_adv->id)
-		timeout_remove(ext_adv->id);
+	if (ext_adv->broadcast_id)
+		timeout_remove(ext_adv->broadcast_id);
+	if (ext_adv->timeout_id)
+		timeout_remove(ext_adv->timeout_id);
 
 	free(ext_adv);
 }
@@ -4756,9 +4759,13 @@  static void ext_adv_disable(void *data, void *user_data)
 	if (handle && ext_adv->handle != handle)
 		return;
 
-	if (ext_adv->id) {
-		timeout_remove(ext_adv->id);
-		ext_adv->id = 0;
+	if (ext_adv->broadcast_id) {
+		timeout_remove(ext_adv->broadcast_id);
+		ext_adv->broadcast_id = 0;
+	}
+	if (ext_adv->timeout_id) {
+		timeout_remove(ext_adv->timeout_id);
+		ext_adv->timeout_id = 0;
 	}
 
 	ext_adv->enable = 0x00;
@@ -4975,9 +4982,10 @@  static void send_ext_adv(struct btdev *btdev, const struct btdev *remote,
 					1 + 24 + meta_event.lear.data_len);
 }
 
-static void le_set_ext_adv_enable_complete(struct btdev *btdev,
-						struct le_ext_adv *ext_adv)
+static bool ext_adv_broadcast(void *user_data)
 {
+	struct le_ext_adv *ext_adv = user_data;
+	struct btdev *btdev = ext_adv->dev;
 	uint16_t report_type;
 	int i;
 
@@ -5013,7 +5021,10 @@  static void le_set_ext_adv_enable_complete(struct btdev *btdev,
 							report_type, true);
 		}
 	}
+
+	return true;
 }
+
 static void adv_set_terminate(struct btdev *dev, uint8_t status, uint8_t handle,
 					uint16_t conn_handle, uint8_t num_evts)
 {
@@ -5032,7 +5043,7 @@  static bool ext_adv_timeout(void *user_data)
 {
 	struct le_ext_adv *adv = user_data;
 
-	adv->id = 0;
+	adv->timeout_id = 0;
 	adv_set_terminate(adv->dev, BT_HCI_ERR_ADV_TIMEOUT, adv->handle,
 								0x0000, 0x00);
 	le_ext_adv_free(adv);
@@ -5117,32 +5128,32 @@  static int cmd_set_ext_adv_enable(struct btdev *dev, const void *data,
 
 		if (!cmd->enable)
 			ext_adv_disable(ext_adv, NULL);
-		else if (eas->duration)
-			ext_adv->id = timeout_add(eas->duration * 10,
-							ext_adv_timeout,
+		else {
+			/* Send the first advertising report right away and
+			 * start the timer for continuous advertising. */
+			ext_adv_broadcast(ext_adv);
+			/* BLE advertising interval shall be between 20 ms
+			 * and 10.24 s in 0.625 ms steps. Most devices which
+			 * require fast advertising use an interval between
+			 * 100 ms and 500 ms.
+			 */
+			ext_adv->broadcast_id = timeout_add(200 /* 200 ms */,
+							ext_adv_broadcast,
 							ext_adv, NULL);
+			if (eas->duration) {
+				unsigned int duration_ms = eas->duration * 10;
+				ext_adv->timeout_id = timeout_add(duration_ms,
+								ext_adv_timeout,
+								ext_adv, NULL);
+			}
+		}
+
 	}
 
 exit_complete:
 	cmd_complete(dev, BT_HCI_CMD_LE_SET_EXT_ADV_ENABLE, &status,
 							sizeof(status));
 
-	if (status == BT_HCI_ERR_SUCCESS && cmd->enable) {
-		/* Go through each sets and send adv event to peer device */
-		for (i = 0; i < cmd->num_of_sets; i++) {
-			const struct bt_hci_cmd_ext_adv_set *eas;
-			struct le_ext_adv *ext_adv;
-
-			eas = data + sizeof(*cmd) + (sizeof(*eas) * i);
-
-			ext_adv = queue_find(dev->le_ext_adv,
-						match_ext_adv_handle,
-						UINT_TO_PTR(eas->handle));
-			if (ext_adv)
-				le_set_ext_adv_enable_complete(dev, ext_adv);
-		}
-	}
-
 	return 0;
 }
 
@@ -5492,43 +5503,6 @@  done:
 	return 0;
 }
 
-static void scan_ext_adv(struct btdev *dev, struct btdev *remote)
-{
-	const struct queue_entry *entry;
-
-	for (entry = queue_get_entries(remote->le_ext_adv); entry;
-							entry = entry->next) {
-		struct le_ext_adv *ext_adv = entry->data;
-		uint16_t report_type;
-
-		if (!ext_adv->enable)
-			continue;
-
-		if (!ext_adv_match_addr(dev, ext_adv))
-			continue;
-
-		report_type = get_ext_adv_type(ext_adv->type);
-		send_ext_adv(dev, remote, ext_adv, report_type, false);
-
-		if (dev->le_scan_type != 0x01)
-			continue;
-
-		/* if scannable bit is set the send scan response */
-		if (ext_adv->type & 0x02) {
-			if (ext_adv->type == 0x13)
-				report_type = 0x1b;
-			else if (ext_adv->type == 0x12)
-				report_type = 0x1a;
-			else if (!(ext_adv->type & 0x10))
-				report_type &= 0x08;
-			else
-				continue;
-
-			send_ext_adv(dev, remote, ext_adv, report_type, true);
-		}
-	}
-}
-
 static void scan_pa(struct btdev *dev, struct btdev *remote)
 {
 	struct le_per_adv *per_adv = queue_find(dev->le_per_adv,
@@ -5557,7 +5531,6 @@  static int cmd_set_ext_scan_enable_complete(struct btdev *dev, const void *data,
 		if (!btdev_list[i] || btdev_list[i] == dev)
 			continue;
 
-		scan_ext_adv(dev, btdev_list[i]);
 		scan_pa(dev, btdev_list[i]);
 	}