Re: [PATCH net-next v4 1/3] net: ethernet: mtk_eth_soc: reset RX DMA

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not accurately describe the change.

--

net: ethernet: mtk_eth_soc: reset RX DMA

This patch moves the DMA_MAPPING_ERROR initialisation of dma_addr in
mtk_poll_rx() to the top of the RX loop, so it is reset on every
iteration. On MTK_36BIT_DMA SoCs, a descriptor that is released through
release_desc without a new mapping then keeps its own high address bits
in rxd2.

> net: ethernet: mtk_eth_soc: reset RX DMA
>
> [ ... ]
>
> With MTK_36BIT_DMA and DRAM above 4 GiB the hardware then DMAs the next
> frame to an address that is neither buffer, and the driver passes the
> stale contents of the untouched buffer up the stack.

[Severity: Low]
This isn't a bug, but could the subject line be more specific?

The patch only resets the local dma_addr variable at the top of each
iteration of the loop in mtk_poll_rx(). No RX DMA engine is reset.

In this driver, "reset" usually refers to the hardware/SER reset code
(MTK_RESETTING, mtk_hw_reset(), reset.monitor_work). Someone reading the
shortlog, or a stable backporter, could take this for a change to the
reset path.

Should the body also say that this completes commit 6e0490fc36cd ("net:
ethernet: mtk_eth_soc: fix SER panic with 4GB+ RAM")?

That commit added this check at release_desc in mtk_poll_rx():

		if (MTK_HAS_CAPS(eth->soc->caps, MTK_36BIT_DMA)) {
			if (unlikely(dma_addr == DMA_MAPPING_ERROR))
				addr64 = FIELD_GET(RX_DMA_ADDR64_MASK,
						   rxd->rxd2);
			else
				addr64 = RX_DMA_PREP_ADDR64(dma_addr);
		}

The check only works if dma_addr is DMA_MAPPING_ERROR on every release
path. Since commit e10a35abb3da ("net: ethernet: mtk_eth_soc: fix
uninitialized variable"), dma_addr is set only once, before the loop. So
the MTK_RESETTING path that 6e0490fc36cd was fixing can still pick up the
high bits of a buffer mapped for an earlier descriptor in the same poll.

mtk_rx_clean() then unmaps an address built from mismatched high and low
bits:

			if (MTK_HAS_CAPS(eth->soc->caps, MTK_36BIT_DMA))
				addr64 = RX_DMA_GET_ADDR64(rxd->rxd2);

			dma_unmap_single(eth->dma_dev, ((u64)rxd->rxd1 | addr64),
					 ring->buf_size, DMA_FROM_DEVICE);

This is the SER panic described in 6e0490fc36cd. The body covers the
misdirected DMA and the stale data passed up the stack, but not this
unmap during SER.

Would it make sense to mention it and reference 6e0490fc36cd in the
commit message?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/patch-fc08a7ef40667bf3017366e452e6d3c0%40loljews.com