All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC 0/2] Add Rx checksum offload support
@ 2021-11-23 13:31 Biju Das
  2021-11-23 13:31 ` [RFC 1/2] ravb: Fillup ravb_set_features_gbeth() stub Biju Das
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Biju Das @ 2021-11-23 13:31 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Biju Das, Sergey Shtylyov, Lad Prabhakar, Yoshihiro Shimoda,
	netdev, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson,
	Biju Das

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

This patch series aims to adds Rx checksum offload supported by TOE.

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

Biju Das (2):
  ravb: Fillup ravb_set_features_gbeth() stub
  ravb: Add Rx checksum offload support

 drivers/net/ethernet/renesas/ravb.h      | 20 +++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 55 +++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 1 deletion(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [RFC 1/2] ravb: Fillup ravb_set_features_gbeth() stub
  2021-11-23 13:31 [RFC 0/2] Add Rx checksum offload support Biju Das
@ 2021-11-23 13:31 ` Biju Das
  2021-11-23 13:31 ` [RFC 2/2] ravb: Add Rx checksum offload support Biju Das
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2021-11-23 13:31 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Biju Das, Sergey Shtylyov, Lad Prabhakar, Yoshihiro Shimoda,
	netdev, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson,
	Biju Das

Fillup ravb_set_features_gbeth() function to support RZ/G2L.
Also set the net_hw_features bits with rx checksum offload
supported by TOE.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 16 ++++++++++++++++
 drivers/net/ethernet/renesas/ravb_main.c | 24 +++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 08062d73df10..a96552348e2d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -205,6 +205,7 @@ enum ravb_reg {
 	RFCR	= 0x0760,
 	MAFCR	= 0x0778,
 	CSR0    = 0x0800,	/* RZ/G2L only */
+	CSR2    = 0x0808,	/* RZ/G2L only */
 };
 
 
@@ -970,6 +971,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 ce09bd45527e..c2b92c6a6cd2 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2314,7 +2314,28 @@ 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 */
+	netdev_features_t changed = ndev->features ^ features;
+	u32 csr0 = ravb_read(ndev, CSR0);
+	int error;
+
+	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
+	error = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
+	if (error) {
+		ravb_write(ndev, csr0, CSR0);
+		return error;
+	}
+
+	if (changed & NETIF_F_RXCSUM) {
+		if (features & NETIF_F_RXCSUM)
+			ravb_write(ndev, CSR2_ALL, CSR2);
+		else
+			ravb_write(ndev, 0, CSR2);
+	}
+
+	ravb_write(ndev, csr0, CSR0);
+
+	ndev->features = features;
+
 	return 0;
 }
 
@@ -2457,6 +2478,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.set_feature = ravb_set_features_gbeth,
 	.dmac_init = ravb_dmac_init_gbeth,
 	.emac_init = ravb_emac_init_gbeth,
+	.net_hw_features = NETIF_F_RXCSUM,
 	.gstrings_stats = ravb_gstrings_stats_gbeth,
 	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
 	.stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [RFC 2/2] ravb: Add Rx checksum offload support
  2021-11-23 13:31 [RFC 0/2] Add Rx checksum offload support Biju Das
  2021-11-23 13:31 ` [RFC 1/2] ravb: Fillup ravb_set_features_gbeth() stub Biju Das
@ 2021-11-23 13:31 ` Biju Das
  2021-11-24 20:10   ` Sergey Shtylyov
  2021-11-23 15:09 ` [RFC 0/2] " Sergey Shtylyov
  2021-11-24 20:16 ` Sergey Shtylyov
  3 siblings, 1 reply; 10+ messages in thread
From: Biju Das @ 2021-11-23 13:31 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Biju Das, Sergey Shtylyov, Lad Prabhakar, Yoshihiro Shimoda,
	netdev, linux-renesas-soc, Geert Uytterhoeven, Chris Paterson,
	Biju Das

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

This patch adds Rx checksum offload supported by TOE.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  4 +++
 drivers/net/ethernet/renesas/ravb_main.c | 31 ++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index a96552348e2d..d0e5eec0636e 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -44,6 +44,10 @@
 #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
