RE: [PATCH v2 net 9/9] net: enetc: fix the off-by-one issue in enetc_map_tx_tso_buffs()

[Wei Fang]

> > > +/**
> > > + * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer
> TX
> > > frame
> > > + * @tx_ring: Pointer to the TX ring on which the buffer descriptors are
> located
> > > + * @count: Number of TX buffer descriptors which need to be unmapped
> > > + * @i: Index of the last successfully mapped TX buffer descriptor
> > > + */
> > > +static void enetc_unwind_tx_frame(struct enetc_bdr *tx_ring, int count,
> int i)
> > > +{
> > > +	while (count--) {
> > > +		struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
> > > +
> > > +		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > +		if (i == 0)
> > > +			i = tx_ring->bd_count;
> > > +		i--;
> > > +	}
> > > +}
> > > +
> > >  /* Let H/W know BD ring has been updated */
> > >  static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
> > >  {
> > > @@ -399,13 +417,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb)
> > >  dma_err:
> > >  	dev_err(tx_ring->dev, "DMA map error");
> > >
> > > -	while (count--) {
> > > -		tx_swbd = &tx_ring->tx_swbd[i];
> > > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > -		if (i == 0)
> > > -			i = tx_ring->bd_count;
> > > -		i--;
> > > -	}
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > > @@ -752,7 +764,6 @@ static int enetc_lso_map_data(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb,
> > >
> > >  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;
> > >
> > > @@ -776,13 +787,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr
> > > *tx_ring, struct sk_buff *skb)
> > >  	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);
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > > @@ -877,13 +882,7 @@ static int enetc_map_tx_tso_buffs(struct
> enetc_bdr
> > > *tx_ring, struct sk_buff *skb
> > >  	dev_err(tx_ring->dev, "DMA map error");
> > >
> > >  err_chained_bd:
> > > -	while (count--) {
> > > -		tx_swbd = &tx_ring->tx_swbd[i];
> > > -		enetc_free_tx_frame(tx_ring, tx_swbd);
> > > -		if (i == 0)
> > > -			i = tx_ring->bd_count;
> > > -		i--;
> > > -	}
> > > +	enetc_unwind_tx_frame(tx_ring, count, i);
> > >
> > >  	return 0;
> > >  }
> > >
> > > With the definitions laid out explicitly in a kernel-doc, doesn't the
> > > rest of the patch look a bit wrong? Why would you increment "count"
> >
> > Sorry, I don't understand what you mean " With the definitions laid ou
> > explicitly in a kernel-doc", which kernel-doc?
> 
> This kernel-doc:
> 
> /**
>  * enetc_unwind_tx_frame() - Unwind the DMA mappings of a multi-buffer TX
> frame
>  * @count: Number of TX buffer descriptors which need to be unmapped
>  * @i: Index of the last successfully mapped TX buffer descriptor
> 
> The definitions of "count" and "i" are what I'm talking about. It's
> clear to me that the "i" that is passed is not the index of the last
> successfully mapped TX BD.
> 
> > > before enetc_map_tx_tso_data() succeeds? Why isn't the problem that "i"
> > > needs to be rolled back on enetc_map_tx_tso_data() failure instead?
> >
> > The root cause of this issue as you said is that "I" and "count" are not
> > synchronized. Either moving count++ before enetc_map_tx_tso_data()
> > or rolling back 'i' after enetc_map_tx_tso_data() fails should solve the
> > issue. There is no problem with the former, it just loops one more time,
> > and actually the loop does nothing.
> 
> Sorry, I don't understand "there is no problem, it just loops one more
> time, actually the loop does nothing"? What do you mean, could you
> explain more? Why wouldn't it be a problem, if the loop runs one more
> time than TX BDs were added to the ring, that we try to unmap the DMA
> buffer of a TXBD that was previously passed to hardware as part of a
> previous enetc_update_tx_ring_tail()?

Based on your definitions of 'i' and 'count', you are right to say that it's
necessary to roll back 'i' because the last 'i' did not successfully map the
Tx BD.

Regarding to I said moving 'count++' to before enetc_map_tx_tso_data() is
fine because the increment of 'i' and 'count' remain in sync, so 'i' will not
roll back to a value less than the initial value when err_map_data occurs.
So it does not affect the DMA buffer of TXBD which was previously passed
to the hardware as part of the previous enetc_update_tx_ring_tail().