[PATCH v3 net-next 0/2] Add HW checksum offload support for RZ/G2L GbEthernet IP

[PATCH v3 net-next 0/2] Add HW checksum offload support for RZ/G2L GbEthernet IP

[Biju Das]

This patch series aims to add HW checksum offload supported by TOE module
found on the RZ/G2L Gb ethernet IP.

TOE has hardware support for calculating IP header and TCP/UDP/ICMP
checksum for both IPv4 and IPv6.

For Rx, the 4-byte result of checksum calculation is attached to the
Ethernet frames.First 2-bytes is result of IPv4 header checksum and next
2-bytes is TCP/UDP/ICMP checksum.

If a frame does not have checksum error, 0x0000 is attached as checksum
calculation result. For unsupported frames 0xFFFF is attached as checksum
calculation result. In case of an IPv6 packet, IPv4 checksum is always set
to 0xFFFF.

For Tx, the result of checksum calculation is set to the checksum field of
each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the unsupported
frames, those fields are not changed. If a transmission frame is an UDPv4
frame and its checksum value in the UDP header field is 0x0000, TOE does
not calculate checksum for UDP part of this frame as it is optional
function as per standards.

Add Tx/Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.

Results of iperf3 in Mbps

RZ/V2L:
TCP(Tx/Rx) results with checksum offload Enabled:	{921,932}
TCP(Tx/Rx) results with checksum offload Disabled:	{867,612}

UDP(Tx/Rx) results with checksum offload Enabled:	{950,946}
UDP(Tx/Rx) results with checksum offload Disabled:	{952,920}

RZ/G2L:
TCP(Tx/Rx) results with checksum offload Enabled:	{920,936}
TCP(Tx/Rx) results with checksum offload Disabled:	{871,626}

UDP(Tx/Rx) results with checksum offload Enabled:	{953,950}
UDP(Tx/Rx) results with checksum offload Disabled:	{954,920}

RZ/G2LC:
TCP(Tx/Rx) results with checksum offload Enabled:	{927,936}
TCP(Tx/Rx) results with checksum offload Disabled:	{889,626}

UDP(Tx/Rx) results with checksum offload Enabled:	{950,946}
UDP(Tx/Rx) results with checksum offload Disabled:	{949,944}

v2->v3:
 * Updated commit header and description as suggested by Sergey for cover letter,
 * patch#1 and patch#2.
 * Dropped TOE_RX_CSUM_OK macro.
 * Renamed ravb_csum_offload_init_gbeth()->ravb_csum_init_gbeth().
 * Moved enabling {RPE,TPE} in ravb_csum_init_gbeth().
 * Updated the error message in ravb_csum_init_gbeth() as
   "Timeout enabling hardware checksum"
 * Introduced ravb_endisable_csum_gbeth() for enabling/disabling CSR{1,2} registers.
 * Replaced NETIF_F_IP_CSUM->NETIF_F_HW_CSUM as we are supporting only IPv4.
 * Updated the comment related to UDP header field.
 * Renamed ravb_is_tx_checksum_offload_gbeth_possible()->ravb_is_tx_csum_gbeth().
v1->v2:
 * Updated covering letter and results
 * Fixed the sparse warnings for patch#1 by replacing __sum16->__wsum.

Note:
 This patches are tested with [1]. Without the patch [1] CPU performance is
 not good which impacts the network throughput.
 [1] https://lore.kernel.org/all/20240117190545.596057-1-vincent.guittot@linaro.org/

Biju Das (2):
  ravb: Add Rx checksum offload support for GbEth
  ravb: Add Tx checksum offload support for GbEth

 drivers/net/ethernet/renesas/ravb.h      |  32 +++++
 drivers/net/ethernet/renesas/ravb_main.c | 149 ++++++++++++++++++++++-
 2 files changed, 178 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for GbEth

[Biju Das]

TOE has hardware support for calculating IP header and TCP/UDP/ICMP
checksum for both IPv4 and IPv6.

Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.

For Rx, the 4-byte result of checksum calculation is attached to the
Ethernet frames.First 2-bytes is result of IPv4 header checksum and next
2-bytes is TCP/UDP/ICMP checksum.

If a frame does not have checksum error, 0x0000 is attached as checksum
calculation result. For unsupported frames 0xFFFF is attached as checksum
calculation result. In case of an IPv6 packet, IPv4 checksum is always set
to 0xFFFF.

We can test this functionality by the below commands

ethtool -K eth0 rx on --> to turn on Rx checksum offload
ethtool -K eth0 rx off --> to turn off Rx checksum offload

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Updated commit header and description suggested by Sergey.
 * Dropped TOE_RX_CSUM_OK macro.
 * Renamed ravb_csum_offload_init_gbeth()->ravb_csum_init_gbeth().
 * Moved enabling {RPE,TPE} in ravb_csum_init_gbeth().
 * Updated the error message in ravb_csum_init_gbeth() as
   "Timeout enabling hardware checksum"
 * Introduced ravb_endisable_csum_gbeth() for enabling/disabling CSR{1,2} registers.
   
v1->v2:
 * Fixed sparse warning by replacing __sum16->__wsum.
---
 drivers/net/ethernet/renesas/ravb.h      | 16 ++++
 drivers/net/ethernet/renesas/ravb_main.c | 93 +++++++++++++++++++++++-
 2 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..a7fe9d8b6b41 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -206,6 +206,7 @@ enum ravb_reg {
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
 
@@ -978,6 +979,21 @@ enum CSR0_BIT {
 	CSR0_RPE	= 0x00000020,
 };
 
+enum CSR2_BIT {
+	CSR2_RIP4	= 0x00000001,
+	CSR2_RTCP4	= 0x00000010,
+	CSR2_RUDP4	= 0x00000020,
+	CSR2_RICMP4	= 0x00000040,
+	CSR2_RTCP6	= 0x00100000,
+	CSR2_RUDP6	= 0x00200000,
+	CSR2_RICMP6	= 0x00400000,
+	CSR2_RHOP	= 0x01000000,
+	CSR2_RROUT	= 0x02000000,
+	CSR2_RAHD	= 0x04000000,
+	CSR2_RDHD	= 0x08000000,
+	CSR2_ALL	= 0x0F700071,
+};
+
 #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 0e3731f50fc2..c4dc6ec54287 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	return -ENOMEM;
 }
 
+static void ravb_csum_init_gbeth(struct net_device *ndev)
+{
+	if (!(ndev->features & NETIF_F_RXCSUM))
+		goto done;
+
+	ravb_write(ndev, 0, CSR0);
+	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
+		netdev_err(ndev, "Timeout enabling hardware checksum\n");
+		ndev->features &= ~NETIF_F_RXCSUM;
+	} else {
+		ravb_write(ndev, CSR2_ALL, CSR2);
+	}
+
+done:
+	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
+}
+
 static void ravb_emac_init_gbeth(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -553,7 +570,8 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 
 	/* E-MAC status register clear */
 	ravb_write(ndev, ECSR_ICD | ECSR_LCHNG | ECSR_PFRI, ECSR);
-	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
+
+	ravb_csum_init_gbeth(ndev);
 
 	/* E-MAC interrupt enable register */
 	ravb_write(ndev, ECSIPR_ICDIP, ECSIPR);
@@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 	}
 }
 
+static void ravb_rx_csum_gbeth(struct sk_buff *skb)
+{
+	__wsum csum_ip_hdr, csum_proto;
+	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 csum and
+	 * last 2 bytes is protocol csum.
+	 */
+	if (unlikely(skb->len < sizeof(__sum16) * 2))
+		return;
+
+	hw_csum = skb_tail_pointer(skb) - 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));
+	skb_trim(skb, skb->len - 2 * sizeof(__sum16));
+
+	/* TODO: IPV6 Rx csum */
+	if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto)
+		/* Hardware validated our checksum */
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -819,6 +862,8 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 				skb = ravb_get_skb_gbeth(ndev, entry, desc);
 				skb_put(skb, pkt_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);
 				stats->rx_packets++;
 				stats->rx_bytes += pkt_len;
@@ -846,6 +891,8 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 				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(skb);
 				napi_gro_receive(&priv->napi[q],
 						 priv->rx_1st_skb);
 				stats->rx_packets++;
