Re: [PATCH v9 7/8] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[Andy Shevchenko]

On Tue, Nov 03, 2026 at 10:54:38AM -0700, James Hilliard wrote:
> On Tue, Nov 3, 2026 at 10:34 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > On Mon, Nov 02, 2026 at 04:14:34PM -0700, James Hilliard wrote:

...

> > > +static void serial8250_rx_dma_flush_work(struct work_struct *work)
> > > +{
> > > +     struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma, rx_flush_work);
> > > +     struct uart_8250_port *p = dma->port;
> > > +     unsigned int delay_us = USEC_PER_MSEC;
> > > +     ktime_t start = ktime_get();
> > > +     bool recover, retry, restore_rx;
> > > +     bool warned = false;
> > > +     int ret;
> > > +
> > > +     guard(serial8250_rpm)(p);
> > > +
> > > +     for (;;) {
> > > +             scoped_guard(uart_port_lock_irqsave, &p->port) {
> > > +                     /* Shutdown cancels this work and drains both DMA channels. */
> > > +                     if (dma->shutting_down)
> > > +                             return;
> > > +                     /* Serialize reset with completion and shutdown notifications. */
> > > +                     WRITE_ONCE(dma->rx_pause_wakeup, false);
> > > +                     retry = dma->rx_pause_pending && !dma->rx_terminating;
> > > +                     if (retry) {
> > > +                             ret = serial8250_rx_dma_try_flush(p);
> > > +                             retry = dma->rx_pause_pending && !dma->rx_terminating;
> > > +                     }
> > > +                     recover = dma->rx_err;
> > > +             }
> > > +             if (!retry)
> > > +                     break;
> > > +             if (!warned && ktime_ms_delta(ktime_get(), start) >= MSEC_PER_SEC) {
> > > +                     dev_warn_ratelimited(p->port.dev,
> > > +                                          "RX DMA pause stalled: %d; retaining transfer and retrying\n",
> > > +                                          ret);
> >
> > _ratelimited() is a slop of AI without understanding the code. It makes nonsense
> > in combination with warned.
> 
> The two controls have different scopes. warned is local to one invocation
> of the flush worker and prevents repeated warning attempts while that
> flush remains stalled. A later flush starts with warned cleared again.
> 
> The rate limit applies across worker invocations and UART ports. It limits
> messages from separate stalls, rather than repeated retries of the same
> stall.

How? The whole _ratelimited() is nonsense here.

> > > +                     warned = true;
> >
> > Altogether NIH _once().
> 
> dev_warn_once() uses a static flag at the call site, shared by all ports.
> After the first warning, subsequent stalls would remain silent even after
> recovery or reopening the port, including stalls on another UART.
> 
> The intended behavior here is at most one warning attempt per prolonged
> stall, while keeping later incidents diagnosable and limiting their
> aggregate logging rate.

Yes, so what the point to use _ratelimited()?

> > > +             }
> > > +             /* Notifications also cover completion or shutdown before sleeping. */
> > > +             wait_event_hrtimeout(dma->rx_pause_wait, READ_ONCE(dma->rx_pause_wakeup),
> > > +                                  ns_to_ktime(delay_us * NSEC_PER_USEC));
> > > +             delay_us = min(delay_us * 2, USEC_PER_SEC);
> > > +     }
> > > +
> > > +     if (recover) {
> > > +             ret = dmaengine_terminate_sync(dma->rxchan);
> > > +             if (ret) {
> > > +                     /* Retain ownership and keep RX masked if DMA cannot be stopped. */
> > > +                     dev_err(p->port.dev, "failed to stop RX DMA: %d\n", ret);
> > > +                     return;
> > > +             }
> > > +     } else {
> > > +             dmaengine_synchronize(dma->rxchan);
> > > +     }
> > > +
> > > +     scoped_guard(uart_port_lock_irqsave, &p->port) {
> > > +             restore_rx = recover || dma->rx_pause_pending;
> > > +             dma->rx_pause_pending = false;
> > > +             dma->rx_terminating = false;
> > > +             dma->rx_running = 0;
> > > +             if (dma->shutting_down)
> > > +                     return;
> > > +
> > > +             if (!restore_rx)
> > > +                     return;
> > > +
> > > +             /* Apply the current enables, including any intervening stop_rx(). */
> > > +             serial8250_out_IER(p, p->ier);
> > > +     }
> > > +     if (recover)
> > > +             dev_warn(p->port.dev, "RX DMA failed, using PIO\n");
> > >  }

-- 
With Best Regards,
Andy Shevchenko