RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux