Re: [PATCH v9 7/8] serial: 8250: retire canceled RX DMA before reusing the receive buffer

[Raul E Rangel]

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