Re: [PATCH net v6 3/4] net: ethernet: mtk_wed: free up rings during detach

[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] After this patch, mtk_wed_free_rx_rings()
  (drivers/net/ethernet/mediatek/mtk_wed.c:954-958) calls…

--

net: ethernet: mtk_wed: free up rings during detach

This commit makes mtk_wed_free_rx_rings() also free the rx_ring[] and
tx_wdma[] descriptor rings. Before this, __mtk_wed_detach() leaked them.
The change comes from mtk-openwrt-feeds and was modified to check
ring->desc instead of an extra flag.

Link: https://github.com/mediatek/mtk-openwrt-feeds/blob/main/25.12/files/target/linux/mediatek/patches-6.12/999-wed-04-Fix-reinsert-wifi-module-cause-memory-leak-issue.patch
Link: https://sashiko.dev/#/message/179000700034.2160803.10594281691384015089%40kernel.org

> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index 3f0be70c3f145..3044a7a8ea03e 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -949,6 +949,14 @@ mtk_wed_free_ring(struct mtk_wed_device *dev, struct mtk_wed_ring *ring)
>  static void
>  mtk_wed_free_rx_rings(struct mtk_wed_device *dev)
>  {
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(dev->rx_ring); i++)
> +		mtk_wed_free_ring(dev, &dev->rx_ring[i]);
> +
> +	for (i = 0; i < ARRAY_SIZE(dev->tx_wdma); i++)
> +		mtk_wed_free_ring(dev, &dev->tx_wdma[i]);

[Severity: Low]
Could the WDMA engine still be writing to the tx_wdma[] descriptors when
they are freed here?

In __mtk_wed_detach(), these rings are freed right after
mtk_wed_wo_reset(), which returns void:

__mtk_wed_detach()
    mtk_wed_wo_reset(dev);
        mtk_wdma_tx_reset(dev);
    mtk_wed_free_rx_rings(dev);
        dma_free_coherent(..., tx_wdma[i].desc, ...)

If the TX_DMA_BUSY poll in mtk_wdma_tx_reset() times out, the timeout is
only logged:

mtk_wdma_tx_reset() {
    ...
	wdma_clr(dev, MTK_WDMA_GLO_CFG, MTK_WDMA_GLO_CFG_TX_DMA_EN);
	if (readx_poll_timeout(mtk_wdma_read_reset, dev, status,
			       !(status & mask), 0, 10000))
		dev_err(dev->hw->dev, "tx reset failed\n");
    ...
	for (i = 0; i < ARRAY_SIZE(dev->tx_wdma); i++)
		wdma_w32(dev,
			 MTK_WDMA_RING_TX(i) + MTK_WED_RING_OFS_CPU_IDX, 0);
}

mtk_wdma_v3_tx_reset() handles timeouts in its prefetch and writeback
BUSY polls the same way.

Only the CPU_IDX registers are cleared. The MTK_WDMA_RING_TX(i) BASE
registers and the WED WDMA_RING_TX BASE registers still point at
desc_phys.

If the engine has not gone idle, can a late descriptor fetch or
writeback land in coherent memory that has already gone back to the
allocator? Before this patch the rings were leaked, so such a write
could only reach memory that nothing else was using.

This depends on a hardware timeout, and I could not confirm from the
code that a late writeback really happens. TX_DMA_EN has already been
cleared in mtk_wed_dma_disable(), and MTK_WED_RESET_WED has been
asserted.

The same function already frees rx_wdma[] in mtk_wed_free_tx_rings()
right after mtk_wdma_rx_reset(), whose -ETIMEDOUT return is ignored, so
this follows an existing pattern in the driver. Would it be worth either
skipping the free on a reset timeout or clearing the ring BASE registers
before freeing?

> +
>  	mtk_wed_free_rx_buffer(dev);
>  	mtk_wed_free_ring(dev, &dev->rro.ring);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-wedfix-v6-0-a52f6b15d2fc%40gmail.com