On Tue, Nov 03, 2026 at 10:54:38AM -0700, James Hilliard wrote:
> On Tue, Nov 3, 2026 at 10:34 AM Andy Shevchenko
> <[email protected]> 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
