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

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")

[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