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

[PATCH net-next v2 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.

The TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
for both IPV4 and IPV6.

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

If frame does not have error "0000" attached to checksum calculation
result. For unsupported frames "ffff" is attached to checksum calculation
result. Cases like IPV6, IPV4 header is always set to "FFFF".

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 UDP
frame of IPv4 and its checksum value in the UDP header field is H’0000,
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}

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 CPU performance is not good 
 [1] https://lore.kernel.org/all/20240117190545.596057-1-vincent.guittot@linaro.org/

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

 drivers/net/ethernet/renesas/ravb.h      |  35 ++++++
 drivers/net/ethernet/renesas/ravb_main.c | 137 ++++++++++++++++++++++-
 2 files changed, 170 insertions(+), 2 deletions(-)

-- 
2.25.1

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

[Biju Das]

TOE has hw 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 result of checksum calculation is attached to last 4byte
of ethernet frames. First 2bytes is result of IPV4 header checksum
and next 2 bytes is TCP/UDP/ICMP.

If frame does not have error "0000" attached to checksum calculation
result. For unsupported frames "ffff" is attached to checksum calculation
result. Cases like IPV6, IPV4 header is always set to "FFFF".

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>
---
v1->v2:
 * Fixed sparse warning by replacing __sum16->__wsum.
---
 drivers/net/ethernet/renesas/ravb.h      | 19 ++++++
 drivers/net/ethernet/renesas/ravb_main.c | 81 +++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..a2c494a85d12 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -44,6 +44,9 @@
 #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
 #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping */
 
+/* GbEthernet TOE hardware checksum values */
+#define TOE_RX_CSUM_OK	0x0000
+
 enum ravb_reg {
 	/* AVB-DMAC registers */
 	CCC	= 0x0000,
@@ -206,6 +209,7 @@ enum ravb_reg {
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
 
@@ -978,6 +982,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..59c7bedacef6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 	return -ENOMEM;
 }
 
+static void ravb_csum_offload_init_gbeth(struct net_device *ndev)
+{
+	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
+	u32 csr0;
+
+	if (!rx_enable)
+		return;
+
+	csr0 = ravb_read(ndev, CSR0);
+	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) {
+		netdev_err(ndev, "Timeout Enabling HW CSUM failed\n");
+		ndev->features &= ~NETIF_F_RXCSUM;
+	} else {
+		ravb_write(ndev, CSR2_ALL, CSR2);
+	}
+
+	ravb_write(ndev, csr0, CSR0);
+}
+
 static void ravb_emac_init_gbeth(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -543,6 +563,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
 			 ECMR_TE | ECMR_RE | ECMR_RCPT |
 			 ECMR_TXF | ECMR_RXF, ECMR);
 
+	ravb_csum_offload_init_gbeth(ndev);
 	ravb_set_rate_gbeth(ndev);
 
 	/* Set MAC address */
@@ -734,6 +755,32 @@ 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 == TOE_RX_CSUM_OK &&
+	    csum_proto == TOE_RX_CSUM_OK)
+		/* Hardware validated our checksum */
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -819,6 +866,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 +895,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++;
@@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
 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;
+	u32 csr0;
+	int ret;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	csr0 = ravb_read(ndev, CSR0);
+	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (ret)
+		goto err_wait;
+
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			ravb_write(ndev, CSR2_ALL, CSR2);
+		else
+			ravb_write(ndev, 0, CSR2);
+	}
+
+	ndev->features = features;
+err_wait:
+	ravb_write(ndev, csr0, CSR0);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return ret;
 }
 
 static int ravb_set_features_rcar(struct net_device *ndev,
@@ -2518,6 +2593,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 net-next v2 2/2] ravb: Add Tx checksum offload support

[Biju Das]

TOE has hw 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 UDP
frame of IPv4 and its checksum value in the UDP header field is H’0000,
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>
---
v1->v2:
 * No change.
