[PATCH net 0/4] ravb Ethernet driver bugfixes

[PATCH net 0/4] ravb Ethernet driver bugfixes

[Paul Barker]

These patches fix bugs found during recent work on the ravb driver.

Patches 1 & 2 affect the R-Car code paths so have been tested on an
R-Car M3N Salvator-XS board - this is the only R-Car board I currently
have access to.

Patches 2, 3 & 4 affect the GbEth code paths so have been tested on
RZ/G2L and RZ/G2UL SMARC EVK boards.

Paul Barker (4):
  net: ravb: Count packets instead of descriptors in R-Car RX path
  net: ravb: Allow RX loop to move past DMA mapping errors
  net: ravb: Fix GbEth jumbo packet RX checksum handling
  net: ravb: Fix RX byte accounting for jumbo packets

 drivers/net/ethernet/renesas/ravb_main.c | 67 +++++++++++-------------
 1 file changed, 32 insertions(+), 35 deletions(-)


base-commit: 47d8ac011fe1c9251070e1bd64cb10b48193ec51
-- 
2.39.2

[PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path

[Paul Barker]

The units of "work done" in the RX path should be packets instead of
descriptors.

Descriptors which are used by the hardware to record error conditions or
are empty in the case of a DMA mapping error should not count towards
our RX work budget.

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ba01c8cc3c90..70f2900648d4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -892,29 +892,25 @@ 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];
-	int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
-			priv->cur_rx[q];
 	struct net_device_stats *stats = &priv->stats[q];
 	struct ravb_ex_rx_desc *desc;
 	struct sk_buff *skb;
 	dma_addr_t dma_addr;
 	struct timespec64 ts;
+	int rx_packets = 0;
 	u8  desc_status;
 	u16 pkt_len;
 	int limit;
+	int i;
 
-	boguscnt = min(boguscnt, *quota);
-	limit = boguscnt;
+	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
 	desc = &priv->rx_ring[q].ex_desc[entry];
-	while (desc->die_dt != DT_FEMPTY) {
+	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
 		desc_status = desc->msc;
 		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
 
-		if (--boguscnt < 0)
-			break;
-
 		/* We use 0-byte descriptors to mark the DMA mapping errors */
 		if (!pkt_len)
 			continue;
@@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 			if (ndev->features & NETIF_F_RXCSUM)
 				ravb_rx_csum(skb);
 			napi_gro_receive(&priv->napi[q], skb);
-			stats->rx_packets++;
+			rx_packets++;
 			stats->rx_bytes += pkt_len;
 		}
 
@@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 		desc->die_dt = DT_FEMPTY;
 	}
 
-	*quota -= limit - (++boguscnt);
-
-	return boguscnt <= 0;
+	stats->rx_packets += rx_packets;
+	*quota -= rx_packets;
+	return *quota == 0;
 }
 
 /* Packet receive function for Ethernet AVB */
-- 
2.39.2

Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path

[Sergey Shtylyov]

On 4/11/24 2:44 PM, Paul Barker wrote:

> The units of "work done" in the RX path should be packets instead of
> descriptors.
> 
> Descriptors which are used by the hardware to record error conditions or
> are empty in the case of a DMA mapping error should not count towards
> our RX work budget.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path

[Niklas Söderlund]

Hi Paul,

Thanks for your patch.

On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
> The units of "work done" in the RX path should be packets instead of
> descriptors.
> 
> Descriptors which are used by the hardware to record error conditions or
> are empty in the case of a DMA mapping error should not count towards
> our RX work budget.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ba01c8cc3c90..70f2900648d4 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -892,29 +892,25 @@ 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];
> -	int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
> -			priv->cur_rx[q];
>  	struct net_device_stats *stats = &priv->stats[q];
>  	struct ravb_ex_rx_desc *desc;
>  	struct sk_buff *skb;
>  	dma_addr_t dma_addr;
>  	struct timespec64 ts;
> +	int rx_packets = 0;
>  	u8  desc_status;
>  	u16 pkt_len;
>  	int limit;
> +	int i;

The loop variable can never be negative, use unsigned int.

>  
> -	boguscnt = min(boguscnt, *quota);
> -	limit = boguscnt;
> +	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>  	desc = &priv->rx_ring[q].ex_desc[entry];
> -	while (desc->die_dt != DT_FEMPTY) {
> +	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>  		/* Descriptor type must be checked before all other reads */
>  		dma_rmb();
>  		desc_status = desc->msc;
>  		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
>  
> -		if (--boguscnt < 0)
> -			break;
> -

nit: It's a matter of taste, but I like this break condition in the code 
instead of modifying the loop as it's much clearer what's going on. But 
feel free to keep it as is as Sergey likes it.

>  		/* We use 0-byte descriptors to mark the DMA mapping errors */
>  		if (!pkt_len)
>  			continue;
> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  			if (ndev->features & NETIF_F_RXCSUM)
>  				ravb_rx_csum(skb);
>  			napi_gro_receive(&priv->napi[q], skb);
> -			stats->rx_packets++;
> +			rx_packets++;

Why do you add this intermediary variable? Is it not confusing to treat 
rx_packets and rx_bytes differently? Why not instead decrement *quota 
here?

>  			stats->rx_bytes += pkt_len;
>  		}
>  
> @@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  		desc->die_dt = DT_FEMPTY;
>  	}
>  
> -	*quota -= limit - (++boguscnt);
> -
> -	return boguscnt <= 0;
> +	stats->rx_packets += rx_packets;
> +	*quota -= rx_packets;
> +	return *quota == 0;
>  }
>  
>  /* Packet receive function for Ethernet AVB */
> -- 
> 2.39.2
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 3390 bytes --]

On 14/04/2024 13:08, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your patch.
> 
> On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
>> The units of "work done" in the RX path should be packets instead of
>> descriptors.
>>
>> Descriptors which are used by the hardware to record error conditions or
>> are empty in the case of a DMA mapping error should not count towards
>> our RX work budget.
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ba01c8cc3c90..70f2900648d4 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -892,29 +892,25 @@ 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];
>> -	int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
>> -			priv->cur_rx[q];
>>  	struct net_device_stats *stats = &priv->stats[q];
>>  	struct ravb_ex_rx_desc *desc;
>>  	struct sk_buff *skb;
>>  	dma_addr_t dma_addr;
>>  	struct timespec64 ts;
>> +	int rx_packets = 0;
>>  	u8  desc_status;
>>  	u16 pkt_len;
>>  	int limit;
>> +	int i;
> 
> The loop variable can never be negative, use unsigned int.

I matched the type we're comparing against - should we also convert
limit to an unsigned int?

> 
>>  
>> -	boguscnt = min(boguscnt, *quota);
>> -	limit = boguscnt;
>> +	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>>  	desc = &priv->rx_ring[q].ex_desc[entry];
>> -	while (desc->die_dt != DT_FEMPTY) {
>> +	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>>  		/* Descriptor type must be checked before all other reads */
>>  		dma_rmb();
>>  		desc_status = desc->msc;
>>  		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
>>  
>> -		if (--boguscnt < 0)
>> -			break;
>> -
> 
> nit: It's a matter of taste, but I like this break condition in the code 
> instead of modifying the loop as it's much clearer what's going on. But 
> feel free to keep it as is as Sergey likes it.
> 
>>  		/* We use 0-byte descriptors to mark the DMA mapping errors */
>>  		if (!pkt_len)
>>  			continue;
>> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>>  			if (ndev->features & NETIF_F_RXCSUM)
>>  				ravb_rx_csum(skb);
>>  			napi_gro_receive(&priv->napi[q], skb);
>> -			stats->rx_packets++;
>> +			rx_packets++;
> 
> Why do you add this intermediary variable? Is it not confusing to treat 
> rx_packets and rx_bytes differently? Why not instead decrement *quota 
> here?

To me, it's simpler to count received packets once instead of twice
inside the loop (once by incrementing stats->rx_packets, a second time
by decrementing *quota). This also makes future refactoring simpler as
we already have the rx_packets count which we will need to be able to
return so that we can properly track work done in ravb_poll().

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path

[Niklas Söderlund]

Hi Paul,

On 2024-04-15 08:04:05 +0100, Paul Barker wrote:
> On 14/04/2024 13:08, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your patch.
> > 
> > On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
> >> The units of "work done" in the RX path should be packets instead of
> >> descriptors.
> >>
> >> Descriptors which are used by the hardware to record error conditions or
> >> are empty in the case of a DMA mapping error should not count towards
> >> our RX work budget.
> >>
> >> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> >> ---
> >>  drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
> >>  1 file changed, 8 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index ba01c8cc3c90..70f2900648d4 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -892,29 +892,25 @@ 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];
> >> -	int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
> >> -			priv->cur_rx[q];
> >>  	struct net_device_stats *stats = &priv->stats[q];
> >>  	struct ravb_ex_rx_desc *desc;
> >>  	struct sk_buff *skb;
> >>  	dma_addr_t dma_addr;
> >>  	struct timespec64 ts;
> >> +	int rx_packets = 0;
> >>  	u8  desc_status;
> >>  	u16 pkt_len;
> >>  	int limit;
> >> +	int i;
> > 
> > The loop variable can never be negative, use unsigned int.
> 
> I matched the type we're comparing against - should we also convert
> limit to an unsigned int?

If it can't be negative I think that is a good idea.

> 
> > 
> >>  
> >> -	boguscnt = min(boguscnt, *quota);
> >> -	limit = boguscnt;
> >> +	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >>  	desc = &priv->rx_ring[q].ex_desc[entry];
> >> -	while (desc->die_dt != DT_FEMPTY) {
> >> +	for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >>  		/* Descriptor type must be checked before all other reads */
> >>  		dma_rmb();
> >>  		desc_status = desc->msc;
> >>  		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
> >>  
> >> -		if (--boguscnt < 0)
> >> -			break;
> >> -
> > 
> > nit: It's a matter of taste, but I like this break condition in the code 
> > instead of modifying the loop as it's much clearer what's going on. But 
> > feel free to keep it as is as Sergey likes it.
> > 
> >>  		/* We use 0-byte descriptors to mark the DMA mapping errors */
> >>  		if (!pkt_len)
> >>  			continue;
> >> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >>  			if (ndev->features & NETIF_F_RXCSUM)
> >>  				ravb_rx_csum(skb);
> >>  			napi_gro_receive(&priv->napi[q], skb);
> >> -			stats->rx_packets++;
> >> +			rx_packets++;
> > 
> > Why do you add this intermediary variable? Is it not confusing to treat 
> > rx_packets and rx_bytes differently? Why not instead decrement *quota 
> > here?
> 
> To me, it's simpler to count received packets once instead of twice
> inside the loop (once by incrementing stats->rx_packets, a second time
> by decrementing *quota). This also makes future refactoring simpler as
> we already have the rx_packets count which we will need to be able to
> return so that we can properly track work done in ravb_poll().

I see your point, I think my point was made with the R-Car code path in 
mind as it do not yet support splitting a packet over multiple 
descriptors. And I agree there is value in trying to keep the two code 
paths as close together as possible so we eventually can merge them.

With the unsigned issue above fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> Thanks,
> 
> -- 
> Paul Barker






-- 
Kind Regards,
Niklas Söderlund

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

[Paul Barker]

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;
+
 		/* 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

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

[Sergey Shtylyov]

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

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

[Niklas Söderlund]

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
> 

-- 
Kind Regards,
Niklas Söderlund

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 4809 bytes --]

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,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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@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






-- 
Kind Regards,
Niklas Söderlund

[PATCH net 3/4] net: ravb: Fix GbEth jumbo packet RX checksum handling

[Paul Barker]

Sending a 7kB ping packet to the RZ/G2L in Linux v6.9-rc2 causes the
following backtrace:

	WARNING: CPU: 0 PID: 0 at include/linux/skbuff.h:3127 skb_trim+0x30/0x38
	Modules linked in:
	CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W          6.9.0-rc1-00222-gde11614025b1 #3
	Hardware name: Renesas SMARC EVK based on r9a07g044l2 (DT)
	pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
	pc : skb_trim+0x30/0x38
	lr : ravb_rx_csum_gbeth+0x40/0x90
	sp : ffff800080003d40
	x29: ffff800080003d40 x28: 0000000000000400 x27: ffff00000d0f0000
	x26: 0000000000000001 x25: ffff800080003e84 x24: 0000000000000e52
	x23: 0000000000000000 x22: ffff00000d0f0960 x21: 000000000000ffff
	x20: 000000000000f6ff x19: ffff00000cf4cb00 x18: 0000000000000000
	x17: ffff7ffffdd4f000 x16: ffff800080000000 x15: e7e6e5e4e3e2e1e0
	x14: dfdedddcdbdad9d8 x13: 0000131211100f0e x12: 0d0c0b0a09080706
	x11: 0000000013121110 x10: 0000000000000000 x9 : 0000000000000001
	x8 : ffff800080003cf0 x7 : 0000000000000000 x6 : ffff00007faf4590
	x5 : 000000000010000b x4 : a1a8362deecb53ea x3 : 0000000000000080
	x2 : 00000000ffff0000 x1 : 000000000cf4ccfc x0 : ffff00000cf4cb00
	Call trace:
	 skb_trim+0x30/0x38
	 ravb_rx_gbeth+0x56c/0x5cc
	 ravb_poll+0xa0/0x204
	 __napi_poll+0x38/0x17c
	 net_rx_action+0x124/0x268
	 __do_softirq+0x100/0x26c
	 ____do_softirq+0x10/0x1c
	 call_on_irq_stack+0x24/0x4c
	 do_softirq_own_stack+0x1c/0x2c
	 irq_exit_rcu+0xbc/0xd8
	 el1_interrupt+0x38/0x68
	 el1h_64_irq_handler+0x18/0x24
	 el1h_64_irq+0x64/0x68
	 default_idle_call+0x28/0x3c
	 do_idle+0x204/0x25c
	 cpu_startup_entry+0x38/0x3c
	 kernel_init+0x0/0x1d8
	 start_kernel+0x504/0x5f0
	 __primary_switched+0x80/0x88
	---[ end trace 0000000000000000 ]---

This is caused by ravb_rx_gbeth() calling ravb_rx_csum_gbeth() with the
wrong skb for a packet which spans multiple descriptors. To fix this,
use the correct skb.

Fixes: c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 028ab5c6aaf7..e1e39f65224c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -843,7 +843,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 				priv->rx_1st_skb->protocol =
 					eth_type_trans(priv->rx_1st_skb, ndev);
 				if (ndev->features & NETIF_F_RXCSUM)
-					ravb_rx_csum_gbeth(skb);
+					ravb_rx_csum_gbeth(priv->rx_1st_skb);
 				napi_gro_receive(&priv->napi[q],
 						 priv->rx_1st_skb);
 				rx_packets++;
-- 
2.39.2

Re: [PATCH net 3/4] net: ravb: Fix GbEth jumbo packet RX checksum handling

[Sergey Shtylyov]

On 4/11/24 2:44 PM, Paul Barker wrote:

> Sending a 7kB ping packet to the RZ/G2L in Linux v6.9-rc2 causes the
> following backtrace:
> 
> 	WARNING: CPU: 0 PID: 0 at include/linux/skbuff.h:3127 skb_trim+0x30/0x38
> 	Modules linked in:
> 	CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W          6.9.0-rc1-00222-gde11614025b1 #3
> 	Hardware name: Renesas SMARC EVK based on r9a07g044l2 (DT)
> 	pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> 	pc : skb_trim+0x30/0x38
> 	lr : ravb_rx_csum_gbeth+0x40/0x90
> 	sp : ffff800080003d40
> 	x29: ffff800080003d40 x28: 0000000000000400 x27: ffff00000d0f0000
> 	x26: 0000000000000001 x25: ffff800080003e84 x24: 0000000000000e52
> 	x23: 0000000000000000 x22: ffff00000d0f0960 x21: 000000000000ffff
> 	x20: 000000000000f6ff x19: ffff00000cf4cb00 x18: 0000000000000000
> 	x17: ffff7ffffdd4f000 x16: ffff800080000000 x15: e7e6e5e4e3e2e1e0
> 	x14: dfdedddcdbdad9d8 x13: 0000131211100f0e x12: 0d0c0b0a09080706
> 	x11: 0000000013121110 x10: 0000000000000000 x9 : 0000000000000001
> 	x8 : ffff800080003cf0 x7 : 0000000000000000 x6 : ffff00007faf4590
> 	x5 : 000000000010000b x4 : a1a8362deecb53ea x3 : 0000000000000080
> 	x2 : 00000000ffff0000 x1 : 000000000cf4ccfc x0 : ffff00000cf4cb00
> 	Call trace:
> 	 skb_trim+0x30/0x38
> 	 ravb_rx_gbeth+0x56c/0x5cc
> 	 ravb_poll+0xa0/0x204
> 	 __napi_poll+0x38/0x17c
> 	 net_rx_action+0x124/0x268
> 	 __do_softirq+0x100/0x26c
> 	 ____do_softirq+0x10/0x1c
> 	 call_on_irq_stack+0x24/0x4c
> 	 do_softirq_own_stack+0x1c/0x2c
> 	 irq_exit_rcu+0xbc/0xd8
> 	 el1_interrupt+0x38/0x68
> 	 el1h_64_irq_handler+0x18/0x24
> 	 el1h_64_irq+0x64/0x68
> 	 default_idle_call+0x28/0x3c
> 	 do_idle+0x204/0x25c
> 	 cpu_startup_entry+0x38/0x3c
> 	 kernel_init+0x0/0x1d8
> 	 start_kernel+0x504/0x5f0
> 	 __primary_switched+0x80/0x88
> 	---[ end trace 0000000000000000 ]---
> 
> This is caused by ravb_rx_gbeth() calling ravb_rx_csum_gbeth() with the
> wrong skb for a packet which spans multiple descriptors. To fix this,
> use the correct skb.
> 
> Fixes: c2da9408579d ("ravb: Add Rx checksum offload support for GbEth")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[PATCH net 4/4] net: ravb: Fix RX byte accounting for jumbo packets

[Paul Barker]

The RX byte accounting for jumbo packets was changed to fix a potential
use-after-free bug. However, that fix used the wrong variable and so
only accounted for the number of bytes in the final descriptor, not the
number of bytes in the whole packet.

To fix this, we can simply update our stats with the correct number of
bytes before calling napi_gro_receive().

Also rename pkt_len to desc_len in ravb_rx_gbeth() to avoid any future
confusion. The variable name pkt_len is correct in ravb_rx_rcar() as
that function does not handle packets spanning multiple descriptors.

Fixes: 5a5a3e564de6 ("ravb: Fix potential use-after-free in ravb_rx_gbeth()")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e1e39f65224c..c4ac9fbe0af4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -769,7 +769,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 	dma_addr_t dma_addr;
 	int rx_packets = 0;
 	u8  desc_status;
-	u16 pkt_len;
+	u16 desc_len;
 	u8  die_dt;
 	int entry;
 	int limit;
@@ -787,10 +787,10 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 		/* Descriptor type must be checked before all other reads */
 		dma_rmb();
 		desc_status = desc->msc;
-		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
+		desc_len = le16_to_cpu(desc->ds_cc) & RX_DS;
 
 		/* We use 0-byte descriptors to mark the DMA mapping errors */
-		if (!pkt_len)
+		if (!desc_len)
 			continue;
 
 		if (desc_status & MSC_MC)
@@ -811,25 +811,25 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 			switch (die_dt) {
 			case DT_FSINGLE:
 				skb = ravb_get_skb_gbeth(ndev, entry, desc);
-				skb_put(skb, pkt_len);
+				skb_put(skb, desc_len);
 				skb->protocol = eth_type_trans(skb, ndev);
 				if (ndev->features & NETIF_F_RXCSUM)
 					ravb_rx_csum_gbeth(skb);
 				napi_gro_receive(&priv->napi[q], skb);
 				rx_packets++;
-				stats->rx_bytes += pkt_len;
+				stats->rx_bytes += desc_len;
 				break;
 			case DT_FSTART:
 				priv->rx_1st_skb = ravb_get_skb_gbeth(ndev, entry, desc);
-				skb_put(priv->rx_1st_skb, pkt_len);
+				skb_put(priv->rx_1st_skb, desc_len);
 				break;
 			case DT_FMID:
 				skb = ravb_get_skb_gbeth(ndev, entry, desc);
 				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
 							       priv->rx_1st_skb->len,
 							       skb->data,
-							       pkt_len);
-				skb_put(priv->rx_1st_skb, pkt_len);
+							       desc_len);
+				skb_put(priv->rx_1st_skb, desc_len);
 				dev_kfree_skb(skb);
 				break;
 			case DT_FEND:
