Re: [PATCH net-next v2 07/11] net: ethernet: cortina: Synchronize RX fragments for the CPU

[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 for "net: ethernet: cortina: Synchronize RX
  fragments for the CPU" presents the change as a bug fix.

--

net: ethernet: cortina: Synchronize RX fragments for the CPU

This commit adds a dma_sync_single_range_for_cpu() for each free queue
fragment when geth_freeq_claim() claims it. It also changes the final
full-page unmap to use DMA_ATTR_SKIP_CPU_SYNC. During teardown,
geth_freeq_release_pages() now syncs only the fragments the hardware still
owns before doing a skip-sync unmap.

> Free queue pages use streaming DMA mappings with DMA_FROM_DEVICE. The RX
> path reads packet data after the device has filled a fragment without
> first transferring ownership of that range to the CPU.

[Severity: Low]

Could the commit message say which configurations are affected and what
the user-visible symptom is? As written, it reads like a fix for RX data
corruption, but there is no Fixes: tag.

The missing sync_for_cpu seems to go back to the original driver, commit
4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet").
That commit added gmac_rx() and the claim/unmap logic.

This IP only appears on the Gemini SoC (ARCH_GEMINI, ARCH_MULTI_V4, FA526
core, no outer cache). On Gemini, arch_sync_dma_for_cpu() in
arch/arm/mm/dma-mapping.c does:

	if (dir != DMA_TO_DEVICE) {
		outer_inv_range(paddr, paddr + size);

		dma_cache_maint_page(paddr, size, dir, dmac_unmap_area);
	}

outer_inv_range() does nothing here. dmac_unmap_area resolves to
fa_dma_unmap_area in arch/arm/mm/cache-fa.S:

SYM_TYPED_FUNC_START(fa_dma_unmap_area)
	ret	lr
SYM_FUNC_END(fa_dma_unmap_area)

So the new dma_sync_single_range_for_cpu() in geth_freeq_claim() does not
seem to do any cache maintenance on this hardware. The old full-page
dma_unmap_single() did none either.

> Once all fragments have been claimed, tear down the full-page mapping with
> DMA_ATTR_SKIP_CPU_SYNC. Each range has already moved to the CPU domain, and
> another full-page synchronization could invalidate a sibling fragment
> already being consumed by the network stack.

Given the above, can the sibling-invalidation case happen on FA526 at
all?

If this fixes real RX corruption on some configuration, should it carry
"Fixes: 4d5ae32f5e1e"? If it is a DMA API correctness change with no
functional effect on Gemini, could the commit message say so? That would
let stable backporters tell the two cases apart.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-gemini-ethernet-fixes-3-v2-0-758a795d7a78%40kernel.org