---
 drivers/net/ethernet/renesas/ravb.h      | 16 ++++++
 drivers/net/ethernet/renesas/ravb_main.c | 66 ++++++++++++++++++++++--
 2 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a2c494a85d12..3cf869fb9a68 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -209,6 +209,7 @@ enum ravb_reg {
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR1    = 0x0804,	/* RZ/G2L only */
 	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
@@ -982,6 +983,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 59c7bedacef6..3c748a54fae0 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,19 +525,29 @@ static int ravb_ring_init(struct net_device *ndev, int q)
 
 static void ravb_csum_offload_init_gbeth(struct net_device *ndev)
 {
+	bool tx_enable = ndev->features & NETIF_F_HW_CSUM;
 	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
 	u32 csr0;
 
-	if (!rx_enable)
+	if (!(tx_enable || rx_enable))
 		return;
 
 	csr0 = ravb_read(ndev, CSR0);
 	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
 	if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) {
 		netdev_err(ndev, "Timeout Enabling HW CSUM failed\n");
-		ndev->features &= ~NETIF_F_RXCSUM;
+
+		if (tx_enable)
+			ndev->features &= ~NETIF_F_HW_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);
 	}
 
 	ravb_write(ndev, csr0, CSR0);
@@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct work_struct *work)
 	rtnl_unlock();
 }
 
