On Fri, Oct 30, 2026 at 11:36 AM Raul E Rangel <[email protected]> wrote: > > On Thu, Oct 29, 2026 at 6:35 PM James Hilliard <[email protected]> > wrote: > > That is the current behavior. Returning from shutdown allows serial > > core to free the TX buffer and removal to release the callback > > context. The retry loop prevents those releases, but a permanent > > termination failure blocks teardown indefinitely > ... > > Retaining the buffers and channels does not preserve the complete > > callback context. > > > > The callbacks carry a uart_8250_port pointer. For DW, the DMA state is > > embedded in devm-managed private data, which can be freed after > > unbind. The registered 8250 port slot can also be reused. Retaining > > the DMA channel does not prevent either operation. > ... > > Clearing up->dma does not stop hardware accesses. After unsuccessful > > termination, the channel may still access the UART FIFO, so reopening > > with PIO can introduce concurrent FIFO access. > > > > A hardware-specific stop/reset sequence could establish quiescence, > > but its implementation and effects on other channels depend on the > > controller. If the channel cannot be stopped, quarantine also needs > > to preserve callback-context lifetimes and prevent unsafe port reuse. > ... > > The current callbacks fetch p->dma before acquiring the port lock. > > A callback can therefore retain the old non-NULL pointer across the > > assignment of NULL. Checking that cached pointer does not protect > > its backing allocation from being freed. > > > > When termination returns an error, dmaengine_terminate_sync() also > > returns without calling dmaengine_synchronize(), so callback draining > > has not been established. > > Those are all good points. We added port reopen and a `NoConcurrentFifoAccess` > invariant to `Serial8250TxDmaV8.tla` > (`Serial8250TxDmaV8_QuarantinePioReopenHazard.cfg`), and TLC immediately > confirmed your point on FIFO contention: clearing `up->dma = NULL` and > falling back to PIO when reopening after a failed `dmaengine_terminate_sync()` > violates `NoConcurrentFifoAccess` in 7 states because the un-terminated DMA > descriptor can still access the hardware UART FIFO concurrently with PIO. > > We can satisfy all three constraints while still preventing normal `close()`, > `hangup()`, and system `suspend()` from hanging forever in > `TASK_UNINTERRUPTIBLE` under `port->mutex` across the three lifecycle edges:
Keeping the DMA state intact and refusing PIO reuse addresses the earlier reopen hazard. However, implementing quarantine safely requires changes beyond retaining the buffers and rejecting startup. For example, serial8250_do_shutdown() clears the FIFOs and reads UART_RX after serial8250_release_dma() returns. Those operations also need to be excluded while the retained channel may still access the UART. Close and suspend subsequently enter power-down paths, so retaining the allocations alone does not establish that either transition is safe. There is also a serial-core exception to the proposed reopen contract: startup errors permit an administrative open for CAP_SYS_ADMIN. That allows reconfiguration paths which can release port resources. Returning -EIO from the driver startup callback therefore does not by itself enforce quarantine. > > 1. **On `close()` / `hangup()` / `uart_suspend_port()` > (`serial8250_release_dma()` & `uart_free_xmit_buf()`)**: > - Bound the retry loop in `serial8250_terminate_dma()` (with a short > backoff rather than an unbounded `msleep(100)`, guarding > `if (dma->rxchan)` / `if (dma->txchan)`) and release whichever > channel(s) terminated cleanly (`rx_ret == 0` or `tx_ret == 0`). > - If `rx_ret != 0` or `tx_ret != 0`, **keep `up->dma` intact** (do not set > `up->dma = NULL`, including in `8250_omap.c:omap_8250_shutdown()`), keep > `dma->shutting_down = true`, and retain the failed channel and its > buffer (`dma->rxchan` / `dma->rx_buf` when `rx_ret != 0`; `dma->txchan` > and mapped `dma->tx_addr` when `tx_ret != 0`). > - When `tx_ret != 0`, explicitly call `uart_xmit_block(&p->port)` under > `p->port.lock` (`state->xmit_blocked = true`, since `xmit_blocked` is > otherwise only set when `uart_flush_buffer()` ran prior to `close()`), > and in `serial_core.c:uart_free_xmit_buf()`, return early under > `uport->lock` if `state->xmit_blocked` is set (without clearing > `port->xmit_buf = NULL`, resetting `port->xmit_fifo`, or calling > `kfree(xmit_buf)`). > - Because `up->dma` is not cleared and `struct uart_8250_dma` / > `struct uart_8250_port` remain allocated on the bound device during > `close()`, `hangup()`, and `suspend()`, any callback that was not > drained by `dmaengine_terminate_sync()` safely loads a valid `p->dma`, > observes `dma->shutting_down == true` under `port->lock`, and returns > without UAF—while `close()`, `hangup()`, and system `suspend()` return > cleanly without hanging in `D` state or triggering `hung_task_panic`. > > 2. **On subsequent `open()` (`serial8250_do_startup()` & > `omap_8250_startup()`)**: > - At the very start of `serial8250_do_startup()` (`8250_port.c:2356`) and > `omap_8250_startup()` (`8250_omap.c:724`)—after acquiring runtime PM, > and **before** clearing `up->dma->shutting_down = false`, touching > `UART_FCR` / `UART_RX` / `UART_TX`, or setting up IRQs/timers—if > `up->dma && (up->dma->rxchan || up->dma->txchan)` is still non-NULL from > a prior failed termination, retry `serial8250_release_dma(up)` first: > - If `up->dma->rxchan || up->dma->txchan` is **still** non-NULL > (termination still failed), immediately return `-EIO` and **fail > `startup()`** (do *not* clear `up->dma = NULL` and do *not* fall back > to PIO while the wedged DMA channel may still touch the UART FIFO). > - If termination succeeded on retry (`rxchan == NULL && txchan == NULL`, > which also unblocked `state->xmit_blocked`), reset `xmit_fifo` and > proceed with normal startup and `serial8250_request_dma(up)`. > > 3. **On driver unbind (`serial8250_unregister_port()` / > `serial_core_remove_one_port()`)**: > - Driver removal is the *only* point where `devm`-allocated > `struct uart_8250_dma` and the `serial8250_ports[]` slot are about to be > freed/reused (note also that `serial_core_remove_one_port()` at > `serial_core.c:3336` only calls `uart_shutdown()` when > `tty_port_initialized(port)` is true, so if the port was already closed > with a quarantined channel, unbind must still drain/release it). > - Restricting blocking termination wait to driver unbind (when a channel is > still active/quarantined) prevents `close()`, `hangup()`, and system > `suspend()` from ever hanging the kernel in `msleep(100)`. This looks like a useful direction, but it introduces a broader lifecycle contract across serial core, the individual drivers and power management. In particular, suspend may need to fail until the hardware is quiescent, rather than succeed with a quarantined channel. The current retry loop still allows permanent termination failure to block teardown; changing the backoff does not resolve that limitation. Would it make sense to address quarantine separately, with that limitation documented explicitly here? I would prefer not to fold this larger redesign into the cancellation series before those paths have been worked through. > > We verified this quarantine + `-EIO` reopen contract in > `Serial8250TxDmaV8_TerminateRetryHang_Fixed.cfg` (`AllowReopen = TRUE`, > `ReopenFallbackToPio = FALSE`), which passes all safety and liveness > properties (`NoUsersAtFree`, `BoundedTerminateRetries`, > `NoConcurrentFifoAccess`, `QuarantineImpliesXmitBlocked`, `ShutdownProgress`) > with 0 errors: > https://github.com/ismell/linux-8250-dma-tla > > Thanks, > Raul
