Re: [PATCH net 2/4] net: ravb: Allow RX loop to move past DMA mapping errors

[Niklas Söderlund]

Hello Paul,

On 2024-04-15 08:12:06 +0100, Paul Barker wrote:
> On 14/04/2024 13:17, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your patch.
> > 
> > On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
> >> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> >> interation if a zero-length descriptor is seen (indicating a DMA mapping
> >> error). However, the current rx descriptor index `priv->cur_rx[q]` was
> >> incremented at the end of the loop and so would not be incremented when
> >> we skip to the next loop iteration. This would cause the loop to keep
> >> seeing the same zero-length descriptor instead of moving on to the next
> >> descriptor.
> >>
> >> As the loop counter `i` still increments, the loop would eventually
> >> terminate so there is no risk of being stuck here forever - but we
> >> should still fix this to avoid wasting cycles.
> >>
> >> To fix this, the rx descriptor index is incremented at the top of the
> >> loop, in the for statement itself. The assignments of `entry` and `desc`
> >> are brought into the loop to avoid the need for duplication.
> >>
> >> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> >> Signed-off-by: Paul Barker <paul.barker.ct@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
> >>  1 file changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 70f2900648d4..028ab5c6aaf7 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >>  	int limit;
> >>  	int i;
> >>  
> >> -	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >>  	stats = &priv->stats[q];
> >>  
> >> -	desc = &priv->rx_ring[q].desc[entry];
> >> -	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> +	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> +		desc = &priv->rx_ring[q].desc[entry];
> >> +		if (desc->die_dt == DT_FEMPTY)
> >> +			break;
> >> +
> >>  		/* Descriptor type must be checked before all other reads */
> >>  		dma_rmb();
> >>  		desc_status = desc->msc;
> >> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >>  				break;
> >>  			}
> >>  		}
> >> -
> >> -		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> >> -		desc = &priv->rx_ring[q].desc[entry];
> >>  	}
> >>  
> >>  	/* Refill the RX ring buffers. */
> >> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >>  {
> >>  	struct ravb_private *priv = netdev_priv(ndev);
> >>  	const struct ravb_hw_info *info = priv->info;
> >> -	int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >>  	struct net_device_stats *stats = &priv->stats[q];
> >>  	struct ravb_ex_rx_desc *desc;
> >>  	struct sk_buff *skb;
> >> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >>  	int rx_packets = 0;
> >>  	u8  desc_status;
> >>  	u16 pkt_len;
> >> +	int entry;
> >>  	int limit;
> >>  	int i;
> >>  
> >>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> -	desc = &priv->rx_ring[q].ex_desc[entry];
> >> -	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> +	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> +		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> +		desc = &priv->rx_ring[q].ex_desc[entry];
> >> +		if (desc->die_dt == DT_FEMPTY)
> >> +			break;
> > 
> > I really like moving the assignment of entry and desc to the top of the 
> > loop. But I don't like the loop limits as it's hard, at least for me, to 
> > immediately see what's going on. How about,
> > 
> >     limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);
> > 
> >     for (i = 0; i < limit; i++) {
> >         entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >         desc = &priv->rx_ring[q].ex_desc[entry];
> > 
> >         /* There are no more valid descriptors after an empty one. */
> >         if (desc->die_dt == DT_FEMPTY)
> >                 break;
> > 
> >         ...
> >     }
> 
> We need to count received packets separately from the number of
> descriptors processed, as done in the previous commit in this series,
> so we can't just have a single check against limit.

As noted in 1/4 I was only considering the R-Car code path where split 
descriptors are not supported. I agree it's good to keep the two code 
paths in sync and with that in mind I'm OK with this approach.

> 
> We also need to increment priv->cur_rx[q]. If we put `priv->cur_rx[q]++`
> at the end of the loop then we're back to having to worry about it when
> we have a continue statement.
> 
> We could move the `rx_packets < *quota` check inside the loop itself,
> but I don't see that as any clearer myself.

I do think this is a good idea however. As this would split the logic in 
two distinct sets. The loop would only deal with descriptors and the 
stop conditions based on number of packets / work done would be a stop
condition inside the loop.

Thinking a head a bit I think it would be nice if in future the private 
data variable rx_1st_skb could be reworked as this will not play nice 
with multiple queues. And with a split of the loop to only consider 
descriptors we could try and look a head and only process a packet if 
all descriptors for it are available to us. Lets cross that bridge when 
we get to it. But I think having the loop only consider descriptors 
would make this easier.

> 
> Thanks,
> 
> -- 
> Paul Barker






-- 
Kind Regards,
Niklas Söderlund