Re: [PATCH v2 4/4] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[James Hilliard]

On Fri, Oct 23, 2026 at 2:30 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 22, 2026 at 06:34:05PM -0600, James Hilliard wrote:
> > The RX timeout path pauses a transfer, copies its data and terminates it
> > asynchronously. Clearing rx_running at that point lets another receive
> > reuse the buffer and cookie while the old callback is still waiting for
> > the port lock. Inspecting the new cookie cannot distinguish that callback
> > from the completion of a new transfer.
> >
> > Reserve the receive buffer until a worker has drained the canceled
> > callbacks. Deliver the paused data before allowing the interrupt handler
> > to drain the FIFO through PIO. Normal retirement only releases ownership;
> > leave subsequent receive handling to the UART interrupt instead of reading
> > LSR and restarting DMA from the worker.
> >
> > Reject new flush work during shutdown, including late Intel MID DMA
> > timeouts. Serialize those timeout callers with the port lock. Stop and
> > drain retirement work before freeing the channels and buffer.
> >
> > Check pause failures before accessing the buffer. A successful pause is
> > the quiescence guarantee, even when a provider still reports IN_PROGRESS.
> > On pause, status or termination failure, mask receive interrupts and let
> > the worker stop DMA before handing the FIFO to PIO. Preserve stop_rx() and
> > shutdown while restoring deferred interrupts, and hold a runtime PM
> > reference around worker register accesses. Keep RX on PIO until reopen.
> >
> > Abort the DesignWare idle sequence when RX cannot be stopped. Preserve its
> > empty-FIFO timeout workaround during PIO recovery. If the provider still
> > cannot terminate the channel in process context, retain its buffer and
> > leave receive interrupts masked.
>
> ...
>
> > struct uart_8250_dma {
>
> >       bool                    shutting_down;
> >       bool                    tx_terminating;
> >       bool                    tx_flushed;
> > +     bool                    rx_terminating;
> > +     bool                    rx_err;
>
> If they are protected by the same lock, we may actually move to bitfields.
> Example:
>
>         unsigned int            rx_err:1;
>
> > +     /* Receive interrupt enables deferred during RX DMA recovery. */
> > +     u8                      rx_ier;
> >  };
>
> ...
>
> >  static void dma_rx_complete(void *param)
> >  {
> >       struct uart_8250_port *p = param;
> >       struct uart_8250_dma *dma = p->dma;
> > +     struct dma_tx_state state;
> > +     enum dma_status status;
> >       unsigned long flags;
> >
> >       uart_port_lock_irqsave(&p->port, &flags);
> > -     if (dma->shutting_down)
> > +     if (dma->shutting_down || dma->rx_terminating)
> >               goto out;
>
> Again repeating the same idiom you have in previous patch.
>
> > -     if (dma->rx_running)
> > -             __dma_rx_complete(p);
> > +     if (dma->rx_running) {
> > +             status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> > +             if (status == DMA_ERROR) {
> > +                     serial8250_rx_dma_recover(p);
> > +                     goto out;
> > +             }
> > +             if (status != DMA_COMPLETE)
> > +                     goto out;
> > +             serial8250_rx_dma_copy(p, &state);
> > +             dma->rx_running = 0;
> > +     }
> >
> >       /*
> > -      * Cannot be combined with the previous check because __dma_rx_complete()
> > -      * changes dma->rx_running.
> > +      * The completed transfer no longer owns the receive buffer.
> >        */
> > -     if (!dma->rx_running && (serial_lsr_in(p) & UART_LSR_DR))
> > +     if (!dma->rx_running && (p->ier & UART_IER_RDI) &&
> > +         (serial_lsr_in(p) & UART_LSR_DR))
>
> This has a side-effect. Earlier we do read LSR_DR, but now only if _RDI is set.
> Would it be problematic?

The RDI check is intended to prevent a completion from restarting RX DMA
after stop_rx(). That only requires gating resubmission. I'll preserve
the previous LSR read when rx_running is clear, then check RDI before
submitting another transfer.

>
> >               p->dma->rx_dma(p);
> >  out:
> >       uart_port_unlock_irqrestore(&p->port, flags);
> >  }
>
> ...
>
> > +     dma->rx_terminating = false;
> > +     dma->rx_running = 0;
> > +     if (dma->shutting_down)
> > +             return;
>
> > +     if (dma->rx_err) {
>
> Perhaps
>
>         if (!dma->rx_err)
>                 return;
>
> ?
>
> > +             /* PIO may now drain the FIFO; stop_rx() can cancel these enables. */
> > +             p->ier |= dma->rx_ier;
> > +             dma->rx_ier = 0;
> > +             serial_port_out(&p->port, UART_IER, p->ier);
> > +             dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
> > +     }
> > +}
>
> ...
>
> > +     /* Some providers still report IN_PROGRESS after a successful pause. */
>
> Yeah, we need to address that. (I used to have some patches locally,
> but they were rebased uncountable number of times without looking at
> the result of those rebases, so it's probably rotten enough and may
> not be used anymore.)
>
> ...
>
> > -      * This problem has only been observed so far when not in DMA mode
> > -      * so we limit the workaround only to non-DMA mode.
> > +      * Only read the FIFO when DMA no longer owns it, including PIO fallback
> > +      * after a DMA error.
>
> > -     if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) && !up->dma && rx_timeout) {
> > +     if (!(quirks & DW_UART_QUIRK_SKIP_EMPTY_FIFO_READ) &&
> > +         (!up->dma || !up->dma->rx_running) && rx_timeout) {
> >               status = serial_lsr_in(up);
>
> This is a good question. We need to have this be tested on the affected
> platforms.

My hardware tests used H616 with additional local UART and sun6i DMAengine
changes applied.

There is also a distinction the current condition misses: rx_running
remains set while callbacks are being retired, even after DMA has stopped
accessing the FIFO. I'll make the FIFO-ownership check explicit rather
than using buffer retirement as a proxy. The dummy read must remain
excluded while DMA can access the FIFO. That revised handling needs
targeted testing of the empty-FIFO timeout case.

I'll also apply the early-return and shared-helper cleanups. Before
packing the flags into bitfields, I'll check the worker accesses as well
as the port-locked paths.

>
> ...
>
> >       guard(serial8250_rpm)(up);
> >
> > +     if (up->dma)
> > +             up->dma->rx_ier = 0;
>
> Now we have a copy of IER in UART port and in DMA structure. Why do we need
> both?

rx_ier records the receive-enable bits temporarily removed from up->ier
during recovery. Those interrupts must stay masked while DMA may still
access the FIFO, but recovery needs to distinguish that temporary mask
from an intentional stop_rx().

The separate saved enables make that distinction cumbersome, especially
when termios or RS485 changes IER during recovery. I'll rework this around
one desired-enable state and consistent application of the temporary
recovery mask, so the worker cannot restore an obsolete receive-enable
decision.

>
> >       up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> >       serial_port_out(port, UART_IER, up->ier);
>
> --
> With Best Regards,
> Andy Shevchenko
>
>