| Message ID | 20240124102115.132154-2-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 1/24/24 1:21 PM, Biju Das wrote: > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum s/hw/hardware/. > for both IPV4 and IPV6. Those are usually called IPv4 and IPv6, no? > Add Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols. > > For Rx, the result of checksum calculation is attached to last 4byte > of ethernet frames. "For Rx, the 4-byte result of checksum calculation is attached to the Ethernet frames", you wanted to say? > First 2bytes is result of IPV4 header checksum > and next 2 bytes is TCP/UDP/ICMP. TCP/UDP/ICMP checksum, you mean? Also, you alternatively say TCP/UDP/ICMP and just TCP/UDP -- which one is correct? > If frame does not have error "0000" attached to checksum calculation "If a frame does not have checksum error, 0x0000 is attached as a checksum calculation result", you wanted to say? > result. For unsupported frames "ffff" is attached to checksum calculation s/to/as/? > result. Cases like IPV6, IPV4 header is always set to "FFFF". "In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF", you wanted to say? > We can test this functionality by the below commands > > ethtool -K eth0 rx on --> to turn on Rx checksum offload > ethtool -K eth0 rx off --> to turn off Rx checksum offload > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index e0f8276cffed..a2c494a85d12 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -44,6 +44,9 @@ > #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 As I said before, this is hardly needed... [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 0e3731f50fc2..59c7bedacef6 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev, int q) > return -ENOMEM; > } > > +static void ravb_csum_offload_init_gbeth(struct net_device *ndev) I'd leave out _offload... > +{ > + bool rx_enable = ndev->features & NETIF_F_RXCSUM; > + u32 csr0; > + > + if (!rx_enable) > + return; > + > + csr0 = ravb_read(ndev, CSR0); Why read it here, if we'll write a constant to this reg at the end of ravb_emac_init_gbeth()? > + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); We can just write 0 here, no? > + if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) { > + netdev_err(ndev, "Timeout Enabling HW CSUM failed\n"); "Timeout enabling hardware checksum\n", perhaps? [...] > + > + ravb_write(ndev, csr0, CSR0); I think we should move: ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); from ravb_emac_init_gbeth() here... > +} > + [...] > @@ -734,6 +755,32 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) > } > } > > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) > +{ > + __wsum csum_ip_hdr, csum_proto; > + u8 *hw_csum; > + > + /* The hardware checksum status 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 -= sizeof(__sum16); > + csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > + skb_trim(skb, skb->len - 2 * sizeof(__sum16)); > + > + /* TODO: IPV6 Rx csum */ > + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK && > + csum_proto == TOE_RX_CSUM_OK) > + /* Hardware validated our checksum */ > + skb->ip_summed = CHECKSUM_UNNECESSARY; Don't we need to set skb->csum_level? [...] > @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable) > static int ravb_set_features_gbeth(struct net_device *ndev, > netdev_features_t features) > { > - /* Place holder */ > - return 0; > + netdev_features_t changed = ndev->features ^ features; > + struct ravb_private *priv = netdev_priv(ndev); > + unsigned long flags; > + u32 csr0; > + int ret; > + > + spin_lock_irqsave(&priv->lock, flags); > + csr0 = ravb_read(ndev, CSR0); > + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); > + ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0); > + if (ret) > + goto err_wait; I don't understand: why do you clear the CSR0 bits even if (changed & NETIF_F_RXCSUM) is 0? This looks very wrong... > + > + if (changed & NETIF_F_RXCSUM) { > + if (features & NETIF_F_RXCSUM) > + ravb_write(ndev, CSR2_ALL, CSR2); > + else > + ravb_write(ndev, 0, CSR2); > + } I think you should put that into a separate function, like is done for the EhterAVB... [...] > @@ -2518,6 +2593,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, What about NETIF_F_IP_CSUM, BTW? [...] MBR, Sergey
Hi Sergey Shtylyov, Thanks for the feedback. > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@omp.ru> > Sent: Thursday, January 25, 2024 8:42 PM > Subject: Re: [PATCH net-next v2 1/2] ravb: Add Rx 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 > > s/hw/hardware/. Ok. > > > for both IPV4 and IPV6. > > Those are usually called IPv4 and IPv6, no? Agreed. > > > Add Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols. > > > > For Rx, the result of checksum calculation is attached to last 4byte > > of ethernet frames. > > "For Rx, the 4-byte result of checksum calculation is attached to the > Ethernet frames", you wanted to say? Yes. > > > First 2bytes is result of IPV4 header checksum and next 2 bytes is > > TCP/UDP/ICMP. > > TCP/UDP/ICMP checksum, you mean? Also, you alternatively say > TCP/UDP/ICMP and just TCP/UDP -- which one is correct? As per the hardware manual, it supports TCP/UDP/ICMP checksum. So you are correct, it is TCP/UDP/ICMP checksum. > > > If frame does not have error "0000" attached to checksum calculation > > "If a frame does not have checksum error, 0x0000 is attached as a > checksum calculation result", you wanted to say? Ok. > > > result. For unsupported frames "ffff" is attached to checksum > > calculation > > s/to/as/? Correct. > > > result. Cases like IPV6, IPV4 header is always set to "FFFF". > > "In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF", you > wanted to say? Correct. > > > We can test this functionality by the below commands > > > > ethtool -K eth0 rx on --> to turn on Rx checksum offload ethtool -K > > eth0 rx off --> to turn off Rx checksum offload > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > [...] > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index e0f8276cffed..a2c494a85d12 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -44,6 +44,9 @@ > > #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 > > As I said before, this is hardly needed... It is needed to match with the Checksum status as mentioned in the hardware manual. > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 0e3731f50fc2..59c7bedacef6 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev, > int q) > > return -ENOMEM; > > } > > > > +static void ravb_csum_offload_init_gbeth(struct net_device *ndev) > > I'd leave out _offload... Ok. > > > +{ > > + bool rx_enable = ndev->features & NETIF_F_RXCSUM; > > + u32 csr0; > > + > > + if (!rx_enable) > > + return; > > + > > + csr0 = ravb_read(ndev, CSR0); > > Why read it here, if we'll write a constant to this reg at the end of > ravb_emac_init_gbeth()? The correct flow is Disable tx/rx Enable Checksum Reenable Tx/rx if it is already enabled. > > > + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); > > We can just write 0 here, no? See above. > > > + if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) { > > + netdev_err(ndev, "Timeout Enabling HW CSUM failed\n"); > > "Timeout enabling hardware checksum\n", perhaps? OK. > > [...] > > + > > + ravb_write(ndev, csr0, CSR0); > > I think we should move: > > ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); > > from ravb_emac_init_gbeth() here... I am providing flexible options here. > > > +} > > + > [...] > > @@ -734,6 +755,32 @@ static void ravb_get_tx_tstamp(struct net_device > *ndev) > > } > > } > > > > +static void ravb_rx_csum_gbeth(struct sk_buff *skb) { > > + __wsum csum_ip_hdr, csum_proto; > > + u8 *hw_csum; > > + > > + /* The hardware checksum status 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 -= sizeof(__sum16); > > + csum_ip_hdr = csum_unfold((__force > __sum16)get_unaligned_le16(hw_csum)); > > + skb_trim(skb, skb->len - 2 * sizeof(__sum16)); > > + > > + /* TODO: IPV6 Rx csum */ > > + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == > TOE_RX_CSUM_OK && > > + csum_proto == TOE_RX_CSUM_OK) > > + /* Hardware validated our checksum */ > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > Don't we need to set skb->csum_level? As per my knowledge, it is not needed. I may be wrong. Why do you think it is needed? > > [...] > > @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device > > *ndev, bool enable) static int ravb_set_features_gbeth(struct > net_device *ndev, > > netdev_features_t features) > > { > > - /* Place holder */ > > - return 0; > > + netdev_features_t changed = ndev->features ^ features; > > + struct ravb_private *priv = netdev_priv(ndev); > > + unsigned long flags; > > + u32 csr0; > > + int ret; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + csr0 = ravb_read(ndev, CSR0); > > + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); > > + ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0); > > + if (ret) > > + goto err_wait; > > I don't understand: why do you clear the CSR0 bits even if (changed & > NETIF_F_RXCSUM) is 0? This looks very wrong... I made the code simple. Can you please suggest a much simpler way than this? > > > + > > + if (changed & NETIF_F_RXCSUM) { > > + if (features & NETIF_F_RXCSUM) > > + ravb_write(ndev, CSR2_ALL, CSR2); > > + else > > + ravb_write(ndev, 0, CSR2); > > + } > > I think you should put that into a separate function, like is done for > the EhterAVB... you mean add this if else block to separate function?? Can you please elaborate?? > > [...] > > @@ -2518,6 +2593,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, > > What about NETIF_F_IP_CSUM, BTW? Why is it needed? Can you please clarify? Cheers, Biju
On 1/26/24 1:15 AM, Biju Das wrote: > Hi Sergey Shtylyov, Hi! :-) > Thanks for the feedback. Not at all! >> -----Original Message----- >> From: Sergey Shtylyov <s.shtylyov@omp.ru> >> Sent: Thursday, January 25, 2024 8:42 PM >> Subject: Re: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support [...] >>> We can test this functionality by the below commands >>> >>> ethtool -K eth0 rx on --> to turn on Rx checksum offload ethtool -K >>> eth0 rx off --> to turn off Rx checksum offload >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >> [...] >> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index e0f8276cffed..a2c494a85d12 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -44,6 +44,9 @@ >>> #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 >> >> As I said before, this is hardly needed... > > It is needed to match with the Checksum status as mentioned in the hardware manual. I think you can just compare to 0... ISTR that checksumming result should indeed be 0 for a good IP header, so this # does not seem device specific... >> [...] >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c >>> b/drivers/net/ethernet/renesas/ravb_main.c >>> index 0e3731f50fc2..59c7bedacef6 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] >>> +{ >>> + bool rx_enable = ndev->features & NETIF_F_RXCSUM; >>> + u32 csr0; >>> + >>> + if (!rx_enable) >>> + return; >>> + >>> + csr0 = ravb_read(ndev, CSR0); >> >> Why read it here, if we'll write a constant to this reg at the end of >> ravb_emac_init_gbeth()? > > The correct flow is > > Disable tx/rx > Enable Checksum > Reenable Tx/rx if it is already enabled. Yeah, I figured. :-) However I can't find this flow in the RZ/G2L[C] user's manual... >>> + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); >> >> We can just write 0 here, no? > > See above. I have to repeat: either don't do read/modife/write CSR0 accesses here or remove the below line from ravb_emac_init_gbeth(): ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); Well, I think this line should be removed in any case -- we shouldn't enable RX/TX checksumming in CSR0 if we don't also set up CSR1/2... [...] >>> + >>> + ravb_write(ndev, csr0, CSR0); >> >> I think we should move: >> >> ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); >> >> from ravb_emac_init_gbeth() here... > > I am providing flexible options here. I don't get it... what flexibility do you mean? [...] >>> @@ -734,6 +755,32 @@ static void ravb_get_tx_tstamp(struct net_device >> *ndev) >>> } >>> } >>> >>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) { >>> + __wsum csum_ip_hdr, csum_proto; >>> + u8 *hw_csum; >>> + >>> + /* The hardware checksum status 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 -= sizeof(__sum16); >>> + csum_ip_hdr = csum_unfold((__force >> __sum16)get_unaligned_le16(hw_csum)); >>> + skb_trim(skb, skb->len - 2 * sizeof(__sum16)); >>> + >>> + /* TODO: IPV6 Rx csum */ >>> + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == >> TOE_RX_CSUM_OK && >>> + csum_proto == TOE_RX_CSUM_OK) >>> + /* Hardware validated our checksum */ >>> + skb->ip_summed = CHECKSUM_UNNECESSARY; >> >> Don't we need to set skb->csum_level? > > As per my knowledge, it is not needed. I may be wrong. Why do you > think it is needed? * CHECKSUM_UNNECESSARY is applicable to following protocols: * TCP: IPv6 and IPv4. * UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a * zero UDP checksum for either IPv4 or IPv6, the networking stack * may perform further validation in this case. * GRE: only if the checksum is present in the header. * SCTP: indicates the CRC in SCTP header has been validated. * FCOE: indicates the CRC in FC frame has been validated. * * skb->csum_level indicates the number of consecutive checksums found in * the packet minus one that have been verified as CHECKSUM_UNNECESSARY. * For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet * and a device is able to verify the checksums for UDP (possibly zero), * GRE (checksum flag is set) and TCP, skb->csum_level would be set to * two. If the device were only able to verify the UDP checksum and not * GRE, either because it doesn't support GRE checksum or because GRE * checksum is bad, skb->csum_level would be set to zero (TCP checksum is * not considered in this case). It would seem we should set this field to 1 if the TCP/UDP checksum was successfully verified? >> [...] >>> @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device >>> *ndev, bool enable) static int ravb_set_features_gbeth(struct >> net_device *ndev, >>> netdev_features_t features) >>> { >>> - /* Place holder */ >>> - return 0; >>> + netdev_features_t changed = ndev->features ^ features; >>> + struct ravb_private *priv = netdev_priv(ndev); >>> + unsigned long flags; >>> + u32 csr0; >>> + int ret; >>> + >>> + spin_lock_irqsave(&priv->lock, flags); >>> + csr0 = ravb_read(ndev, CSR0); >>> + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); >>> + ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0); >>> + if (ret) >>> + goto err_wait; >> >> I don't understand: why do you clear the CSR0 bits even if (changed & >> NETIF_F_RXCSUM) is 0? This looks very wrong... > > I made the code simple. Can you please suggest a much simpler way than this? Of course, I can. I don't think clearing CSR0.TPE makes sense if you only modify CSR2, and clearing CSR0.RPE makes sense if you only modify CSR1... >>> + >>> + if (changed & NETIF_F_RXCSUM) { >>> + if (features & NETIF_F_RXCSUM) >>> + ravb_write(ndev, CSR2_ALL, CSR2); >>> + else >>> + ravb_write(ndev, 0, CSR2); >>> + } >> >> I think you should put that into a separate function, like is done for >> the EhterAVB... > > you mean add this if else block to separate function?? Can you please elaborate?? No, you need to 1st clear CSR0.{RPE|TPE}, then set up CSR1/2, then restore CSR0... something like that. >> [...] >>> @@ -2518,6 +2593,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, >> >> What about NETIF_F_IP_CSUM, BTW? > > Why is it needed? Can you please clarify? Ignore me -- this one seems to be used for the TX path. I just had to learn how the checksum offloading works while reviewing your patches... :-) > Cheers, > Biju MBR, Sergey
Hi Sergey Shtylyov, Thanks for the feedback. > >> Don't we need to set skb->csum_level? > > > > As per my knowledge, it is not needed. I may be wrong. Why do you > > think it is needed? > > * CHECKSUM_UNNECESSARY is applicable to following protocols: > * TCP: IPv6 and IPv4. > * UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a > * zero UDP checksum for either IPv4 or IPv6, the networking stack > * may perform further validation in this case. > * GRE: only if the checksum is present in the header. > * SCTP: indicates the CRC in SCTP header has been validated. > * FCOE: indicates the CRC in FC frame has been validated. > * > * skb->csum_level indicates the number of consecutive checksums found > in > * the packet minus one that have been verified as CHECKSUM_UNNECESSARY. > * For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet > * and a device is able to verify the checksums for UDP (possibly zero), > * GRE (checksum flag is set) and TCP, skb->csum_level would be set to > * two. If the device were only able to verify the UDP checksum and not > * GRE, either because it doesn't support GRE checksum or because GRE > * checksum is bad, skb->csum_level would be set to zero (TCP checksum > is > * not considered in this case). > > It would seem we should set this field to 1 if the TCP/UDP checksum was > successfully verified? I guess it is for encapsulated packets. For just IP and UDP/TCP Checksum it is not required. See https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/3com/3c59x.c#L2669 https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/amazon/ena/ena_netdev.c#L1626 https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/alx/main.c#L272 https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/am65-cpsw-nuss.c#L711 > > >> [...] > >>> @@ -2518,6 +2593,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, > >> > >> What about NETIF_F_IP_CSUM, BTW? > > > > Why is it needed? Can you please clarify? > > Ignore me -- this one seems to be used for the TX path. > I just had to learn how the checksum offloading works while reviewing > your patches... :-) OK. For other comments. I will test and respond. Cheers, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index e0f8276cffed..a2c494a85d12 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -44,6 +44,9 @@ #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 + enum ravb_reg { /* AVB-DMAC registers */ CCC = 0x0000, @@ -206,6 +209,7 @@ enum ravb_reg { RFCR = 0x0760, MAFCR = 0x0778, CSR0 = 0x0800, /* RZ/G2L only */ + CSR2 = 0x0808, /* RZ/G2L only */ }; @@ -978,6 +982,21 @@ enum CSR0_BIT { CSR0_RPE = 0x00000020, }; +enum CSR2_BIT { + CSR2_RIP4 = 0x00000001, + CSR2_RTCP4 = 0x00000010, + CSR2_RUDP4 = 0x00000020, + CSR2_RICMP4 = 0x00000040, + CSR2_RTCP6 = 0x00100000, + CSR2_RUDP6 = 0x00200000, + CSR2_RICMP6 = 0x00400000, + CSR2_RHOP = 0x01000000, + CSR2_RROUT = 0x02000000, + CSR2_RAHD = 0x04000000, + CSR2_RDHD = 0x08000000, + CSR2_ALL = 0x0F700071, +}; + #define DBAT_ENTRY_NUM 22 #define RX_QUEUE_OFFSET 4 #define NUM_RX_QUEUE 2 diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 0e3731f50fc2..59c7bedacef6 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev, int q) return -ENOMEM; } +static void ravb_csum_offload_init_gbeth(struct net_device *ndev) +{ + bool rx_enable = ndev->features & NETIF_F_RXCSUM; + u32 csr0; + + if (!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; + } else { + ravb_write(ndev, CSR2_ALL, CSR2); + } + + ravb_write(ndev, csr0, CSR0); +} + static void ravb_emac_init_gbeth(struct net_device *ndev) { struct ravb_private *priv = netdev_priv(ndev); @@ -543,6 +563,7 @@ static void ravb_emac_init_gbeth(struct net_device *ndev) ECMR_TE | ECMR_RE | ECMR_RCPT | ECMR_TXF | ECMR_RXF, ECMR); + ravb_csum_offload_init_gbeth(ndev); ravb_set_rate_gbeth(ndev); /* Set MAC address */ @@ -734,6 +755,32 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) } } +static void ravb_rx_csum_gbeth(struct sk_buff *skb) +{ + __wsum csum_ip_hdr, csum_proto; + u8 *hw_csum; + + /* The hardware checksum status 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 -= sizeof(__sum16); + csum_ip_hdr = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); + skb_trim(skb, skb->len - 2 * sizeof(__sum16)); + + /* TODO: IPV6 Rx csum */ + if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr == TOE_RX_CSUM_OK && + csum_proto == TOE_RX_CSUM_OK) + /* Hardware validated our checksum */ + skb->ip_summed = CHECKSUM_UNNECESSARY; +} + static void ravb_rx_csum(struct sk_buff *skb) { u8 *hw_csum; @@ -819,6 +866,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; @@ -846,6 +895,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++; @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device *ndev, bool enable) static int ravb_set_features_gbeth(struct net_device *ndev, netdev_features_t features) { - /* Place holder */ - return 0; + netdev_features_t changed = ndev->features ^ features; + struct ravb_private *priv = netdev_priv(ndev); + unsigned long flags; + u32 csr0; + int ret; + + spin_lock_irqsave(&priv->lock, flags); + csr0 = ravb_read(ndev, CSR0); + ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0); + ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0); + if (ret) + goto err_wait; + + if (changed & NETIF_F_RXCSUM) { + if (features & NETIF_F_RXCSUM) + ravb_write(ndev, CSR2_ALL, CSR2); + else + ravb_write(ndev, 0, CSR2); + } + + ndev->features = features; +err_wait: + ravb_write(ndev, csr0, CSR0); + spin_unlock_irqrestore(&priv->lock, flags); + + return ret; } static int ravb_set_features_rcar(struct net_device *ndev, @@ -2518,6 +2593,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, .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 Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols. For Rx, the result of checksum calculation is attached to last 4byte of ethernet frames. First 2bytes is result of IPV4 header checksum and next 2 bytes is TCP/UDP/ICMP. If frame does not have error "0000" attached to checksum calculation result. For unsupported frames "ffff" is attached to checksum calculation result. Cases like IPV6, IPV4 header is always set to "FFFF". We can test this functionality by the below commands ethtool -K eth0 rx on --> to turn on Rx checksum offload ethtool -K eth0 rx off --> to turn off Rx checksum offload Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v1->v2: * Fixed sparse warning by replacing __sum16->__wsum. --- drivers/net/ethernet/renesas/ravb.h | 19 ++++++ drivers/net/ethernet/renesas/ravb_main.c | 81 +++++++++++++++++++++++- 2 files changed, 98 insertions(+), 2 deletions(-)