Hi James,
Thanks for replacing the fixed 100 ms sleep with exponential backoff
(1 ms -> 1 s) and adding the initial/ongoing/recovery diagnostics in v9.
Following up on our v8 discussion regarding blocking in
serial8250_terminate_dma() versus non-blocking resource quarantine:
1. Why non-blocking quarantine cannot be done purely inside 8250_dma:
I agree with your v8 reply and the updated commit message here that
serial8250_release_dma() cannot simply return early while leaving
serial_core to proceed with normal teardown:
a) Immediately after serial8250_release_dma(up) returns in
serial8250_do_shutdown() (8250_port.c:2474-2511, and similarly in
omap_8250_shutdown() at 8250_omap.c:794-803), the 8250 driver itself
clears the hardware FIFOs (serial8250_clear_fifos()), reads UART_RX,
frees the IRQ (up->ops->release_irq(up)), and drops runtime PM
(serial8250_rpm_put_tx(up) / serial8250_rpm_put(up)).
b) In uart_tty_port_shutdown() (serial_core.c:1841-1853), immediately
after uart_port_shutdown(port) returns, serial_core unconditionally
calls uart_free_xmit_buf(port) (line 1850) — freeing the page that
dma->tx_addr is still DMA-mapped to — and powers down the UART via
uart_change_pm(state, UART_PM_STATE_OFF) (line 1852). Likewise,
uart_suspend_port() (serial_core.c:2502-2514) calls ops->shutdown(uport)
(line 2502) and unconditionally sets UART_PM_STATE_OFF (line 2512).
c) Even if startup() returns -EIO on reopen when DMA is still active,
uart_port_startup() (serial_core.c:422-423) has:
if (retval && capable(CAP_SYS_ADMIN))
return 1;
which causes uart_port_activate() (serial_core.c:2060-2062) to set
tty_port_set_active(port, true) with tty_port_initialized(port) == false
and allows root open() (serial_core.c:2040-2042) to succeed (0). From
that state, ioctl(TIOCSSERIAL) -> uart_set_info() (serial_core.c:1068)
-> uart_shutdown() (serial_core.c:466-511, which calls
uart_free_xmit_buf() at line 510 outside if (tty_port_initialized))
or uart_hangup() (serial_core.c:1942-1950) or uart_change_port() ->
release_port() (serial_core.c:932-933, 1072) can free the TX buffer,
power down the UART, and release MMIO mappings while DMA is active.
We modeled these cross-layer paths in Serial8250TxDmaV8.tla
(QuarantineGuardsShutdown, QuarantineGuardsAdminOpen, and
NoDmaOnPoweredOffUart in Serial8250TxDmaV8_UnguardedQuarantineHazard.cfg
and Serial8250TxDmaV8_AdminReopenQuarantineHazard.cfg vs.
Serial8250TxDmaV8_TerminateRetryHang_Fixed.cfg, alongside
Serial8250TxDmaV8_QuarantinePioReopenHazard.cfg at
https://github.com/ismell/linux-8250-dma-tla), and TLC confirms that
quarantine without those serial_core and PM guards violates
NoDmaOnPoweredOffUart / NoConcurrentFifoAccess (in 6 states) and
NoUsersAtFree (in 7 states). Given that enforcing non-blocking quarantine
requires coordinated changes across serial_core.c, runtime PM, and 8250
shutdown, addressing non-blocking quarantine in a separate follow-up
series and documenting the limitation here makes sense.
2. What in-tree DMAengine providers actually return from terminate_all:
For context on when the retry loop in serial8250_terminate_dma() can
trigger, I audited all 78 .device_terminate_all implementations in
drivers/dma/:
- 64 out of 78 unconditionally return 0.
- Among DMA controllers used with 8250 UARTs, synchronous FIFO-draining
drivers (drivers/dma/dw/core.c dwc_chan_pause ~40 us,
drivers/dma/idma64.c idma64_chan_deactivate ~100 us) already poll in
microseconds inside terminate_all and return 0.
- The only 8250-paired DMA controllers that can return non-zero are:
* drivers/dma/tegra186-gpc-dma.c (tegra_dma_terminate_all, lines
714-743), which already polls readl_relaxed_poll_timeout_atomic()
for up to 5000 us (5 ms) in tegra_dma_stop_client() / tegra_dma_pause()
/ tegra_dma_pause_noerr() (lines 389-414, 676-712) before returning
-ETIMEDOUT; and
* drivers/dma/at_xdmac.c (at_xdmac_device_terminate_all, lines
1999-2039), which only returns < 0 if pm_runtime_resume_and_get()
fails.
(Even across all 14 non-zero .device_terminate_all implementations in
drivers/dma/, the only one designed around a caller retry loop is
cppi41_stop_chan() -> cppi41_tear_down_chan() in drivers/dma/ti/cppi41.c:
652-770, which is a USB MUSB controller used only by musb_cppi41.c.)
Thanks,
Raul
