Re: [PATCH 1/2] serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] serial: amba-pl011: race and TX restart in pl011_dma_shutdown()
- [High] serial: amba-pl011: TX DMA re-enabled while dmatx.flushing is true
- [Medium] serial: amba-pl011: pl011_dma_tx_flush_work() bypasses pl011_start_tx()
--

commit fb5633a8b4f6925fce77e6c969ed222b6e66d5c1
Author: Samarth Narang <samarth.colleges@gmail.com>

serial: amba-pl011: keep the flushed TX buffer mapped until DMA stops

This commit defers unmapping a flushed TX DMA buffer from
pl011_dma_flush_buffer() to a work item that calls dmaengine_synchronize()
before releasing the mapping and restarting transmission. It keeps
dmatx.queued set while dmatx.flushing is active and flushes the work item in
pl011_dma_shutdown().

> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index e6f652905ca6f..863a050cf654e 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c

[ ... ]

> @@ -867,13 +876,47 @@ __acquires(&uap->port.lock)
>  
>  	dmaengine_terminate_async(uap->dmatx.chan);
>  
> -	if (uap->dmatx.queued) {
> -		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan), uap->dmatx.dma,
> -				 uap->dmatx.len, DMA_TO_DEVICE);
> +	/*
> +	 * The terminated transfer may still be reading dmatx.buf: keep it
> +	 * mapped, and queued set so that nothing refills it, until
> +	 * pl011_dma_tx_flush_work() has synchronized the channel.
> +	 */
> +	if (uap->dmatx.queued && !uap->dmatx.flushing) {
> +		uap->dmatx.flushing = true;
> +		uap->dmacr &= ~UART011_TXDMAE;
> +		pl011_write(uap->dmacr, uap, REG_DMACR);
> +		schedule_work(&uap->dmatx.flush_work);
> +	}
> +}

[Severity: High]
Can UART011_TXDMAE be re-enabled in REG_DMACR while uap->dmatx.flushing is
still true and before pl011_dma_tx_flush_work() finishes
dmaengine_synchronize()?

When uart_flush_buffer() calls pl011_dma_flush_buffer(), uap->dmatx.queued is
kept true while UART011_TXDMAE is cleared. Immediately after
pl011_dma_flush_buffer() returns, uart_flush_buffer() drops uap->port.lock and
calls tty_port_tty_wakeup(). If a subsequent write or wakeup calls
pl011_start_tx() -> pl011_dma_tx_start() before pl011_dma_tx_flush_work()
runs, pl011_dma_tx_start() sees uap->dmatx.queued set and UART011_TXDMAE clear:

drivers/tty/serial/amba-pl011.c:pl011_dma_tx_start() {
	...
		if (!uap->dmatx.queued) {
			...
		} else if (!(uap->dmacr & UART011_TXDMAE)) {
			uap->dmacr |= UART011_TXDMAE;
			pl011_write(uap->dmacr, uap, REG_DMACR);
		}
		return ret;
}

Similarly, if a TX interrupt fires (for example after sending an x_char) and
calls pl011_dma_tx_irq(), it also checks uap->dmatx.queued without checking
uap->dmatx.flushing:

drivers/tty/serial/amba-pl011.c:pl011_dma_tx_irq() {
	...
	if (uap->dmatx.queued) {
		uap->dmacr |= UART011_TXDMAE;
		pl011_write(uap->dmacr, uap, REG_DMACR);
		uap->im &= ~UART011_TXIM;
		pl011_write(uap->im, uap, REG_IMSC);
		return true;
	}
}

In both cases, re-enabling UART011_TXDMAE causes the UART to assert TX DMA
requests to the DMA controller for the terminated transfer and pull stale
flushed bytes from dmatx.buf into the TX FIFO, and a second call to
pl011_dma_flush_buffer() while uap->dmatx.flushing is already set will not
clear UART011_TXDMAE again. Should pl011_dma_tx_start() and
pl011_dma_tx_irq() check uap->dmatx.flushing before setting UART011_TXDMAE?

> +
> +/*
> + * flush_buffer runs under the port lock and can only request termination.
> + * Unmap the flushed TX buffer here, once the terminated transfer and its
> + * completion callback are known to have finished, then restart TX.
> + */
> +static void pl011_dma_tx_flush_work(struct work_struct *work)
> +{
> +	struct uart_amba_port *uap =
> +	    container_of(work, struct uart_amba_port, dmatx.flush_work);
> +	struct tty_port *tport = &uap->port.state->port;
> +	unsigned long flags;
> +
> +	dmaengine_synchronize(uap->dmatx.chan);
> +
> +	uart_port_lock_irqsave(&uap->port, &flags);
> +	if (uap->dmatx.flushing) {
> +		dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan),
> +				 uap->dmatx.dma, uap->dmatx.len, DMA_TO_DEVICE);
> +		uap->dmatx.flushing = false;
>  		uap->dmatx.queued = false;
>  		uap->dmacr &= ~UART011_TXDMAE;
>  		pl011_write(uap->dmacr, uap, REG_DMACR);
> +		if (!uart_tx_stopped(&uap->port) &&
> +		    !kfifo_is_empty(&tport->xmit_fifo) &&
> +		    pl011_dma_tx_refill(uap) <= 0)
> +			pl011_start_tx_pio(uap);
>  	}
> +	uart_port_unlock_irqrestore(&uap->port, flags);
>  }

