Re: [PATCH 3/3] arm64: dts: allwinner: h616: identify the UART integration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: allwinner: h616: enabling UART RX DMA on sun6i-dma loses RX data
--

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

arm64: dts: allwinner: h616: identify the UART integration

This commit updates the UART0 through UART5 nodes in sun50i-h616.dtsi to
use the "allwinner,sun50i-h616-uart" compatible string with
"snps,dw-apb-uart" as a fallback so the driver selects the H616 256-byte
FIFO configuration and register layout.

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> index 1598e86259ab5..6905c9eaa2493 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
> @@ -538,7 +538,7 @@ mmc2: mmc@4022000 {
>  		};
>  
>  		uart0: serial@5000000 {
> -			compatible = "snps,dw-apb-uart";
> +			compatible = "allwinner,sun50i-h616-uart", "snps,dw-apb-uart";

[Severity: High]
Could switching uart0 through uart5 to "allwinner,sun50i-h616-uart" cause RX
data loss and wedge RX DMA on non-console H616 UART ports when paired with
their existing dmas properties referencing sun6i-dma?

Previously, matching "snps,dw-apb-uart" caused dw8250_setup_port() to read
DW_UART_CPR as 0, leaving p->fifosize == 0 and up->dma == NULL. Matching
"allwinner,sun50i-h616-uart" selects dw8250_h616_data in dw8250_probe(),
which attaches 8250 DMA to the port:

drivers/tty/serial/8250/8250_dw.c:dw8250_probe() {
	...
	if (data->pdata && data->pdata->type) {
		dw8250_setup_port_fixed(up, data->pdata->type);
		data->data.dma.rxconf.src_maxburst = data->pdata->dma_maxburst;
		data->data.dma.txconf.dst_maxburst = data->pdata->dma_maxburst;
		up->dma = &data->data.dma;
	}
	...
}

Because uart0 through uart5 in arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
already specify dmas = <&dma ...> and dma-names = "tx", "rx" pointing to
"allwinner,sun50i-h616-dma" (handled by drivers/dma/sun6i-dma.c), opening any
non-console UART calls serial8250_request_dma() and enables 8250 RX DMA.

First, when serial8250_rx_dma_flush() runs (for example on
UART_IIR_RX_TIMEOUT, UART_IIR_RDI, UART_IIR_RLSI, or dw8250_idle_enter()), it
pauses the channel, calls __dma_rx_complete(), and then terminates the
channel:

drivers/tty/serial/8250/8250_dma.c:serial8250_rx_dma_flush() {
	...
	dmaengine_pause(dma->rxchan);
	__dma_rx_complete(p);
	dmaengine_terminate_async(dma->rxchan);
}

In __dma_rx_complete(), dmaengine_tx_status() is checked and returns early if
the status is DMA_IN_PROGRESS:

drivers/tty/serial/8250/8250_dma.c:__dma_rx_complete() {
	...
	dma_status = dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
	if (dma_status == DMA_IN_PROGRESS)
		return;

	count = dma->rx_size - state.residue;

	tty_insert_flip_string(tty_port, dma->rx_buf, count);
	p->port.icount.rx += count;
	dma->rx_running = 0;
}

However, in drivers/dma/sun6i-dma.c, sun6i_dma_tx_status() returns the value
of dma_cookie_status() (DMA_IN_PROGRESS) directly and never returns
DMA_PAUSED even after sun6i_dma_pause() has paused the channel:

drivers/dma/sun6i-dma.c:sun6i_dma_tx_status() {
	...
	ret = dma_cookie_status(chan, cookie, state);
	if (ret == DMA_COMPLETE || !state)
		return ret;
	...
	dma_set_residue(state, bytes);

	return ret;
}

As a result, __dma_rx_complete() returns early without pushing the
transferred bytes (dma->rx_size - state.residue) to the TTY flip buffer and
without clearing dma->rx_running = 0. Then dmaengine_terminate_async() frees
the descriptor via sun6i_dma_terminate_all() so the completion callback
dma_rx_complete() never runs, discarding the received bytes and leaving
dma->rx_running stuck at 1.

Second, when UART_IIR_RDI triggers handle_rx_dma() -> serial8250_rx_dma() in
hardirq context, sun6i_dma_issue_pending() does not start a physical DMA
channel immediately; it only queues the virtual channel on sdev->pending and
schedules sdev->task:

drivers/dma/sun6i-dma.c:sun6i_dma_issue_pending() {
	...
	if (vchan_issue_pending(&vchan->vc)) {
		spin_lock(&sdev->lock);

		if (!vchan->phy && list_empty(&vchan->node)) {
			list_add_tail(&vchan->node, &sdev->pending);
			tasklet_schedule(&sdev->task);
	...
}

Since serial8250_rx_dma() neither masks UART_IER_RDI nor drains UART_RX, won't
the level-high UART interrupt (IRQ_TYPE_LEVEL_HIGH) remain asserted and
immediately re-enter handle_rx_dma() with UART_IIR_RDI and
up->dma->rx_running == 1 before sun6i_dma_tasklet() can run, flushing and
terminating the newly queued RX DMA transfer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-submit-h616-uart-fifo-v1-v1-0-e4a3bae2eba8@gmail.com?part=3