[net-next PATCH 00/11] Extend GbEth checksum offload support to VLAN/IPv6 packets

[net-next PATCH 00/11] Extend GbEth checksum offload support to VLAN/IPv6 packets

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP found in Renesas RZ/G2L, RZ/G3S and related SoCs supports
hardware checksum offload for packets in the following cases:

 - there are zero or one VLAN headers with TPID=0x8100
 - the network protocol is IPv4 or IPv6
 - the transport protocol is TCP, UDP or ICMP
 - the packet is not fragmented 

To complete the support for all these cases in the ravb driver, we need
to add handling for ICMP packets, VLAN-tagged packets and IPv6 packets
in both the TX and RX code paths.

These patches also do some refactoring/tidy-up, drop unnecessary checks
from performance sensitive code paths and disable unnecessary IP header
checksum offloading.

Paul Barker (11):
  net: ravb: Factor out checksum offload enable bits
  net: ravb: Disable IP header RX checksum offloading
  net: ravb: Drop IP protocol check from RX csum verification
  net: ravb: Combine if conditions in RX csum validation
  net: ravb: Simplify types in RX csum validation
  net: ravb: Disable IP header TX checksum offloading
  net: ravb: Simplify UDP TX checksum offload
  net: ravb: Support ICMP TX checksum offload for GbEth
  net: ravb: Enable IPv6 RX checksum offloading for GbEth
  net: ravb: Enable IPv6 TX checksum offload for GbEth
  net: ravb: Add VLAN checksum support

 drivers/net/ethernet/renesas/ravb.h      |   7 ++
 drivers/net/ethernet/renesas/ravb_main.c | 108 +++++++++++++----------
 2 files changed, 68 insertions(+), 47 deletions(-)

-- 
2.43.0

[net-next PATCH 01/11] net: ravb: Factor out checksum offload enable bits

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

Introduce new constants for the CSR1 (TX) and CSR2 (RX) checksum enable
bits, removing the risk of inconsistency when we change which flags we
enable.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 4 ++++
 drivers/net/ethernet/renesas/ravb_main.c | 9 ++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a7de5cf6b317..4e1e0a754cd9 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -998,6 +998,8 @@ enum CSR1_BIT {
 	CSR1_TDHD	= 0x08000000,
 };
 
+#define CSR1_CSUM_ENABLE (CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4)
+
 enum CSR2_BIT {
 	CSR2_RIP4	= 0x00000001,
 	CSR2_RTCP4	= 0x00000010,
@@ -1012,6 +1014,8 @@ enum CSR2_BIT {
 	CSR2_RDHD	= 0x08000000,
 };
 
+#define CSR2_CSUM_ENABLE (CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4)
+
 #define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
 #define NUM_RX_QUEUE	2
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index d2a6518532f3..b53f6062a106 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -504,11 +504,10 @@ static void ravb_csum_init_gbeth(struct net_device *ndev)
 			ndev->features &= ~NETIF_F_RXCSUM;
 	} else {
 		if (tx_enable)
-			ravb_write(ndev, CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4, CSR1);
+			ravb_write(ndev, CSR1_CSUM_ENABLE, CSR1);
 
 		if (rx_enable)
-			ravb_write(ndev, CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4,
-				   CSR2);
+			ravb_write(ndev, CSR2_CSUM_ENABLE, CSR2);
 	}
 
 done:
@@ -2531,7 +2530,7 @@ static int ravb_set_features_gbeth(struct net_device *ndev,
 	spin_lock_irqsave(&priv->lock, flags);
 	if (changed & NETIF_F_RXCSUM) {
 		if (features & NETIF_F_RXCSUM)
-			val = CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4;
+			val = CSR2_CSUM_ENABLE;
 		else
 			val = 0;
 
@@ -2542,7 +2541,7 @@ static int ravb_set_features_gbeth(struct net_device *ndev,
 
 	if (changed & NETIF_F_HW_CSUM) {
 		if (features & NETIF_F_HW_CSUM)
-			val = CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4;
+			val = CSR1_CSUM_ENABLE;
 		else
 			val = 0;
 
-- 
2.43.0

Re: [net-next PATCH 01/11] net: ravb: Factor out checksum offload enable bits

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> Introduce new constants for the CSR1 (TX) and CSR2 (RX) checksum enable
> bits, removing the risk of inconsistency when we change which flags we
> enable.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[net-next PATCH 02/11] net: ravb: Disable IP header RX checksum offloading

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

For IPv4 packets, the header checksum will always be checked in software
in the RX path (inet_gro_receive() calls ip_fast_csum() unconditionally)
so there is no advantage in asking the hardware to also calculate this
checksum.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 16 +++++++++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 4e1e0a754cd9..98496aa39f3d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1014,7 +1014,7 @@ enum CSR2_BIT {
 	CSR2_RDHD	= 0x08000000,
 };
 
-#define CSR2_CSUM_ENABLE (CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4)
+#define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4)
 
 #define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index b53f6062a106..f1b487152555 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -749,13 +749,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
-	__wsum csum_ip_hdr, csum_proto;
+	__wsum csum_proto;
 	skb_frag_t *last_frag;
 	u8 *hw_csum;
 
 	/* The hardware checksum status is contained in sizeof(__sum16) * 2 = 4
-	 * bytes appended to packet data. First 2 bytes is ip header checksum
-	 * and last 2 bytes is protocol checksum.
+	 * bytes appended to packet data.
+	 *
+	 * For ipv4, the first 2 bytes are the ip header checksum status. We can
+	 * ignore this as it will always be re-checked in inet_gro_receive().
+	 *
+	 * The last 2 bytes are the protocol checksum status which will be zero
+	 * if the checksum has been validated.
 	 */
 	if (unlikely(skb->len < sizeof(__sum16) * 2))
 		return;
@@ -771,16 +776,13 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 	hw_csum -= sizeof(__sum16);
 	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
 
-	hw_csum -= sizeof(__sum16);
-	csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
-
 	if (skb_is_nonlinear(skb))
 		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
 	else
 		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
 
 	/* TODO: IPV6 Rx checksum */
-	if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto)
+	if (skb->protocol == htons(ETH_P_IP) && !csum_proto)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 
-- 
2.43.0

Re: [net-next PATCH 02/11] net: ravb: Disable IP header RX checksum offloading

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> For IPv4 packets, the header checksum will always be checked in software
> in the RX path (inet_gro_receive() calls ip_fast_csum() unconditionally)
> so there is no advantage in asking the hardware to also calculate this
> checksum.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[net-next PATCH 03/11] net: ravb: Drop IP protocol check from RX csum verification

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

We do not need to confirm that the protocol is IPv4. If the hardware
encounters an unsupported protocol, it will set the checksum value to
0xFFFF.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index f1b487152555..f3913c164161 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -781,8 +781,7 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 	else
 		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
 
-	/* TODO: IPV6 Rx checksum */
-	if (skb->protocol == htons(ETH_P_IP) && !csum_proto)
+	if (!csum_proto)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 
-- 
2.43.0

Re: [net-next PATCH 03/11] net: ravb: Drop IP protocol check from RX csum verification

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> We do not need to confirm that the protocol is IPv4. If the hardware
> encounters an unsupported protocol, it will set the checksum value to
> 0xFFFF.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[net-next PATCH 04/11] net: ravb: Combine if conditions in RX csum validation

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

We can merge the two if conditions on skb_is_nonlinear(). Since
skb_frag_size_sub() and skb_trim() do not free memory, it is still safe
to access the trimmed bytes at the end of the packet after these calls.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index f3913c164161..1dd2152734b0 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -769,18 +769,15 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 		last_frag = &shinfo->frags[shinfo->nr_frags - 1];
 		hw_csum = skb_frag_address(last_frag) +
 			  skb_frag_size(last_frag);
+		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
 	} else {
 		hw_csum = skb_tail_pointer(skb);
+		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
 	}
 
 	hw_csum -= sizeof(__sum16);
 	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
 
-	if (skb_is_nonlinear(skb))
-		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
-	else
-		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
-
 	if (!csum_proto)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
-- 
2.43.0

Re: [net-next PATCH 04/11] net: ravb: Combine if conditions in RX csum validation

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> We can merge the two if conditions on skb_is_nonlinear(). Since
> skb_frag_size_sub() and skb_trim() do not free memory, it is still safe
> to access the trimmed bytes at the end of the packet after these calls.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[net-next PATCH 05/11] net: ravb: Simplify types in RX csum validation

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The HW checksum value is used as a 16-bit flag, it is zero when the
checksum has been validated and non-zero otherwise. Therefore we don't
need to treat this as an actual __wsum type or call csum_unfold(), we
can just use a u16 pointer.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 30 +++++++++++-------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 1dd2152734b0..9350ca10ab22 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -749,12 +749,11 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
-	__wsum csum_proto;
-	skb_frag_t *last_frag;
-	u8 *hw_csum;
+	size_t csum_len;
+	u16 *hw_csum;
 
-	/* The hardware checksum status is contained in sizeof(__sum16) * 2 = 4
-	 * bytes appended to packet data.
+	/* The hardware checksum status is contained in 4 bytes appended to
+	 * packet data.
 	 *
 	 * For ipv4, the first 2 bytes are the ip header checksum status. We can
 	 * ignore this as it will always be re-checked in inet_gro_receive().
@@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 	 * The last 2 bytes are the protocol checksum status which will be zero
 	 * if the checksum has been validated.
 	 */
-	if (unlikely(skb->len < sizeof(__sum16) * 2))
+	csum_len = sizeof(*hw_csum) * 2;
+	if (unlikely(skb->len < csum_len))
 		return;
 
 	if (skb_is_nonlinear(skb)) {
-		last_frag = &shinfo->frags[shinfo->nr_frags - 1];
-		hw_csum = skb_frag_address(last_frag) +
-			  skb_frag_size(last_frag);
-		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
+		skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1];
+
+		hw_csum = (u16 *)(skb_frag_address(last_frag) +
+				  skb_frag_size(last_frag));
+		skb_frag_size_sub(last_frag, csum_len);
 	} else {
-		hw_csum = skb_tail_pointer(skb);
-		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
+		hw_csum = (u16 *)skb_tail_pointer(skb);
+		skb_trim(skb, skb->len - csum_len);
 	}
 
-	hw_csum -= sizeof(__sum16);
-	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
-
-	if (!csum_proto)
+	if (!*--hw_csum)
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 }
 
