Hi James,
Thanks for v9 — fixing asc_startup() locking in Patch 2/8, waking
tty->write_wait for drain waiters in Patch 5/8, splitting OMAP RX locking
and IRQ quiescing into Patches 3/8 and 4/8, and adding rx_pause_wait with
immediate wakeups from dma_rx_complete() and serial8250_do_shutdown() all
look good. Waking rx_pause_wait under port->lock cleanly avoids delaying
normal completion or cancel_work_sync(&dma->rx_flush_work) during shutdown.
Two remaining comments on Patch 7/8:
1. Escalating to serial8250_rx_dma_recover() after a bounded pause-stall
window on partial RX transfers:
The commit message notes:
> A provider which never pauses or completes retains RX ownership until
> an error or shutdown; a timeout alone cannot establish that discarding
> the buffer is safe.
I completely agree that a timeout alone cannot establish that *reusing*
or *freeing* dma->rx_buf is safe while the channel is still running.
However, calling serial8250_rx_dma_recover(p) under p->port.lock once
the retry budget expires does *not* reuse or free dma->rx_buf without
stopping the channel:
- Calling serial8250_rx_dma_recover(p) under p->port.lock inside the
worker's retry block sets dma->rx_err = true and
dma->rx_terminating = true (and keeps RX masked in UART_IER), which
causes serial8250_rx_dma_flush_work() to exit the for (;;) loop
(retry = false, recover = true) and call
dmaengine_terminate_sync(dma->rxchan) outside p->port.lock in process
context (8250_dma.c:233-239).
- If dmaengine_terminate_sync(dma->rxchan) succeeds (ret == 0), the
channel is stopped and its callbacks are drained, so clearing
rx_pause_pending/rx_terminating and restoring p->ier safely falls back
to PIO ("RX DMA failed, using PIO\n").
- If dmaengine_terminate_sync(dma->rxchan) also fails (ret != 0),
lines 235-239 already retain dma->rx_buf and return early with RX
interrupts masked.
Why does this matter in practice?
Look at drivers/dma/tegra186-gpc-dma.c:
- Line 1521 unconditionally assigns .device_pause = tegra_dma_device_pause,
so dma_get_slave_caps() (drivers/dma/dmaengine.c:614) sets
caps.cmd_pause = true and serial8250_request_dma() enables RX DMA.
- On Tegra186, tegra186_dma_chip_data sets .hw_support_pause = false
(line 1351), so tegra_dma_device_pause() (lines 420-423) unconditionally
returns -ENOSYS. (On Tegra194/234/264, tegra_dma_pause() at lines
389-414 polls TEGRA_GPCDMA_STATUS_BUSY after setting
TEGRA_GPCDMA_CHAN_CSRE_PAUSE for 20 us and returns -ETIMEDOUT if the
channel does not pause.)
When an RX timeout interrupt (UART_IIR_RX_TIMEOUT) fires on a partial
transfer (e.g. a few bytes typed at an interactive console or a short
command waiting for a reply, count < 4096), hardware transfer completion
will never occur while the remote peer is waiting for our response. With
an unbounded for (;;) loop:
- Those partial bytes in dma->rx_buf are never delivered anyway (even if
we wait forever until the user closes the port, serial8250_release_dma()
terminates the channel without copying dma->rx_buf).
- UART RX remains permanently dead with RLSI/RDI masked and never falls
back to PIO.
- serial8250_rx_dma_flush_work() holds a runtime PM reference
(guard(serial8250_rpm)(p)) for the lifetime of the open port.
- On 8250_dw, dw8250_idle_enter() (8250_dw.c:151) sees
serial8250_rx_dma_flush(up) == -EAGAIN and silently aborts
dw8250_set_divisor() and dw8250_check_lcr(), breaking baud-rate and
LCR updates while rx_pause_pending is stuck.
If serial8250_rx_dma_flush_work() escalates to serial8250_rx_dma_recover(p)
under p->port.lock once the stall threshold is reached (or immediately when
dmaengine_pause() returns -ENOSYS / -EOPNOTSUPP), dmaengine_terminate_sync()
safely stops the channel before any buffer state is released and the UART
recovers on PIO instead of wedging until close().
2. Minor nit in drivers/tty/serial/8250/8250_omap.c:783-784:
In omap_8250_shutdown():
WRITE_ONCE(up->dma->rx_pause_wakeup, true);
wake_up(&up->dma->rx_pause_wait);
8250_omap uses its own synchronous omap_8250_rx_dma_flush() and never
schedules rx_flush_work or sleeps on rx_pause_wait, so those two lines
in omap_8250_shutdown() can be dropped.
Thanks,
Raul
