Re: [PATCH V2] tty: serial: fsl_lpuart: add timeout for wait_event_interruptible in .shutdown()

[Jiri Slaby <jirislaby@kernel.org>]

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@nxp.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