> -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@xxxxxx> > Sent: Friday, February 2, 2024 7:12 PM > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; David S. Miller > <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski > <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx> > Cc: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>; Yoshihiro Shimoda > <yoshihiro.shimoda.uh@xxxxxxxxxxx>; Wolfram Sang <wsa+renesas@sang- > engineering.com>; nikita.yoush <nikita.yoush@xxxxxxxxxxxxxxxxxx>; > netdev@xxxxxxxxxxxxxxx; linux-renesas-soc@xxxxxxxxxxxxxxx; Geert > Uytterhoeven <geert+renesas@xxxxxxxxx>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>; biju.das.au > <biju.das.au@xxxxxxxxx> > Subject: Re: [PATCH v3 net-next 1/2] ravb: Add Rx checksum offload support > for GbEth > > 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 Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols. > > > > For Rx, the 4-byte result of checksum calculation is attached to the > > Ethernet frames.First 2-bytes is result of IPv4 header checksum and > > next 2-bytes is TCP/UDP/ICMP checksum. > > > > If a frame does not have checksum error, 0x0000 is attached as > > checksum calculation result. For unsupported frames 0xFFFF is attached > > as checksum calculation result. In case of an IPv6 packet, IPv4 > > checksum is always set to 0xFFFF. > > > > 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..a7fe9d8b6b41 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, > > Would be good to add this comment here: > > /* TOE registers */ OK will change as below, so that we don't need to repeat /* RZ/G2L only */ + + /* RZ/G2L TOE registers */ + CSR0 = 0x0800, + CSR2 = 0x0808, > > > CSR0 = 0x0800, /* RZ/G2L only */ > > + CSR2 = 0x0808, /* RZ/G2L only */ > > }; > > > > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 0e3731f50fc2..c4dc6ec54287 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -522,6 +522,23 @@ static int ravb_ring_init(struct net_device *ndev, > int q) > > return -ENOMEM; > > } > > > > +static void ravb_csum_init_gbeth(struct net_device *ndev) { > > + if (!(ndev->features & NETIF_F_RXCSUM)) > > + goto done; > > + > > + ravb_write(ndev, 0, CSR0); > > + if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) { > > + netdev_err(ndev, "Timeout enabling hardware checksum\n"); > > + ndev->features &= ~NETIF_F_RXCSUM; > > + } else { > > + ravb_write(ndev, CSR2_ALL, CSR2); > > Does it make sense to set the IPv6 specific bits if we don't yet > support IPv6 checksumming anyways? OK will drop IPv6 bits now and will add when we enable IPv6. > > > + } > > + > > +done: > > + ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0); } > > + > > static void ravb_emac_init_gbeth(struct net_device *ndev) { > > struct ravb_private *priv = netdev_priv(ndev); > [...] > > @@ -734,6 +752,31 @@ 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. > > Hm, maybe spell out csum as checksum? OK. > > > + */ > > + 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 && !csum_proto) > > + /* Hardware validated our checksum */ > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > I think you need {} because of the comment (but would be good without > it as well)... OK, I will drop the unnecessary comment "Hardware validated our checksum" to avoid confusion related to multi-line comment. > > [...] > > @@ -2334,11 +2381,49 @@ static void ravb_set_rx_csum(struct net_device > *ndev, bool enable) > > spin_unlock_irqrestore(&priv->lock, flags); } > > > > +static int ravb_endisable_csum_gbeth(struct net_device *ndev, enum > ravb_reg reg, > > + u32 val, u32 mask) > > I'd suggest to mimic ravb_wait() with the the mask param followed by > the val[ue] param... > > > +{ > > + u32 csr0; > > + int ret; > > + > > + csr0 = ravb_read(ndev, CSR0); > > Hm... I think we already know CSR0's value as ravb_csum_init_gbeth() > sets it to (CSR0_TPE | CSR0_RPE) on exit... OK will drop register read. > > > + ravb_write(ndev, csr0 & ~mask, CSR0); > > + ret = ravb_wait(ndev, CSR0, mask, 0); > > + if (!ret) > > + ravb_write(ndev, val, reg); > > + > > + ravb_write(ndev, csr0, CSR0); > > + > > + return ret; > > +} > > + > > 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; > > + int ret = 0; > > + u32 val; > > + > > + spin_lock_irqsave(&priv->lock, flags); > > + if (changed & NETIF_F_RXCSUM) { > > + if (features & NETIF_F_RXCSUM) > > + val = CSR2_ALL; > > Again, does it make sense to set the IPv6 bits in CSR2? OK will drop it. I am agreeing to all the comments and will send v4. Cheers, Biju > > > + else > > + val = 0; > > + > > + ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE); > > + if (ret) > > + goto done; > > + } > > + > > + ndev->features = features; > > +done: > > + spin_unlock_irqrestore(&priv->lock, flags); > > + > > + return ret; > > } > > > > static int ravb_set_features_rcar(struct net_device *ndev, > [...] > > MBR, Sergey