Hi Sergey,
Thanks for the feedback.
> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
>
> On 10/7/21 8:49 AM, Biju Das wrote:
>
> [...]
> >>>>> Fillup ravb_rx_gbeth() function to support RZ/G2L.
> >>>>>
> >>>>> This patch also renames ravb_rcar_rx to ravb_rx_rcar to be
> >>>>> consistent with the naming convention used in sh_eth driver.
> >>>>>
> >>>>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >>>>> Reviewed-by: Lad Prabhakar
> >>>>> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>[...]
> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> index 37164a983156..42573eac82b9 100644
> >>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>>> @@ -720,6 +720,23 @@ static void ravb_get_tx_tstamp(struct
> >>>>> net_device
> >>>> *ndev)
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
> >>>>> + u8 *hw_csum;
> >>>>> +
> >>>>> + /* The hardware checksum is contained in sizeof(__sum16) (2)
> bytes
> >>>>> + * appended to packet data
> >>>>> + */
> >>>>> + if (unlikely(skb->len < sizeof(__sum16)))
> >>>>> + return;
> >>>>> + hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
> >>>>
> >>>> Not 32-bit? The manual says the IP checksum is stored in the
> >>>> first
> >>>> 2 bytes.
> >>>
> >>> It is 16 bit. It is on last 2 bytes.
>
> The IP checksum is at the 1st 2 bytes of the overall 4-byte checksum
> (coming after the packet payload), no?
Sorry, I got confused with your question earlier. Now it is clear for me.
I agree the checksum part is stored in last 4bytes. Of this, the first 2 bytes IPV4 checksum
and last 2 bytes TCP/UDP/ICMP checksum.
>
> >> So you're saying the manual is wrong?
> >
> > I am not sure which manual you are referring here.
> >
> > I am referring to Rev.1.00 Sep, 2021 of RZ/G2L hardware manual and
>
> Same here.
>
> [...]
>
> > Please check the section 30.5.6.1 checksum calculation handling> And
> > figure 30.25 the field of checksum attaching field
>
> I have.
>
> > Also see Table 30.17 for checksum values for non-error conditions.
>
> > TCP/UDP/ICPM checksum is at last 2bytes.
>
> What are you arguing with then? :-)
> My point was that your code fetched the TCP/UDP/ICMP checksum ISO the
> IP checksum because it subtracts sizeof(__sum16), while should probably
> subtract sizeof(__wsum)
Agreed. My code missed IP4 checksum result. May be we need to extract 2 checksum info
from last 4 bytes. First checksum(2bytes) is IP4 header checksum and next checksum(2 bytes) for TCP/UDP/ICMP and use this info finding the non error case mentioned in Table 30.17.
For eg:-
IPV6 non error-condition --> "0xFFFF"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value
IPV4 non error-condition --> "0x0000"-->IPV4HeaderCSum value and "0x0000" TCP/UDP/ICMP CSUM value
Do you agree?
Regards,
Biju
>
> >>>>> +
> >>>>> + if (*hw_csum == 0)
> >>>>
> >>>> You only check the 1st byte, not the full checksum!
> >>>
> >>> As I said earlier, "0" value on last 16 bit, means no checksum error.
> >>
> >> How's that? 'hw_csum' is declared as 'u8 *'!
> >
> > It is my mistake, which will be taken care in the next patch by using
> u16 *.
>
> Note that this 'u16' halfword can be unaligned, that's why the current
> code uses get_unaligned_le16().
>
> >>>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>>>> + else
> >>>>> + skb->ip_summed = CHECKSUM_NONE;
> >>>>
> >>>> So the TCP/UDP/ICMP checksums are not dealt with? Why enable them
> >> then?
> >>>
> >>> If last 2bytes is zero, means there is no checksum error w.r.to
> >> TCP/UDP/ICMP checksums.
> >>
> >> Why checksum them independently then?
> >
> > It is a hardware feature.
>
> Switchable, isn't it?
>
> > Regards,
> > Biju
>
> [...]
>
> MBR, Sergey
