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

[Raul E Rangel]

Hi James,

Thanks for looking into the asynchronous termination race with xmit_fifo
reuse. A few issues with this patch:

> +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;
> +
> +	dmaengine_synchronize(dma->txchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->tx_terminating = false;
> +	if (!dma->shutting_down)
> +		dma->tx_complete(p);
> +}

1. Missing runtime PM reference in process context:
   `serial8250_tx_dma_flush_work()` runs asynchronously on `system_wq` and
   calls `dma->tx_complete(p)` (`serial8250_tx_dma_complete()` or
   `omap_8250_tx_dma_complete()`), which accesses UART registers
   (`UART_IER`, `RZN1_UART_TDMACR`, `UART_TX`, `omap8250_restore_regs()`)
   without calling `serial8250_rpm_get(p)` / `serial8250_rpm_put(p)`. If the
   port autosuspended before the work item ran, this will access unclocked
   registers. Also, on `UART_CAP_RPM` ports (such as `8250_omap`), if
   `dma->shutting_down` is true here, `dma->tx_complete(p)` is skipped and
   `serial8250_rpm_put_tx(p)` is never called, leaking `rpm_tx_active`.

2. `dmaengine_synchronize()` is a no-op on several 8250 DMA providers:
   `dmaengine_synchronize()` only waits if the provider implements
   `.device_synchronize`. Several DMA controllers used with 8250 — including
   `drivers/dma/sun6i-dma.c` (Allwinner H616, mentioned in your commit
   message), `drivers/dma/dw/core.c` (`dw_dmac`), `drivers/dma/pl330.c`, and
   `drivers/dma/mediatek/mtk-uart-apdma.c` — schedule callbacks from a
   tasklet (`virt-dma` `vc.task` or driver tasklet) but do not wire up
   `.device_synchronize`. On those controllers, `dmaengine_synchronize()`
   returns immediately while a completion tasklet from the old transfer can
   still be scheduled, so clearing `dma->tx_terminating = false` and starting
   a new transfer re-exposes the stale-callback race.

> +void serial8250_tx_dma_flush(struct uart_8250_port *p)
> +{
...
>  	dma->tx_size = 0;
> +	dma->tx_terminating = true;
> +	ret = dmaengine_terminate_async(dma->txchan);
> +	if (ret) {
> +		/* Keep the buffer owned by the transfer until its callback runs. */
> +		dma->tx_terminating = false;
> +		return;
> +	}

3. Zeroing `dma->tx_size = 0` before `dmaengine_terminate_async()`:
   If `dmaengine_terminate_async()` fails (`ret != 0`), the transfer
   continues to completion, but `dma->tx_size` has already been zeroed so
   `uart_xmit_advance()` in `tx_complete()` will not account for the
   transmitted bytes in `icount.tx`.

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

4. Bounce buffer layout and DMA sync:
   - Both `serial8250_tx_dma()` and `omap_8250_tx_dma()` call
     `dmaengine_prep_slave_sg()` *before* `serial8250_tx_dma_prepare_buffer()`
     copies the bytes and calls `dma_sync_single_for_device()`. The copy and
     DMA sync should happen before preparing the descriptor.
   - Syncing the full `UART_XMIT_SIZE` (4 KB) on every transfer (even 1 byte)
     is unnecessary overhead, especially with swiotlb or non-coherent DMA.
   - If we are copying out of `xmit_fifo` into a dedicated `dma->tx_buf`
     anyway, why preserve the `kfifo` ring offsets (`sgl[2]`) instead of
     copying contiguously to `dma->tx_buf` at offset 0 and syncing only
     `dma->tx_size` bytes?

5. Please also consider splitting the Renesas RZ/N1 `serial8250_do_prepare_tx_dma()`
   `tx_size` ordering fix (`Fixes: 16b539ffb574`) and the `8250_omap`
   completion refactoring into separate preparatory patches rather than
   bundling them into a single `Fixes:` commit.

Thanks,
Raul