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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] serial: 8250: serial8250_rx_dma_flush() returns -ESHUTDOWN when DMA is stopped
- [High] serial: 8250: serial8250_rx_dma_flush() drops data when pause fails on complete
- [High] serial: 8250_dw: DLAB write failure in dw8250_set_divisor() corrupts UART_IER
--

commit f21a925e2ab02b4a2962f22b9f5a07f563e44794
Author: James Hilliard <james.hilliard1@gmail.com>

serial: 8250: retire canceled RX DMA before reusing the receive buffer

This commit retires canceled RX DMA callbacks via a work item before
allowing the receive buffer to be reused, separates FIFO ownership from
callback retirement, and adds recovery and shutdown handling for RX DMA
failures.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index 611e4dee98297..2485029b989c4 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
[ ... ]
> @@ -262,16 +308,49 @@ int serial8250_rx_dma(struct uart_8250_port *p)
>  	return 0;
>  }
>  
> -void serial8250_rx_dma_flush(struct uart_8250_port *p)
> +int serial8250_rx_dma_flush(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma *dma = p->dma;
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	int ret;
>  
> -	if (!dma || !dma->rxchan || !dma->rx_running)
> -		return;
> +	lockdep_assert_held_once(&p->port.lock);
>  
> -	dmaengine_pause(dma->rxchan);
> -	__dma_rx_complete(p);
> -	dmaengine_terminate_async(dma->rxchan);
> +	if (!dma)
> +		return 0;
> +	/* A late interrupt must not hand a still-active receiver to PIO. */
> +	if (dma->shutting_down)
> +		return -ESHUTDOWN;
> +	if (!dma->rx_running)
> +		return 0;

[Severity: High]
Because serial8250_rx_dma_flush() checks dma->shutting_down before
!dma->rx_running, it returns -ESHUTDOWN even when RX DMA is not running.

During port shutdown in serial8250_do_shutdown(), up->dma->shutting_down is
set to true and serial8250_release_dma() releases the DMA channels and sets
dma->rx_running = 0, but leaves up->dma non-NULL and dma->shutting_down set
to true until serial8250_request_dma() runs near the end of a subsequent
serial8250_do_startup():

drivers/tty/serial/8250/8250_port.c:serial8250_do_shutdown() {
	scoped_guard(uart_port_lock_irqsave, port) {
		if (up->dma)
			up->dma->shutting_down = true;
		up->ier = 0;
		serial_port_out(port, UART_IER, 0);
	}

	synchronize_irq(port->irq);

	if (up->dma)
		serial8250_release_dma(up);
	...
	/* Disable break condition */
	lcr = serial_port_in(port, UART_LCR);
	lcr &= ~UART_LCR_SBC;
	serial_port_out(port, UART_LCR, lcr);
}

On DesignWare UARTs, if DW_UART_USR_BUSY is asserted during that UART_LCR
write in serial8250_do_shutdown(), or during serial8250_initialize() in
serial8250_do_startup() before serial8250_request_dma() clears
dma->shutting_down, the call chain
dw8250_serial_out() -> dw8250_check_lcr() -> dw8250_idle_enter() calls
serial8250_rx_dma_flush(up), which returns -ESHUTDOWN and aborts the LCR
write even though DMA is already stopped and released.

Similarly, on port reopen in serial8250_do_startup(), up->ops->setup_irq()
and serial8250_initialize() run while dma->shutting_down is still true, so
any interrupt firing before serial8250_request_dma() hits the
up->dma->shutting_down early return in dw8250_handle_irq() and
serial8250_handle_irq_locked() without reading status registers to clear the
interrupt.

Should dma->shutting_down be cleared once serial8250_release_dma() finishes
releasing the channels (or at the start of serial8250_do_startup()), and
should serial8250_rx_dma_flush() check !dma->rx_running before
dma->shutting_down?

> +	if (dma->rx_terminating)
> +		return -EIO;
> +
> +	ret = dmaengine_pause(dma->rxchan);
> +	if (ret)
> +		goto recover;
> +
> +	status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> +	if (status == DMA_ERROR) {
> +		ret = -EIO;
> +		goto recover;
> +	}
> +	/* Some providers still report IN_PROGRESS after a successful pause. */
> +	serial8250_rx_dma_copy(p, &state);

[Severity: High]
Can calling dmaengine_pause() before checking dmaengine_tx_status() drop a
completed RX DMA buffer?

dma_rx_complete() runs asynchronously (for example in a vchan tasklet) and
must acquire p->port.lock before copying dma->rx_buf and clearing
dma->rx_running. If a full dma->rx_size transfer completes in hardware and a
UART interrupt (UART_IIR_RDI, UART_IIR_RX_TIMEOUT, or UART_IIR_RLSI in
serial8250_handle_irq_locked() -> handle_rx_dma()) or dw8250_idle_enter()
calls serial8250_rx_dma_flush() before dma_rx_complete() acquires
p->port.lock, dma->rx_running is still 1 while the descriptor status is
already DMA_COMPLETE.

Several DMA engine drivers that advertise cmd_pause return an error from
device_pause when the transfer has already completed in hardware: for
example, dw-axi-dmac's dma_chan_pause() polls for DWAXIDMAC_IRQ_SUSPENDED on
a channel already disabled by axi_chan_block_xfer_complete() and returns
-EAGAIN, while stm32_dma_pause() and dw_edma_device_pause() return -EPERM
and edma_dma_pause() and omap_dma_pause() return -EINVAL when the channel is
not currently active.

When dmaengine_pause() returns an error here, serial8250_rx_dma_flush()
jumps to recover, where serial8250_rx_dma_recover() sets dma->rx_err = true
and dma->rx_terminating = true without calling serial8250_rx_dma_copy().
When the pending dma_rx_complete() callback then acquires p->port.lock, it
sees dma->rx_terminating and returns early without copying dma->rx_buf
either, losing the completed buffer and disabling RX DMA until reopen.

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index eca02e8bd6d13..3880ec6a9368e 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
[ ... ]
> @@ -147,7 +148,12 @@ static int dw8250_idle_enter(struct uart_port *p)
>  	serial_port_out(p, UART_IER, 0);
>  
>  	if (up->dma) {
> -		serial8250_rx_dma_flush(up);
> +		ret = serial8250_rx_dma_flush(up);
> +		if (ret) {
> +			serial8250_out_IER(up, up->ier);
> +			d->in_idle = 0;
> +			return ret;
> +		}
>  		if (serial8250_tx_dma_running(up))
>  			serial8250_tx_dma_pause(up);
>  	}

[Severity: High]
When dw8250_idle_enter() returns an error here, dw8250_check_lcr() aborts
without writing UART_LCR:

drivers/tty/serial/8250/8250_dw.c:dw8250_check_lcr() {
	...
	ret = dw8250_idle_enter(p);
	if (ret < 0)
		goto write_err;

	serial_port_out(p, UART_LCR, value);
	dw8250_idle_exit(p);
	return;
}

On DesignWare UARTs with fractional divisor support (where DW_UART_DLF is
non-zero), dw8250_setup_port() in 8250_dwlib.c overrides p->set_divisor with
its own dw8250_set_divisor(), which calls serial8250_do_set_divisor():

drivers/tty/serial/8250/8250_dwlib.c:dw8250_set_divisor() {
	dw8250_writel_ext(p, DW_UART_DLF, quot_frac);
	serial8250_do_set_divisor(p, baud, quot);
}

drivers/tty/serial/8250/8250_port.c:serial8250_do_set_divisor() {
	if (!(up->capabilities & UART_CAP_MINI))
		serial_port_out(port, UART_LCR, up->lcr | UART_LCR_DLAB);

	serial_dl_write(up, quot);
}

If serial8250_do_set_termios() runs while the UART is busy and
serial8250_rx_dma_flush() returns an error in dw8250_idle_enter() (for
example, while RX DMA recovery is active with dma->rx_running &&
dma->rx_terminating, or when pausing/terminating RX DMA fails), the write to
set UART_LCR_DLAB fails in dw8250_check_lcr() and DLAB remains 0 in
hardware.

serial8250_do_set_divisor() then unconditionally calls serial_dl_write() ->
default_serial_dl_write(), which writes quot & 0xff to offset 0 (UART_TX
when DLAB is 0) and (quot >> 8) & 0xff to offset 1 (UART_IER when DLAB is 0)
via raw serial_out(). Can this corrupt UART_IER and bypass
serial8250_out_IER() to unmask receive interrupts in hardware during RX DMA
recovery?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-submit-serial-8250-dma-cancel-v1-v3-0-d78f9e4c4932@gmail.com?part=4