diff mbox series

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

Message ID 20240411114434.26186-3-paul.barker.ct@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ravb Ethernet driver bugfixes | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: claudiu.beznea.uj@bp.renesas.com
netdev/build_clang success Errors and warnings before: 953 this patch: 953
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 953 this patch: 953
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 62 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-11--21-00 (tests: 959)

Commit Message

Paul Barker April 11, 2024, 11:44 a.m. UTC
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@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

Comments

Sergey Shtylyov April 11, 2024, 4:37 p.m. UTC | #1
On 4/11/24 2:44 PM, 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

   Iteration. :-)

> error). However, the current rx descriptor index `priv->cur_rx[q]` was

   RX?

> 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

   RX?

> 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@bp.renesas.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

MBR, Sergey
Niklas Söderlund April 14, 2024, 12:17 p.m. UTC | #2
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@bp.renesas.com>
> ---
>  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;

        ...
    }

> +
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
>  		desc_status = desc->msc;
> @@ -959,9 +963,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  			rx_packets++;
>  			stats->rx_bytes += pkt_len;
>  		}
> -
> -		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> -		desc = &priv->rx_ring[q].ex_desc[entry];
>  	}
>  
>  	/* Refill the RX ring buffers. */
> -- 
> 2.39.2
>
Paul Barker April 15, 2024, 7:12 a.m. UTC | #3
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@bp.renesas.com>
>> ---
>>  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.

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.

Thanks,
Niklas Söderlund April 15, 2024, 9:52 a.m. UTC | #4
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@bp.renesas.com>
> >> ---
> >>  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
diff mbox series

Patch

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;
+
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
 		desc_status = desc->msc;
@@ -959,9 +963,6 @@  static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 			rx_packets++;
 			stats->rx_bytes += pkt_len;
 		}
-
-		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
-		desc = &priv->rx_ring[q].ex_desc[entry];
 	}
 
 	/* Refill the RX ring buffers. */