netdev - Re: [PATCH v3 net-next 2/2] ravb: Add Tx 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: <d3a6657d-0a67-a826-24b1-17ec8f43ee81@omp.ru>
Date: Thu, 1 Feb 2024 23:55:57 +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 v3 net-next 2/2] ravb: Add Tx 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 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@...renesas.com>
> ---
> 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.

   You do vice versa, NETIF_F_HW_CSUM->NETIF_F_IP_CSUM. :-)
   However, I'm now seeing this comment under CHECKSM_PATIAL:

 *   %NETIF_F_IP_CSUM and %NETIF_F_IPV6_CSUM are being deprecated in favor of
 *   %NETIF_F_HW_CSUM. New devices should use %NETIF_F_HW_CSUM to indicate
 *   checksum offload capability.

   So probably we should've kept NETIF_F_HW_CSUM? :-/
 
>  * 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.
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index c4dc6ec54287..042dc565d1a5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> @@ -524,15 +525,27 @@ 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_IP_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_IP_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);
>  	}
>  
>  done:
> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work)
>  	rtnl_unlock();
>  }
>  
> +static bool ravb_is_tx_csum_gbeth(struct sk_buff *skb)

   Hm, this new name doesn't parse well for me... :-(
   Maybe ravb_can_tx_csum_gbeth() or 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;
> +
> +	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;
> +	/* TODO: Need to add HW checksum for ICMP */

   s/HW/hardware/?

> +	case IPPROTO_ICMP:
> +		fallthrough;

   You don't even need fallthrough, actually...
   But why do you return false for ICMP? Isn't it supported by TOE?

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

MBR, Sergey