| 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
| ||
|
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