-- 
2.43.0

Re: [net-next PATCH 05/11] net: ravb: Simplify types in RX csum validation

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The HW checksum value is used as a 16-bit flag, it is zero when the

   I think I prefer s/HW/hardware/ but there's no hard feelings... :-)

> checksum has been validated and non-zero otherwise. Therefore we don't
> need to treat this as an actual __wsum type or call csum_unfold(), we
> can just use a u16 pointer.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 1dd2152734b0..9350ca10ab22 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>  	 * The last 2 bytes are the protocol checksum status which will be zero
>  	 * if the checksum has been validated.
>  	 */
> -	if (unlikely(skb->len < sizeof(__sum16) * 2))
> +	csum_len = sizeof(*hw_csum) * 2;

   Could've been done by an initializer instead?

> +	if (unlikely(skb->len < csum_len))
>  		return;
>  
>  	if (skb_is_nonlinear(skb)) {
> -		last_frag = &shinfo->frags[shinfo->nr_frags - 1];
> -		hw_csum = skb_frag_address(last_frag) +
> -			  skb_frag_size(last_frag);
> -		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
> +		skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1];

   Could've been done in the previous patch...

> +
> +		hw_csum = (u16 *)(skb_frag_address(last_frag) +
> +				  skb_frag_size(last_frag));
> +		skb_frag_size_sub(last_frag, csum_len);
>  	} else {
> -		hw_csum = skb_tail_pointer(skb);
> -		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
> +		hw_csum = (u16 *)skb_tail_pointer(skb);
> +		skb_trim(skb, skb->len - csum_len);
>  	}
>  
> -	hw_csum -= sizeof(__sum16);
> -	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
> -
> -	if (!csum_proto)
> +	if (!*--hw_csum)

   Hm, you lost get_unaligned_le16() here. The checksum can be anywhere,
unaligned too...

[...]

MBR, Sergey

Re: [net-next PATCH 05/11] net: ravb: Simplify types in RX csum validation

[Sergey Shtylyov]

On 9/30/24 22:11, Sergey Shtylyov wrote:
[...]

>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The HW checksum value is used as a 16-bit flag, it is zero when the
> 
>    I think I prefer s/HW/hardware/ but there's no hard feelings... :-)
> 
>> checksum has been validated and non-zero otherwise. Therefore we don't
>> need to treat this as an actual __wsum type or call csum_unfold(), we
>> can just use a u16 pointer.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 1dd2152734b0..9350ca10ab22 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
[...]
>> +	if (unlikely(skb->len < csum_len))
>>  		return;
>>  
>>  	if (skb_is_nonlinear(skb)) {
>> -		last_frag = &shinfo->frags[shinfo->nr_frags - 1];
>> -		hw_csum = skb_frag_address(last_frag) +
>> -			  skb_frag_size(last_frag);
>> -		skb_frag_size_sub(last_frag, 2 * sizeof(__sum16));
>> +		skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1];
> 
>    Could've been done in the previous patch...

   Even fit better there, I think...

