| Message ID | 20240124102115.132154-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add HW checksum offload support for RZ/G2L GbEthernet IP | expand |
On 1/24/24 1:21 PM, Biju Das wrote: > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum for s/hw/hardware/, please... > 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 UDP > frame of IPv4 and its checksum value in the UDP header field is H’0000, > 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 59c7bedacef6..3c748a54fae0 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> What do you need from that header, BTW? [...] > @@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct work_struct *work) > rtnl_unlock(); > } > > +static bool ravb_is_tx_checksum_offload_gbeth_possible(struct sk_buff *skb) I'd suggest s/th shorter and more consistent with the used naming, like 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; > + > + /* TODO: Need to add HW checksum for IPV6 */ > + if (skb->protocol != htons(ETH_P_IP)) > + return false; So maybe we need to report just NETIF_F_IP_CSUM, not NETIF_F_HW_CSUM ATM? > + > + switch (ip->protocol) { > + case IPPROTO_TCP: > + break; > + case IPPROTO_UDP: > + /* If the checksum value in the UDP header field is “H’0000”, Use 0x0000 or just 0, please. I don't know where Renesas found this weird hex notation... [...] > @@ -2005,6 +2049,11 @@ 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) { > + if (!ravb_is_tx_checksum_offload_gbeth_possible(skb)) I'd collapse those 2 *if* statements... [...] MBR, Sergey
Hi Sergey Shtylyov, Thanks for the feedback. > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@omp.ru> > Sent: Friday, January 26, 2024 9:00 PM > Subject: Re: [PATCH net-next v2 2/2] ravb: Add Tx checksum offload support > > On 1/24/24 1:21 PM, Biju Das wrote: > > > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum > > for > > s/hw/hardware/, please... Agreed. > > > 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 UDP frame of IPv4 and its checksum value in the UDP header > > field is H’0000, 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 59c7bedacef6..3c748a54fae0 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> > > What do you need from that header, BTW? It is giving compilation error for ip_hdr() and udp_hdr(). > > [...] > > @@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct > work_struct *work) > > rtnl_unlock(); > > } > > > > +static bool ravb_is_tx_checksum_offload_gbeth_possible(struct sk_buff > > +*skb) > > I'd suggest s/th shorter and more consistent with the used naming, like > ravb_tx_csum_possible_gbeth()... OK. > > > +{ > > + struct iphdr *ip = ip_hdr(skb); > > + > > + /* TODO: Need to add support for VLAN tag 802.1Q */ > > + if (skb_vlan_tag_present(skb)) > > + return false; > > + > > + /* TODO: Need to add HW checksum for IPV6 */ > > + if (skb->protocol != htons(ETH_P_IP)) > > + return false; > > So maybe we need to report just NETIF_F_IP_CSUM, not NETIF_F_HW_CSUM > ATM? I agree, at the moment we are supporting only IPv4 Checksum offload. Later when we can reintroduce NETIF_F_HW_CSUM when we add support for IPv6. > > > + > > + switch (ip->protocol) { > > + case IPPROTO_TCP: > > + break; > > + case IPPROTO_UDP: > > + /* If the checksum value in the UDP header field is “H’0000”, > > Use 0x0000 or just 0, please. I don't know where Renesas found this > weird hex notation... OK. > > [...] > > @@ -2005,6 +2049,11 @@ 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) { > > + if (!ravb_is_tx_checksum_offload_gbeth_possible(skb)) > > I'd collapse those 2 *if* statements... Agreed Cheers, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a2c494a85d12..3cf869fb9a68 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -209,6 +209,7 @@ enum ravb_reg { RFCR = 0x0760, MAFCR = 0x0778, CSR0 = 0x0800, /* RZ/G2L only */ + CSR1 = 0x0804, /* RZ/G2L only */ CSR2 = 0x0808, /* RZ/G2L only */ }; @@ -982,6 +983,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 59c7bedacef6..3c748a54fae0 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,19 +525,29 @@ static int ravb_ring_init(struct net_device *ndev, int q) static void ravb_csum_offload_init_gbeth(struct net_device *ndev) { + bool tx_enable = ndev->features & NETIF_F_HW_CSUM; bool rx_enable = ndev->features & NETIF_F_RXCSUM; u32 csr0; - if (!rx_enable) + if (!(tx_enable || rx_enable)) return; csr0 = ravb_read(ndev, CSR0); ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) { netdev_err(ndev, "Timeout Enabling HW CSUM failed\n"); - ndev->features &= ~NETIF_F_RXCSUM; + + if (tx_enable) + ndev->features &= ~NETIF_F_HW_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); } ravb_write(ndev, csr0, CSR0); @@ -1990,6 +2001,39 @@ static void ravb_tx_timeout_work(struct work_struct *work) rtnl_unlock(); } +static bool ravb_is_tx_checksum_offload_gbeth_possible(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; + + /* TODO: Need to add HW checksum for IPV6 */ + if (skb->protocol != htons(ETH_P_IP)) + return false; + + switch (ip->protocol) { + case IPPROTO_TCP: + break; + case IPPROTO_UDP: + /* If the checksum value in the UDP header field is “H’0000”, + * 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) { @@ -2005,6 +2049,11 @@ 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) { + if (!ravb_is_tx_checksum_offload_gbeth_possible(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) { @@ -2408,6 +2457,13 @@ static int ravb_set_features_gbeth(struct net_device *ndev, ravb_write(ndev, 0, CSR2); } + if (changed & NETIF_F_HW_CSUM) { + if (features & NETIF_F_HW_CSUM) + ravb_write(ndev, CSR1_ALL, CSR1); + else + ravb_write(ndev, 0, CSR1); + } + ndev->features = features; err_wait: ravb_write(ndev, csr0, CSR0); @@ -2593,8 +2649,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_HW_CSUM, + .net_features = NETIF_F_RXCSUM | NETIF_F_HW_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 hw 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 UDP frame of IPv4 and its checksum value in the UDP header field is H’0000, 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> --- v1->v2: * No change. --- drivers/net/ethernet/renesas/ravb.h | 16 ++++++ drivers/net/ethernet/renesas/ravb_main.c | 66 ++++++++++++++++++++++-- 2 files changed, 77 insertions(+), 5 deletions(-)