@@ -837,17 +837,17 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
 							       priv->rx_1st_skb->len,
 							       skb->data,
-							       pkt_len);
-				skb_put(priv->rx_1st_skb, pkt_len);
+							       desc_len);
+				skb_put(priv->rx_1st_skb, desc_len);
 				dev_kfree_skb(skb);
 				priv->rx_1st_skb->protocol =
 					eth_type_trans(priv->rx_1st_skb, ndev);
 				if (ndev->features & NETIF_F_RXCSUM)
 					ravb_rx_csum_gbeth(priv->rx_1st_skb);
+				stats->rx_bytes += priv->rx_1st_skb->len;
 				napi_gro_receive(&priv->napi[q],
 						 priv->rx_1st_skb);
 				rx_packets++;
-				stats->rx_bytes += pkt_len;
 				break;
 			}
 		}
-- 
2.39.2

Re: [PATCH net 4/4] net: ravb: Fix RX byte accounting for jumbo packets

[Sergey Shtylyov]

On 4/11/24 2:44 PM, Paul Barker wrote:

> The RX byte accounting for jumbo packets was changed to fix a potential
> use-after-free bug. However, that fix used the wrong variable and so
> only accounted for the number of bytes in the final descriptor, not the
> number of bytes in the whole packet.
> 
> To fix this, we can simply update our stats with the correct number of
> bytes before calling napi_gro_receive().
> 
> Also rename pkt_len to desc_len in ravb_rx_gbeth() to avoid any future
> confusion. The variable name pkt_len is correct in ravb_rx_rcar() as
> that function does not handle packets spanning multiple descriptors.
> 
> Fixes: 5a5a3e564de6 ("ravb: Fix potential use-after-free in ravb_rx_gbeth()")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey