Re: [PATCH v2 3/4] serial: 8250: drain canceled TX DMA before reusing its buffer

[Andy Shevchenko]

On Thu, Oct 22, 2026 at 06:34:04PM -0600, James Hilliard wrote:
> uart_flush_buffer() resets the transmit kfifo before calling the driver.
> serial8250_tx_dma_flush() terminates DMA asynchronously and clears
> tx_running, allowing a new write to replace tx_size while the old callback
> is still waiting for the port lock. That callback can then consume bytes
> from the new transmission. Reusing the reset kfifo can also overwrite the
> source of a transfer that has not stopped yet.
> 
> Reserve the flushed FIFO storage through serial core before returning
> from the flush callback. Keep tx_running set until a worker synchronizes
> the terminated channel, suppressing old callbacks and new submissions
> during that interval. Return the mapping to CPU ownership before releasing
> the reservation and waking writers. Normal transfers continue to use the
> FIFO directly.
> 
> Reuse the locked completion processing after retirement, including OMAP
> delayed register restoration and transmitter-empty handling. Take a runtime
> PM reference around worker UART accesses and drain the worker before
> releasing the channels. Shutdown releases the reservation after stopping
> DMA and unmapping its source.
> 
> Track a flushed FIFO separately from the transfer length. If termination
> fails, retain the reservation until normal completion and account the
> completed bytes without advancing the reset FIFO. After successful
> cancellation, process a zero-length completion once callbacks have drained.
> 
> The race was found by inspecting cancellation and restart while preparing
> H616 UART DMA support.

...

> -	if (!dma->tx_running)
> +	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
>  		return;

...

> +	if (!dma->shutting_down && !dma->tx_terminating)
> +		serial8250_tx_dma_complete(p);

...

> +static void serial8250_tx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma, tx_flush_work);
> +	struct uart_8250_port *p = dma->port;
> +
> +	/* Neither the channel nor its buffer may be reused before this returns. */
> +	dmaengine_synchronize(dma->txchan);
> +
> +	guard(serial8250_rpm)(p);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +
> +	dma->tx_terminating = false;
> +	dma->tx_size = 0;
> +	if (!dma->shutting_down)

If you write this as

	if (!dma->tx_terminating && !dma->shutting_down)

> +		dma->tx_complete(p);
>  }

...

>  	if (dma->shutting_down)
>  		return 0;
> +	if (dma->tx_terminating)
> +		return 0;

And this

	if (dma->tx_terminating || dma->shutting_down)
		return 0;

...

> -	if (!dma->tx_running)
> +	if (!dma->tx_running || dma->tx_terminating || dma->shutting_down)
>  		return;
>  
>  	/*
>  	 * kfifo_reset() has been called by the serial core, avoid
> -	 * advancing and underflowing in __dma_tx_complete().
> +	 * advancing the reset FIFO when the old transfer completes.
>  	 */

...

> +	dma_sync_single_for_cpu(tx_dev, dma->tx_addr,
> +				UART_XMIT_SIZE, DMA_TO_DEVICE);

Make it a single line.

...

> +free_rx:
> +	dma_free_coherent(rx_dev, dma->rx_size,
> +			  dma->rx_buf, dma->rx_addr);

Ditto.

...

>  	int skip_byte = -1;
>  	int ret;
>  
> -	if (dma->shutting_down || dma->tx_running)
> +	if (dma->shutting_down || dma->tx_terminating || dma->tx_running)
>  		return 0;

>  	if (uart_tx_stopped(&p->port) || kfifo_is_empty(&tport->xmit_fifo)) {

So you will have a couple of patterns for the combinations of the flags.
With some static helpers you may optimise that. Also in some cases you even
might consider checking tx_running while it's known it's true (or false)
to reduce amount of variations of the combinations of the flags.

-- 
With Best Regards,
Andy Shevchenko