diff mbox series

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

Message ID 20240203142559.130466-3-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add HW checksum offload support for RZ/G2L GbEthernet IP | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1048 this patch: 1048
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1065 this patch: 1065
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1065 this patch: 1065
netdev/checkpatch warning WARNING: line length of 87 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-04--21-00 (tests: 721)

Commit Message

Biju Das Feb. 3, 2024, 2:25 p.m. UTC
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>
---
v3->v4:
 * Restored NETIF_F_HW_CSUM and associated changes.
 * Dropped enabling IPv6 specific bits in CSR1.
 * Dropped enabling ICMPv4 specific bit and associated handling as linux
   does not support it.
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 | 73 +++++++++++++++++++++---
 2 files changed, 82 insertions(+), 7 deletions(-)

Comments

Sergey Shtylyov Feb. 5, 2024, 8:27 p.m. UTC | #1
On 2/3/24 5:25 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>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 64bf29d01ad0..d7b1c6d15a17 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
[...]
> @@ -981,6 +982,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,

   I doubt we really need CSR1_ALL...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4f310bcee7c0..fee771f14fc5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -524,16 +525,29 @@ 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_HW_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_HW_CSUM;
> +
> +		if (rx_enable)
> +			ndev->features &= ~NETIF_F_RXCSUM;
>  	} else {
> -		ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
> -					      CSR2_RICMP6), CSR2);
> +		if (tx_enable)
> +			ravb_write(ndev, CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> +						      CSR1_TUDP6 | CSR1_TICMP6), CSR1);

   With the v6 bits 20...22 being 0, the bits 24...27 are ignored anyway,
the manual says. So I think I'd prefer:

			ravb_write(ndev, CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4, CSR1);
[...]
> @@ -2418,6 +2465,18 @@ static int ravb_set_features_gbeth(struct net_device *ndev,
>  			goto done;
>  	}
>  
> +	if (changed & NETIF_F_HW_CSUM) {
> +		if (features & NETIF_F_HW_CSUM)
> +			val = CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> +					   CSR1_TUDP6 | CSR1_TICMP6);

   Likewise, I'd prefer:

			val = CSR1_TIP4 | CSR1_TTCP4 | CS12_TUDP4;

[...]

MBR, Sergey
Biju Das Feb. 6, 2024, 5:47 p.m. UTC | #2
Hi Sergey,

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@omp.ru>
> Sent: Monday, February 5, 2024 8:27 PM
> Subject: Re: [PATCH v4 net-next 2/2] ravb: Add Tx checksum offload support
> for GbEth
> 
> On 2/3/24 5:25 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>
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 64bf29d01ad0..d7b1c6d15a17 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> [...]
> > @@ -981,6 +982,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,
> 
>    I doubt we really need CSR1_ALL...
OK, will drop it.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 4f310bcee7c0..fee771f14fc5 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> > @@ -524,16 +525,29 @@ 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_HW_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_HW_CSUM;
> > +
> > +		if (rx_enable)
> > +			ndev->features &= ~NETIF_F_RXCSUM;
> >  	} else {
> > -		ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
> > -					      CSR2_RICMP6), CSR2);
> > +		if (tx_enable)
> > +			ravb_write(ndev, CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> > +						      CSR1_TUDP6 | CSR1_TICMP6),
> CSR1);
> 
>    With the v6 bits 20...22 being 0, the bits 24...27 are ignored anyway,
> the manual says. So I think I'd prefer:
> 
> 			ravb_write(ndev, CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4,
> CSR1); [...]

OK.

> > @@ -2418,6 +2465,18 @@ static int ravb_set_features_gbeth(struct
> net_device *ndev,
> >  			goto done;
> >  	}
> >
> > +	if (changed & NETIF_F_HW_CSUM) {
> > +		if (features & NETIF_F_HW_CSUM)
> > +			val = CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
> > +					   CSR1_TUDP6 | CSR1_TICMP6);
> 
>    Likewise, I'd prefer:
> 
> 			val = CSR1_TIP4 | CSR1_TTCP4 | CS12_TUDP4;

OK, val = CSR1_TIP4 | CSR1_TTCP4 | CSR1_TUDP4;

Cheers,
Biju

> 
> [...]
> 
> MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 64bf29d01ad0..d7b1c6d15a17 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -208,6 +208,7 @@  enum ravb_reg {
 
 	/* RZ/G2L TOE registers */
 	CSR0    = 0x0800,
+	CSR1    = 0x0804,
 	CSR2    = 0x0808,
 };
 
@@ -981,6 +982,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 4f310bcee7c0..fee771f14fc5 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,16 +525,29 @@  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_HW_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_HW_CSUM;
+
+		if (rx_enable)
+			ndev->features &= ~NETIF_F_RXCSUM;
 	} else {
-		ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
-					      CSR2_RICMP6), CSR2);
+		if (tx_enable)
+			ravb_write(ndev, CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
+						      CSR1_TUDP6 | CSR1_TICMP6), CSR1);
+
+		if (rx_enable)
+			ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
+						      CSR2_RICMP6), CSR2);
 	}
 
 done:
@@ -1986,6 +2000,36 @@  static void ravb_tx_timeout_work(struct work_struct *work)
 	rtnl_unlock();
 }
 
+static bool ravb_can_tx_csum_gbeth(struct sk_buff *skb)
+{
+	struct iphdr *ip = ip_hdr(skb);
+
+	/* TODO: Need to add support for VLAN tag 802.1Q */
+	if (skb_vlan_tag_present(skb))
+		return false;
+
+	/* TODO: Need to add hardware checksum for IPv6 */
+	if (skb->protocol != htons(ETH_P_IP))
+		return false;
+
+	switch (ip->protocol) {
+	case IPPROTO_TCP:
+		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;
+	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 +2045,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_can_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) {
@@ -2418,6 +2465,18 @@  static int ravb_set_features_gbeth(struct net_device *ndev,
 			goto done;
 	}
 
+	if (changed & NETIF_F_HW_CSUM) {
+		if (features & NETIF_F_HW_CSUM)
+			val = CSR1_ALL & ~(CSR1_TICMP4 | CSR1_TTCP6 |
+					   CSR1_TUDP6 | CSR1_TICMP6);
+		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);
@@ -2602,8 +2661,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,