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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dd3ffb2d-23f4-49a6-e427-2b6afb96ddfd@omp.ru>
Date: Mon, 5 Feb 2024 22:01:21 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
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 Yushchenko
	<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 <biju.das.au@...il.com>
Subject: Re: [PATCH v4 net-next 1/2] ravb: Add Rx 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 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..64bf29d01ad0 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -205,7 +205,10 @@ enum ravb_reg {
>  	TLFRCR	= 0x0758,
>  	RFCR	= 0x0760,
>  	MAFCR	= 0x0778,
> -	CSR0    = 0x0800,	/* RZ/G2L only */
> +
> +	/* RZ/G2L TOE registers */

   Thanks. Though I think I'd prefer /* TOE registers (RZ/G2L only) */...

[...]
> @@ -978,6 +981,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,

  I doubt we really need CSR2_ALL...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0e3731f50fc2..4f310bcee7c0 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -522,6 +522,24 @@ 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_RTCP6 | CSR2_RUDP6 |
> +					      CSR2_RICMP6), CSR2);

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

		ravb_write(ndev, CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4,
			   CSR2);

> +	}
> +
> +done:
> +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);

   I think we shouldn't set CSR0.TPE yet at this point, as we n't setup
CSR1 yet...

[...]
> @@ -2334,11 +2381,48 @@ 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;
> +	int ret = 0;
> +	u32 val;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	if (changed & NETIF_F_RXCSUM) {
> +		if (features & NETIF_F_RXCSUM)
> +			val = CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6);

   Likewise, I'd prefer:

			val = CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4;

> +		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,
[...]

   Otherwise LGTM. We're close! :-)

MBR, Sergey