Re: [PATCH net v4 3/3] net: ethernet: mtk_wed: setup WDMA_RING_TX(0) for non-DBDC MT7986

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In mtk_wed_start()
  (drivers/net/ethernet/mediatek/mtk_wed.c:2346-2347) the newly added…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  widens it: dev->tx_wdma[] descriptor rings are allocated with…

--

net: ethernet: mtk_wed: setup WDMA_RING_TX(0) for non-DBDC MT7986

On a non-DBDC MT7986 only rx_ring[1]/tx_wdma[1] get set up, so
WDMA_RING_TX(0) is never programmed even though WED needs it for RX
offload.  The patch adds a call to mtk_wed_wdma_tx_ring_setup() for ring 0
from mtk_wed_start() when the device is WED v2 and rx_ring[0] was not
allocated, passing reset=true when tx_wdma[0] already exists.

> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index 3f0be70c3f145..3800e602d3c48 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -2339,6 +2339,13 @@ mtk_wed_start(struct mtk_wed_device *dev, u32 irq_mask)
>  		if (!dev->rx_wdma[i].desc)
>  			mtk_wed_wdma_rx_ring_setup(dev, i, 16, false);
>  
> +	/*
> +	 * non-DBDC MT7986 allocates only rx_ring[1] and tx_wdma[1] during setup
> +	 * but tx_wdma[0] is also needed for WED to function.
> +	 */
> +	if (mtk_wed_is_v2(dev->hw) && !dev->rx_ring[0].desc)
> +		mtk_wed_wdma_tx_ring_setup(dev, 0, MTK_WED_WDMA_RING_SIZE, !!dev->tx_wdma[0].desc);

[Severity: Medium]
Should the return value of mtk_wed_wdma_tx_ring_setup() be checked here?

On the first start of a non-DBDC v2 device dev->tx_wdma[0].desc is NULL, so
reset is false and the helper takes the allocation path:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_wdma_tx_ring_setup() {
	wdma = &dev->tx_wdma[idx];
	if (!reset && mtk_wed_ring_alloc(dev, wdma, MTK_WED_WDMA_RING_SIZE,
					 dev->hw->soc->wdma_desc_size, true))
		return -ENOMEM;
	...
}

mtk_wed_ring_alloc() does a dma_alloc_coherent() of
MTK_WED_WDMA_RING_SIZE * wdma_desc_size (32 KiB on MT7986), and on failure
the helper returns before writing MTK_WDMA_RING_TX(0) BASE/COUNT/CPU_IDX/
DMA_IDX and before the "if (!idx)" block that programs the WED side
MTK_WED_WDMA_RING_TX window.

Since the status is dropped, mtk_wed_start() keeps going through
mtk_wed_hw_init(), mtk_wed_configure_irq() and mtk_wed_dma_enable(), which
arms MTK_WDMA_GLO_CFG_TX_DMA_EN and, for rx-capable v2,
MTK_WED_WDMA_GLO_CFG_TX_DRV_EN | MTK_WED_WDMA_GLO_CFG_TX_DDONE_CHK, and
then sets dev->running = true.  Can the hardware then fetch descriptors
from a ring whose base register was left at 0 by the preceding
mtk_wdma_tx_reset()/mtk_wed_reset(), leaving the WED offload silently
non-functional with no message?

The other caller of the same helper propagates the failure:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_rx_ring_setup() {
	if (mtk_wed_wdma_tx_ring_setup(dev, idx, MTK_WED_WDMA_RING_SIZE,
				       reset))
		return -ENOMEM;
	...
}

and mtk_wed_start() itself already bails out on mtk_wed_rx_buffer_alloc()
and mtk_wed_rro_cfg() failures.  Since mtk_wed_ops.start is void and
mt7915_dma_start() cannot observe the error, would it make sense to abort
the start sequence locally here (skipping mtk_wed_dma_enable() and
dev->running = true) and log the failure?

[Severity: Medium]
This isn't a problem introduced by this patch, but does this new call add
another leak of dev->tx_wdma[0]?

dev->tx_wdma[] rings are allocated by mtk_wed_ring_alloc() via
mtk_wed_wdma_tx_ring_setup(), but the ring-release helpers never free that
array:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_free_tx_rings() {
	for (i = 0; i < ARRAY_SIZE(dev->tx_ring); i++)
		mtk_wed_free_ring(dev, &dev->tx_ring[i]);
	for (i = 0; i < ARRAY_SIZE(dev->rx_wdma); i++)
		mtk_wed_free_ring(dev, &dev->rx_wdma[i]);
}

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_free_rx_rings() {
	mtk_wed_free_rx_buffer(dev);
	mtk_wed_free_ring(dev, &dev->rro.ring);
}

__mtk_wed_detach() calls those helpers and then does
memset(dev, 0, sizeof(*dev)), so tx_wdma[].desc/desc_phys are discarded and
the 32 KiB coherent allocation can no longer be released.

The existing allocation site is mtk_wed_rx_ring_setup(), so the omission
predates this change, but non-DBDC MT7986/MT7981 previously never allocated
tx_wdma[0] at all, and now every attach/start/detach cycle would leak one
more ring.  Would adding dev->tx_wdma[] to mtk_wed_free_tx_rings() be the
right way to restore alloc/free symmetry?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917144335.19466-1-hujy652%40gmail.com