@@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
+static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
+				     u32 val, u32 mask)
+{
+	u32 csr0;
+	int ret;
+
+	csr0 = ravb_read(ndev, CSR0);
+	ravb_write(ndev, csr0 & ~mask, CSR0);
+	ret = ravb_wait(ndev, CSR0, mask, 0);
+	if (!ret)
+		ravb_write(ndev, val, reg);
+
+	ravb_write(ndev, csr0, CSR0);
+
+	return ret;
+}
+
 static int ravb_set_features_gbeth(struct net_device *ndev,
 				   netdev_features_t features)
 {
-	/* Place holder */
-	return 0;
+	netdev_features_t changed = ndev->features ^ features;
+	struct ravb_private *priv = netdev_priv(ndev);
+	unsigned long flags;
+	int ret = 0;
+	u32 val;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			val = CSR2_ALL;
+		else
+			val = 0;
+
+		ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE);
+		if (ret)
+			goto done;
+	}
+
+	ndev->features = features;
+done:
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
 }
 
 static int ravb_set_features_rcar(struct net_device *ndev,
@@ -2518,6 +2603,8 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.emac_init = ravb_emac_init_gbeth,
 	.gstrings_stats = ravb_gstrings_stats_gbeth,
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
+	.net_hw_features = NETIF_F_RXCSUM,
+	.net_features = NETIF_F_RXCSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tccr_mask = TCCR_TSRQ0,
-- 
2.25.1

[PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

[Biju Das]

TOE has hardware support for calculating IP header and TCP/UDP/ICMP
checksum for both IPv4 and IPv6.

Add Tx checksum offload supported by TOE for IPv4 and TCP/UDP.

For Tx, the result of checksum calculation is set to the checksum field of
each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the unsupported
frames, those fields are not changed. If a transmission frame is an UDPv4
frame and its checksum value in the UDP header field is 0x0000, TOE does
not calculate checksum for UDP part of this frame as it is optional
function as per standards.

We can test this functionality by the below commands

ethtool -K eth0 tx on --> to turn on Tx checksum offload
ethtool -K eth0 tx off --> to turn off Tx checksum offload

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * Updated commit header and description as suggested by Sergey.
 * Replaced NETIF_F_IP_CSUM->NETIF_F_HW_CSUM as we are supporting only IPv4.
 * Updated the comment related to UDP header field.
 * Renamed ravb_is_tx_checksum_offload_gbeth_possible()->ravb_is_tx_csum_gbeth().
v1->v2:
 * No change.
---
 drivers/net/ethernet/renesas/ravb.h      | 16 ++++++
 drivers/net/ethernet/renesas/ravb_main.c | 68 +++++++++++++++++++++---
 2 files changed, 78 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a7fe9d8b6b41..53bd1fa51b32 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -206,6 +206,7 @@ enum ravb_reg {
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR1    = 0x0804,	/* RZ/G2L only */
 	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
@@ -979,6 +980,21 @@ enum CSR0_BIT {
 	CSR0_RPE	= 0x00000020,
 };
 
+enum CSR1_BIT {
+	CSR1_TIP4	= 0x00000001,
+	CSR1_TTCP4	= 0x00000010,
+	CSR1_TUDP4	= 0x00000020,
+	CSR1_TICMP4	= 0x00000040,
+	CSR1_TTCP6	= 0x00100000,
+	CSR1_TUDP6	= 0x00200000,
+	CSR1_TICMP6	= 0x00400000,
+	CSR1_THOP	= 0x01000000,
+	CSR1_TROUT	= 0x02000000,
+	CSR1_TAHD	= 0x04000000,
+	CSR1_TDHD	= 0x08000000,
+	CSR1_ALL	= 0x0F700071,
+};
+
 enum CSR2_BIT {
 	CSR2_RIP4	= 0x00000001,
 	CSR2_RTCP4	= 0x00000010,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c4dc6ec54287..042dc565d1a5 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -29,6 +29,7 @@
 #include <linux/spinlock.h>
 #include <linux/reset.h>
 #include <linux/math64.h>
+#include <net/ip.h>
 
 #include "ravb.h"
 
@@ -524,15 +525,27 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 
 static void ravb_csum_init_gbeth(struct net_device *ndev)
 {
-	if (!(ndev->features & NETIF_F_RXCSUM))
+	bool tx_enable = ndev->features & NETIF_F_IP_CSUM;
+	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
+
+	if (!(tx_enable || rx_enable))
 		goto done;
 
 	ravb_write(ndev, 0, CSR0);
-	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
+	if (ravb_wait(ndev, CSR0, CSR0_TPE | CSR0_RPE, 0)) {
 		netdev_err(ndev, "Timeout enabling hardware checksum\n");
-		ndev->features &= ~NETIF_F_RXCSUM;
+
+		if (tx_enable)
+			ndev->features &= ~NETIF_F_IP_CSUM;
+
+		if (rx_enable)
+			ndev->features &= ~NETIF_F_RXCSUM;
 	} else {
-		ravb_write(ndev, CSR2_ALL, CSR2);
+		if (tx_enable)
+			ravb_write(ndev, CSR1_ALL, CSR1);
+
+		if (rx_enable)
+			ravb_write(ndev, CSR2_ALL, CSR2);
 	}
 
 done:
@@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 	rtnl_unlock();
 }
 
+static bool ravb_is_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;
+
+	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;
+	/* TODO: Need to add HW checksum for ICMP */
+	case IPPROTO_ICMP:
+		fallthrough;
+	default:
+		return false;
+	}
+
+	return true;
+}
+
 /* Packet transmit function for Ethernet AVB */
 static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
@@ -2001,6 +2043,9 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u32 entry;
 	u32 len;
 
+	if (skb->ip_summed == CHECKSUM_PARTIAL && !ravb_is_tx_csum_gbeth(skb))
+		skb_checksum_help(skb);
+
 	spin_lock_irqsave(&priv->lock, flags);
 	if (priv->cur_tx[q] - priv->dirty_tx[q] > (priv->num_tx_ring[q] - 1) *
 	    num_tx_desc) {
@@ -2419,6 +2464,17 @@ static int ravb_set_features_gbeth(struct net_device *ndev,
 			goto done;
 	}
 
+	if (changed & NETIF_F_IP_CSUM) {
+		if (features & NETIF_F_IP_CSUM)
+			val = CSR1_ALL;
+		else
+			val = 0;
+
+		ret = ravb_endisable_csum_gbeth(ndev, CSR1, val, CSR0_TPE);
+		if (ret)
+			goto done;
+	}
+
 	ndev->features = features;
 done:
 	spin_unlock_irqrestore(&priv->lock, flags);
@@ -2603,8 +2659,8 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.emac_init = ravb_emac_init_gbeth,
 	.gstrings_stats = ravb_gstrings_stats_gbeth,
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
-	.net_hw_features = NETIF_F_RXCSUM,
-	.net_features = NETIF_F_RXCSUM,
+	.net_hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM,
+	.net_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM,
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
 	.max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
 	.tccr_mask = TCCR_TSRQ0,
-- 
2.25.1

Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

[Sergey Shtylyov]

On 2/1/24 10:45 PM, Biju Das wrote:

> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> checksum for both IPv4 and IPv6.
> 
> Add Tx checksum offload supported by TOE for IPv4 and TCP/UDP.
> 
> For Tx, the result of checksum calculation is set to the checksum field of
> each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the unsupported
> frames, those fields are not changed. If a transmission frame is an UDPv4
> frame and its checksum value in the UDP header field is 0x0000, TOE does
> not calculate checksum for UDP part of this frame as it is optional
> function as per standards.
> 
> We can test this functionality by the below commands
> 
> ethtool -K eth0 tx on --> to turn on Tx checksum offload
> ethtool -K eth0 tx off --> to turn off Tx checksum offload
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * Updated commit header and description as suggested by Sergey.
>  * Replaced NETIF_F_IP_CSUM->NETIF_F_HW_CSUM as we are supporting only IPv4.

   You do vice versa, NETIF_F_HW_CSUM->NETIF_F_IP_CSUM. :-)
   However, I'm now seeing this comment under CHECKSM_PATIAL:

 *   %NETIF_F_IP_CSUM and %NETIF_F_IPV6_CSUM are being deprecated in favor of
 *   %NETIF_F_HW_CSUM. New devices should use %NETIF_F_HW_CSUM to indicate
 *   checksum offload capability.

   So probably we should've kept NETIF_F_HW_CSUM? :-/
 
>  * Updated the comment related to UDP header field.
>  * Renamed ravb_is_tx_checksum_offload_gbeth_possible()->ravb_is_tx_csum_gbeth().
> v1->v2:
>  * No change.
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c4dc6ec54287..042dc565d1a5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -524,15 +525,27 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  
>  static void ravb_csum_init_gbeth(struct net_device *ndev)
>  {
> -	if (!(ndev->features & NETIF_F_RXCSUM))
> +	bool tx_enable = ndev->features & NETIF_F_IP_CSUM;
> +	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
> +
> +	if (!(tx_enable || rx_enable))
>  		goto done;
>  
>  	ravb_write(ndev, 0, CSR0);
> -	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> +	if (ravb_wait(ndev, CSR0, CSR0_TPE | CSR0_RPE, 0)) {
>  		netdev_err(ndev, "Timeout enabling hardware checksum\n");
> -		ndev->features &= ~NETIF_F_RXCSUM;
> +
> +		if (tx_enable)
> +			ndev->features &= ~NETIF_F_IP_CSUM;
> +
> +		if (rx_enable)
> +			ndev->features &= ~NETIF_F_RXCSUM;
>  	} else {
> -		ravb_write(ndev, CSR2_ALL, CSR2);
> +		if (tx_enable)
> +			ravb_write(ndev, CSR1_ALL, CSR1);
> +
> +		if (rx_enable)
> +			ravb_write(ndev, CSR2_ALL, CSR2);
>  	}
>  
>  done:
> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	rtnl_unlock();
>  }
>  
> +static bool ravb_is_tx_csum_gbeth(struct sk_buff *skb)

   Hm, this new name doesn't parse well for me... :-(
   Maybe ravb_can_tx_csum_gbeth() or ravb_tx_csum_possible_gbeth()?

> +{
> +	struct iphdr *ip = ip_hdr(skb);
> +
> +	/* TODO: Need to add support for VLAN tag 802.1Q */
> +	if (skb_vlan_tag_present(skb))
> +		return false;
> +
> +	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;
> +	/* TODO: Need to add HW checksum for ICMP */

   s/HW/hardware/?

> +	case IPPROTO_ICMP:
> +		fallthrough;

   You don't even need fallthrough, actually...
   But why do you return false for ICMP? Isn't it supported by TOE?

> +	default:
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>  /* Packet transmit function for Ethernet AVB */
>  static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  {
[...]

MBR, Sergey

Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

[Biju Das]

Hi Sergey,

On Thu, Feb 1, 2024 at 8:56 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
>
> On 2/1/24 10:45 PM, Biju Das wrote:
>
> > TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> > checksum for both IPv4 and IPv6.
> >
> > Add Tx checksum offload supported by TOE for IPv4 and TCP/UDP.
> >
> > For Tx, the result of checksum calculation is set to the checksum field of
> > each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the unsupported
> > frames, those fields are not changed. If a transmission frame is an UDPv4
> > frame and its checksum value in the UDP header field is 0x0000, TOE does
> > not calculate checksum for UDP part of this frame as it is optional
> > function as per standards.
> >
> > We can test this functionality by the below commands
> >
> > ethtool -K eth0 tx on --> to turn on Tx checksum offload
> > ethtool -K eth0 tx off --> to turn off Tx checksum offload
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2->v3:
> >  * Updated commit header and description as suggested by Sergey.
> >  * Replaced NETIF_F_IP_CSUM->NETIF_F_HW_CSUM as we are supporting only IPv4.
>
>    You do vice versa, NETIF_F_HW_CSUM->NETIF_F_IP_CSUM. :-)
>    However, I'm now seeing this comment under CHECKSM_PATIAL:
>
>  *   %NETIF_F_IP_CSUM and %NETIF_F_IPV6_CSUM are being deprecated in favor of
>  *   %NETIF_F_HW_CSUM. New devices should use %NETIF_F_HW_CSUM to indicate
>  *   checksum offload capability.
>
>    So probably we should've kept NETIF_F_HW_CSUM? :-/

Ok, Will do in the next version.
>
> >  * Updated the comment related to UDP header field.
> >  * Renamed ravb_is_tx_checksum_offload_gbeth_possible()->ravb_is_tx_csum_gbeth().
> > v1->v2:
> >  * No change.
> [...]
>
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index c4dc6ec54287..042dc565d1a5 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -524,15 +525,27 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> >
> >  static void ravb_csum_init_gbeth(struct net_device *ndev)
> >  {
> > -     if (!(ndev->features & NETIF_F_RXCSUM))
> > +     bool tx_enable = ndev->features & NETIF_F_IP_CSUM;
> > +     bool rx_enable = ndev->features & NETIF_F_RXCSUM;
> > +
> > +     if (!(tx_enable || rx_enable))
> >               goto done;
> >
> >       ravb_write(ndev, 0, CSR0);
> > -     if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> > +     if (ravb_wait(ndev, CSR0, CSR0_TPE | CSR0_RPE, 0)) {
> >               netdev_err(ndev, "Timeout enabling hardware checksum\n");
> > -             ndev->features &= ~NETIF_F_RXCSUM;
> > +
> > +             if (tx_enable)
> > +                     ndev->features &= ~NETIF_F_IP_CSUM;
> > +
> > +             if (rx_enable)
> > +                     ndev->features &= ~NETIF_F_RXCSUM;
> >       } else {
> > -             ravb_write(ndev, CSR2_ALL, CSR2);
> > +             if (tx_enable)
> > +                     ravb_write(ndev, CSR1_ALL, CSR1);
> > +
> > +             if (rx_enable)
> > +                     ravb_write(ndev, CSR2_ALL, CSR2);
> >       }
> >
> >  done:
> > @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work)
> >       rtnl_unlock();
> >  }
> >
> > +static bool ravb_is_tx_csum_gbeth(struct sk_buff *skb)
>
>    Hm, this new name doesn't parse well for me... :-(
>    Maybe ravb_can_tx_csum_gbeth() or ravb_tx_csum_possible_gbeth()?

OK, ravb_can_tx_csum_gbeth() as it is shorter.

>
> > +{
> > +     struct iphdr *ip = ip_hdr(skb);
> > +
> > +     /* TODO: Need to add support for VLAN tag 802.1Q */
> > +     if (skb_vlan_tag_present(skb))
> > +             return false;
> > +
> > +     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;
> > +     /* TODO: Need to add HW checksum for ICMP */
>
>    s/HW/hardware/?

OK.
>
> > +     case IPPROTO_ICMP:
> > +             fallthrough;
>
>    You don't even need fallthrough, actually...

Clang Compiler will complain.

[1] https://patchwork.kernel.org/project/xfs/patch/20210420230652.GA70650@embeddedor/#24129659

https://patches.linaro.org/project/netdev/patch/20210305094850.GA141221@embeddedor/#617482

>    But why do you return false for ICMP? Isn't it supported by TOE?

It is supported by the hardware, but not the network subsystem.

Cheers,
Biju

Re: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for GbEth

[Sergey Shtylyov]

On 2/1/24 10:45 PM, Biju Das wrote:

> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> checksum for both IPv4 and IPv6.
> 
> Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.
> 
> For Rx, the 4-byte result of checksum calculation is attached to the
> Ethernet frames.First 2-bytes is result of IPv4 header checksum and next
> 2-bytes is TCP/UDP/ICMP checksum.
> 
> If a frame does not have checksum error, 0x0000 is attached as checksum
> calculation result. For unsupported frames 0xFFFF is attached as checksum
> calculation result. In case of an IPv6 packet, IPv4 checksum is always set
> to 0xFFFF.
> 
> We can test this functionality by the below commands
> 
> ethtool -K eth0 rx on --> to turn on Rx checksum offload
> ethtool -K eth0 rx off --> to turn off Rx checksum offload
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..a7fe9d8b6b41 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -206,6 +206,7 @@ enum ravb_reg {
>  	RFCR	= 0x0760,
>  	MAFCR	= 0x0778,

   Would be good to add this comment here:

	/* TOE registers */

>  	CSR0    = 0x0800,	/* RZ/G2L only */
> +	CSR2    = 0x0808,	/* RZ/G2L only */
>  };
>  
>  
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0e3731f50fc2..c4dc6ec54287 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  	return -ENOMEM;
>  }
>  
> +static void ravb_csum_init_gbeth(struct net_device *ndev)
> +{
> +	if (!(ndev->features & NETIF_F_RXCSUM))
> +		goto done;
> +
> +	ravb_write(ndev, 0, CSR0);
> +	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> +		netdev_err(ndev, "Timeout enabling hardware checksum\n");
> +		ndev->features &= ~NETIF_F_RXCSUM;
> +	} else {
> +		ravb_write(ndev, CSR2_ALL, CSR2);

   Does it make sense to set the IPv6 specific bits if we don't yet
support IPv6 checksumming anyways?

> +	}
> +
> +done:
> +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> +}
> +
>  static void ravb_emac_init_gbeth(struct net_device *ndev)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
[...]
> @@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  	}
>  }
>  
> +static void ravb_rx_csum_gbeth(struct sk_buff *skb)
> +{
> +	__wsum csum_ip_hdr, csum_proto;
> +	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 csum and
> +	 * last 2 bytes is protocol csum.

   Hm, maybe spell out csum as checksum?

> +	 */
> +	if (unlikely(skb->len < sizeof(__sum16) * 2))
> +		return;
> +
> +	hw_csum = skb_tail_pointer(skb) - 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));
> +	skb_trim(skb, skb->len - 2 * sizeof(__sum16));
> +
> +	/* TODO: IPV6 Rx csum */
> +	if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto)
> +		/* Hardware validated our checksum */
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;

   I think you need {} because of the comment (but would be good without it
as well)...

[...]
> @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  }
>  
> +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
> +				     u32 val, u32 mask)

   I'd suggest to mimic ravb_wait() with the the mask param followed by
the val[ue] param...

> +{
> +	u32 csr0;
> +	int ret;
> +
> +	csr0 = ravb_read(ndev, CSR0);

   Hm... I think we already know CSR0's value as ravb_csum_init_gbeth()
sets it to (CSR0_TPE | CSR0_RPE) on exit...

> +	ravb_write(ndev, csr0 & ~mask, CSR0);
> +	ret = ravb_wait(ndev, CSR0, mask, 0);
> +	if (!ret)
> +		ravb_write(ndev, val, reg);
> +
> +	ravb_write(ndev, csr0, CSR0);
> +
> +	return ret;
> +}
> +
>  static int ravb_set_features_gbeth(struct net_device *ndev,
>  				   netdev_features_t features)
>  {
> -	/* Place holder */
> -	return 0;
> +	netdev_features_t changed = ndev->features ^ features;
> +	struct ravb_private *priv = netdev_priv(ndev);
> +	unsigned long flags;
> +	int ret = 0;
> +	u32 val;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (changed & NETIF_F_RXCSUM) {
> +		if (features & NETIF_F_RXCSUM)
> +			val = CSR2_ALL;

   Again, does it make sense to set the IPv6 bits in CSR2?

> +		else
> +			val = 0;
> +
> +		ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE);
> +		if (ret)
> +			goto done;
> +	}
> +
> +	ndev->features = features;
> +done:
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return ret;
>  }
>  
>  static int ravb_set_features_rcar(struct net_device *ndev,
[...]

MBR, Sergey

Re: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for GbEth

[Sergey Shtylyov]

On 2/2/24 10:12 PM, Sergey Shtylyov wrote:

>> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
>> checksum for both IPv4 and IPv6.
>>
>> Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.
>>
>> For Rx, the 4-byte result of checksum calculation is attached to the
>> Ethernet frames.First 2-bytes is result of IPv4 header checksum and next
>> 2-bytes is TCP/UDP/ICMP checksum.
>>
>> If a frame does not have checksum error, 0x0000 is attached as checksum
>> calculation result. For unsupported frames 0xFFFF is attached as checksum
>> calculation result. In case of an IPv6 packet, IPv4 checksum is always set
>> to 0xFFFF.
>>
>> We can test this functionality by the below commands
>>
>> ethtool -K eth0 rx on --> to turn on Rx checksum offload
>> ethtool -K eth0 rx off --> to turn off Rx checksum offload
>>
>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
[...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 0e3731f50fc2..c4dc6ec54287 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>> @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>>  	spin_unlock_irqrestore(&priv->lock, flags);
>>  }
>>  
>> +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum ravb_reg reg,
>> +				     u32 val, u32 mask)
> 
>    I'd suggest to mimic ravb_wait() with the the mask param followed by
> the val[ue] param...

   Nevermind, I see now they are for different registers...

