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@xxxxxxxxxxxxxx>
> > ---
> > 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