[Severity: Medium]
Does calling pl011_dma_tx_refill() and pl011_start_tx_pio() directly in
pl011_dma_tx_flush_work() bypass the RS485 state machine and UART011_TXIM
masking normally handled by pl011_start_tx()?

In pl011_start_tx(), when SER_RS485_ENABLED is set and uap->rs485_tx_state is
not SEND, the driver calls pl011_rs485_tx_start() and waits for
WAIT_AFTER_RTS before starting DMA or PIO:

drivers/tty/serial/amba-pl011.c:pl011_start_tx() {
	if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
	    uap->rs485_tx_state != SEND) {
		/* trigger_start_tx starts TX once delay_rts_before_send passed */
		if (uap->rs485_tx_state == WAIT_AFTER_RTS)
			return;

		pl011_rs485_tx_start(uap);
		if (uap->rs485_tx_state == WAIT_AFTER_RTS)
			return;
	}

	if (!pl011_dma_tx_start(uap))
		pl011_start_tx_pio(uap);
}

In addition, when pl011_dma_tx_refill() > 0 succeeds in pl011_dma_tx_start(),
it clears UART011_TXIM in uap->im and writes REG_IMSC, which is skipped when
pl011_dma_tx_flush_work() calls pl011_dma_tx_refill() directly. Should
pl011_dma_tx_flush_work() call pl011_start_tx(&uap->port) instead?

[ ... ]

> @@ -1236,6 +1281,9 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap)
>  	if (!(uap->using_tx_dma || uap->using_rx_dma))
>  		return;
>  
> +	if (uap->using_tx_dma)
> +		flush_work(&uap->dmatx.flush_work);
> +
>  	/* Disable RX and TX DMA */
>  	while (pl011_read(uap, REG_FR) & uap->vendor->fr_busy)
>  		cpu_relax();

[Severity: Critical]
Can flushing dmatx.flush_work at the start of pl011_dma_shutdown() allow TX
to restart during shutdown or race with a concurrent uart_flush_buffer()?

First, pl011_shutdown() calls pl011_disable_interrupts(uap) before calling
pl011_dma_shutdown(uap). When flush_work(&uap->dmatx.flush_work) runs at the
start of pl011_dma_shutdown() while tport->xmit_fifo is non-empty and
!uart_tx_stopped(&uap->port) (such as during uart_suspend_port()),
pl011_dma_tx_flush_work() will either start a new DMA transfer via
pl011_dma_tx_refill() and advance xmit_fifo right before
dmaengine_terminate_sync() discards it, or call pl011_start_tx_pio() and
re-enable UART011_TXIM in REG_IMSC after pl011_disable_interrupts() masked
all interrupts.

Second, after flush_work(&uap->dmatx.flush_work) returns,
pl011_dma_shutdown() spins on REG_FR and calls dmaengine_terminate_sync()
without holding uap->port.lock while uap->using_tx_dma and uap->dmatx.queued
are still true:

drivers/tty/serial/amba-pl011.c:pl011_dma_shutdown() {
	...
	if (uap->using_tx_dma) {
		/* In theory, this should already be done by pl011_dma_flush_buffer */
		dmaengine_terminate_sync(uap->dmatx.chan);
		if (uap->dmatx.queued) {
			dma_unmap_single(dmaengine_get_dma_device(uap->dmatx.chan),
					 uap->dmatx.dma, uap->dmatx.len,
					 DMA_TO_DEVICE);
			uap->dmatx.queued = false;
		}

		kfree(uap->dmatx.buf);
		uap->using_tx_dma = false;
	}
}

Because uart_flush_buffer() only takes uap->port.lock, a concurrent
uart_flush_buffer() -> pl011_dma_flush_buffer() after flush_work() returns
can see uap->using_tx_dma and uap->dmatx.queued still set, mark
uap->dmatx.flushing = true, and schedule uap->dmatx.flush_work again. Once
pl011_dma_shutdown() unmaps uap->dmatx.dma, frees uap->dmatx.buf, and
pl011_shutdown() disables the UART clock, the newly scheduled
pl011_dma_tx_flush_work() can unmap uap->dmatx.dma a second time, write
REG_DMACR with the clock disabled, and access the freed uap->dmatx.buf.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031123749.3783352-1-samarth.colleges@gmail.com?part=1