[PATCH] Bluetooth: hci_h5: avoid sending two SYNC messages

[PATCH] Bluetooth: hci_h5: avoid sending two SYNC messages

[Javier Nieto]

Previously, h5_open() called h5_link_control() to send a SYNC message.
But h5_link_control() only enqueues the packet and requires the caller
to call hci_uart_tx_wakeup(). Thus, after H5_SYNC_TIMEOUT ran out
(100ms), h5_timed_event() would be called and, realizing that the state
was still H5_UNINITIALIZED, it would re-enqueue the SYNC and call
hci_uart_tx_wakeup(). Consequently, two SYNC packets would be sent and
initialization would unnecessarily wait for 100ms.

The naive solution of calling hci_uart_tx_wakeup() in h5_open() does not
work because it will only schedule tx work if the HCI_PROTO_READY bit is
set and hci_serdev only sets it after h5_open() returns. This patch
removes the extraneous SYNC being enqueued and makes h5_timed_event()
wake up on the next jiffy.

Signed-off-by: Javier Nieto <jgnieto@cs.stanford.edu>
---
 drivers/bluetooth/hci_h5.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index d0d4420c1a0f..863ee93dd8a8 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -213,7 +213,6 @@ static void h5_peer_reset(struct hci_uart *hu)
 static int h5_open(struct hci_uart *hu)
 {
 	struct h5 *h5;
-	const unsigned char sync[] = { 0x01, 0x7e };
 
 	BT_DBG("hu %p", hu);
 
@@ -243,9 +242,11 @@ static int h5_open(struct hci_uart *hu)
 
 	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
 
-	/* Send initial sync request */
-	h5_link_control(hu, sync, sizeof(sync));
-	mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
+	/*
+	 * Wait one jiffy because the UART layer won't set HCI_UART_PROTO_READY,
+	 * which allows us to send link packets, until this function returns.
+	 */
+	mod_timer(&h5->timer, jiffies + 1);
 
 	return 0;
 }
-- 
2.43.0

RE: Bluetooth: hci_h5: avoid sending two SYNC messages

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.28 seconds
GitLint                       PENDING   0.23 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      24.26 seconds
CheckAllWarning               PASS      26.93 seconds
CheckSparse                   PASS      30.17 seconds
BuildKernel32                 PASS      24.49 seconds
TestRunnerSetup               PASS      482.49 seconds
TestRunner_l2cap-tester       PASS      25.04 seconds
TestRunner_iso-tester         PASS      36.78 seconds
TestRunner_bnep-tester        PASS      6.00 seconds
TestRunner_mgmt-tester        FAIL      128.34 seconds
TestRunner_rfcomm-tester      PASS      9.47 seconds
TestRunner_sco-tester         PASS      14.62 seconds
TestRunner_ioctl-tester       PASS      10.06 seconds
TestRunner_mesh-tester        FAIL      11.48 seconds
TestRunner_smp-tester         PASS      8.64 seconds
TestRunner_userchan-tester    PASS      6.26 seconds
IncrementalBuild              PENDING   0.54 seconds

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

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

##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 490, Passed: 484 (98.8%), Failed: 2, Not Run: 4

Failed Test Cases
Read Exp Feature - Success                           Failed       0.101 seconds
LL Privacy - Set Flags 3 (2 Devices to RL)           Failed       0.200 seconds
##############################
Test: TestRunner_mesh-tester - FAIL
Desc: Run mesh-tester with test-runner
Output:
Total: 10, Passed: 8 (80.0%), Failed: 2, Not Run: 0

Failed Test Cases
Mesh - Send cancel - 1                               Timed out    2.155 seconds
Mesh - Send cancel - 2                               Timed out    1.992 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: hci_h5: avoid sending two SYNC messages

[Paul Menzel]

Dear Javier,


Thank you for your patch.


