Re: [PATCH 1/2] serial: 8250: drain canceled TX DMA before reusing its buffer

[Andy Shevchenko]

On Wed, Oct 21, 2026 at 04:37:43PM -0600, James Hilliard wrote:
> uart_flush_buffer() resets the transmit kfifo before calling the driver's
> flush callback. serial8250_tx_dma_flush() terminates DMA asynchronously and
> clears tx_running, allowing a new write to replace tx_size before the old
> completion callback has finished. That callback can then consume bytes
> from the new transmission. The serial core can also overwrite the old
> DMA source as soon as it reuses the reset kfifo.
> 
> Give DMA a private transmit buffer, keeping the kfifo offsets used by
> FIFO-window providers. Copy only the queued ranges before handing the
> buffer to the device. Flushing the kfifo can then no longer change an
> in-flight transfer's source.
> 
> Keep tx_running set while a worker synchronizes a successfully terminated
> channel. Suppress callbacks and new submissions during that interval.
> The DesignWare idle helpers must not pause or resume a retiring channel.
> Once synchronization finishes, process the zero-length completion under
> the port lock and start any data queued after the flush. Reuse the normal
> completion processing for OMAP too, including its delayed register
> restore and runtime-PM handling. Drain the worker before releasing DMA
> resources on shutdown.
> 
> Calculate the queued length before calling the platform preparation hook,
> so it sees the length of the transfer being prepared rather than the
> previous transfer's length.
> 
> If termination fails, leave the buffer owned by the transfer and let its
> normal completion release it. Do not submit another transfer before that
> completion. A DMA completion still does not mean that the UART transmitter
> is empty; the existing line-status checks remain in place.
> 
> The race was found by inspecting cancellation and restart while preparing
> H616 UART DMA support.

...

> +static void __dma_tx_complete(void *param)
> +{
> +	struct uart_8250_port *p = param;
> +	struct uart_8250_dma *dma = p->dma;
> +
> +	guard(uart_port_lock_irqsave)(&p->port);

Add a blank line after guard()() everywhere, otherwise it's harder to see it
in the code.

> +	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);

Make it a single line.

> +	struct uart_8250_port *p = dma->port;
> +
> +	/* Neither the channel nor its buffer may be reused before this returns. */
> +	dmaengine_synchronize(dma->txchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->tx_terminating = false;
> +	if (!dma->shutting_down)
> +		dma->tx_complete(p);
> +}
> +
> +void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
> +				    struct scatterlist *sgl, unsigned int nents)
> +{
> +	struct uart_8250_dma *dma = p->dma;
> +	struct scatterlist *sg;
> +	unsigned int i;
> +
> +	/* Preserve ring offsets for DMA engines which implement a FIFO window. */
> +	for_each_sg(sgl, sg, nents, i) {
> +		size_t offset = sg_dma_address(sg) - dma->tx_addr;
> +
> +		memcpy(dma->tx_buf + offset,
> +		       p->port.state->port.xmit_buf + offset, sg_dma_len(sg));
> +	}
> +	dma_sync_single_for_device(dmaengine_get_dma_device(dma->txchan), dma->tx_addr,
> +				   UART_XMIT_SIZE, DMA_TO_DEVICE);
> +}

This is a bounce buffer, why do we need it? Shouldn't this be rather the
respective DMAengine issue on how to treat the pieces?

...

Overall I have a feeling that we turned somewhere in a wrong direction.
This all activity on 8250 DMA support looks like over engineered approach.

Perhaps we need to reconsider the whole picture?

-- 
With Best Regards,
Andy Shevchenko