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

[Raul E Rangel]

Hi James,

Beyond the two issues Sashiko already flagged (`dma->shutting_down` not
checked in `serial8250_rx_dma_flush()`, and `DMA_IN_PROGRESS` rejected after
`dmaengine_pause()` leading to an unhandled `UART_IIR_RX_TIMEOUT` interrupt
storm when `!serial8250_rx_dma_flush(up)` returns `false` in `handle_rx_dma()`),
there are a few additional issues here:

> -static void __dma_rx_complete(struct uart_8250_port *p)
> +static enum dma_status serial8250_rx_dma_complete(struct uart_8250_port *p)
>  {
...
> -	ret = tty_insert_flip_string(tty_port, dma->rx_buf, count);
> -	p->port.icount.rx += ret;
> -	p->port.icount.buf_overrun += count - ret;
> +	count = dma->rx_size - state.residue - dma->rx_pos;
> +
> +	if (tty_insert_flip_string(tty_port, dma->rx_buf + dma->rx_pos,
> +				   count) != count)
> +		p->port.icount.buf_overrun++;
> +	p->port.icount.rx += count;
> +	dma->rx_pos += count;

1. Accidental revert of prerequisite patch [2]:
   This chunk reverts your prerequisite patch ("serial: 8250: account for
   partial receive DMA delivery to TTY") in `8250_dma.c`, restoring the old
   `icount.rx += count` and `icount.buf_overrun++` accounting.

> +static void serial8250_rx_dma_flush_work(struct work_struct *work)
> +{
> +	struct uart_8250_dma *dma = container_of(work, struct uart_8250_dma,
> +						 rx_flush_work);
> +	struct uart_8250_port *p = dma->port;
> +
> +	dmaengine_synchronize(dma->rxchan);
> +
> +	guard(uart_port_lock_irqsave)(&p->port);
> +	dma->rx_terminating = false;
> +	dma->rx_running = 0;
> +	if (!dma->shutting_down && (p->ier & UART_IER_RDI) &&
> +	    (serial_lsr_in(p) & UART_LSR_DR))
> +		dma->rx_dma(p);
> +}

2. Spurious RX DMA restart during `dw8250_idle_enter()`:
   In `drivers/tty/serial/8250/8250_dw.c`, `dw8250_idle_enter()` sets
   `d->in_idle = 1` and writes `0` to hardware `UART_IER`, but intentionally
   leaves `UART_IER_RDI` set in `up->ier` so `dw8250_idle_exit()` can restore
   it. `dw8250_idle_enter()` then calls `serial8250_rx_dma_flush(up)`, which
   schedules `rx_flush_work`. If `rx_flush_work` runs while `d->in_idle == 1`
   (while the port is in loopback mode reconfiguring LCR/divisors),
   `p->ier & UART_IER_RDI` is still true and `rx_flush_work` will restart
   `dma->rx_dma(p)` in the middle of the idle sequence.

3. Missing runtime PM reference and `serial_lsr_in()` side effects in
   `serial8250_rx_dma_flush_work()`:
   - `serial8250_rx_dma_flush_work()` runs on `system_wq` and reads
     `serial_lsr_in(p)` / starts `dma->rx_dma(p)` without acquiring
     `serial8250_rpm_get(p)` / `serial8250_rpm_put(p)`. After the RX timeout
     ISR exits and drops its runtime PM reference, the port can autosuspend
     before `rx_flush_work` executes.
   - Reading `serial_lsr_in(p)` outside IRQ context clears any pending
     `UART_IIR_RLSI` in hardware and starts RX DMA before any saved LSR error
     bits (`OE`/`PE`/`FE`/`BI`) are processed with the corresponding byte in
     the FIFO.
   - If `UART_IIR_RDI` is enabled in hardware, why does `rx_flush_work` need
     to poll `UART_LSR_DR` and restart `rx_dma(p)` at all rather than letting
     the UART interrupt handler start RX DMA (or drain via PIO) when data is
     present?

4. `dmaengine_resume()` on error in `serial8250_rx_dma_flush()`:
   `serial8250_request_dma()` only checks `caps.cmd_pause` and
   `caps.cmd_terminate`, not `caps.cmd_resume` (e.g. `pl330` and
   `mtk-uart-apdma` implement pause to read residue before termination, but
   do not implement `.device_resume`). Also, on Renesas RZ/N1
   (`DW_UART_QUIRK_IS_DMA_FC`), `dw8250_handle_irq()` has already cleared
   `RZN1_UART_RDMACR = 0` before calling `serial8250_rx_dma_flush()`, so the
   error-recovery path leaves RX DMA disabled.

Thanks,
Raul