Re: [PATCH 3/5] serial: sprd: fix TX kfifo underflow on flush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: sprd: TX DMA left running in sprd_flush_buffer() corrupts xmit_buf
- [High] serial: sprd: DMA mapping leak and TX wedge after sprd_flush_buffer()
- [Medium] serial: sprd: race in sprd_stop_tx_dma() when TX DMA just completed
--

commit 8173f9afc9b36f262ce001244563fa23087de599
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: sprd: fix TX kfifo underflow on flush

This commit adds a flush_buffer() hook to sprd_serial that marks an in-flight
TX DMA transfer with tx_flushed instead of terminating it under the port
lock, so that sprd_complete_tx_dma() and sprd_stop_tx_dma() skip calling
uart_xmit_advance() after the transmit kfifo has been reset.

> diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c
> index 3fdf5939a62c1..718732b683f1b 100644
> --- a/drivers/tty/serial/sprd_serial.c
> +++ b/drivers/tty/serial/sprd_serial.c
> @@ -133,6 +133,7 @@ struct sprd_uart_port {
>  	struct clk *clk;
>  	struct sprd_uart_dma tx_dma;
>  	struct sprd_uart_dma rx_dma;
> +	bool tx_flushed;
>  	dma_addr_t pos;
>  	unsigned char *rx_buf_tail;
>  	const struct sprd_uart_data *pdata;
> @@ -236,13 +237,15 @@ static void sprd_stop_tx_dma(struct uart_port *port)
>  	dmaengine_tx_status(sp->tx_dma.chn, sp->tx_dma.cookie, &state);
>  	if (state.residue) {
>  		trans_len = state.residue - sp->tx_dma.phys_addr;
> -		uart_xmit_advance(port, trans_len);
> +		if (!sp->tx_flushed)
> +			uart_xmit_advance(port, trans_len);
>  		dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
>  				 sp->tx_dma.trans_len, DMA_TO_DEVICE);
>  	}
>  
>  	dmaengine_terminate_all(sp->tx_dma.chn);
>  	sp->tx_dma.trans_len = 0;
> +	sp->tx_flushed = false;
>  }

[Severity: Medium]
What happens if sprd_stop_tx_dma() runs just after the hardware finishes the
flushed DMA transfer, before the sprd_complete_tx_dma() tasklet runs?

After uart_flush_buffer() resets tport->xmit_fifo while leaving the TX DMA
transfer running, kfifo_is_empty(&tport->xmit_fifo) is true. Any subsequent
call to sprd_stop_tx() or to sprd_start_tx() -> sprd_start_tx_dma() while the
kfifo is still empty (such as from uart_flush_chars() or uart_start()) will
enter sprd_stop_tx_dma():

drivers/tty/serial/sprd_serial.c:sprd_start_tx_dma() {
	if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(port)) {
		sprd_stop_tx_dma(port);
		return;
	}

In drivers/dma/sprd-dma.c, when the transfer completes in hardware,
dma_irq_handle() calls vchan_cookie_complete(), which marks sp->tx_dma.cookie
as DMA_COMPLETE and schedules the vchan_complete tasklet to invoke
sprd_complete_tx_dma(). Once the cookie is marked complete,
sprd_dma_tx_status() returns with state.residue == 0.

When sprd_stop_tx_dma() sees state.residue == 0, it skips the dma_unmap_single()
block, calls dmaengine_terminate_all(sp->tx_dma.chn), and clears both
sp->tx_dma.trans_len = 0 and sp->tx_flushed = false.

If vchan_complete has not yet spliced vc->desc_completed,
sprd_dma_terminate_all() -> vchan_get_all_descriptors() frees the completed
descriptor without calling sprd_complete_tx_dma(), leaking the DMA mapping on
sp->tx_dma.phys_addr. If vchan_complete on another CPU has already spliced
vc->desc_completed and sprd_complete_tx_dma() is waiting on port->lock,
sprd_complete_tx_dma() will run after sp->tx_dma.trans_len and sp->tx_flushed
have been cleared (or after a new TX DMA transfer has started), unmapping with
trans_len == 0 or prematurely unmapping and advancing the kfifo for the new
transfer.