[...]

MBR, Sergey

Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

[Sergey Shtylyov]

On 2/2/24 12:13 AM, Biju Das wrote:
[...]

>>> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
>>> checksum for both IPv4 and IPv6.
>>>
>>> Add Tx checksum offload supported by TOE for IPv4 and TCP/UDP.
>>>
>>> For Tx, the result of checksum calculation is set to the checksum field of
>>> each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the unsupported
>>> frames, those fields are not changed. If a transmission frame is an UDPv4
>>> frame and its checksum value in the UDP header field is 0x0000, TOE does
>>> not calculate checksum for UDP part of this frame as it is optional
>>> function as per standards.
>>>
>>> We can test this functionality by the below commands
>>>
>>> ethtool -K eth0 tx on --> to turn on Tx checksum offload
>>> ethtool -K eth0 tx off --> to turn off Tx checksum offload
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
[...]

>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index c4dc6ec54287..042dc565d1a5 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work)
[...]
>>> +     case IPPROTO_ICMP:
>>> +             fallthrough;
>>
>>    You don't even need fallthrough, actually...
> 
> Clang Compiler will complain.
> 
> [1] https://patchwork.kernel.org/project/xfs/patch/20210420230652.GA70650@embeddedor/#24129659
> 
> https://patches.linaro.org/project/netdev/patch/20210305094850.GA141221@embeddedor/#617482

   That's not like our case. If clang can't compile:

	case IPPROTO_ICMP:
	default:
		return false;

