Message-ID:
 <TYCPR01MB11269DE6466E989CB1DEEE7BF86412@TYCPR01MB11269.jpnprd01.prod.outlook.com>
Date: Sat, 3 Feb 2024 14:01:58 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Sergey Shtylyov <s.shtylyov@....ru>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
CC: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, Yoshihiro Shimoda
	<yoshihiro.shimoda.uh@...esas.com>, Wolfram Sang
	<wsa+renesas@...g-engineering.com>, nikita.yoush
	<nikita.yoush@...entembedded.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-renesas-soc@...r.kernel.org"
	<linux-renesas-soc@...r.kernel.org>, Geert Uytterhoeven
	<geert+renesas@...der.be>, Prabhakar Mahadev Lad
	<prabhakar.mahadev-lad.rj@...renesas.com>, biju.das.au
	<biju.das.au@...il.com>
Subject: RE: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support for
 GbEth



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

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

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

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

> 
> > +	}
> > +
> > +done:
> > +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); }
> > +
> >  static void ravb_emac_init_gbeth(struct net_device *ndev)  {
> >  	struct ravb_private *priv = netdev_priv(ndev);
> [...]
> > @@ -734,6 +752,31 @@ static void ravb_get_tx_tstamp(struct net_device
> *ndev)
> >  	}
> >  }
> >
> > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> > +	__wsum csum_ip_hdr, csum_proto;
> > +	u8 *hw_csum;
> > +
> > +	/* The hardware checksum status is contained in sizeof(__sum16) * 2
> = 4
> > +	 * bytes appended to packet data. First 2 bytes is ip header csum
> and
> > +	 * last 2 bytes is protocol csum.
> 
>    Hm, maybe spell out csum as checksum?

OK.

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

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

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

OK will drop register read.

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

OK will drop it.

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

Cheers,
Biju

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

Powered by blists - more mailing lists