>  
>  static int sprd_tx_buf_remap(struct uart_port *port)
> @@ -273,7 +276,9 @@ static void sprd_complete_tx_dma(void *data)
>  	dma_unmap_single(port->dev, sp->tx_dma.phys_addr,
>  			 sp->tx_dma.trans_len, DMA_TO_DEVICE);
>  
> -	uart_xmit_advance(port, sp->tx_dma.trans_len);
> +	if (!sp->tx_flushed)
> +		uart_xmit_advance(port, sp->tx_dma.trans_len);
> +	sp->tx_flushed = false;
>  
>  	if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
>  		uart_write_wakeup(port);
> @@ -555,6 +560,20 @@ static void sprd_stop_tx(struct uart_port *port)
>  	serial_out(port, SPRD_ICLR, iclr);
>  }
>  
> +static void sprd_flush_buffer(struct uart_port *port)
> +{
> +	struct sprd_uart_port *sp = container_of(port, struct sprd_uart_port,
> +						 port);
> +
> +	/*
> +	 * The kfifo has been reset. Let the running transfer finish, so that
> +	 * its buffer is unmapped only once it has stopped, but do not advance
> +	 * the kfifo for it.
> +	 */
> +	if (sp->tx_dma.trans_len)
> +		sp->tx_flushed = true;
> +}

[Severity: High]
Can leaving the TX DMA transfer running in sprd_flush_buffer() cause subsequent
writes to overwrite the active DMA buffer in place and transmit post-flush data
twice?

In sprd_tx_buf_remap(), TX DMA does not use a separate bounce buffer; it maps
the kfifo backing buffer (state->port.xmit_buf) directly in place:

drivers/tty/serial/sprd_serial.c:sprd_tx_buf_remap() {
	sp->tx_dma.trans_len = kfifo_out_linear_ptr(&tport->xmit_fifo, &tail,
			UART_XMIT_SIZE);

	sp->tx_dma.phys_addr = dma_map_single(port->dev, tail,
					      sp->tx_dma.trans_len,
					      DMA_TO_DEVICE);

In uart_flush_buffer(), kfifo_reset(&state->port.xmit_fifo) resets both
xmit_fifo.in and xmit_fifo.out to 0 before calling port->ops->flush_buffer(),
and then wakes blocked writers via tty_port_tty_wakeup().

If a subsequent uart_write() or uart_put_char() runs while the flushed TX DMA
transfer is still in flight, kfifo_in() writes the new bytes into xmit_buf
starting at offset 0, overwriting the DMA_TO_DEVICE region while the DMA
controller is actively reading from it. In sprd_start_tx_dma(), the new write
then returns early because sp->tx_dma.trans_len is still non-zero:

drivers/tty/serial/sprd_serial.c:sprd_start_tx_dma() {
	if (sp->tx_dma.trans_len)
		return;

When the flushed DMA transfer later finishes, sprd_complete_tx_dma() sees
sp->tx_flushed == true and skips uart_xmit_advance(), leaving the newly written
bytes at xmit_fifo.out == 0, and then starts a second DMA transfer for those
same bytes:

drivers/tty/serial/sprd_serial.c:sprd_complete_tx_dma() {
	if (!sp->tx_flushed)
		uart_xmit_advance(port, sp->tx_dma.trans_len);
	sp->tx_flushed = false;
	...
	if (kfifo_is_empty(&tport->xmit_fifo) || sprd_tx_buf_remap(port) ||
	    sprd_tx_dma_config(port))
		sp->tx_dma.trans_len = 0;

[Severity: High]
Also in sprd_flush_buffer(), what happens if the port is shut down after
uart_flush_buffer() before the in-flight TX DMA transfer completes (for example
during uart_hangup() or close)?

When sprd_flush_buffer() sets sp->tx_flushed = true and leaves the transfer
running with sp->tx_dma.trans_len > 0, it relies on sprd_complete_tx_dma() (or
sprd_stop_tx_dma()) to unmap sp->tx_dma.phys_addr and reset
sp->tx_dma.trans_len and sp->tx_flushed.

However, uart_shutdown() -> uart_port_shutdown() invokes sprd_shutdown() ->
sprd_release_dma() and then frees state->port.xmit_buf in uart_free_xmit_buf():

drivers/tty/serial/sprd_serial.c:sprd_release_dma() {
	sprd_uart_dma_enable(port, false);

	if (sp->rx_dma.enable)
		dma_release_channel(sp->rx_dma.chn);

	if (sp->tx_dma.enable)
		dma_release_channel(sp->tx_dma.chn);

	sp->tx_dma.enable = false;
	sp->rx_dma.enable = false;
}

In sprd_dma_free_chan_resources(), releasing sp->tx_dma.chn stops the channel
and frees all descriptors without invoking the completion callback. Because
neither sprd_release_dma() nor sprd_startup() unmaps sp->tx_dma.phys_addr or
clears sp->tx_dma.trans_len and sp->tx_flushed, won't xmit_buf be freed while
still DMA-mapped, and won't stale non-zero sp->tx_dma.trans_len cause
sprd_start_tx_dma() to return early on every write when the port is reopened?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123911.3784690-1-samarth.colleges@gmail.com?part=3