it's seriously broken!

>>    But why do you return false for ICMP? Isn't it supported by TOE?
> 
> It is supported by the hardware, but not the network subsystem.

   Then I don't think we should set CSR1.TICMP{4|6}, so TOE wouldn't
try to replace the checksum in the ICMP packets...

> Cheers,
> Biju

MBR, Sergey

RE: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for GbEth

[Biju Das]

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Friday, February 2, 2024 7:12 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski
> <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Cc: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>; Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; nikita.yoush <nikita.yoush@cogentembedded.com>;
> netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Geert
> Uytterhoeven <geert+renesas@glider.be>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; biju.das.au
> <biju.das.au@gmail.com>
> Subject: Re: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support
> for GbEth
> 
> On 2/1/24 10:45 PM, Biju Das wrote:
> 
> > TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> > checksum for both IPv4 and IPv6.
> >
> > Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.
> >
> > For Rx, the 4-byte result of checksum calculation is attached to the
> > Ethernet frames.First 2-bytes is result of IPv4 header checksum and
> > next 2-bytes is TCP/UDP/ICMP checksum.
> >
> > If a frame does not have checksum error, 0x0000 is attached as
> > checksum calculation result. For unsupported frames 0xFFFF is attached
> > as checksum calculation result. In case of an IPv6 packet, IPv4
> > checksum is always set to 0xFFFF.
> >
> > We can test this functionality by the below commands
> >
> > ethtool -K eth0 rx on --> to turn on Rx checksum offload ethtool -K
> > eth0 rx off --> to turn off Rx checksum offload
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index e0f8276cffed..a7fe9d8b6b41 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -206,6 +206,7 @@ enum ravb_reg {
> >  	RFCR	= 0x0760,
> >  	MAFCR	= 0x0778,
> 
>    Would be good to add this comment here:
> 
> 	/* TOE registers */

OK will change as below, so that we don't need to repeat /* RZ/G2L only */
+
+	/* RZ/G2L TOE registers */
+	CSR0    = 0x0800,
+	CSR2    = 0x0808,

> 
> >  	CSR0    = 0x0800,	/* RZ/G2L only */
> > +	CSR2    = 0x0808,	/* RZ/G2L only */
> >  };
> >
> >
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 0e3731f50fc2..c4dc6ec54287 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev,
> int q)
> >  	return -ENOMEM;
> >  }
> >
> > +static void ravb_csum_init_gbeth(struct net_device *ndev) {
> > +	if (!(ndev->features & NETIF_F_RXCSUM))
> > +		goto done;
> > +
> > +	ravb_write(ndev, 0, CSR0);
> > +	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> > +		netdev_err(ndev, "Timeout enabling hardware checksum\n");
> > +		ndev->features &= ~NETIF_F_RXCSUM;
> > +	} else {
> > +		ravb_write(ndev, CSR2_ALL, CSR2);
> 
>    Does it make sense to set the IPv6 specific bits if we don't yet
> support IPv6 checksumming anyways?

OK will drop IPv6 bits now and will add when we enable IPv6.

> 
> > +	}
> > +
> > +done:
> > +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); }
> > +
> >  static void ravb_emac_init_gbeth(struct net_device *ndev)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> [...]
> > @@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > +	__wsum csum_ip_hdr, csum_proto;
> > +	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 csum
> and
> > +	 * last 2 bytes is protocol csum.
> 
>    Hm, maybe spell out csum as checksum?

