Re: [PATCH net v3] net: bgmac: fix TX DMA unmapping and slot cleanup

[Simon Horman]

On Tue, Oct 27, 2026 at 05:22:41PM +0000, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022215235.3692083-1-rosenp@xxxxxxxxx?part=1
> ---
> net: bgmac: fix TX DMA unmapping and slot cleanup
> 
> This patch fixes the bgmac_dma_tx_add() error path so that the head and
> each mapped fragment are unmapped from their own slots. It also clears
> dma_addr on every slot the failed packet touched. In
> bgmac_dma_tx_ring_free() it picks the unmap function from the SOF flag
> of the descriptor, unmaps before freeing, and clears slot->skb and
> slot->dma_addr.
> 
> > This was found through code inspection by an LLM while hitting other
> > bugs on an Asus RT-N16. It did not hit these ones specifically.
> >
> > Fixes: 9cde94506eac ("bgmac: implement scatter/gather support")
> > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> 
> [Severity: Low]
> This isn't a bug, but the commit message says an LLM found these
> problems. Should the trailers include an Assisted-by: tag?
> 
> Documentation/process/submitting-patches.rst says:
> 
>   If you used any sort of advanced coding tool in the creation of your
>   patch, you need to acknowledge that use by adding an Assisted-by tag.
> 
> Documentation/process/coding-assistants.rst gives the format as
> "Assisted-by: LLM [TOOL1] [TOOL2]". Step 6 of its procedure for finding
> and fixing bugs also asks for an Assisted-by tag.
> 
> > diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> > index 3e9c57196a397..d44ee8ed32459 100644
> > --- a/drivers/net/ethernet/broadcom/bgmac.c
> > +++ b/drivers/net/ethernet/broadcom/bgmac.c
> 
> [ ... ]
> 
> > @@ -535,17 +540,19 @@ static void bgmac_dma_tx_ring_free(struct bgmac *bgmac,
> >  		unsigned int len = ctl1 & BGMAC_DESC_CTL1_LEN;
> >  
> >  		slot = &ring->slots[i];
> > -		dev_kfree_skb(slot->skb);
> >  
> > -		if (!slot->dma_addr)
> > -			continue;
> > +		if (slot->dma_addr) {
> > +			if (le32_to_cpu(dma_desc[i].ctl0) & BGMAC_DESC_CTL0_SOF)
> > +				dma_unmap_single(dma_dev, slot->dma_addr,
> > +						 len, DMA_TO_DEVICE);
> > +			else
> > +				dma_unmap_page(dma_dev, slot->dma_addr,
> > +					       len, DMA_TO_DEVICE);
> > +			slot->dma_addr = 0;
> > +		}
> >  
> > -		if (slot->skb)
> > -			dma_unmap_single(dma_dev, slot->dma_addr,
> > -					 len, DMA_TO_DEVICE);
> > -		else
> > -			dma_unmap_page(dma_dev, slot->dma_addr,
> > -				       len, DMA_TO_DEVICE);
> > +		dev_kfree_skb(slot->skb);
> > +		slot->skb = NULL;
> 
> [Severity: Low]
> If a pending packet wraps around the end of the ring, is the skb still
> freed before all of its slots are unmapped?
> 
> The commit message says bgmac_dma_tx_ring_free() will "unmap before
> freeing". That ordering only holds within a single slot, because the
> loop walks slots in physical order from 0 to BGMAC_TX_RING_SLOTS - 1.
> 
> bgmac_dma_tx_add() puts fragments in consecutive slots modulo the ring
> size, and sets slot->skb only on the last one:
> 
> drivers/net/ethernet/broadcom/bgmac.c:bgmac_dma_tx_add() {
>     ...
> 		index = (index + 1) % BGMAC_TX_RING_SLOTS;
> 		slot = &ring->slots[index];
>     ...
> 	slot->skb = skb;
>     ...
> }
> 
> Take a scatter/gather packet whose head (SOF) is in the last slot of the
> ring and whose last fragment, which owns the skb, is in slot 0. The loop
> reaches i == 0 first and calls dev_kfree_skb(), which releases skb->head
> and the frag pages. Only later, at i == BGMAC_TX_RING_SLOTS - 1, does it
> call dma_unmap_single() on the head buffer, which is already freed.
> 
> bgmac_stop() and the suspend path call bgmac_chip_reset() before
> bgmac_dma_cleanup(), and the mappings are DMA_TO_DEVICE. So this may
> have no visible effect, but it still breaks the DMA API rule that a
> buffer is unmapped before it is freed.
> 
> bgmac_dma_tx_free() avoids this by walking from ring->start to
> ring->end. Could the same be done here? Another option is to unmap every
> slot in a first pass and free the skbs in a second pass.

Hi Rosen,

The issue flagged above does seem somewhat concerning.
What is your take on it?

...