Re: [PATCH 2/5] serial: tegra: don't advance the TX kfifo for a flushed transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] serial: tegra: tx_in_progress ABA race in tegra_uart_tx_dma_complete()
- [Medium] serial: tegra: missing async_tx_ack() in tegra_uart_flush_buffer()
--

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

serial: tegra: don't advance the TX kfifo for a flushed transfer

This commit clears tx_in_progress in tegra_uart_flush_buffer() and checks
whether tx_in_progress is still TEGRA_UART_TX_DMA in
tegra_uart_tx_dma_complete() before advancing the transmit kfifo. This aims
to prevent an in-flight DMA completion callback from advancing a reset
kfifo after a flush or stop_tx.

> diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
> index b0fc4ae3686f8..8b712a9bc293e 100644
> --- a/drivers/tty/serial/serial-tegra.c
> +++ b/drivers/tty/serial/serial-tegra.c
> @@ -537,6 +537,11 @@ static void tegra_uart_tx_dma_complete(void *args)
>  	count = tup->tx_bytes_requested - state.residue;
>  	async_tx_ack(tup->tx_dma_desc);
>  	uart_port_lock_irqsave(&tup->uport, &flags);
> +	/* Already retired by stop_tx or flush_buffer */
> +	if (tup->tx_in_progress != TEGRA_UART_TX_DMA) {
> +		uart_port_unlock_irqrestore(&tup->uport, flags);
> +		return;
> +	}

[Severity: High]
Can this still race if a new TX DMA transfer is started before
tegra_uart_tx_dma_complete() acquires tup->uport.lock?

In tegra_uart_tx_dma_complete(), dmaengine_tx_status(), count, and
async_tx_ack() run before uart_port_lock_irqsave():

drivers/tty/serial/serial-tegra.c:tegra_uart_tx_dma_complete() {
	dmaengine_tx_status(tup->tx_dma_chan, tup->tx_cookie, &state);
	count = tup->tx_bytes_requested - state.residue;
	async_tx_ack(tup->tx_dma_desc);
	uart_port_lock_irqsave(&tup->uport, &flags);
	/* Already retired by stop_tx or flush_buffer */
	if (tup->tx_in_progress != TEGRA_UART_TX_DMA) {
		uart_port_unlock_irqrestore(&tup->uport, flags);
		return;
	}
	uart_xmit_advance(&tup->uport, count);
	tup->tx_in_progress = 0;

If tegra_uart_flush_buffer() or tegra_uart_stop_tx() runs while
tegra_uart_tx_dma_complete() for a first transfer is already executing or
waiting on tup->uport.lock, it clears tup->tx_in_progress = 0 and releases
the port lock. Because tup->tx_in_progress is now 0 (and kfifo_reset() in
uart_flush_buffer() resets the kfifo tail to 0, which is aligned), a
subsequent uart_write() -> tegra_uart_start_tx() ->
tegra_uart_start_next_tx() -> tegra_uart_start_tx_dma() can acquire
tup->uport.lock first, start a second DMA transfer, overwrite
tup->tx_dma_desc, tup->tx_bytes_requested, and tup->tx_cookie, and set
tup->tx_in_progress back to TEGRA_UART_TX_DMA.

When the first transfer's tegra_uart_tx_dma_complete() then takes
tup->uport.lock, tup->tx_in_progress == TEGRA_UART_TX_DMA is true again:

1. If the callback computed count from the first transfer before the second
transfer started, it calls uart_xmit_advance() with the first transfer's
count on the second transfer's kfifo (which can still underflow xmit_fifo
if the first transfer was larger than the newly queued data), clears
tup->tx_in_progress = 0 while the second transfer is active in hardware,
and calls tegra_uart_start_next_tx().

2. If the unlocked preamble in tegra_uart_tx_dma_complete() runs
concurrently with or after tegra_uart_start_tx_dma(), it reads the second
transfer's tup->tx_cookie, tup->tx_bytes_requested, and tup->tx_dma_desc
(or a torn mix of both transfers), acknowledging and retiring the second
transfer while it is still DMA_IN_PROGRESS.

By comparison, tegra_uart_rx_dma_complete() acquires tup->uport.lock before
calling dmaengine_tx_status() and bails out when status == DMA_IN_PROGRESS:

drivers/tty/serial/serial-tegra.c:tegra_uart_rx_dma_complete() {
	uart_port_lock_irqsave(u, &flags);

	status = dmaengine_tx_status(tup->rx_dma_chan, tup->rx_cookie, &state);

	if (status == DMA_IN_PROGRESS) {
		dev_dbg(tup->uport.dev, "RX DMA is in progress\n");
		goto done;
	}

Should tegra_uart_tx_dma_complete() similarly take tup->uport.lock before
querying dmaengine_tx_status() and accessing tup->tx_cookie,
tup->tx_bytes_requested, and tup->tx_dma_desc, and check for
DMA_IN_PROGRESS?

> @@ -1276,6 +1281,9 @@ static void tegra_uart_flush_buffer(struct uart_port *u)
>  	tup->tx_bytes = 0;
>  	if (tup->tx_dma_chan)
>  		dmaengine_terminate_all(tup->tx_dma_chan);
> +	/* The kfifo has been reset: the completion must not advance it */
> +	if (tup->tx_in_progress == TEGRA_UART_TX_DMA)
> +		tup->tx_in_progress = 0;

[Severity: Medium]
Does clearing tup->tx_in_progress in tegra_uart_flush_buffer() without
calling async_tx_ack(tup->tx_dma_desc) leave the aborted TX DMA descriptor
unacknowledged on tegra20-apb-dma?

In tegra_uart_start_tx_dma(), tup->tx_dma_desc is prepared with
DMA_PREP_INTERRUPT and without DMA_CTRL_ACK:

	tup->tx_dma_desc = dmaengine_prep_slave_single(tup->tx_dma_chan,
				tx_phys_addr, tup->tx_bytes, DMA_MEM_TO_DEV,
				DMA_PREP_INTERRUPT);

When tegra_uart_flush_buffer() calls dmaengine_terminate_all(),
tegra_dma_terminate_all() in drivers/dma/tegra20-apb-dma.c moves the
descriptor to tdc->free_dma_desc without invoking the completion callback.
Previously, a subsequent tegra_uart_stop_tx() saw tup->tx_in_progress ==
TEGRA_UART_TX_DMA and called async_tx_ack(tup->tx_dma_desc), but with
tegra_uart_flush_buffer() setting tup->tx_in_progress = 0, neither
tegra_uart_stop_tx() nor tegra_uart_tx_dma_complete() will acknowledge it.

Because tegra_dma_desc_get() only reuses descriptors on tdc->free_dma_desc
when async_tx_test_ack(&dma_desc->txd) is true:

drivers/dma/tegra20-apb-dma.c:tegra_dma_desc_get() {
	/* Do not allocate if desc are waiting for ack */
	list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
		if (async_tx_test_ack(&dma_desc->txd) && !dma_desc->cb_count) {
			list_del(&dma_desc->node);
			spin_unlock_irqrestore(&tdc->lock, flags);
			dma_desc->txd.flags = 0;
			return dma_desc;
		}
	}

won't each flushed TX DMA transfer leave an unacknowledged tegra_dma_desc
stranded on tdc->free_dma_desc until the port is shut down? Also, since
tegra186-gpc-dma frees the descriptor inside dmaengine_terminate_all(),
should async_tx_ack(tup->tx_dma_desc) be called before
dmaengine_terminate_all(tup->tx_dma_chan) when tup->tx_in_progress ==
TEGRA_UART_TX_DMA?

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