+#define TOE_RX_CSUM_UNSUPPORTED	0xFFFF
+
 enum ravb_reg {
 	/* AVB-DMAC registers */
 	CCC	= 0x0000,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index c2b92c6a6cd2..2533e3401593 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 	}
 }
 
+static void ravb_rx_csum_gbeth(struct sk_buff *skb)
+{
+	u32 csum_ip_hdr, csum_proto;
+	u8 *hw_csum;
+
+	/* The hardware checksum 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 = skb_tail_pointer(skb) - 2 * sizeof(__sum16);
+	csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
+
+	skb->ip_summed = CHECKSUM_NONE;
+	if (csum_proto == TOE_RX_CSUM_OK) {
+		if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK)
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		else if (skb->protocol == htons(ETH_P_IPV6) &&
+			 csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED)
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+	}
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -805,6 +832,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;
@@ -832,6 +861,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++;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [RFC 0/2] Add Rx checksum offload support
  2021-11-23 13:31 [RFC 0/2] Add Rx checksum offload support Biju Das
  2021-11-23 13:31 ` [RFC 1/2] ravb: Fillup ravb_set_features_gbeth() stub Biju Das
  2021-11-23 13:31 ` [RFC 2/2] ravb: Add Rx checksum offload support Biju Das
@ 2021-11-23 15:09 ` Sergey Shtylyov
  2021-11-23 15:40   ` Biju Das
  2021-11-24 20:16 ` Sergey Shtylyov
  3 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2021-11-23 15:09 UTC (permalink / raw)
  To: Biju Das, David S. Miller, Jakub Kicinski
  Cc: Lad Prabhakar, Yoshihiro Shimoda, netdev, linux-renesas-soc,
	Geert Uytterhoeven, Chris Paterson, Biju Das

Hello!

On 23.11.2021 16:31, Biju Das wrote:

> TOE has hw support for calculating IP header checkum for IPV4 and

    hw == hardware? And checksum. :-)

> TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> 
> This patch series aims to adds Rx checksum offload supported by TOE.
> 
> 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".

    You just said IPv4 header checksum is supported?

> 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
> 
> Biju Das (2):
>    ravb: Fillup ravb_set_features_gbeth() stub
>    ravb: Add Rx checksum offload support
> 
>   drivers/net/ethernet/renesas/ravb.h      | 20 +++++++++
>   drivers/net/ethernet/renesas/ravb_main.c | 55 +++++++++++++++++++++++-
>   2 files changed, 74 insertions(+), 1 deletion(-)

    Dave, Jakub, I'll try reviewing these later today.

MBR, Sergey

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [RFC 0/2] Add Rx checksum offload support
  2021-11-23 15:09 ` [RFC 0/2] " Sergey Shtylyov
@ 2021-11-23 15:40   ` Biju Das
  0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2021-11-23 15:40 UTC (permalink / raw)
  To: Sergey Shtylyov, David S. Miller, Jakub Kicinski
  Cc: Prabhakar Mahadev Lad, Yoshihiro Shimoda, netdev,
	linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das

Hi Sergey Shtylyov,

> Subject: Re: [RFC 0/2] Add Rx checksum offload support
> 
> Hello!
> 
> On 23.11.2021 16:31, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header checkum for IPV4 and
> 
>     hw == hardware? And checksum. :-)

Oops typo. My mistake.

> 
> > TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> >
> > This patch series aims to adds Rx checksum offload supported by TOE.
> >
> > 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".
> 
>     You just said IPv4 header checksum is supported?

Yes you are correct. 

for IPV4, IPv4 header checksum is supported. If it is supported case and no error
          the result is set to "0000" by the hardware.

Where as for IPv6, IPV4 header is unsupported case,
        so the result is always set to "ffff" by the hardware

Cheers,
Biju
> 
> > 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
> >
> > Biju Das (2):
> >    ravb: Fillup ravb_set_features_gbeth() stub
> >    ravb: Add Rx checksum offload support
> >
> >   drivers/net/ethernet/renesas/ravb.h      | 20 +++++++++
> >   drivers/net/ethernet/renesas/ravb_main.c | 55 +++++++++++++++++++++++-
> >   2 files changed, 74 insertions(+), 1 deletion(-)
> 
>     Dave, Jakub, I'll try reviewing these later today.
> 
> MBR, Sergey

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [RFC 2/2] ravb: Add Rx checksum offload support
  2021-11-23 13:31 ` [RFC 2/2] ravb: Add Rx checksum offload support Biju Das
@ 2021-11-24 20:10   ` Sergey Shtylyov
  2021-11-25 10:15     ` Biju Das
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2021-11-24 20:10 UTC (permalink / raw)
  To: Biju Das, David S. Miller, Jakub Kicinski
  Cc: Lad Prabhakar, Yoshihiro Shimoda, netdev, linux-renesas-soc,
	Geert Uytterhoeven, Chris Paterson, Biju Das

Hello!

On 11/23/21 4:31 PM, Biju Das wrote:

> TOE has hw support for calculating IP header checkum for IPV4 and
> TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> 
> This patch adds Rx checksum offload supported by TOE.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  4 +++
>  drivers/net/ethernet/renesas/ravb_main.c | 31 ++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index a96552348e2d..d0e5eec0636e 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -44,6 +44,10 @@
>  #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
> +#define TOE_RX_CSUM_UNSUPPORTED	0xFFFF

   These are hardly needed IMO.

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c2b92c6a6cd2..2533e3401593 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  	}
>  }
>  
> +static void ravb_rx_csum_gbeth(struct sk_buff *skb)
> +{
> +	u32 csum_ip_hdr, csum_proto;

   Why u32 if both sums are 16-bit?

> +	u8 *hw_csum;
> +
> +	/* The hardware checksum 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 = skb_tail_pointer(skb) - 2 * sizeof(__sum16);
> +	csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum));
> +
> +	skb->ip_summed = CHECKSUM_NONE;
> +	if (csum_proto == TOE_RX_CSUM_OK) {
> +		if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK)
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> +		else if (skb->protocol == htons(ETH_P_IPV6) &&
> +			 csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED)
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;

   Checksum is unsupported and you declare it unnecessary?

> +	}

   Now where's a call to skb_trim()?

[...]

MBR, Sergey

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [RFC 0/2] Add Rx checksum offload support
  2021-11-23 13:31 [RFC 0/2] Add Rx checksum offload support Biju Das
                   ` (2 preceding siblings ...)
  2021-11-23 15:09 ` [RFC 0/2] " Sergey Shtylyov
@ 2021-11-24 20:16 ` Sergey Shtylyov
  2021-11-25 10:16   ` Biju Das
  3 siblings, 1 reply; 10+ messages in thread
From: Sergey Shtylyov @ 2021-11-24 20:16 UTC (permalink / raw)
  To: Biju Das, David S. Miller, Jakub Kicinski
  Cc: Lad Prabhakar, Yoshihiro Shimoda, netdev, linux-renesas-soc,
	Geert Uytterhoeven, Chris Paterson, Biju Das

On 11/23/21 4:31 PM, Biju Das wrote:

> TOE has hw support for calculating IP header checkum for IPV4 and
> TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> 
> This patch series aims to adds Rx checksum offload supported by TOE.
> 
> 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
> 
> Biju Das (2):
>   ravb: Fillup ravb_set_features_gbeth() stub
>   ravb: Add Rx checksum offload support

   That's all fine but why in the world did you separate these patches?

MBR, Sergey

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [RFC 2/2] ravb: Add Rx checksum offload support
  2021-11-24 20:10   ` Sergey Shtylyov
@ 2021-11-25 10:15     ` Biju Das
  2021-11-26 21:27       ` Sergey Shtylyov
  0 siblings, 1 reply; 10+ messages in thread
From: Biju Das @ 2021-11-25 10:15 UTC (permalink / raw)
  To: Sergey Shtylyov, David S. Miller, Jakub Kicinski
  Cc: Prabhakar Mahadev Lad, Yoshihiro Shimoda, netdev,
	linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das

Hi Sergey Shtylyov,

> Subject: Re: [RFC 2/2] ravb: Add Rx checksum offload support
> 
> Hello!
> 
> On 11/23/21 4:31 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header checkum for IPV4 and
> > TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> >
> > This patch adds Rx checksum offload supported by TOE.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  4 +++
> >  drivers/net/ethernet/renesas/ravb_main.c | 31
> > ++++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index a96552348e2d..d0e5eec0636e 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -44,6 +44,10 @@
> >  #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
> > +#define TOE_RX_CSUM_UNSUPPORTED	0xFFFF
> 
>    These are hardly needed IMO.

OK.

> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index c2b92c6a6cd2..2533e3401593 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > +	u32 csum_ip_hdr, csum_proto;
> 
>    Why u32 if both sums are 16-bit?
OK, will make it 16-bit.

> 
> > +	u8 *hw_csum;
> > +
> > +	/* The hardware checksum 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 = skb_tail_pointer(skb) - 2 * sizeof(__sum16);
> > +	csum_ip_hdr = csum_unfold((__force
> > +__sum16)get_unaligned_le16(hw_csum));
> > +
> > +	skb->ip_summed = CHECKSUM_NONE;
> > +	if (csum_proto == TOE_RX_CSUM_OK) {
> > +		if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr ==
> TOE_RX_CSUM_OK)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +		else if (skb->protocol == htons(ETH_P_IPV6) &&
> > +			 csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> 
>    Checksum is unsupported and you declare it unnecessary?


Do you mean takeout the check for unsupported headercsum for IPV6 and the code like one below?

If(!csum_proto) {
	if ((skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr) || skb->protocol == htons(ETH_P_IPV6))
		skb->ip_summed = CHECKSUM_UNNECESSARY;
}

Snippet from H/W manual for reception handling

(1) Reception Handling
The result of Checksum Calculation is attached to last 4 byte of Ethernet Frames like Figure 30.25. And then the 
handled frames are transferred to memory by DMAC. If the frame does not have checksum error at the part of IPv4 
Header or TCP/UDP/ICMP, the value of “0000h” is attached to each part as the result of Checksum Calculation. The 
case of Unsupported Frame, the value of “FFFFh” is attached. For example, if the part of IP Header is unsupported, 
“FFFFh” is set to both field of IPv4 Header and TCP/UDP/ICMP. The case of IPv6, IPv4 Header field is always set to 
“FFFFh”. 

 
> 
> > +	}
> 
>    Now where's a call to skb_trim()?

Currently I haven't seen any issue without using skb_trim.

OK, as you suggested, will check and add skb_trim to takeout the last 4bytes.

Regards,
Biju

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [RFC 0/2] Add Rx checksum offload support
  2021-11-24 20:16 ` Sergey Shtylyov
@ 2021-11-25 10:16   ` Biju Das
  0 siblings, 0 replies; 10+ messages in thread
From: Biju Das @ 2021-11-25 10:16 UTC (permalink / raw)
  To: Sergey Shtylyov, David S. Miller, Jakub Kicinski
  Cc: Prabhakar Mahadev Lad, Yoshihiro Shimoda, netdev,
	linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das

Hi Sergey Shtylyov,

> Subject: Re: [RFC 0/2] Add Rx checksum offload support
> 
> On 11/23/21 4:31 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header checkum for IPV4 and
> > TCP/UDP/ICMP checksum for both IPV4 and IPV6.
> >
> > This patch series aims to adds Rx checksum offload supported by TOE.
> >
> > 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
> >
> > Biju Das (2):
> >   ravb: Fillup ravb_set_features_gbeth() stub
> >   ravb: Add Rx checksum offload support
> 
>    That's all fine but why in the world did you separate these patches?

OK, as you suggested will merge this patches and send an RFC for further feedback.

Regards,
Biju

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [RFC 2/2] ravb: Add Rx checksum offload support
  2021-11-25 10:15     ` Biju Das
@ 2021-11-26 21:27       ` Sergey Shtylyov
  0 siblings, 0 replies; 10+ messages in thread
From: Sergey Shtylyov @ 2021-11-26 21:27 UTC (permalink / raw)
  To: Biju Das, David S. Miller, Jakub Kicinski
  Cc: Prabhakar Mahadev Lad, Yoshihiro Shimoda, netdev,
	linux-renesas-soc, Geert Uytterhoeven, Chris Paterson, Biju Das

Hello!

On 11/25/21 1:15 PM, Biju Das wrote:

>>> TOE has hw support for calculating IP header checkum for IPV4 and
>>> TCP/UDP/ICMP checksum for both IPV4 and IPV6.
>>>
>>> This patch adds Rx checksum offload supported by TOE.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb.h      |  4 +++
>>>  drivers/net/ethernet/renesas/ravb_main.c | 31
>>> ++++++++++++++++++++++++
>>>  2 files changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>> b/drivers/net/ethernet/renesas/ravb.h
>>> index a96552348e2d..d0e5eec0636e 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index c2b92c6a6cd2..2533e3401593 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device
[...]
>>> +	u8 *hw_csum;
>>> +
>>> +	/* The hardware checksum 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 = skb_tail_pointer(skb) - 2 * sizeof(__sum16);
>>> +	csum_ip_hdr = csum_unfold((__force
>>> +__sum16)get_unaligned_le16(hw_csum));
>>> +
>>> +	skb->ip_summed = CHECKSUM_NONE;
>>> +	if (csum_proto == TOE_RX_CSUM_OK) {
>>> +		if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr ==
>> TOE_RX_CSUM_OK)
>>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +		else if (skb->protocol == htons(ETH_P_IPV6) &&
>>> +			 csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED)
>>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>>    Checksum is unsupported and you declare it unnecessary?
> 
> 
> Do you mean takeout the check for unsupported headercsum for IPV6 and the code like one below?

   No, I meant that the code looks strange. 

> If(!csum_proto) {
> 	if ((skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr) || skb->protocol == htons(ETH_P_IPV6))

   I think this statement doesn't make sense, unless the TOE itself doesn't check the protocol ID in
the Ethernet frame (which it should). It also seems (after reading <linux/skbuff.h>) that enabling
IPv4 header checksum calculation is pointless -- you don't have the means to report it anyway...
You may want to set skb-csum_level though (not sure if that's not already 0).

> 		skb->ip_summed = CHECKSUM_UNNECESSARY;
> }
> 
> Snippet from H/W manual for reception handling
> 
> (1) Reception Handling
> The result of Checksum Calculation is attached to last 4 byte of Ethernet Frames like Figure 30.25. And then the 
> handled frames are transferred to memory by DMAC. If the frame does not have checksum error at the part of IPv4 
> Header or TCP/UDP/ICMP, the value of “0000h” is attached to each part as the result of Checksum Calculation. The 
> case of Unsupported Frame, the value of “FFFFh” is attached. For example, if the part of IP Header is unsupported, 
> “FFFFh” is set to both field of IPv4 Header and TCP/UDP/ICMP. The case of IPv6, IPv4 Header field is always set to 
> “FFFFh”. 
> 
>  
>>
>>> +	}
>>
>>    Now where's a call to skb_trim()?
> 
> Currently I haven't seen any issue without using skb_trim.
> 
> OK, as you suggested, will check and add skb_trim to takeout the last 4bytes.

   We do it for EtherAVB, why not do it for GbEther?

> Regards,
> Biju

MBR, Sergey

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-26 21:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 13:31 [RFC 0/2] Add Rx checksum offload support Biju Das
2021-11-23 13:31 ` [RFC 1/2] ravb: Fillup ravb_set_features_gbeth() stub Biju Das
2021-11-23 13:31 ` [RFC 2/2] ravb: Add Rx checksum offload support Biju Das
2021-11-24 20:10   ` Sergey Shtylyov
2021-11-25 10:15     ` Biju Das
2021-11-26 21:27       ` Sergey Shtylyov
2021-11-23 15:09 ` [RFC 0/2] " Sergey Shtylyov
2021-11-23 15:40   ` Biju Das
2021-11-24 20:16 ` Sergey Shtylyov
2021-11-25 10:16   ` Biju Das

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.