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

[Andy Shevchenko]

On Tue, Nov 03, 2026 at 11:16:46AM -0700, James Hilliard wrote:
> On Tue, Nov 3, 2026 at 11:06 AM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> > 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.
> 
> dev_warn_ratelimited() uses a static ratelimit_state at the call site,
> shared by all ports. The intent was to limit warning bursts across ports
> and separate flushes, while warned only covers one flush.

Right and imagine if the two or more devices are trying to printing the same
message in parallel. What will this give to the user? Something like

	$DEVICE: ...error message...
	$FUNC: N callbacks suppressed

And one may not know how many devices are affected by this N callbacks that
suppressed.

> > > > > +                     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()?
> 
> It was intended to limit warning bursts across multiple ports and
> separate stalls, while warned prevented repeated messages for one
> stalled flush.
> 
> I'll use dev_warn() with the per-flush warned flag and one-second
> threshold instead. That keeps one warning per prolonged stall and lets
> each port report its own failure independently.

This sounds better.

> > > > > +             }
> > > > > +             /* 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