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

[James Hilliard]

On Thu, Oct 22, 2026 at 8:00 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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?

The problem is ownership of the serial core's transmit storage across
flush. uart_flush_buffer() resets the FIFO before calling the driver,
and writers can reuse that storage after the callback returns.

dmaengine_terminate_async() does not guarantee that the transfer or its
completion callback has finished when it returns. The driver's flush
callback holds the port spinlock, so it cannot wait there using
dmaengine_synchronize(). This lifetime gap therefore exists even with
a provider implementing the DMAengine contract correctly.

The bounce buffer in v1 protected the transfer's source from those new
writes, but it added an allocation and copying to normal transmission.

> ...
>
> 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?

I've reworked TX in v2[0] around a serial-core storage reservation instead.
The logical FIFO is still flushed immediately, but the driver can reserve
its storage until DMA and the old callbacks have retired. During that
interval write_room() reports zero, and write()/put_char() accept no data.

Normal transmission continues to DMA directly from the FIFO. A worker
performs synchronization outside the port lock, returns the mapping to
CPU ownership and releases the reservation. This removes the private TX
buffer and copying while retaining the required lifetime protection.

[0] https://lore.kernel.org/r/20261022-submit-serial-8250-dma-cancel-v1-v2-0-088ea51e3891@xxxxxxxxx/

>
> --
> With Best Regards,
> Andy Shevchenko
>
>