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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <06787d51-5d5e-4f74-bda6-b80cead5ed6d@molgen.mpg.de>
Date: Fri, 22 Aug 2025 13:41:41 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Javier Nieto <jgnieto@...stanford.edu>
Cc: luiz.dentz@...il.com, marcel@...tmann.org,
 linux-bluetooth@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: hci_h5: avoid sending two SYNC messages

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@...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@...gen.mpg.de>


Kind regards,

Paul