Re: [PATCH net v5 1/5] net: ethernet: mtk_wed: skip ring reset in wdma_tx_ring_setup for WED v3

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] net: mtk_wed: memory leak of rx_ring and tx_ring on setup
  failure

--

> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -1938,7 +1938,7 @@ mtk_wed_wdma_tx_ring_setup(struct mtk_wed_device *dev, int idx, int size,
>  	wdma_w32(dev, MTK_WDMA_RING_TX(idx) + MTK_WED_RING_OFS_CPU_IDX, 0);
>  	wdma_w32(dev, MTK_WDMA_RING_TX(idx) + MTK_WED_RING_OFS_DMA_IDX, 0);
>  
> -	if (reset)
> +	if (!mtk_wed_is_v3_or_greater(dev->hw) && reset)
>  		mtk_wed_ring_reset(wdma, MTK_WED_WDMA_RING_SIZE, true);
>  
>  	if (!idx)  {
>

[Severity: High]
The change here looks fine to me. Since mtk_wed_wdma_tx_ring_setup()
is one of the failure points involved, I'm raising the ring teardown
changes later in this series here. That is the patch that gates
mtk_wed_free_ring() on MTK_WED_RING_CONFIGURED.

First, mtk_wed_free_rx_rings() walks dev->rx_ring[] but tests
dev->tx_ring[i].flags before calling mtk_wed_free_ring() on
dev->rx_ring[i]. That looks like a copy/paste slip. If tx_ring[i] was
never configured, an allocated rx_ring[i] is never freed. Should that
be dev->rx_ring[i].flags?

Second, mtk_wed_tx_ring_setup() and mtk_wed_rx_ring_setup() call
mtk_wed_ring_alloc() first. They then call
mtk_wed_wdma_rx_ring_setup() or mtk_wed_wdma_tx_ring_setup(). If the
wdma step fails, they return -ENOMEM before MTK_WED_RING_CONFIGURED
is set on the ring that was already allocated.

Once the free paths only call mtk_wed_free_ring() for rings with
MTK_WED_RING_CONFIGURED set, __mtk_wed_detach() skips those partially
set up rings. Their coherent DMA descriptor memory is then leaked.
Before this series, mtk_wed_free_ring() keyed off ring->desc, which
handled this case.

I see two options:

 - Keep relying on the ring->desc check in mtk_wed_free_ring() for
   freeing, so the flag gating is not needed in the free paths.
 - Free the just-allocated ring on the error path in the setup
   functions.

The right choice depends on what the flag gating is meant to protect
against, so could you explain the intent there?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-wedfix-v5-0-8b02e78fd34f%40gmail.com