>> +
>> +		hw_csum = (u16 *)(skb_frag_address(last_frag) +
>> +				  skb_frag_size(last_frag));
>> +		skb_frag_size_sub(last_frag, csum_len);
>>  	} else {
>> -		hw_csum = skb_tail_pointer(skb);
>> -		skb_trim(skb, skb->len - 2 * sizeof(__sum16));
>> +		hw_csum = (u16 *)skb_tail_pointer(skb);
>> +		skb_trim(skb, skb->len - csum_len);
>>  	}
>>  
>> -	hw_csum -= sizeof(__sum16);
>> -	csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
>> -
>> -	if (!csum_proto)
>> +	if (!*--hw_csum)
> 
>    Hm, you lost get_unaligned_le16() here. The checksum can be anywhere,
> unaligned too...

   No need to keep using get_unaligned_le16() itself but you should then switch to 
using get_unaligned().

[...]

MBR, Sergey

Re: [net-next PATCH 05/11] net: ravb: Simplify types in RX csum validation

[Paul Barker]

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

On 30/09/2024 20:11, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The HW checksum value is used as a 16-bit flag, it is zero when the
> 
>    I think I prefer s/HW/hardware/ but there's no hard feelings... :-)
> 
>> checksum has been validated and non-zero otherwise. Therefore we don't
>> need to treat this as an actual __wsum type or call csum_unfold(), we
>> can just use a u16 pointer.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 1dd2152734b0..9350ca10ab22 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
>> @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>>  	 * The last 2 bytes are the protocol checksum status which will be zero
>>  	 * if the checksum has been validated.
>>  	 */
>> -	if (unlikely(skb->len < sizeof(__sum16) * 2))
>> +	csum_len = sizeof(*hw_csum) * 2;
> 
>    Could've been done by an initializer instead?

So, if I move this to the initializers at the start of the function,
csum_len must be declared after hw_csum which breaks reverse Christmas
tree ordering:

    struct skb_shared_info *shinfo = skb_shinfo(skb);
    u16 *hw_csum;
    size_t csum_len = sizeof(*hw_csum) * 2;

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: [net-next PATCH 05/11] net: ravb: Simplify types in RX csum validation

[Sergey Shtylyov]

On 10/3/24 12:23, Paul Barker wrote:
[...]

>>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>>
>>> The HW checksum value is used as a 16-bit flag, it is zero when the
>>
>>    I think I prefer s/HW/hardware/ but there's no hard feelings... :-)
>>
>>> checksum has been validated and non-zero otherwise. Therefore we don't
>>> need to treat this as an actual __wsum type or call csum_unfold(), we
>>> can just use a u16 pointer.
>>>
>>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 1dd2152734b0..9350ca10ab22 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> [...]
>>> @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>>>  	 * The last 2 bytes are the protocol checksum status which will be zero
>>>  	 * if the checksum has been validated.
>>>  	 */
>>> -	if (unlikely(skb->len < sizeof(__sum16) * 2))
>>> +	csum_len = sizeof(*hw_csum) * 2;
>>
>>    Could've been done by an initializer instead?
> 
> So, if I move this to the initializers at the start of the function,
> csum_len must be declared after hw_csum which breaks reverse Christmas
> tree ordering:
> 
>     struct skb_shared_info *shinfo = skb_shinfo(skb);
>     u16 *hw_csum;
>     size_t csum_len = sizeof(*hw_csum) * 2;

   Could use sizeof(u16) instead, but it's OK to ignore me on this
matter. :-)

> Thanks,

MBR, Sergey

[net-next PATCH 06/11] net: ravb: Disable IP header TX checksum offloading

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

