RE: [PATCH net 1/8] net: enetc: fix the off-by-one issue in enetc_map_tx_buffs()

[Wei Fang]

> > -----Original Message-----
> > From: Wei Fang <wei.fang@xxxxxxx>
> > Sent: Tuesday, February 18, 2025 4:03 AM
> [,,,]
> > Subject: RE: [PATCH net 1/8] net: enetc: fix the off-by-one issue in
> > enetc_map_tx_buffs()
> >
> > > > -----Original Message-----
> > > > From: Wei Fang <wei.fang@xxxxxxx>
> > > > Sent: Monday, February 17, 2025 11:39 AM
> > > [...]
> > > > Subject: [PATCH net 1/8] net: enetc: fix the off-by-one issue in
> > > > enetc_map_tx_buffs()
> > > >
> > > > When the DMA mapping error occurs, it will attempt to free 'count + 1'
> > >
> > > Hi Wei,
> > > Are you sure?
> > >
> > > dma_err occurs before count is incremented, at least that's the design.
> > >
> > > First step already contradicts your claim:
> > > ```
> > > static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff
> *skb)
> > > { [...]
> > > 	int i, count = 0;
> > > [...]
> > > 	dma = dma_map_single(tx_ring->dev, skb->data, len,
> > DMA_TO_DEVICE);
> > > 	if (unlikely(dma_mapping_error(tx_ring->dev, dma)))
> > > 		goto dma_err;
> > >
> > > ===> count is 0 on goto path!
> > > [...]
> > > 	count++;
> > > ```
> > >
> >
> > Hi Claudiu,
> >
> > I think it's fine in your case, the count is 0, and the tx_swbd is not set,
> > so it's unnecessary to call enetc_free_tx_frame() in the dma_err path.
> >
> 
> enetc_free_tx_frame() call on dma_err path is still needed for count 0 because
> it needs to free the skb.

First, the tx_swbd is not set when count is 0, so tx_swbd->skb is NULL when
the error occurs.

Second, the skb is freed in enetc_start_xmit() not enetc_free_tx_frame().