RE: [PATCH v8 net-next 3/4] net: enetc: add LSO support for i.MX95 ENETC PF

[Wei Fang]

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: 2024年12月17日 17:20
> To: Wei Fang <wei.fang@xxxxxxx>; Claudiu Manoil
> <claudiu.manoil@xxxxxxx>; Vladimir Oltean <vladimir.oltean@xxxxxxx>;
> Clark Wang <xiaoning.wang@xxxxxxx>; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; horms@xxxxxxxxxx; idosch@xxxxxxxxxx
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 net-next 3/4] net: enetc: add LSO support for i.MX95
> ENETC PF
> 
> On 12/13/24 03:17, Wei Fang wrote:
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> > index 09ca4223ff9d..41a3798c7564 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> > @@ -533,6 +533,224 @@ static void enetc_tso_complete_csum(struct
> enetc_bdr *tx_ring, struct tso_t *tso
> >  	}
> >  }
> >
> > +static inline int enetc_lso_count_descs(const struct sk_buff *skb)
> 
> Please, don't use inline in c files
> 
> > +{
> > +	/* 4 BDs: 1 BD for LSO header + 1 BD for extended BD + 1 BD
> > +	 * for linear area data but not include LSO header, namely
> > +	 * skb_headlen(skb) - lso_hdr_len. And 1 BD for gap.
> > +	 */
> > +	return skb_shinfo(skb)->nr_frags + 4;
> > +}
> > +static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff
> *skb)
> > +{
> > +	struct enetc_tx_swbd *tx_swbd;
> > +	struct enetc_lso_t lso = {0};
> > +	int err, i, count = 0;
> > +
> > +	/* Initialize the LSO handler */
> > +	enetc_lso_start(skb, &lso);
> > +	i = tx_ring->next_to_use;
> > +
> > +	enetc_lso_map_hdr(tx_ring, skb, &i, &lso);
> > +	/* First BD and an extend BD */
> > +	count += 2;
> > +
> > +	err = enetc_lso_map_data(tx_ring, skb, &i, &lso, &count);
> > +	if (err)
> > +		goto dma_err;
> > +
> > +	/* Go to the next BD */
> > +	enetc_bdr_idx_inc(tx_ring, &i);
> > +	tx_ring->next_to_use = i;
> > +	enetc_update_tx_ring_tail(tx_ring);
> > +
> > +	return count;
> > +
> > +dma_err:
> > +	do {
> > +		tx_swbd = &tx_ring->tx_swbd[i];
> > +		enetc_free_tx_frame(tx_ring, tx_swbd);
> > +		if (i == 0)
> > +			i = tx_ring->bd_count;
> > +		i--;
> > +	} while (count--);
> > +
> > +	return 0;
> > +}
> 
> I'm sorry for not catching the issue early, but apparently there is an
> off-by-one in the above loop: if 'count' bds have been used, it will
> attempt to free 'count + 1' of them.
> 
> The minimal fix should be using:
> 
> 	} while (--count);
> 
> /P

Thanks for reminder, I will fix it. :)