[PATCH v8 6/6] serial: 8250: retain DMA resources when termination fails

[James Hilliard]

On Sat, Oct 31, 2026 at 2:44 AM Andy Shevchenko
<andriy.shevchenko at linux.intel.com> wrote:
>
> On Fri, Oct 30, 2026 at 12:08:52PM -0600, James Hilliard wrote:
> > On Fri, Oct 30, 2026 at 11:54 AM Andy Shevchenko
> > <andriy.shevchenko at linux.intel.com> wrote:
> > > On Fri, Oct 30, 2026 at 11:09:53AM -0600, James Hilliard wrote:
> > > > On Fri, Oct 30, 2026 at 3:40 AM Andy Shevchenko
> > > > <andriy.shevchenko at linux.intel.com> wrote:
> > > > > On Thu, Oct 29, 2026 at 11:19:59AM -0600, James Hilliard wrote:
> > > > > > On Thu, Oct 29, 2026 at 8:24 AM Andy Shevchenko
> > > > > > <andriy.shevchenko at linux.intel.com> wrote:
> > > > > > > On Thu, Oct 29, 2026 at 01:37:02AM -0600, James Hilliard wrote:
>
> ...
>
> > > > > > > > +     while (rx_ret || tx_ret) {
> > > > > > > > +             dev_err_ratelimited(p->port.dev,
> > > > > > > > +                                 "DMA termination failed: RX %d, TX %d; retrying\n",
> > > > > > > > +                                 rx_ret, tx_ret);
> > > > > > >
> > > > > > > > +             msleep(100);
> > > > > > >
> > > > > > > Huh?!
> > > > > > >
> > > > > > > > +             if (rx_ret)
> > > > > > > > +                     rx_ret = dmaengine_terminate_sync(dma->rxchan);
> > > > > > > > +             if (tx_ret)
> > > > > > > > +                     tx_ret = dmaengine_terminate_sync(dma->txchan);
> > > > > > > > +     }
> > > > > > >
> > > > > > > This is unacceptable. Sleeping is okay, but value should really match the
> > > > > > > baudspeed and other parameters of the transfer.
> > > > > >
> > > > > > This sleep is backoff after dmaengine_terminate_sync() has returned
> > > > > > an error, rather than an estimate of UART transmission time. Each
> > > > > > retry asks the provider to stop the channel again; elapsed time is
> > > > > > never treated as evidence that the resources are safe to free.
> > > > > >
> > > > > > The particular 100 ms interval has no hardware-derived justification.
> > > > > > However, I don't yet see a sound way to derive the retry cadence from
> > > > > > baud rate either. RX can be waiting for input, TX can be held by flow
> > > > > > control, and callback retirement is a separate requirement from
> > > > > > moving bytes through the UART.
> > > > > >
> > > > > > Do you have a particular provider failure mode in mind where retrying
> > > > > > termination should follow the serial transfer timing? I'd like to
> > > > > > base the interval on the condition that can make termination succeed,
> > > > > > rather than substitute a wire-time estimate for an arbitrary delay.
> > > > >
> > > > > Termination of DMA sometimes require to clean up internal FIFOs and other state
> > > > > machines. That's what happens on HW side. In the software waiting for too much
> > > > > may lead to the very big undesired latencies. The value of 100ms sounds insane
> > > > > to me for the HSUART cases (megabaud range of speed). Also note, not every PCB
> > > > > design uses HW flow control, so this may be even worse for them.
> > > >
> > > > The 100 ms interval can add unnecessary teardown latency if a failed
> > > > termination becomes retryable much sooner. There is no hardware-derived
> > > > justification for that particular value.
> > > >
> > > > The FIFO/state-machine draining is part of the provider's stop sequence.
> > > > For example, DW performs its FIFO-empty polling inside
> > > > dwc_terminate_all(). If termination succeeds, this patch proceeds without
> > > > the retry sleep, regardless of baud rate. The sleep is only reached
> > > > after an error during DMA release, not during normal RX delivery.
> > > >
> > > > Baud rate can influence how quickly a transmit FIFO drains, even without
> > > > flow control. However, the termination error does not tell 8250 whether
> > > > that drain is still progressing, how much remains, or whether the
> > > > controller needs a different recovery operation. That distinction matters
> > > > when choosing a retry interval: a shorter interval reduces the added
> > > > latency, but does not establish that retrying can recover the channel.
> > > >
> > > > There is also a specific API detail here: dmaengine_terminate_sync()
> > > > returns immediately if device_terminate_all() fails, without reaching
> > > > dmaengine_synchronize(). Consequently, an error leaves callback draining
> > > > unconfirmed as well. The retry cadence can change, but resource release
> > > > still requires successful termination and synchronization.
> > >
> > > Most likely this is AI slop. I can't see the justification for 100ms.
> > > Can you explain that?
> >
> > 100 ms was an arbitrary backoff to avoid a tight retry loop if
> > termination keeps failing. It was not derived from the baud rate,
> > FIFO depth, or a DMA provider's recovery timing.
> >
> > That motivation explains sleeping between retries, but not choosing
> > 100 ms. I don't have evidence supporting that particular interval.
>
> Yep. And that's what my point is about. Any big sleeps has to be explained
> and justified.

100 ms was an arbitrary backoff for what I expected to be a rarely
exercised termination-error path. It avoids a tight retry loop, but
neither that nor the expected rarity justifies the particular value.
Successful termination on the first attempt adds no retry delay.

The constraint is that failed termination does not establish that DMA
and its callbacks have stopped. Returning from the current shutdown path
would let the serial core free the TX buffer while DMA might still own
it. Retrying preserves that ownership, but a permanently failing provider
can block shutdown indefinitely.

Would you suggest a different approach to handling termination failure
here, or a better basis for choosing the retry interval?

>
> > > > > > Regardless of the retry interval, shutdown cannot release the buffers
> > > > > > until the provider establishes that DMA and its callbacks have stopped.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>