+static bool ravb_is_tx_checksum_offload_gbeth_possible(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 HW checksum for IPV6 */
+	if (skb->protocol != htons(ETH_P_IP))
+		return false;
+
+	switch (ip->protocol) {
+	case IPPROTO_TCP:
+		break;
+	case IPPROTO_UDP:
+		/* If the checksum value in the UDP header field is “H’0000”,
+		 * 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)
 {
@@ -2005,6 +2049,11 @@ 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) {
+		if (!ravb_is_tx_checksum_offload_gbeth_possible(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) {
@@ -2408,6 +2457,13 @@ static int ravb_set_features_gbeth(struct net_device *ndev,
 			ravb_write(ndev, 0, CSR2);
 	}
 
+	if (changed & NETIF_F_HW_CSUM) {
+		if (features & NETIF_F_HW_CSUM)
+			ravb_write(ndev, CSR1_ALL, CSR1);
+		else
+			ravb_write(ndev, 0, CSR1);
+	}
+
 	ndev->features = features;
 err_wait:
 	ravb_write(ndev, csr0, CSR0);
@@ -2593,8 +2649,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_HW_CSUM,
+	.net_features = NETIF_F_RXCSUM | NETIF_F_HW_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 net-next v2 0/2] Add HW checksum offload support for RZ/G2L GbEthernet IP

[Sergey Shtylyov]

Hello!

On 1/24/24 1:21 PM, Biju Das wrote:

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

   Your previous try was back in 2021, still the cover letter has the same
issues (hm, I didn't point out those back then).

> The TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
> for both IPV4 and IPV6.
> 
> For Rx, the result of checksum calculation is attached to last 4byte
> of ethernet frames.

   "For Rx, the 4-byte result of checksum calculation is attached to the
Ethernet frames", you wanted to say?

> First 2bytes is result of IPV4 header checksum
> and next 2 bytes is TCP/UDP/ICMP.

   TCP/UDP/ICMP checksum, you mean?

> If frame does not have error "0000" attached to checksum calculation

   "If a frame does not have error, 0x0000 is attached as a checksum
calculation result", you wanted to say?

> result. For unsupported frames "ffff" is attached to checksum calculation

   s/to/as/, again?

> result. Cases like IPV6, IPV4 header is always set to "FFFF".

   In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF",
you wanted to say?

> 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 UDP
> frame of IPv4 and its checksum value in the UDP header field is H’0000,

   I think you can call it just UDPv4...

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

   Too many figures, I think... :-)
   How RZ/G2L SoC is different from RZ/G2LC?

> 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 CPU performance is not good

   Without CPU? I guess the performance would be 0. Seriously, this is
hardly parseable... :-)
 
>  [1] https://lore.kernel.org/all/20240117190545.596057-1-vincent.guittot@linaro.org/
> 
> Biju Das (2):
>   ravb: Add Rx checksum offload support
>   ravb: Add Tx checksum offload support

   These summaries sound like you're adding checksum offload support for
all supported SoCs while you only do that for those having GbEther...

[...]

MBR, Sergey

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

[Sergey Shtylyov]

Hello!

On 1/24/24 1:21 PM, Biju Das wrote:

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

   Your previous try was back in 2021, still the cover letter has the same
issues (hm, I didn't point out those back then).

> The TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
> for both IPV4 and IPV6.
> 
> For Rx, the result of checksum calculation is attached to last 4byte
> of ethernet frames.

   "For Rx, the 4-byte result of checksum calculation is attached to the
Ethernet frames", you wanted to say?

> First 2bytes is result of IPV4 header checksum
> and next 2 bytes is TCP/UDP/ICMP.

   TCP/UDP/ICMP checksum, you mean?

> If frame does not have error "0000" attached to checksum calculation

   "If a frame does not have checksum error, 0x0000 is attached as
a checksum calculation result", you wanted to say?

> result. For unsupported frames "ffff" is attached to checksum calculation

   s/to/as/, again?

> result. Cases like IPV6, IPV4 header is always set to "FFFF".

   In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF",
you wanted to say?

> 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 UDP
> frame of IPv4 and its checksum value in the UDP header field is H’0000,

   I think you can call it just UDPv4...

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

   Too many figures, I think... :-)
   How RZ/G2L SoC is different from RZ/G2LC?

> 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 CPU performance is not good

   Without CPU? I guess the performance would be 0. Seriously, this is
hardly parseable... :-)
 
>  [1] https://lore.kernel.org/all/20240117190545.596057-1-vincent.guittot@linaro.org/
> 
> Biju Das (2):
>   ravb: Add Rx checksum offload support
>   ravb: Add Tx checksum offload support

   These summaries sound like you're adding checksum offload support for
all supported SoCs while you only do that for those having GbEther...

[...]

MBR, Sergey

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

[Sergey Shtylyov]

On 1/24/24 1:21 PM, Biju Das wrote:

> TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum

   s/hw/hardware/.

> for both IPV4 and IPV6.

   Those are usually called IPv4 and IPv6, no?

> Add Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols.
> 
> For Rx, the result of checksum calculation is attached to last 4byte
> of ethernet frames.

   "For Rx, the 4-byte result of checksum calculation is attached to the
Ethernet frames", you wanted to say?

> First 2bytes is result of IPV4 header checksum
> and next 2 bytes is TCP/UDP/ICMP.

   TCP/UDP/ICMP checksum, you mean? Also, you alternatively say
TCP/UDP/ICMP and just TCP/UDP -- which one is correct?

> If frame does not have error "0000" attached to checksum calculation

   "If a frame does not have checksum error, 0x0000 is attached as
a checksum calculation result", you wanted to say?

> result. For unsupported frames "ffff" is attached to checksum calculation

   s/to/as/?

> result. Cases like IPV6, IPV4 header is always set to "FFFF".

   "In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF",
you wanted to say?

> 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..a2c494a85d12 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -44,6 +44,9 @@
>  #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
>  #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping */
>  
> +/* GbEthernet TOE hardware checksum values */
> +#define TOE_RX_CSUM_OK	0x0000

   As I said before, this is hardly needed...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0e3731f50fc2..59c7bedacef6 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>  	return -ENOMEM;
>  }
>  
> +static void ravb_csum_offload_init_gbeth(struct net_device *ndev)

   I'd leave out _offload...

> +{
> +	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
> +	u32 csr0;
> +
> +	if (!rx_enable)
> +		return;
> +
> +	csr0 = ravb_read(ndev, CSR0);

   Why read it here, if we'll write a constant to this reg at the end
of ravb_emac_init_gbeth()?

> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);

   We can just write 0 here, no?

> +	if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) {
> +		netdev_err(ndev, "Timeout Enabling HW CSUM failed\n");

   "Timeout enabling hardware checksum\n", perhaps?

[...]
> +
> +	ravb_write(ndev, csr0, CSR0);

    I think we should move:

	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);

from ravb_emac_init_gbeth() here...