For IPv4 packets, the header checksum will always be calculated in software
in the TX path (Documentation/networking/checksum-offloads.rst says "No
offloading of the IP header checksum is performed; it is always done in
software.") so there is no advantage in asking the hardware to also
calculate this checksum.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 98496aa39f3d..a5b4f4fe77b1 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -998,7 +998,7 @@ enum CSR1_BIT {
 	CSR1_TDHD	= 0x08000000,
 };
 
-#define CSR1_CSUM_ENABLE (CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4)
+#define CSR1_CSUM_ENABLE (CSR1_TTCP4 | CSR1_TUDP4)
 
 enum CSR2_BIT {
 	CSR2_RIP4	= 0x00000001,
-- 
2.43.0

Re: [net-next PATCH 06/11] net: ravb: Disable IP header TX checksum offloading

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> For IPv4 packets, the header checksum will always be calculated in software
> in the TX path (Documentation/networking/checksum-offloads.rst says "No
> offloading of the IP header checksum is performed; it is always done in
> software.") so there is no advantage in asking the hardware to also
> calculate this checksum.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

MBR, Sergey

[net-next PATCH 07/11] net: ravb: Simplify UDP TX checksum offload

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP will pass through a null UDP checksum without asserting any
error flags so we do not need to resort to software checksum calculation
in this case.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9350ca10ab22..ae0268f2eb04 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2075,20 +2075,12 @@ static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
 
 	switch (ip->protocol) {
 	case IPPROTO_TCP:
-		break;
 	case IPPROTO_UDP:
-		/* If the checksum value in the UDP header field is 0, TOE does
-		 * not calculate checksum for UDP part of this frame as it is
-		 * optional function as per standards.
-		 */
-		if (udp_hdr(skb)->check == 0)
-			return false;
-		break;
+		return true;
+
 	default:
 		return false;
 	}
-
-	return true;
 }
 
 /* Packet transmit function for Ethernet AVB */
-- 
2.43.0

Re: [net-next PATCH 07/11] net: ravb: Simplify UDP TX checksum offload

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The GbEth IP will pass through a null UDP checksum without asserting any

   s/null/zero/, perhaps?

> error flags so we do not need to resort to software checksum calculation
> in this case.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

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

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9350ca10ab22..ae0268f2eb04 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2075,20 +2075,12 @@ static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>  
>  	switch (ip->protocol) {
>  	case IPPROTO_TCP:
> -		break;
>  	case IPPROTO_UDP:
> -		/* If the checksum value in the UDP header field is 0, TOE does
> -		 * not calculate checksum for UDP part of this frame as it is
> -		 * optional function as per standards.
> -		 */
> -		if (udp_hdr(skb)->check == 0)
> -			return false;
> -		break;
> +		return true;
> +

   No need for an empty line here, it's not used elsewhere in the *switch*
statements...

>  	default:
>  		return false;
>  	}

[...]

MBR, Sergey

[net-next PATCH 08/11] net: ravb: Support ICMP TX checksum offload for GbEth

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

This aligns TX capabilities with RX capabilities.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a5b4f4fe77b1..ef24d9f26a2e 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -998,7 +998,7 @@ enum CSR1_BIT {
 	CSR1_TDHD	= 0x08000000,
 };
 
-#define CSR1_CSUM_ENABLE (CSR1_TTCP4 | CSR1_TUDP4)
+#define CSR1_CSUM_ENABLE (CSR1_TTCP4 | CSR1_TUDP4 | CSR1_TICMP4)
 
 enum CSR2_BIT {
 	CSR2_RIP4	= 0x00000001,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ae0268f2eb04..69d71fbf3e02 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2076,6 +2076,7 @@ static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
 	switch (ip->protocol) {
 	case IPPROTO_TCP:
 	case IPPROTO_UDP:
+	case IPPROTO_ICMP:
 		return true;
 
 	default:
-- 
2.43.0

Re: [net-next PATCH 08/11] net: ravb: Support ICMP TX checksum offload for GbEth

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> This aligns TX capabilities with RX capabilities.

   I suspect the ICMP TX checksums aren't offloaded anywway...

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

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

MBR, Sergey

[net-next PATCH 09/11] net: ravb: Enable IPv6 RX checksum offloading for GbEth

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP supports offloading IPv6 TCP, UDP & ICMPv6 checksums.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index ef24d9f26a2e..8523b89ba1c6 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1014,7 +1014,8 @@ enum CSR2_BIT {
 	CSR2_RDHD	= 0x08000000,
 };
 
-#define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4)
+#define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
+			  CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
 
 #define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
-- 
2.43.0

Re: [net-next PATCH 09/11] net: ravb: Enable IPv6 RX checksum offloading for GbEth

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The GbEth IP supports offloading IPv6 TCP, UDP & ICMPv6 checksums.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

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

MBR, Sergey

[net-next PATCH 10/11] net: ravb: Enable IPv6 TX checksum offload for GbEth

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP supports offloading IPv6 TCP, UDP & ICMPv6 checksums.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  3 ++-
 drivers/net/ethernet/renesas/ravb_main.c | 31 ++++++++++++++----------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 8523b89ba1c6..2cb6c4ef1330 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -998,7 +998,8 @@ enum CSR1_BIT {
 	CSR1_TDHD	= 0x08000000,
 };
 
-#define CSR1_CSUM_ENABLE (CSR1_TTCP4 | CSR1_TUDP4 | CSR1_TICMP4)
+#define CSR1_CSUM_ENABLE (CSR1_TTCP4 | CSR1_TUDP4 | CSR1_TICMP4 | \
+			  CSR1_TTCP6 | CSR1_TUDP6 | CSR1_TICMP6)
 
 enum CSR2_BIT {
 	CSR2_RIP4	= 0x00000001,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 69d71fbf3e02..832132d44fb4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2063,25 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
 {
-	struct iphdr *ip = ip_hdr(skb);
-
 	/* TODO: Need to add support for VLAN tag 802.1Q */
 	if (skb_vlan_tag_present(skb))
 		return false;
 
-	/* TODO: Need to add hardware checksum for IPv6 */
-	if (skb->protocol != htons(ETH_P_IP))
-		return false;
-
-	switch (ip->protocol) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_ICMP:
-		return true;
+	switch (ntohs(skb->protocol)) {
+	case ETH_P_IP:
+		switch (ip_hdr(skb)->protocol) {
+		case IPPROTO_TCP:
+		case IPPROTO_UDP:
+		case IPPROTO_ICMP:
+			return true;
+		}
+		break;
 
-	default:
-		return false;
+	case ETH_P_IPV6:
+		switch (ipv6_hdr(skb)->nexthdr) {
+		case IPPROTO_TCP:
+		case IPPROTO_UDP:
+		case IPPROTO_ICMPV6:
+			return true;
+		}
 	}
+
+	return false;
 }
 
 /* Packet transmit function for Ethernet AVB */
-- 
2.43.0

Re: [net-next PATCH 10/11] net: ravb: Enable IPv6 TX checksum offload for GbEth

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The GbEth IP supports offloading IPv6 TCP, UDP & ICMPv6 checksums.

   TX, you mean (and RX in previous patch?

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

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

MBR, Sergey

[net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Paul Barker]

From: Paul Barker <paul.barker.ct@bp.renesas.com>

The GbEth IP supports offloading checksum calculation for VLAN-tagged
packets, provided that the EtherType is 0x8100 and only one VLAN tag is
present.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  1 +
 drivers/net/ethernet/renesas/ravb_main.c | 27 +++++++++++++++++++++---
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 2cb6c4ef1330..bfa834afc03a 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1056,6 +1056,7 @@ struct ravb_hw_info {
 	size_t gstrings_size;
 	netdev_features_t net_hw_features;
 	netdev_features_t net_features;
+	netdev_features_t vlan_features;
 	int stats_len;
 	u32 tccr_mask;
 	u32 tx_max_frame_size;
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 832132d44fb4..eb7499d42a9b 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 
 static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
 {
-	/* TODO: Need to add support for VLAN tag 802.1Q */
-	if (skb_vlan_tag_present(skb))
+	u16 net_protocol = ntohs(skb->protocol);
+
+	/* GbEth IP can calculate the checksum if:
+	 * - there are zero or one VLAN headers with TPID=0x8100
+	 * - the network protocol is IPv4 or IPv6
+	 * - the transport protocol is TCP, UDP or ICMP
+	 * - the packet is not fragmented
+	 */
+
+	if (skb_vlan_tag_present(skb) &&
+	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
 		return false;
 
-	switch (ntohs(skb->protocol)) {
+	if (net_protocol == ETH_P_8021Q) {
+		struct vlan_hdr vhdr, *vh;
+
+		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
+		if (!vh)
+			return false;
+
+		net_protocol = ntohs(vh->h_vlan_encapsulated_proto);
+	}
+
+	switch (net_protocol) {
 	case ETH_P_IP:
 		switch (ip_hdr(skb)->protocol) {
 		case IPPROTO_TCP:
@@ -2772,6 +2791,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
 	.net_hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
+	.vlan_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.tccr_mask = TCCR_TSRQ0,
 	.tx_max_frame_size = 1522,
@@ -2914,6 +2934,7 @@ static int ravb_probe(struct platform_device *pdev)
 
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
+	ndev->vlan_features = info->vlan_features;
 
 	error = reset_control_deassert(rstc);
 	if (error)
-- 
2.43.0

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Sergey Shtylyov]

On 9/30/24 19:08, Paul Barker wrote:

> From: Paul Barker <paul.barker.ct@bp.renesas.com>
> 
> The GbEth IP supports offloading checksum calculation for VLAN-tagged
> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
> present.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 832132d44fb4..eb7499d42a9b 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  
>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>  {
> -	/* TODO: Need to add support for VLAN tag 802.1Q */
> -	if (skb_vlan_tag_present(skb))
> +	u16 net_protocol = ntohs(skb->protocol);
> +
> +	/* GbEth IP can calculate the checksum if:
> +	 * - there are zero or one VLAN headers with TPID=0x8100
> +	 * - the network protocol is IPv4 or IPv6
> +	 * - the transport protocol is TCP, UDP or ICMP
> +	 * - the packet is not fragmented
> +	 */
> +
> +	if (skb_vlan_tag_present(skb) &&
> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))

   Not sure I understand this check... Maybe s/==/!=/?

>  		return false;
>  
> -	switch (ntohs(skb->protocol)) {
> +	if (net_protocol == ETH_P_8021Q) {
> +		struct vlan_hdr vhdr, *vh;
> +
> +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);

   Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...

[...]

MBR, Sergey

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Sergey Shtylyov]

On 9/30/24 23:36, Sergey Shtylyov wrote:
[...]

>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>> present.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 832132d44fb4..eb7499d42a9b 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
[...]
>> -	switch (ntohs(skb->protocol)) {
>> +	if (net_protocol == ETH_P_8021Q) {
>> +		struct vlan_hdr vhdr, *vh;
>> +
>> +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
> 
>    Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...

    Wikipedia indeed says that the VLAN header begins with TPID word (0x8100)
and then the TCI word follows -- while in Linux, *struct* vlan_hgr starts with
the TCI word -- hence my confusion... :-)

> [...]

MBR, Sergey

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Simon Horman]

On Mon, Sep 30, 2024 at 11:36:40PM +0300, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
> > From: Paul Barker <paul.barker.ct@bp.renesas.com>
> > 
> > The GbEth IP supports offloading checksum calculation for VLAN-tagged
> > packets, provided that the EtherType is 0x8100 and only one VLAN tag is
> > present.
> > 
> > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 832132d44fb4..eb7499d42a9b 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> >  
> >  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
> >  {
> > -	/* TODO: Need to add support for VLAN tag 802.1Q */
> > -	if (skb_vlan_tag_present(skb))
> > +	u16 net_protocol = ntohs(skb->protocol);
> > +
> > +	/* GbEth IP can calculate the checksum if:
> > +	 * - there are zero or one VLAN headers with TPID=0x8100
> > +	 * - the network protocol is IPv4 or IPv6
> > +	 * - the transport protocol is TCP, UDP or ICMP
> > +	 * - the packet is not fragmented
> > +	 */
> > +
> > +	if (skb_vlan_tag_present(skb) &&
> > +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
> 
>    Not sure I understand this check... Maybe s/==/!=/?

A minor nit if the check stays in some form:
vlan_proto is big endian, while ETH_P_8021Q is host byte order.

> 
> >  		return false;
> >  
> > -	switch (ntohs(skb->protocol)) {
> > +	if (net_protocol == ETH_P_8021Q) {
> > +		struct vlan_hdr vhdr, *vh;
> > +
> > +		vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
> 
>    Hm, I thought the VLAN header starts at ETH_HLEN - 2, not at ETH_HLEN...
> 
> [...]
> 
> MBR, Sergey
> 
> 

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Sergey Shtylyov]

On 10/1/24 13:44, Simon Horman wrote:
[...]

>>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>>
>>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>>> present.
>>>
>>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 832132d44fb4..eb7499d42a9b 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>>>  
>>>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>>>  {
>>> -	/* TODO: Need to add support for VLAN tag 802.1Q */
>>> -	if (skb_vlan_tag_present(skb))
>>> +	u16 net_protocol = ntohs(skb->protocol);
>>> +
>>> +	/* GbEth IP can calculate the checksum if:
>>> +	 * - there are zero or one VLAN headers with TPID=0x8100
>>> +	 * - the network protocol is IPv4 or IPv6
>>> +	 * - the transport protocol is TCP, UDP or ICMP
>>> +	 * - the packet is not fragmented
>>> +	 */
>>> +
>>> +	if (skb_vlan_tag_present(skb) &&
>>> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
>>
>>    Not sure I understand this check... Maybe s/==/!=/?
> 
> A minor nit if the check stays in some form:
> vlan_proto is big endian, while ETH_P_8021Q is host byte order.

   Not minor at all, thanks for spotting!
   Luckily, we also have a kernel test robot which runs sparse. :-)

[...]

MBR, Sergey

Re: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[Paul Barker]

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

On 30/09/2024 21:36, Sergey Shtylyov wrote:
> On 9/30/24 19:08, Paul Barker wrote:
> 
>> From: Paul Barker <paul.barker.ct@bp.renesas.com>
>>
>> The GbEth IP supports offloading checksum calculation for VLAN-tagged
>> packets, provided that the EtherType is 0x8100 and only one VLAN tag is
>> present.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 832132d44fb4..eb7499d42a9b 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -2063,11 +2063,30 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>>  
>>  static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
>>  {
>> -	/* TODO: Need to add support for VLAN tag 802.1Q */
>> -	if (skb_vlan_tag_present(skb))
>> +	u16 net_protocol = ntohs(skb->protocol);
>> +
>> +	/* GbEth IP can calculate the checksum if:
>> +	 * - there are zero or one VLAN headers with TPID=0x8100
>> +	 * - the network protocol is IPv4 or IPv6
>> +	 * - the transport protocol is TCP, UDP or ICMP
>> +	 * - the packet is not fragmented
>> +	 */
>> +
>> +	if (skb_vlan_tag_present(skb) &&
>> +	    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
> 
>    Not sure I understand this check... Maybe s/==/!=/?

So, after a bit more investigation, I think this was based on a faulty
understanding. I can't find any clear documentation of this so I've gone
wandering through the code.

In vlan_dev_init() in net/8021q/vlan_dev.c, there is a check for
vlan_hw_offload_capable() on the underlying network device. If this is
false (as it will be for the GbEth IP), a set of header_ops is selected
which inserts the vlan tag into the skb data. So, we can ignore
skb->vlan_proto as skb->protocol will always be set to the VLAN protocol
for VLAN tagged packets.

The conclusion is that we can drop this if condition completely and just
keep the following if (net_protocol == ETH_P_8021Q) condition.

Will fix this in v2...

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: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support

[kernel test robot]

Hi Paul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Paul-Barker/net-ravb-Factor-out-checksum-offload-enable-bits/20241001-001351
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240930160845.8520-12-paul%40pbarker.dev
patch subject: [net-next PATCH 11/11] net: ravb: Add VLAN checksum support
config: arc-randconfig-r123-20241001 (https://download.01.org/0day-ci/archive/20241002/202410020057.Z9KJHqVt-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20241002/202410020057.Z9KJHqVt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410020057.Z9KJHqVt-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/renesas/ravb_main.c:2076:17: sparse: sparse: restricted __be16 degrades to integer
   drivers/net/ethernet/renesas/ravb_main.c: note: in included file (through include/linux/mutex.h, include/linux/notifier.h, include/linux/clk.h):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +2076 drivers/net/ethernet/renesas/ravb_main.c

  2063	
  2064	static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
  2065	{
  2066		u16 net_protocol = ntohs(skb->protocol);
  2067	
  2068		/* GbEth IP can calculate the checksum if:
  2069		 * - there are zero or one VLAN headers with TPID=0x8100
  2070		 * - the network protocol is IPv4 or IPv6
  2071		 * - the transport protocol is TCP, UDP or ICMP
  2072		 * - the packet is not fragmented
  2073		 */
  2074	
  2075		if (skb_vlan_tag_present(skb) &&
> 2076		    (skb->vlan_proto != ETH_P_8021Q || net_protocol == ETH_P_8021Q))
  2077			return false;
  2078	
  2079		if (net_protocol == ETH_P_8021Q) {
  2080			struct vlan_hdr vhdr, *vh;
  2081	
  2082			vh = skb_header_pointer(skb, ETH_HLEN, sizeof(vhdr), &vhdr);
  2083			if (!vh)
  2084				return false;
  2085	
  2086			net_protocol = ntohs(vh->h_vlan_encapsulated_proto);
  2087		}
  2088	
  2089		switch (net_protocol) {
  2090		case ETH_P_IP:
  2091			switch (ip_hdr(skb)->protocol) {
  2092			case IPPROTO_TCP:
  2093			case IPPROTO_UDP:
  2094			case IPPROTO_ICMP:
  2095				return true;
  2096			}
  2097			break;
  2098	
  2099		case ETH_P_IPV6:
  2100			switch (ipv6_hdr(skb)->nexthdr) {
  2101			case IPPROTO_TCP:
  2102			case IPPROTO_UDP:
  2103			case IPPROTO_ICMPV6:
  2104				return true;
  2105			}
  2106		}
  2107	
  2108		return false;
  2109	}
  2110	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki