Re: [PATCH net 07/11] net: au1000: fix buffer descriptor free list insertion

[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 and Fixes: tag present this as a functional
  bug fix, but the bug never affects runtime behaviour.

--

net: au1000: fix buffer descriptor free list insertion

The patch changes au1000_ReleaseDB() so the returned descriptor is pushed
onto the head of aup->pDBfree with its pnext pointing at the old head.
This matches au1000_GetFreeDB() and the list setup loop in au1000_probe().
It is tagged as fixing the initial git import.

> ReleaseDB links the old head to the returned descriptor and makes the
> returned descriptor the new head without updating its stale next
> pointer. This loses free descriptors and can create cycles. Link the
> returned descriptor to the old head instead, matching GetFreeDB and the
> list initialization.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
> index c0e38a507f892..f8976e324f0e7 100644
> --- a/drivers/net/ethernet/amd/au1000_eth.c
> +++ b/drivers/net/ethernet/amd/au1000_eth.c
> @@ -583,9 +583,7 @@ static struct db_dest *au1000_GetFreeDB(struct au1000_private *aup)
>
>  static void au1000_ReleaseDB(struct au1000_private *aup, struct db_dest *pDB)
>  {
> -	struct db_dest *pDBfree = aup->pDBfree;
> -	if (pDBfree)
> -		pDBfree->pnext = pDB;
> +	pDB->pnext = aup->pDBfree;
>  	aup->pDBfree = pDB;
>  }

[Severity: Low]
The new linking looks correct. Can the old behaviour actually be seen at
runtime, though?

au1000_GetFreeDB() is only called during the first ring fill in
au1000_probe(), before any au1000_ReleaseDB() call can happen.

au1000_ReleaseDB() is only called from two teardown paths. In both, the
next steps free the descriptor buffers and the au1000_private that holds
pDBfree and db[]. The first is the au1000_probe() error path:

au1000_probe() {
    ...
	for (i = 0; i < NUM_RX_DMA; i++) {
		if (aup->rx_db_inuse[i])
			au1000_ReleaseDB(aup, aup->rx_db_inuse[i]);
	}
	for (i = 0; i < NUM_TX_DMA; i++) {
		if (aup->tx_db_inuse[i])
			au1000_ReleaseDB(aup, aup->tx_db_inuse[i]);
	}
    ...
	dma_free_coherent(&pdev->dev, MAX_BUF_SIZE * (NUM_TX_BUFFS + NUM_RX_BUFFS),
			  aup->vaddr, aup->dma_addr);
err_vaddr:
	free_netdev(dev);
    ...
}

The second is au1000_remove(), which runs the same release loops and then
calls dma_free_coherent() and free_netdev(dev).

So nothing ever walks or pops the list that the old code corrupted. The
lost descriptors and cycles in the commit message would not cause a leak,
a hang or a wrong DMA buffer.

Could the commit message say that there is no runtime impact, and could
the Fixes: tag be dropped so this does not go to stable and the net tree as
a functional fix? Another option is to remove the au1000_ReleaseDB() calls
from the teardown paths, since they have no effect.

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