> +}
> +
[...]
> @@ -734,6 +755,32 @@ 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 == TOE_RX_CSUM_OK &&
> +	    csum_proto == TOE_RX_CSUM_OK)
> +		/* Hardware validated our checksum */
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;

   Don't we need to set skb->csum_level?

[...]
> @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
>  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;
> +	u32 csr0;
> +	int ret;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	csr0 = ravb_read(ndev, CSR0);
> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
> +	ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
> +	if (ret)
> +		goto err_wait;

   I don't understand: why do you clear the CSR0 bits even if
(changed & NETIF_F_RXCSUM) is 0? This looks very wrong...

> +
> +	if (changed & NETIF_F_RXCSUM) {
> +		if (features & NETIF_F_RXCSUM)
> +			ravb_write(ndev, CSR2_ALL, CSR2);
> +		else
> +			ravb_write(ndev, 0, CSR2);
> +	}

   I think you should put that into a separate function, like
is done for the EhterAVB...

[...]
> @@ -2518,6 +2593,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,

   What about NETIF_F_IP_CSUM, BTW?

[...]

MBR, Sergey

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

[Biju Das]

Hello Sergey,

Thanks for the feedback.

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Thursday, January 25, 2024 7:11 PM
> Subject: Re: [PATCH net-next v2 0/2] Add HW checksum offload support for
> RZ/G2L GbEthernet IP
> 
> Hello!
> 
> On 1/24/24 1:21 PM, Biju Das wrote:
> 
> > This patch series aims to add HW checksum offload supported by TOE
> > module found on the RZ/G2L Gb ethernet IP.
> 
>    Your previous try was back in 2021, still the cover letter has the same
> issues (hm, I didn't point out those back then).

Thanks for correcting my bad English.

> 
> > The TOE has hw support for calculating IP header and TCP/UDP/ICMP
> > checksum for both IPV4 and IPV6.
> >
> > For Rx, the result of checksum calculation is attached to last 4byte
> > of ethernet frames.
> 
>    "For Rx, the 4-byte result of checksum calculation is attached to the
> Ethernet frames", you wanted to say?

Ok.

> 
> > First 2bytes is result of IPV4 header checksum and next 2 bytes is
> > TCP/UDP/ICMP.
> 
>    TCP/UDP/ICMP checksum, you mean?

Yes.

> 
> > If frame does not have error "0000" attached to checksum calculation
> 
>    "If a frame does not have error, 0x0000 is attached as a checksum
> calculation result", you wanted to say?
> 
> > result. For unsupported frames "ffff" is attached to checksum
> > calculation
> 
>    s/to/as/, again?

OK.

> 
> > result. Cases like IPV6, IPV4 header is always set to "FFFF".
> 
>    In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF", you
> wanted to say?
> 
> > 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 UDP frame of IPv4 and its checksum value in the UDP header
> > field is H'0000,
> 
>    I think you can call it just UDPv4...

OK.

> 
> > 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}
> 
>    Too many figures, I think... :-)
>    How RZ/G2L SoC is different from RZ/G2LC?

Just want to share with the wider community how the HW checksum is
improving the performance of various SoCs in the RZ/G2L family.

and the results show improved performance on all 3 SoCs.
> 
> > 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 CPU performance is not
> > good
> 
>    Without CPU? I guess the performance would be 0. Seriously, this is
> hardly parseable... :-)

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/

Cheers,
Biju

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

[Biju Das]

Hi Sergey Shtylyov,

Thanks for the feedback.

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Thursday, January 25, 2024 8:42 PM
> Subject: Re: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support
> 
> On 1/24/24 1:21 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
> 
>    s/hw/hardware/.

Ok.

> 
> > for both IPV4 and IPV6.
> 
>    Those are usually called IPv4 and IPv6, no?
Agreed.
> 
> > Add Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols.
> >
> > For Rx, the result of checksum calculation is attached to last 4byte
> > of ethernet frames.
> 
>    "For Rx, the 4-byte result of checksum calculation is attached to the
> Ethernet frames", you wanted to say?

Yes.

