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

From
James Hilliard <james.hilliard1@gmail.com>
Date
2026-10-29 17:20:14
Also in
imx, linux-arm-kernel, linux-serial, lkml
Reply to
Andy Shevchenko, 2026-10-29
On Thu, Oct 29, 2026 at 8:24 AM Andy Shevchenko
[off-list ref] wrote:
On Thu, Oct 29, 2026 at 01:37:02AM -0600, James Hilliard wrote:
quoted
serial8250_release_dma() ignores the return from dmaengine_terminate_sync()
before freeing the coherent RX buffer, unmapping the TX FIFO and releasing
the channels. If the provider fails to stop a channel, the DMAengine
wrapper returns the error without synchronizing its callbacks. Hardware
or a detached callback can then access the released storage and state.

Checking the error and returning early would not preserve their lifetime:
serial core frees the TX FIFO after the driver's shutdown callback returns,
and port removal can then release the callback context as well.

Require successful synchronous termination of both channels before
releasing any DMA resources. Retry failed channels after a sleep outside
the port lock, leaving channels that have already stopped and drained
alone. Keep the shutdown gate and storage reservation in place throughout
the retries.

A permanently failed DMA engine blocks close or removal. This deliberately
retains the buffers, mappings, channels and callback context until the
provider can establish that they are no longer in use.
...
quoted
+     while (rx_ret || tx_ret) {
+             dev_err_ratelimited(p->port.dev,
+                                 "DMA termination failed: RX %d, TX %d; retrying\n",
+                                 rx_ret, tx_ret);
quoted
+             msleep(100);
Huh?!
quoted
+             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.

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
  
Thread (29 messages) 29 messages, 3 authors, last reply 2d ago
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help