Am 22.08.25 um 02:39 schrieb Javier Nieto:
> Previously, h5_open() called h5_link_control() to send a SYNC message.
> But h5_link_control() only enqueues the packet and requires the caller
> to call hci_uart_tx_wakeup(). Thus, after H5_SYNC_TIMEOUT ran out
> (100ms), h5_timed_event() would be called and, realizing that the state
> was still H5_UNINITIALIZED, it would re-enqueue the SYNC and call
> hci_uart_tx_wakeup(). Consequently, two SYNC packets would be sent and
> initialization would unnecessarily wait for 100ms.
> 
> The naive solution of calling hci_uart_tx_wakeup() in h5_open() does not
> work because it will only schedule tx work if the HCI_PROTO_READY bit is
> set and hci_serdev only sets it after h5_open() returns. This patch
> removes the extraneous SYNC being enqueued and makes h5_timed_event()
> wake up on the next jiffy.

Great commit message, thank you. I’d appreciate it if you documented 
your test environment, and maybe paste the logs (for the timestamps) 
before and after, so others could easily reproduce the issue.

> Signed-off-by: Javier Nieto <jgnieto@cs.stanford.edu>
> ---
>   drivers/bluetooth/hci_h5.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index d0d4420c1a0f..863ee93dd8a8 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -213,7 +213,6 @@ static void h5_peer_reset(struct hci_uart *hu)
>   static int h5_open(struct hci_uart *hu)
>   {
>   	struct h5 *h5;
> -	const unsigned char sync[] = { 0x01, 0x7e };
>   
>   	BT_DBG("hu %p", hu);
>   
> @@ -243,9 +242,11 @@ static int h5_open(struct hci_uart *hu)
>   
>   	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
>   
> -	/* Send initial sync request */
> -	h5_link_control(hu, sync, sizeof(sync));
> -	mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);
> +	/*
> +	 * Wait one jiffy because the UART layer won't set HCI_UART_PROTO_READY,
> +	 * which allows us to send link packets, until this function returns.
> +	 */
> +	mod_timer(&h5->timer, jiffies + 1);
>   
>   	return 0;
>   }

Makes sense.

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [PATCH] Bluetooth: hci_h5: avoid sending two SYNC messages

[Javier Nieto]

Dear Paul,

> Great commit message, thank you. I´d appreciate it if you documented your
> test environment, and maybe paste the logs (for the timestamps) before and
> after, so others could easily reproduce the issue.

Appologies for that. My test environment is a MangoPi MQ-Pro board
(with a Realtek RTL8723DS Bluetooth chip) running the tip of
bluetooth-next, although I also observed this issue on 6.8 kernel.
Originally, I spotted it using a logic analyzer on the UART PCB traces.
I added a temporary log message to h5_link_control(), which prints the
first byte and the length of the packet being sent. Here is the relevant
part before the patch:

[   67.328445] Bluetooth: h5_link_control sending 1 with len 2
[   67.432393] Bluetooth: h5_link_control sending 1 with len 2
[   67.436424] Bluetooth: h5_link_control sending 3 with len 3
[   67.436592] Bluetooth: h5_link_control sending 2 with len 2
[   67.436693] Bluetooth: h5_link_control sending 3 with len 3
[   67.439510] Bluetooth: h5_link_control sending 2 with len 2
[   67.440004] Bluetooth: h5_link_control sending 4 with len 2
[   67.440030] Bluetooth: h5_link_control sending 3 with len 3

And here after the patch:

[   67.498228] Bluetooth: h5_link_control sending 1 with len 2
[   67.501444] Bluetooth: h5_link_control sending 3 with len 3
[   67.501615] Bluetooth: h5_link_control sending 2 with len 2
[   67.504976] Bluetooth: h5_link_control sending 2 with len 2
[   67.505141] Bluetooth: h5_link_control sending 4 with len 2
[   67.505168] Bluetooth: h5_link_control sending 3 with len 3

Notice that in the first case, two SYNC packets (type 1) are sent, one 100ms
after the other, while in the second case only one is sent. In both
cases, using bluetoothctl to connect to a device later on works fine.

Thanks for the feedback,

Javier