Hi Sergey Shtylyov, Thanks for the feedback. > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@xxxxxx> > 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@xxxxxxxxxxxxxx> > [...] > > > 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