> 
> > First 2bytes is result of IPV4 header checksum and next 2 bytes is
> > TCP/UDP/ICMP.
> 
>    TCP/UDP/ICMP checksum, you mean? Also, you alternatively say
> TCP/UDP/ICMP and just TCP/UDP -- which one is correct?


As per the hardware manual, it supports TCP/UDP/ICMP checksum.
So you are correct, it is TCP/UDP/ICMP checksum.

> 
> > If frame does not have error "0000" attached to checksum calculation
> 
>    "If a frame does not have checksum error, 0x0000 is attached as a
> checksum calculation result", you wanted to say?

Ok.

> 
> > result. For unsupported frames "ffff" is attached to checksum
> > calculation
> 
>    s/to/as/?

Correct.

> 
> > result. Cases like IPV6, IPV4 header is always set to "FFFF".
> 
>    "In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF", you
> wanted to say?

Correct.

> 
> > 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..a2c494a85d12 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -44,6 +44,9 @@
> >  #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
> >  #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping
> */
> >
> > +/* GbEthernet TOE hardware checksum values */
> > +#define TOE_RX_CSUM_OK	0x0000
> 
>    As I said before, this is hardly needed...

It is needed to match with the Checksum status as mentioned in the hardware manual.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 0e3731f50fc2..59c7bedacef6 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev,
> int q)
> >  	return -ENOMEM;
> >  }
> >
> > +static void ravb_csum_offload_init_gbeth(struct net_device *ndev)
> 
>    I'd leave out _offload...

Ok.

> 
> > +{
> > +	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
> > +	u32 csr0;
> > +
> > +	if (!rx_enable)
> > +		return;
> > +
> > +	csr0 = ravb_read(ndev, CSR0);
> 
>    Why read it here, if we'll write a constant to this reg at the end of
> ravb_emac_init_gbeth()?

The correct flow is

Disable tx/rx
Enable Checksum
Reenable Tx/rx if it is already enabled.

> 
> > +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
> 
>    We can just write 0 here, no?

See above.

> 
> > +	if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) {
> > +		netdev_err(ndev, "Timeout Enabling HW CSUM failed\n");
> 
>    "Timeout enabling hardware checksum\n", perhaps?

OK.

> 
> [...]
> > +
> > +	ravb_write(ndev, csr0, CSR0);
> 
>     I think we should move:
> 
> 	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> 
> from ravb_emac_init_gbeth() here...

I am providing flexible options here.

> 
> > +}
> > +
> [...]
> > @@ -734,6 +755,32 @@ 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 ==
> TOE_RX_CSUM_OK &&
> > +	    csum_proto == TOE_RX_CSUM_OK)
> > +		/* Hardware validated our checksum */
> > +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> 
>    Don't we need to set skb->csum_level?

As per my knowledge, it is not needed. I may be wrong. Why do you
think it is needed?

> 
> [...]
> > @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device
> > *ndev, bool enable)  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;
> > +	u32 csr0;
> > +	int ret;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	csr0 = ravb_read(ndev, CSR0);
> > +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
> > +	ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
> > +	if (ret)
> > +		goto err_wait;
> 
>    I don't understand: why do you clear the CSR0 bits even if (changed &
> NETIF_F_RXCSUM) is 0? This looks very wrong...

I made the code simple. Can you please suggest a much simpler way than this?

> 
> > +
> > +	if (changed & NETIF_F_RXCSUM) {
> > +		if (features & NETIF_F_RXCSUM)
> > +			ravb_write(ndev, CSR2_ALL, CSR2);
> > +		else
> > +			ravb_write(ndev, 0, CSR2);
> > +	}
> 
>    I think you should put that into a separate function, like is done for
> the EhterAVB...

you mean add this if else block to separate function?? Can you please elaborate??


> 
> [...]
> > @@ -2518,6 +2593,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,
> 
>    What about NETIF_F_IP_CSUM, BTW?


Why is it needed? Can you please clarify?

Cheers,
Biju

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

[Sergey Shtylyov]

On 1/26/24 1:08 AM, Biju Das wrote:
[...]

