Re: [PATCH v2] serial: 8250: Fix fifo underflow on flush

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 7 Feb 2025, John Keeping wrote:

> When flushing the serial port's buffer, uart_flush_buffer() calls
> kfifo_reset() but if there is an outstanding DMA transfer then the
> completion function will consume data from the kfifo via
> uart_xmit_advance(), underflowing and leading to ongoing DMA as the
> driver tries to transmit another 2^32 bytes.
> 
> This is readily reproduced with serial-generic and amidi sending even
> short messages as closing the device on exit will wait for the fifo to
> drain and in the underflow case amidi hangs for 30 seconds on exit in
> tty_wait_until_sent().  A trace of that gives:
> 
>      kworker/1:1-84    [001]    51.769423: bprint:               serial8250_tx_dma: tx_size=3 fifo_len=3
>            amidi-763   [001]    51.769460: bprint:               uart_flush_buffer: resetting fifo
>  irq/21-fe530000-76    [000]    51.769474: bprint:               __dma_tx_complete: tx_size=3
>  irq/21-fe530000-76    [000]    51.769479: bprint:               serial8250_tx_dma: tx_size=4096 fifo_len=4294967293
>  irq/21-fe530000-76    [000]    51.781295: bprint:               __dma_tx_complete: tx_size=4096
>  irq/21-fe530000-76    [000]    51.781301: bprint:               serial8250_tx_dma: tx_size=4096 fifo_len=4294963197
>  irq/21-fe530000-76    [000]    51.793131: bprint:               __dma_tx_complete: tx_size=4096
>  irq/21-fe530000-76    [000]    51.793135: bprint:               serial8250_tx_dma: tx_size=4096 fifo_len=4294959101
>  irq/21-fe530000-76    [000]    51.804949: bprint:               __dma_tx_complete: tx_size=4096
> 
> Since the port lock is held in when the kfifo is reset in
> uart_flush_buffer() and in __dma_tx_complete(), adding a flush_buffer
> hook to adjust the outstanding DMA byte count is sufficient to avoid the
> kfifo underflow.
> 
> Fixes: 9ee4b83e51f74 ("serial: 8250: Add support for dmaengine")
> Cc: stable@vger.kernel.org
> Signed-off-by: John Keeping <jkeeping@inmusicbrands.com>
> ---
> Changes in v2:
> - Add Fixes: tag
> - Return early to reduce indentation in serial8250_tx_dma_flush()
> 
>  drivers/tty/serial/8250/8250.h      |  1 +
>  drivers/tty/serial/8250/8250_dma.c  | 16 ++++++++++++++++
>  drivers/tty/serial/8250/8250_port.c |  9 +++++++++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 11e05aa014e54..8ef45622e4363 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -374,6 +374,7 @@ static inline int is_omap1510_8250(struct uart_8250_port *pt)
>  
>  #ifdef CONFIG_SERIAL_8250_DMA
>  extern int serial8250_tx_dma(struct uart_8250_port *);
> +extern void serial8250_tx_dma_flush(struct uart_8250_port *);
>  extern int serial8250_rx_dma(struct uart_8250_port *);
>  extern void serial8250_rx_dma_flush(struct uart_8250_port *);
>  extern int serial8250_request_dma(struct uart_8250_port *);

Will build fail if !CONFIG_SERIAL_8250_DMA ?

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index d215c494ee24c..f245a84f4a508 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -149,6 +149,22 @@ int serial8250_tx_dma(struct uart_8250_port *p)
>  	return ret;
>  }
>  
> +void serial8250_tx_dma_flush(struct uart_8250_port *p)
> +{
> +	struct uart_8250_dma *dma = p->dma;
> +
> +	if (!dma->tx_running)
> +		return;
> +
> +	/*
> +	 * kfifo_reset() has been called by the serial core, avoid
> +	 * advancing and underflowing in __dma_tx_complete().
> +	 */
> +	dma->tx_size = 0;
> +
> +	dmaengine_terminate_async(dma->rxchan);
> +}
> +
>  int serial8250_rx_dma(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma		*dma = p->dma;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index d7976a21cca9c..442967a6cd52d 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -2555,6 +2555,14 @@ static void serial8250_shutdown(struct uart_port *port)
>  		serial8250_do_shutdown(port);
>  }
>  
> +static void serial8250_flush_buffer(struct uart_port *port)
> +{
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +
> +	if (up->dma)
> +		serial8250_tx_dma_flush(up);
> +}
> +
>  static unsigned int serial8250_do_get_divisor(struct uart_port *port,
>  					      unsigned int baud,
>  					      unsigned int *frac)
> @@ -3244,6 +3252,7 @@ static const struct uart_ops serial8250_pops = {
>  	.break_ctl	= serial8250_break_ctl,
>  	.startup	= serial8250_startup,
>  	.shutdown	= serial8250_shutdown,
> +	.flush_buffer	= serial8250_flush_buffer,
>  	.set_termios	= serial8250_set_termios,
>  	.set_ldisc	= serial8250_set_ldisc,
>  	.pm		= serial8250_pm,
> 

-- 
 i.