linux-kernel - Re: [PATCH V2] tty: serial: fsl_lpuart: add timeout for wait_event

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fd033ff-d1a5-9f1c-d8b9-5f51d63697fd@kernel.org>
Date:   Wed, 8 Dec 2021 08:21:22 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Sherry Sun <sherry.sun@....com>, gregkh@...uxfoundation.org
Cc:     linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-imx@....com
Subject: Re: [PATCH V2] tty: serial: fsl_lpuart: add timeout for
 wait_event_interruptible in .shutdown()

Hi,

On 03. 12. 21, 4:04, Sherry Sun wrote:
> Use wait_event_interruptible in lpuart_dma_shutdown isn't a reasonable
> behavior, since it may cause the system hang here if the condition

Wait, _interruptible causes hangs? Under what circumstances?

> !sport->dma_tx_in_progress never to be true in some corner case, such as
> when enable the flow control, the dma tx request may never be completed
> due to the peer's CTS setting when run .shutdown().
> 
> So here change to use wait_event_interruptible_timeout instead of
> wait_event_interruptible, the tx dma will be forcibly terminated if the
> tx dma request cannot be completed within 300ms.
> Considering the worst tx dma case is to have a 4K bytes tx buffer, which
> would require about 300ms to complete when the baudrate is 115200.

300 looks like a magic number -- what if the rate is < 115200? Why not 
using port->timeout?

Anyway, in what scenario is this a problem? Both lpuart*_tx_empty() do:
if (sport->dma_tx_in_progress)
         return 0;

So wait_until_sent() should have waited for long enough already.

> Signed-off-by: Sherry Sun <sherry.sun@....com>
> ---
> changes in V2
> 1. Increase the timeout to 300ms, need to consider the worst tx dma case.
> ---
>   drivers/tty/serial/fsl_lpuart.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index ac5112def40d..3affe52a364d 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1793,8 +1793,8 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
>   	}
>   
>   	if (sport->lpuart_dma_tx_use) {
> -		if (wait_event_interruptible(sport->dma_wait,
> -			!sport->dma_tx_in_progress) != false) {
> +		if (wait_event_interruptible_timeout(sport->dma_wait,
> +			!sport->dma_tx_in_progress, msecs_to_jiffies(300)) <= 0) {
>   			sport->dma_tx_in_progress = false;
>   			dmaengine_terminate_all(sport->dma_tx_chan);
>   		}
> 


-- 
js
suse labs