>> -----Original Message-----
>> From: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Sent: Thursday, January 25, 2024 7:11 PM
>> Subject: Re: [PATCH net-next v2 0/2] Add HW checksum offload support for
>> RZ/G2L GbEthernet IP
>>
>> Hello!
>>
>> On 1/24/24 1:21 PM, Biju Das wrote:
>>
>>> This patch series aims to add HW checksum offload supported by TOE
>>> module found on the RZ/G2L Gb ethernet IP.
>>
>>    Your previous try was back in 2021, still the cover letter has the same
>> issues (hm, I didn't point out those back then).
> 
> Thanks for correcting my bad English.

   I don't think you were the author of the e.g. RZ/G2L User's Manual that
has the same wording... Or were you? :-)

[...]

>>> 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}
>>
>>    Too many figures, I think... :-)
>>    How RZ/G2L SoC is different from RZ/G2LC?

   At least they are described by a single manual...

> Just want to share with the wider community how the HW checksum is
> improving the performance of various SoCs in the RZ/G2L family.
> 
> and the results show improved performance on all 3 SoCs.

   I guess RZ/V2L and RZ/G2L would've been enough... but I'm probably
quibbling... :-)

>>> 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 CPU performance is not
>>> good
>>
>>    Without CPU? I guess the performance would be 0. Seriously, this is
>> hardly parseable... :-)
> 
> 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/

   Thanks, that's much better. :-)

> Cheers,
> Biju

MBR, Sergey

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

[Sergey Shtylyov]

On 1/24/24 1:21 PM, Biju Das wrote:

> TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum for

   s/hw/hardware/, please...

> 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 UDP
> frame of IPv4 and its checksum value in the UDP header field is H’0000,
> 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 59c7bedacef6..3c748a54fae0 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>

   What do you need from that header, BTW?

[...]
> @@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	rtnl_unlock();
>  }
>  
> +static bool ravb_is_tx_checksum_offload_gbeth_possible(struct sk_buff *skb)

   I'd suggest s/th shorter and more consistent with the used naming,
like 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;
> +
> +	/* TODO: Need to add HW checksum for IPV6 */
> +	if (skb->protocol != htons(ETH_P_IP))
> +		return false;

   So maybe we need to report just NETIF_F_IP_CSUM, not NETIF_F_HW_CSUM
ATM?

