Re: [PATCH 3/3] arm64: dts: agilex5: Enable pagepool Tx buffer quirk

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsafe use of page_pool_recycle_direct() from hardirq context causes fast-cache corruption.
- [Medium] Redundant full-page DMA sync due to PP_FLAG_DMA_SYNC_DEV on a TX bounce buffer.
- [Low] Encoding a software optimization policy into Devicetree.
--

--- Patch [3]: [PATCH 3/3] arm64: dts: agilex5: Enable pagepool Tx buffer quirk ---
commit 4b5b56ba27a76622b7269e1715c07a2f16e86382
Author: Nazim Amirul <[email protected]>

arm64: dts: agilex5: Enable pagepool Tx buffer quirk

Enable snps,pagepool-tx-buf-quirk for Agilex5 gmac0/1/2. These ports
use an IOMMU, where avoiding per-packet dma map/unmap invalidate cost
improves Tx performance.

> diff --git a/arch/arm64/boot/dts/intel/socfpga_agilex5.dtsi b/arch/arm64/boot/dts/intel/socfpga_agilex5.dtsi
> index b06c6d5d60eed..525e6a2319c10 100644
> --- a/arch/arm64/boot/dts/intel/socfpga_agilex5.dtsi
> +++ b/arch/arm64/boot/dts/intel/socfpga_agilex5.dtsi
> @@ -574,6 +574,7 @@ gmac0: ethernet@10810000 {
>  			snps,mtl-tx-config = <&mtl_tx_emac0_setup>;
>  			snps,pbl = <32>;
>  			snps,tso;
> +			snps,pagepool-tx-buf-quirk;

[Severity: Low]
Does a software optimization policy belong in Devicetree? Usually, Devicetree
describes hardware topology rather than software configuration like this quirk.

[Severity: High]
By enabling this quirk, does this expose a fast-cache corruption regression
in the driver?

When a hardware Tx error occurs, the hardirq handler calls stmmac_tx_err(),
which iterates buffers and eventually calls:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_free_tx_buffer()
			page_pool_recycle_direct(tx_q->page_pool,
						 tx_q->tx_skbuff_dma[i].page);

If this interrupts napi softirq running on the same CPU, or races with napi
on another CPU, won't calling the napi-only page_pool_recycle_direct()
without the Tx lock corrupt the lockless page_pool fast cache?

[Severity: Medium]
Will enabling this quirk introduce redundant performance overhead that defeats
its purpose?

The driver initializes the Tx page pool with PP_FLAG_DMA_SYNC_DEV:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_tx_desc_resources()
	pp_params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;

This causes a full 4KB page sync on every recycle. However, the driver
already performs a precise sync manually:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_xmit()
			dma_sync_single_for_device(priv->device, dma_addr,
						   nopaged_len,
						   DMA_BIDIRECTIONAL);

Could the sync upon recycle be syncing stale data before the next packet is
written, making the PP_FLAG_DMA_SYNC_DEV flag redundant?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803070513.4657-1-muhammad.nazim.amirul.nazle.asmade@altera.com?part=3