| Message ID | 20211123133157.21829-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add Rx checksum offload support | expand |
Hello! On 11/23/21 4:31 PM, Biju Das wrote: > TOE has hw support for calculating IP header checkum for IPV4 and > TCP/UDP/ICMP checksum for both IPV4 and IPV6. > > This patch adds Rx checksum offload supported by TOE. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb.h | 4 +++ > drivers/net/ethernet/renesas/ravb_main.c | 31 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index a96552348e2d..d0e5eec0636e 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -44,6 +44,10 @@ > #define RAVB_RXTSTAMP_TYPE_ALL 0x00000006 > #define RAVB_RXTSTAMP_ENABLED 0x00000010 /* Enable RX timestamping */ > > +/* GbEthernet TOE hardware checksum values */ > +#define TOE_RX_CSUM_OK 0x0000 > +#define TOE_RX_CSUM_UNSUPPORTED 0xFFFF These are hardly needed IMO. [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index c2b92c6a6cd2..2533e3401593 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) > } > } > > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) > +{ > + u32 csum_ip_hdr, csum_proto; Why u32 if both sums are 16-bit? > + u8 *hw_csum; > + > + /* The hardware checksum is contained in sizeof(__sum16) * 2 = 4 bytes > + * appended to packet data. First 2 bytes is ip header csum and last > + * 2 bytes is protocol csum. > + */ > + if (unlikely(skb->len < sizeof(__sum16) * 2)) > + return; > + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16); > + csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > + > + hw_csum = skb_tail_pointer(skb) - 2 * sizeof(__sum16); > + csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > + > + skb->ip_summed = CHECKSUM_NONE; > + if (csum_proto == TOE_RX_CSUM_OK) { > + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK) > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + else if (skb->protocol == htons(ETH_P_IPV6) && > + csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED) > + skb->ip_summed = CHECKSUM_UNNECESSARY; Checksum is unsupported and you declare it unnecessary? > + } Now where's a call to skb_trim()? [...] MBR, Sergey
Hi Sergey Shtylyov, > Subject: Re: [RFC 2/2] ravb: Add Rx checksum offload support > > Hello! > > On 11/23/21 4:31 PM, Biju Das wrote: > > > TOE has hw support for calculating IP header checkum for IPV4 and > > TCP/UDP/ICMP checksum for both IPV4 and IPV6. > > > > This patch adds Rx checksum offload supported by TOE. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb.h | 4 +++ > > drivers/net/ethernet/renesas/ravb_main.c | 31 > > ++++++++++++++++++++++++ > > 2 files changed, 35 insertions(+) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index a96552348e2d..d0e5eec0636e 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -44,6 +44,10 @@ > > #define RAVB_RXTSTAMP_TYPE_ALL 0x00000006 > > #define RAVB_RXTSTAMP_ENABLED 0x00000010 /* Enable RX timestamping > */ > > > > +/* GbEthernet TOE hardware checksum values */ > > +#define TOE_RX_CSUM_OK 0x0000 > > +#define TOE_RX_CSUM_UNSUPPORTED 0xFFFF > > These are hardly needed IMO. OK. > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index c2b92c6a6cd2..2533e3401593 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device > *ndev) > > } > > } > > > > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) { > > + u32 csum_ip_hdr, csum_proto; > > Why u32 if both sums are 16-bit? OK, will make it 16-bit. > > > + u8 *hw_csum; > > + > > + /* The hardware checksum is contained in sizeof(__sum16) * 2 = 4 > bytes > > + * appended to packet data. First 2 bytes is ip header csum and last > > + * 2 bytes is protocol csum. > > + */ > > + if (unlikely(skb->len < sizeof(__sum16) * 2)) > > + return; > > + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16); > > + csum_proto = csum_unfold((__force > > +__sum16)get_unaligned_le16(hw_csum)); > > + > > + hw_csum = skb_tail_pointer(skb) - 2 * sizeof(__sum16); > > + csum_ip_hdr = csum_unfold((__force > > +__sum16)get_unaligned_le16(hw_csum)); > > + > > + skb->ip_summed = CHECKSUM_NONE; > > + if (csum_proto == TOE_RX_CSUM_OK) { > > + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == > TOE_RX_CSUM_OK) > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > + else if (skb->protocol == htons(ETH_P_IPV6) && > > + csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED) > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > Checksum is unsupported and you declare it unnecessary? Do you mean takeout the check for unsupported headercsum for IPV6 and the code like one below? If(!csum_proto) { if ((skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr) || skb->protocol == htons(ETH_P_IPV6)) skb->ip_summed = CHECKSUM_UNNECESSARY; } Snippet from H/W manual for reception handling (1) Reception Handling The result of Checksum Calculation is attached to last 4 byte of Ethernet Frames like Figure 30.25. And then the handled frames are transferred to memory by DMAC. If the frame does not have checksum error at the part of IPv4 Header or TCP/UDP/ICMP, the value of “0000h” is attached to each part as the result of Checksum Calculation. The case of Unsupported Frame, the value of “FFFFh” is attached. For example, if the part of IP Header is unsupported, “FFFFh” is set to both field of IPv4 Header and TCP/UDP/ICMP. The case of IPv6, IPv4 Header field is always set to “FFFFh”. > > > + } > > Now where's a call to skb_trim()? Currently I haven't seen any issue without using skb_trim. OK, as you suggested, will check and add skb_trim to takeout the last 4bytes. Regards, Biju
Hello! On 11/25/21 1:15 PM, Biju Das wrote: >>> TOE has hw support for calculating IP header checkum for IPV4 and >>> TCP/UDP/ICMP checksum for both IPV4 and IPV6. >>> >>> This patch adds Rx checksum offload supported by TOE. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> drivers/net/ethernet/renesas/ravb.h | 4 +++ >>> drivers/net/ethernet/renesas/ravb_main.c | 31 >>> ++++++++++++++++++++++++ >>> 2 files changed, 35 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index a96552348e2d..d0e5eec0636e 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c >>> b/drivers/net/ethernet/renesas/ravb_main.c >>> index c2b92c6a6cd2..2533e3401593 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device [...] >>> + u8 *hw_csum; >>> + >>> + /* The hardware checksum is contained in sizeof(__sum16) * 2 = 4 >> bytes >>> + * appended to packet data. First 2 bytes is ip header csum and last >>> + * 2 bytes is protocol csum. >>> + */ >>> + if (unlikely(skb->len < sizeof(__sum16) * 2)) >>> + return; >>> + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16); >>> + csum_proto = csum_unfold((__force >>> +__sum16)get_unaligned_le16(hw_csum)); >>> + >>> + hw_csum = skb_tail_pointer(skb) - 2 * sizeof(__sum16); >>> + csum_ip_hdr = csum_unfold((__force >>> +__sum16)get_unaligned_le16(hw_csum)); >>> + >>> + skb->ip_summed = CHECKSUM_NONE; >>> + if (csum_proto == TOE_RX_CSUM_OK) { >>> + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == >> TOE_RX_CSUM_OK) >>> + skb->ip_summed = CHECKSUM_UNNECESSARY; >>> + else if (skb->protocol == htons(ETH_P_IPV6) && >>> + csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED) >>> + skb->ip_summed = CHECKSUM_UNNECESSARY; >> >> Checksum is unsupported and you declare it unnecessary? > > > Do you mean takeout the check for unsupported headercsum for IPV6 and the code like one below? No, I meant that the code looks strange. > If(!csum_proto) { > if ((skb->protocol == htons(ETH_P_IP) && !csum_ip_hdr) || skb->protocol == htons(ETH_P_IPV6)) I think this statement doesn't make sense, unless the TOE itself doesn't check the protocol ID in the Ethernet frame (which it should). It also seems (after reading <linux/skbuff.h>) that enabling IPv4 header checksum calculation is pointless -- you don't have the means to report it anyway... You may want to set skb-csum_level though (not sure if that's not already 0). > skb->ip_summed = CHECKSUM_UNNECESSARY; > } > > Snippet from H/W manual for reception handling > > (1) Reception Handling > The result of Checksum Calculation is attached to last 4 byte of Ethernet Frames like Figure 30.25. And then the > handled frames are transferred to memory by DMAC. If the frame does not have checksum error at the part of IPv4 > Header or TCP/UDP/ICMP, the value of “0000h” is attached to each part as the result of Checksum Calculation. The > case of Unsupported Frame, the value of “FFFFh” is attached. For example, if the part of IP Header is unsupported, > “FFFFh” is set to both field of IPv4 Header and TCP/UDP/ICMP. The case of IPv6, IPv4 Header field is always set to > “FFFFh”. > > >> >>> + } >> >> Now where's a call to skb_trim()? > > Currently I haven't seen any issue without using skb_trim. > > OK, as you suggested, will check and add skb_trim to takeout the last 4bytes. We do it for EtherAVB, why not do it for GbEther? > Regards, > Biju MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index a96552348e2d..d0e5eec0636e 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -44,6 +44,10 @@ #define RAVB_RXTSTAMP_TYPE_ALL 0x00000006 #define RAVB_RXTSTAMP_ENABLED 0x00000010 /* Enable RX timestamping */ +/* GbEthernet TOE hardware checksum values */ +#define TOE_RX_CSUM_OK 0x0000 +#define TOE_RX_CSUM_UNSUPPORTED 0xFFFF + enum ravb_reg { /* AVB-DMAC registers */ CCC = 0x0000, diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index c2b92c6a6cd2..2533e3401593 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -720,6 +720,33 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) } } +static void ravb_rx_csum_gbeth(struct sk_buff *skb) +{ + u32 csum_ip_hdr, csum_proto; + u8 *hw_csum; + + /* The hardware checksum is contained in sizeof(__sum16) * 2 = 4 bytes + * appended to packet data. First 2 bytes is ip header csum and last + * 2 bytes is protocol csum. + */ + if (unlikely(skb->len < sizeof(__sum16) * 2)) + return; + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16); + csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); + + hw_csum = skb_tail_pointer(skb) - 2 * sizeof(__sum16); + csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); + + skb->ip_summed = CHECKSUM_NONE; + if (csum_proto == TOE_RX_CSUM_OK) { + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK) + skb->ip_summed = CHECKSUM_UNNECESSARY; + else if (skb->protocol == htons(ETH_P_IPV6) && + csum_ip_hdr == TOE_RX_CSUM_UNSUPPORTED) + skb->ip_summed = CHECKSUM_UNNECESSARY; + } +} + static void ravb_rx_csum(struct sk_buff *skb) { u8 *hw_csum; @@ -805,6 +832,8 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) skb = ravb_get_skb_gbeth(ndev, entry, desc); skb_put(skb, pkt_len); skb->protocol = eth_type_trans(skb, ndev); + if (ndev->features & NETIF_F_RXCSUM) + ravb_rx_csum_gbeth(skb); napi_gro_receive(&priv->napi[q], skb); stats->rx_packets++; stats->rx_bytes += pkt_len; @@ -832,6 +861,8 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) dev_kfree_skb(skb); priv->rx_1st_skb->protocol = eth_type_trans(priv->rx_1st_skb, ndev); + if (ndev->features & NETIF_F_RXCSUM) + ravb_rx_csum_gbeth(skb); napi_gro_receive(&priv->napi[q], priv->rx_1st_skb); stats->rx_packets++;
TOE has hw support for calculating IP header checkum for IPV4 and TCP/UDP/ICMP checksum for both IPV4 and IPV6. This patch adds Rx checksum offload supported by TOE. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/net/ethernet/renesas/ravb.h | 4 +++ drivers/net/ethernet/renesas/ravb_main.c | 31 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+)