> +
> +	switch (ip->protocol) {
> +	case IPPROTO_TCP:
> +		break;
> +	case IPPROTO_UDP:
> +		/* If the checksum value in the UDP header field is “H’0000”,

   Use 0x0000 or just 0, please. I don't know where Renesas found this
weird hex notation...

[...]
> @@ -2005,6 +2049,11 @@ 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) {
> +		if (!ravb_is_tx_checksum_offload_gbeth_possible(skb))

   I'd collapse those 2 *if* statements...

[...]

MBR, Sergey

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

[Biju Das]

Hi Sergey Shtylyov,

Thanks for the feedback.

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Friday, January 26, 2024 9:00 PM
> Subject: Re: [PATCH net-next v2 2/2] ravb: Add Tx checksum offload support
> 
> On 1/24/24 1:21 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
> > for
> 
>    s/hw/hardware/, please...

Agreed.

> 
> > 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 UDP frame of IPv4 and its checksum value in the UDP header
> > field is H’0000, 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 59c7bedacef6..3c748a54fae0 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>
> 
>    What do you need from that header, BTW?

It is giving compilation error for ip_hdr() and udp_hdr().

> 
> [...]
> > @@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct
> work_struct *work)
> >  	rtnl_unlock();
> >  }
> >
> > +static bool ravb_is_tx_checksum_offload_gbeth_possible(struct sk_buff
> > +*skb)
> 
>    I'd suggest s/th shorter and more consistent with the used naming, like
> ravb_tx_csum_possible_gbeth()...

OK.

> 
> > +{
> > +	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 HW checksum for IPV6 */
> > +	if (skb->protocol != htons(ETH_P_IP))
> > +		return false;
> 
>    So maybe we need to report just NETIF_F_IP_CSUM, not NETIF_F_HW_CSUM
> ATM?

I agree, at the moment we are supporting only IPv4 Checksum offload.
Later when we can reintroduce NETIF_F_HW_CSUM when we add support for IPv6.

> 
> > +
> > +	switch (ip->protocol) {
> > +	case IPPROTO_TCP:
> > +		break;
> > +	case IPPROTO_UDP:
> > +		/* If the checksum value in the UDP header field is “H’0000”,
> 
>    Use 0x0000 or just 0, please. I don't know where Renesas found this
> weird hex notation...

OK.

> 
> [...]
> > @@ -2005,6 +2049,11 @@ 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) {
> > +		if (!ravb_is_tx_checksum_offload_gbeth_possible(skb))
> 
>    I'd collapse those 2 *if* statements...

Agreed

Cheers,
Biju

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

[Sergey Shtylyov]

On 1/26/24 1:15 AM, Biju Das wrote:

> Hi Sergey Shtylyov,

   Hi! :-)

> Thanks for the feedback.

   Not at all!

>> -----Original Message-----
>> From: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Sent: Thursday, January 25, 2024 8:42 PM
>> Subject: Re: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support

[...]

>>> 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..a2c494a85d12 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -44,6 +44,9 @@
>>>  #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
>>>  #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping
>> */
>>>
>>> +/* GbEthernet TOE hardware checksum values */
>>> +#define TOE_RX_CSUM_OK	0x0000
>>
>>    As I said before, this is hardly needed...
> 
> It is needed to match with the Checksum status as mentioned in the hardware manual.

   I think you can just compare to 0... ISTR that checksumming result
should indeed be 0 for a good IP header, so this # does not seem device
specific...

>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 0e3731f50fc2..59c7bedacef6 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> +{
>>> +	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
>>> +	u32 csr0;
>>> +
>>> +	if (!rx_enable)
>>> +		return;
>>> +
>>> +	csr0 = ravb_read(ndev, CSR0);
>>
>>    Why read it here, if we'll write a constant to this reg at the end of
>> ravb_emac_init_gbeth()?
> 
> The correct flow is
> 
> Disable tx/rx
> Enable Checksum
> Reenable Tx/rx if it is already enabled.

   Yeah, I figured. :-) However I can't find this flow in the RZ/G2L[C]
user's manual...

>>> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
>>
>>    We can just write 0 here, no?
> 
> See above.

   I have to repeat: either don't do read/modife/write CSR0 accesses here
or remove the below line from ravb_emac_init_gbeth():

	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);

   Well, I think this line should be removed in any case -- we shouldn't
enable RX/TX checksumming in CSR0 if we don't also set up CSR1/2...

[...]
>>> +
>>> +	ravb_write(ndev, csr0, CSR0);
>>
>>     I think we should move:
>>
>> 	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
>>
>> from ravb_emac_init_gbeth() here...
> 
> I am providing flexible options here.

   I don't get it... what flexibility do you mean?

[...]
>>> @@ -734,6 +755,32 @@ 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 ==
>> TOE_RX_CSUM_OK &&
>>> +	    csum_proto == TOE_RX_CSUM_OK)
>>> +		/* Hardware validated our checksum */
>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>>    Don't we need to set skb->csum_level?
> 
> As per my knowledge, it is not needed. I may be wrong. Why do you
> think it is needed?

 *   CHECKSUM_UNNECESSARY is applicable to following protocols:
 *     TCP: IPv6 and IPv4.
 *     UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a
 *       zero UDP checksum for either IPv4 or IPv6, the networking stack
 *       may perform further validation in this case.
 *     GRE: only if the checksum is present in the header.
 *     SCTP: indicates the CRC in SCTP header has been validated.
 *     FCOE: indicates the CRC in FC frame has been validated.
 *
 *   skb->csum_level indicates the number of consecutive checksums found in
 *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.
 *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
 *   and a device is able to verify the checksums for UDP (possibly zero),
 *   GRE (checksum flag is set) and TCP, skb->csum_level would be set to
 *   two. If the device were only able to verify the UDP checksum and not
 *   GRE, either because it doesn't support GRE checksum or because GRE
 *   checksum is bad, skb->csum_level would be set to zero (TCP checksum is
 *   not considered in this case).

   It would seem we should set this field to 1 if the TCP/UDP checksum