OK.

> 
> > +	 */
> > +	if (unlikely(skb->len < sizeof(__sum16) * 2))
> > +		return;
> > +
> > +	hw_csum = skb_tail_pointer(skb) - 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));
> > +	skb_trim(skb, skb->len - 2 * sizeof(__sum16));
> > +
> > +	/* TODO: IPV6 Rx csum */
> > +	if (skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr && !csum_proto)
> > +		/* Hardware validated our checksum */
> > +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> 
>    I think you need {} because of the comment (but would be good without
> it as well)...

OK, I will drop the unnecessary comment "Hardware validated our checksum"
to avoid confusion related to multi-line comment.

> 
> [...]
> > @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device
> *ndev, bool enable)
> >  	spin_unlock_irqrestore(&priv->lock, flags);  }
> >
> > +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum
> ravb_reg reg,
> > +				     u32 val, u32 mask)
> 
>    I'd suggest to mimic ravb_wait() with the the mask param followed by
> the val[ue] param...
> 
> > +{
> > +	u32 csr0;
> > +	int ret;
> > +
> > +	csr0 = ravb_read(ndev, CSR0);
> 
>    Hm... I think we already know CSR0's value as ravb_csum_init_gbeth()
> sets it to (CSR0_TPE | CSR0_RPE) on exit...

OK will drop register read.

> 
> > +	ravb_write(ndev, csr0 & ~mask, CSR0);
> > +	ret = ravb_wait(ndev, CSR0, mask, 0);
> > +	if (!ret)
> > +		ravb_write(ndev, val, reg);
> > +
> > +	ravb_write(ndev, csr0, CSR0);
> > +
> > +	return ret;
> > +}
> > +
> >  static int ravb_set_features_gbeth(struct net_device *ndev,
> >  				   netdev_features_t features)
> >  {
> > -	/* Place holder */
> > -	return 0;
> > +	netdev_features_t changed = ndev->features ^ features;
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +	unsigned long flags;
> > +	int ret = 0;
> > +	u32 val;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	if (changed & NETIF_F_RXCSUM) {
> > +		if (features & NETIF_F_RXCSUM)
> > +			val = CSR2_ALL;
> 
>    Again, does it make sense to set the IPv6 bits in CSR2?

OK will drop it.

I am agreeing to all the comments and will send v4.

Cheers,
Biju

> 
> > +		else
> > +			val = 0;
> > +
> > +		ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE);
> > +		if (ret)
> > +			goto done;
> > +	}
> > +
> > +	ndev->features = features;
> > +done:
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return ret;
> >  }
> >
> >  static int ravb_set_features_rcar(struct net_device *ndev,
> [...]
> 
> MBR, Sergey

RE: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

[Biju Das]

Hi Sergey,

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Friday, February 2, 2024 8:00 PM
> Subject: Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support
> for GbEth
> 
> On 2/2/24 12:13 AM, Biju Das wrote:
> [...]
> 
> >>> TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> >>> checksum for both IPv4 and IPv6.
> >>>
> >>> Add Tx checksum offload supported by TOE for IPv4 and TCP/UDP.
> >>>
> >>> For Tx, the result of checksum calculation is set to the checksum
> >>> field of each IPv4 Header/TCP/UDP/ICMP of ethernet frames. For the
> >>> unsupported frames, those fields are not changed. If a transmission
> >>> frame is an UDPv4 frame and its checksum value in the UDP header
> >>> field is 0x0000, TOE does not calculate checksum for UDP part of
> >>> this frame as it is optional function as per standards.
> >>>
> >>> We can test this functionality by the below commands
> >>>
> >>> ethtool -K eth0 tx on --> to turn on Tx checksum offload ethtool -K
> >>> eth0 tx off --> to turn off Tx checksum offload
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> [...]
> 
> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>> index c4dc6ec54287..042dc565d1a5 100644
> >>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> >>> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct
> >>> work_struct *work)
> [...]
> >>> +     case IPPROTO_ICMP:
> >>> +             fallthrough;
> >>
> >>    You don't even need fallthrough, actually...
> >
> > Clang Compiler will complain.
> >
> 
>    That's not like our case. If clang can't compile:
> 
> 	case IPPROTO_ICMP:
> 	default:
> 		return false;
> 
> it's seriously broken!
> 
> >>    But why do you return false for ICMP? Isn't it supported by TOE?
> >
> > It is supported by the hardware, but not the network subsystem.
> 
>    Then I don't think we should set CSR1.TICMP{4|6}, so TOE wouldn't try
> to replace the checksum in the ICMP packets...

OK will drop ICMP and send v4.

Cheers,
Biju