| Message ID | 20240201194521.139472-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add HW checksum offload support for RZ/G2L GbEthernet IP | expand |
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@bp.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
Hi Sergey, On Thu, Feb 1, 2024 at 8:56 PM Sergey Shtylyov <s.shtylyov@omp.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@bp.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
On 2/2/24 12:13 AM, 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@bp.renesas.com> [...] >>> 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 [...] >>> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct work_struct *work) [...] >>> + 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 That's not like our case. If clang can't compile: case IPPROTO_ICMP: default: return false; it's seriously broken! >> 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. Then I don't think we should set CSR1.TICMP{4|6}, so TOE wouldn't try to replace the checksum in the ICMP packets... > Cheers, > Biju MBR, Sergey
Hi Sergey, > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@omp.ru> > Sent: Friday, February 2, 2024 8:00 PM > Subject: Re: [PATCH v3 net-next 2/2] ravb: Add Tx checksum offload support > for GbEth > > On 2/2/24 12:13 AM, 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@bp.renesas.com> > [...] > > >>> 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 > [...] > >>> @@ -1986,6 +1999,35 @@ static void ravb_tx_timeout_work(struct > >>> work_struct *work) > [...] > >>> + case IPPROTO_ICMP: > >>> + fallthrough; > >> > >> You don't even need fallthrough, actually... > > > > Clang Compiler will complain. > > > > That's not like our case. If clang can't compile: > > case IPPROTO_ICMP: > default: > return false; > > it's seriously broken! > > >> 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. > > Then I don't think we should set CSR1.TICMP{4|6}, so TOE wouldn't try > to replace the checksum in the ICMP packets... OK will drop ICMP and send v4. Cheers, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a7fe9d8b6b41..53bd1fa51b32 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, CSR0 = 0x0800, /* RZ/G2L only */ + CSR1 = 0x0804, /* RZ/G2L only */ CSR2 = 0x0808, /* RZ/G2L only */ }; @@ -979,6 +980,21 @@ enum CSR0_BIT { CSR0_RPE = 0x00000020, }; +enum CSR1_BIT { + CSR1_TIP4 = 0x00000001, + CSR1_TTCP4 = 0x00000010, + CSR1_TUDP4 = 0x00000020, + CSR1_TICMP4 = 0x00000040, + CSR1_TTCP6 = 0x00100000, + CSR1_TUDP6 = 0x00200000, + CSR1_TICMP6 = 0x00400000, + CSR1_THOP = 0x01000000, + CSR1_TROUT = 0x02000000, + CSR1_TAHD = 0x04000000, + CSR1_TDHD = 0x08000000, + CSR1_ALL = 0x0F700071, +}; + enum CSR2_BIT { CSR2_RIP4 = 0x00000001, CSR2_RTCP4 = 0x00000010, 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 @@ -29,6 +29,7 @@ #include <linux/spinlock.h> #include <linux/reset.h> #include <linux/math64.h> +#include <net/ip.h> #include "ravb.h" @@ -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) +{ + 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 */ + case IPPROTO_ICMP: + fallthrough; + 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) { @@ -2001,6 +2043,9 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev) u32 entry; u32 len; + if (skb->ip_summed == CHECKSUM_PARTIAL && !ravb_is_tx_csum_gbeth(skb)) + skb_checksum_help(skb); + spin_lock_irqsave(&priv->lock, flags); if (priv->cur_tx[q] - priv->dirty_tx[q] > (priv->num_tx_ring[q] - 1) * num_tx_desc) { @@ -2419,6 +2464,17 @@ static int ravb_set_features_gbeth(struct net_device *ndev, goto done; } + if (changed & NETIF_F_IP_CSUM) { + if (features & NETIF_F_IP_CSUM) + val = CSR1_ALL; + else + val = 0; + + ret = ravb_endisable_csum_gbeth(ndev, CSR1, val, CSR0_TPE); + if (ret) + goto done; + } + ndev->features = features; done: spin_unlock_irqrestore(&priv->lock, flags); @@ -2603,8 +2659,8 @@ static const struct ravb_hw_info gbeth_hw_info = { .emac_init = ravb_emac_init_gbeth, .gstrings_stats = ravb_gstrings_stats_gbeth, .gstrings_size = sizeof(ravb_gstrings_stats_gbeth), - .net_hw_features = NETIF_F_RXCSUM, - .net_features = NETIF_F_RXCSUM, + .net_hw_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM, + .net_features = NETIF_F_RXCSUM | NETIF_F_IP_CSUM, .stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth), .max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN), .tccr_mask = TCCR_TSRQ0,
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@bp.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. * 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. --- drivers/net/ethernet/renesas/ravb.h | 16 ++++++ drivers/net/ethernet/renesas/ravb_main.c | 68 +++++++++++++++++++++--- 2 files changed, 78 insertions(+), 6 deletions(-)