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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: 8250: missing shutting_down check in serial8250_rx_dma_flush()
- [High] serial: 8250: rx dma flush fails on paused DMA_IN_PROGRESS and DMA_ERROR
--

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

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

This commit reserves the RX DMA buffer across asynchronous termination and
synchronizes in-flight completion callbacks in a workqueue worker before
allowing a new RX DMA transfer to reuse the buffer. It also checks pause and
status results during RX DMA flushes and serializes Intel MID timeout callers
with the UART port lock.

> diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
> index d4b1e2a059eb1..821ccc7a3a54d 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -77,31 +77,29 @@ void serial8250_tx_dma_prepare_buffer(struct uart_8250_port *p,
>  }
>  EXPORT_SYMBOL_GPL(serial8250_tx_dma_prepare_buffer);
>  
> -static void __dma_rx_complete(struct uart_8250_port *p)
> +static enum dma_status serial8250_rx_dma_complete(struct uart_8250_port *p)
>  {
>  	struct uart_8250_dma	*dma = p->dma;
>  	struct tty_port		*tty_port = &p->port.state->port;
>  	struct dma_tx_state	state;
>  	enum dma_status		dma_status;
> -	int			count;
> +	size_t			count;
>  
> -	/*
> -	 * New DMA Rx can be started during the completion handler before it
> -	 * could acquire port's lock and it might still be ongoing. Don't to
> -	 * anything in such case.
> -	 */
> +	/* Only inspect the buffer after completion or a successful pause. */
>  	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
> -	if (dma_status == DMA_IN_PROGRESS)
> -		return;
> +	if (dma_status != DMA_COMPLETE && dma_status != DMA_PAUSED)
> +		return dma_status;

[ ... ]

> +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);
> +}

[ ... ]

> -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;
> +	enum dma_status status;
> +	int ret;
>  
> -	if (!dma || !dma->rxchan || !dma->rx_running)
> -		return;
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	if (!dma || !dma->rxchan || !dma->rx_running || dma->rx_terminating)
> +		return 0;

[Severity: High]
Should serial8250_rx_dma_flush() also check dma->shutting_down before
proceeding, similar to serial8250_tx_dma_flush(), dma_rx_complete(), and
serial8250_rx_dma()?

During shutdown, serial8250_do_shutdown() in
drivers/tty/serial/8250/8250_port.c sets up->dma->shutting_down = true and
clears UART_IER under port->lock, calls synchronize_irq(port->irq), and then
calls serial8250_release_dma(up) while port->irq is still registered.

In drivers/tty/serial/8250/8250_dma.c, serial8250_release_dma() cancels
rx_flush_work before terminating dma->rxchan and clearing dma->rx_running:

drivers/tty/serial/8250/8250_dma.c:serial8250_release_dma() {
	...
	cancel_work_sync(&dma->tx_flush_work);
	cancel_work_sync(&dma->rx_flush_work);

	/* Release RX resources */
	dmaengine_terminate_sync(dma->rxchan);
	dma->rx_running = 0;
	dma_free_coherent(dmaengine_get_dma_device(dma->rxchan), dma->rx_size,
			  dma->rx_buf, dma->rx_addr);
	dma_release_channel(dma->rxchan);
	dma->rxchan = NULL;
	...
}

On Intel MID platforms, tng_handle_irq() and dnv_handle_irq() in
drivers/tty/serial/8250/8250_mid.c handle HSU DMA channel timeout interrupts
on shared port->irq, which are not masked by clearing UART_IER. While the RX
DMA transfer is still active prior to dmaengine_terminate_sync(dma->rxchan)
(or if a concurrent IRQ handler read UART_IIR just before
serial8250_do_shutdown() took port->lock), the handler can acquire port->lock
and call serial8250_rx_dma_flush(up) after cancel_work_sync(&dma->rx_flush_work)
has already returned.

Without a dma->shutting_down check, serial8250_rx_dma_flush() will push flip
buffer data after shutdown has started and call
schedule_work(&dma->rx_flush_work). When serial8250_rx_dma_flush_work()
subsequently runs, serial8250_release_dma() may have already released
dma->rxchan and set dma->rxchan = NULL, causing
dmaengine_synchronize(dma->rxchan) to dereference dma->rxchan == NULL or a
freed channel.

> +
> +	ret = dmaengine_pause(dma->rxchan);
> +	if (ret)
> +		return ret;
> +
> +	status = serial8250_rx_dma_complete(p);
> +	if (status != DMA_PAUSED && status != DMA_COMPLETE) {
> +		dmaengine_resume(dma->rxchan);
> +		return -EIO;
> +	}

[Severity: High]
Could rejecting statuses other than DMA_PAUSED and DMA_COMPLETE here and in
serial8250_rx_dma_complete() wedge RX DMA or cause an unhandled interrupt
storm on RX timeout?

First, several dmaengine providers that pass the capability checks in
serial8250_request_dma() (such as sun6i-dma in drivers/dma/sun6i-dma.c and
dw-axi-dmac in drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c, used with
8250_dw) pause the hardware channel in .device_pause and return 0, but their
.device_tx_status callbacks still return dma_cookie_status() (DMA_IN_PROGRESS)
rather than DMA_PAUSED. With this check, serial8250_rx_dma_flush() resumes
the channel and fails with -EIO on every RX timeout flush and in
dw8250_idle_enter().

Second, if an RX DMA transfer encounters a channel error (DMA_ERROR, for
example in drivers/dma/hsu/hsu.c or drivers/dma/idma64.c where .device_pause
only sets DMA_PAUSED when the descriptor is DMA_IN_PROGRESS), neither
dma_rx_complete() nor serial8250_rx_dma_flush() terminates the transfer or
clears dma->rx_running, leaving RX DMA permanently stuck.

Third, when serial8250_rx_dma_flush() returns an error on UART_IIR_RX_TIMEOUT,
UART_IIR_RDI, or UART_IIR_RLSI, handle_rx_dma() in
drivers/tty/serial/8250/8250_port.c now returns false, causing
serial8250_handle_irq_locked() to skip serial8250_rx_chars():

drivers/tty/serial/8250/8250_port.c:handle_rx_dma() {
	...
	case UART_IIR_RLSI:
	case UART_IIR_RX_TIMEOUT:
		return !serial8250_rx_dma_flush(up);
}

drivers/tty/serial/8250/8250_port.c:serial8250_handle_irq_locked() {
	...
	if (!up->dma || handle_rx_dma(up, iir))
		status = serial8250_rx_chars(up, status);
}

Because UART_IIR_RX_TIMEOUT is only cleared by reading UART_RX and the FIFO
level on a timeout is below the DMA burst watermark (and in
dw8250_handle_irq() in drivers/tty/serial/8250/8250_dw.c with
DW_UART_QUIRK_IS_DMA_FC, RZN1_UART_RDMACR has already been cleared to 0
before serial8250_handle_irq_locked() is called), returning from the ISR
without reading UART_RX via PIO leaves the level-triggered UART interrupt
asserted.

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-submit-serial-8250-dma-cancel-v1-v1-0-efe2b89f2303@gmail.com?part=2