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: <CADT+UeAeT0dC2AV1RP8H10V7LY2WLeET=6C-U_cXEDW95eYgYg@mail.gmail.com>
Date: Thu, 1 Feb 2024 21:13:54 +0000
From: Biju Das <biju.das.au@...il.com>
To: Sergey Shtylyov <s.shtylyov@....ru>
Cc: 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>, 
	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>
Subject: Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support for GbEth

Hi Sergey,

On Thu, Feb 1, 2024 at 8:56 PM Sergey Shtylyov <s.shtylyov@....ru> wrote:
>
> 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? :-/

Ok, Will do in the next version.
>
> >  * 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()?

OK, ravb_can_tx_csum_gbeth() as it is shorter.

>
> > +{
> > +     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/?

OK.
>
> > +     case IPPROTO_ICMP:
> > +             fallthrough;
>
>    You don't even need fallthrough, actually...

Clang Compiler will complain.

[1] https://patchwork.kernel.org/project/xfs/patch/20210420230652.GA70650@embeddedor/#24129659

https://patches.linaro.org/project/netdev/patch/20210305094850.GA141221@embeddedor/#617482

>    But why do you return false for ICMP? Isn't it supported by TOE?

It is supported by the hardware, but not the network subsystem.

Cheers,
Biju

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