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@xxxxxxxxxxxxxxx
> Signed-off-by: John Keeping <jkeeping@xxxxxxxxxxxxxxxxx>
> ---
> 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.