was successfully verified?

>> [...]
>>> @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device
>>> *ndev, bool enable)  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;
>>> +	u32 csr0;
>>> +	int ret;
>>> +
>>> +	spin_lock_irqsave(&priv->lock, flags);
>>> +	csr0 = ravb_read(ndev, CSR0);
>>> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
>>> +	ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
>>> +	if (ret)
>>> +		goto err_wait;
>>
>>    I don't understand: why do you clear the CSR0 bits even if (changed &
>> NETIF_F_RXCSUM) is 0? This looks very wrong...
> 
> I made the code simple. Can you please suggest a much simpler way than this?

   Of course, I can. I don't think clearing CSR0.TPE makes sense if you
only modify CSR2, and clearing CSR0.RPE makes sense if you only modify CSR1...

>>> +
>>> +	if (changed & NETIF_F_RXCSUM) {
>>> +		if (features & NETIF_F_RXCSUM)
>>> +			ravb_write(ndev, CSR2_ALL, CSR2);
>>> +		else
>>> +			ravb_write(ndev, 0, CSR2);
>>> +	}
>>
>>    I think you should put that into a separate function, like is done for
>> the EhterAVB...
> 
> you mean add this if else block to separate function?? Can you please elaborate??

   No, you need to 1st clear CSR0.{RPE|TPE}, then set up CSR1/2, then restore
CSR0... something like that.

>> [...]
>>> @@ -2518,6 +2593,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,
>>
>>    What about NETIF_F_IP_CSUM, BTW?
> 
> Why is it needed? Can you please clarify?

   Ignore me -- this one seems to be used for the TX path.
   I just had to learn how the checksum offloading works while reviewing
your patches... :-)

> Cheers,
> Biju

MBR, Sergey

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

[Biju Das]

Hi Sergey Shtylyov,

Thanks for the feedback.

> >>    Don't we need to set skb->csum_level?
> >
> > As per my knowledge, it is not needed. I may be wrong. Why do you
> > think it is needed?
> 
>  *   CHECKSUM_UNNECESSARY is applicable to following protocols:
>  *     TCP: IPv6 and IPv4.
>  *     UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a
>  *       zero UDP checksum for either IPv4 or IPv6, the networking stack
>  *       may perform further validation in this case.
>  *     GRE: only if the checksum is present in the header.
>  *     SCTP: indicates the CRC in SCTP header has been validated.
>  *     FCOE: indicates the CRC in FC frame has been validated.
>  *
>  *   skb->csum_level indicates the number of consecutive checksums found
> in
>  *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.
>  *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
>  *   and a device is able to verify the checksums for UDP (possibly zero),
>  *   GRE (checksum flag is set) and TCP, skb->csum_level would be set to
>  *   two. If the device were only able to verify the UDP checksum and not
>  *   GRE, either because it doesn't support GRE checksum or because GRE
>  *   checksum is bad, skb->csum_level would be set to zero (TCP checksum
> is
>  *   not considered in this case).
> 
>    It would seem we should set this field to 1 if the TCP/UDP checksum was
> successfully verified?

I guess it is for encapsulated packets. For just IP and UDP/TCP
Checksum it is not required.

See

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/3com/3c59x.c#L2669
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/amazon/ena/ena_netdev.c#L1626
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/alx/main.c#L272
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/am65-cpsw-nuss.c#L711

> 
> >> [...]
> >>> @@ -2518,6 +2593,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,
> >>
> >>    What about NETIF_F_IP_CSUM, BTW?
> >
> > Why is it needed? Can you please clarify?
> 
>    Ignore me -- this one seems to be used for the TX path.
>    I just had to learn how the checksum offloading works while reviewing
> your patches... :-)

OK.

For other comments. I will test and respond.

Cheers,
Biju