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

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/29/26 09:15, netdev-bot+sashiko@kernel.org wrote:
> 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.
> 

Agreed. The free list is never consumed after au1000_ReleaseDB() runs, 
so the incorrect linking has no runtime impact. I’ll drop this patch 
from the fixes series and leave removal of the redundant teardown 
bookkeeping for separate cleanup.

pw-bot: cr
-- 
Florian