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

Commit Message

Biju Das Oct. 5, 2021, 11:06 a.m. UTC

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 <[e-mail]>
Reviewed-by: Lad Prabhakar <[e-mail]>
---
RFC changes:
 * renamed "rxtop_skb" to "rx_1st_skb" and moved near to rx_skb.
 * removed parameter q from "ravb_get_skb_gbeth"
 * renamed ravb_rcar_rx to ravb_rx_rcar
---
 drivers/net/ethernet/renesas/ravb.h      |   1 +
 drivers/net/ethernet/renesas/ravb_main.c | 167 ++++++++++++++++++++++-
 2 files changed, 163 insertions(+), 5 deletions(-)

Comments

Sergey Shtylyov Oct. 6, 2021, 7:38 p.m. UTC | #1

On 10/5/21 2:06 PM, 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 <[e-mail]>
> Reviewed-by: Lad Prabhakar <[e-mail]>[...]
> 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.

> +
> +	if (*hw_csum == 0)

   You only check the 1st byte, not the full checksum!

> +		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?

> +}
> +
>  static void ravb_rx_csum(struct sk_buff *skb)

static void ravb_rx_csum_rcar(struct sk_buff *skb)?

[...]

MBR, Sergey

Biju Das Oct. 6, 2021, 8:22 p.m. UTC | #2

Hi Sergey,

Thanks for thefeedback.

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/5/21 2:06 PM, 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 <[e-mail]>
> > Reviewed-by: Lad Prabhakar
> > <[e-mail]>[...]
> > 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.

> 
> > +
> > +	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.

> 
> > +		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.

RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block at all for R-Car Gen3.

Regards,
Biju

> 
> > +}
> > +
> >  static void ravb_rx_csum(struct sk_buff *skb)
> 
> static void ravb_rx_csum_rcar(struct sk_buff *skb)?
> 
> [...]
> 
> MBR, Sergey

Sergey Shtylyov Oct. 6, 2021, 8:32 p.m. UTC | #3

On 10/6/21 11:22 PM, 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 <[e-mail]>
>>> Reviewed-by: Lad Prabhakar
>>> <[e-mail]>[...]
>>> 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.

   So you're saying the manual is wrong?

>>
>>> +
>>> +	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 declaread as 'u8 *'!

>>> +		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?

> RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block at all for R-Car Gen3.
> 
> Regards,
> Biju

[...]

MBR, Sergey

Biju Das Oct. 7, 2021, 5:49 a.m. UTC | #4

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/6/21 11:22 PM, 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 <[e-mail]>
> >>> Reviewed-by: Lad Prabhakar
> >>> <[e-mail]>[...]
> >>> 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.
> 
>    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
I have shared the link[1] for you to download. Hope you are referring same manual


[1] https://www.renesas.com/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en&r=1467981

Please check the section 30.5.6.1 checksum calculation handling
And figure 30.25 the field of checksum attaching field

Also see Table 30.17 for checksum values for non-error conditions.

TCP/UDP/ICPM checksum is at last 2bytes.

> 
> >>
> >>> +
> >>> +	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 *.

> 
> >>> +		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. 

Regards,
Biju

> 
> > RZ/G2L checksum part is different from R-Car Gen3. There is no TOE block
> at all for R-Car Gen3.
> >
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey

Sergey Shtylyov Oct. 7, 2021, 7:04 p.m. UTC | #5

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 <[e-mail]>
>>>>> Reviewed-by: Lad Prabhakar
>>>>> <[e-mail]>[...]
>>>>> 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?

>>    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).

>>>>> +
>>>>> +	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

Biju Das Oct. 7, 2021, 8:09 p.m. UTC | #6

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 <[e-mail]>
> >>>>> Reviewed-by: Lad Prabhakar
> >>>>> <[e-mail]>[...]
> >>>>> 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

Biju Das Oct. 8, 2021, 6:46 a.m. UTC | #7

Hi Sergey,

> Subject: RE: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> 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 <[e-mail]>
> > >>>>> Reviewed-by: Lad Prabhakar
> > >>>>> <[e-mail]>[...]
> > >>>>> 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?

What I meant here is some thing like below, please let me know if you have any issues with
this, otherwise I would like to send the patch with below changes.

Further improvements can happen later.

Please let me know.

+/* Hardware checksum status */
+#define IPV4_RX_CSUM_OK                0x00000000
+#define IPV6_RX_CSUM_OK                0xFFFF0000
+
 enum ravb_reg {
        /* AVB-DMAC registers */
        CCC     = 0x0000,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index bbb42e5328e4..d9201fbbd472 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
 
 static void ravb_rx_csum_gbeth(struct sk_buff *skb)
 {
-       u16 *hw_csum;
+       u32 csum_result;
+       u8 *hw_csum;
 
        /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
         * appended to packet data
         */
-       if (unlikely(skb->len < sizeof(__sum16)))
+       if (unlikely(skb->len < sizeof(__wsum)))
                return;
-       hw_csum = (u16*)(skb_tail_pointer(skb) - sizeof(__sum16));
+       hw_csum = skb_tail_pointer(skb) - sizeof(__wsum);
+       csum_result = get_unaligned_le32(hw_csum);
 
-       if (*hw_csum == 0)
+       if (csum_result == IPV4_RX_CSUM_OK || csum_result == IPV6_RX_CSUM_OK)

Regards,
Biju

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

Sergey Shtylyov Oct. 8, 2021, 5:59 p.m. UTC | #8

On 10/8/21 9:46 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 <[e-mail]>
>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>> <[e-mail]>[...]
>>>>>>>> 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);
[...]
>>>> 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?

> What I meant here is some thing like below, please let me know if you have any issues with
> this, otherwise I would like to send the patch with below changes.
> 
> Further improvements can happen later.
> 
> Please let me know.
> 
> +/* Hardware checksum status */
> +#define IPV4_RX_CSUM_OK                0x00000000
> +#define IPV6_RX_CSUM_OK                0xFFFF0000

   Mhm, this should prolly come from the IP headers...

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index bbb42e5328e4..d9201fbbd472 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>  
>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>  {
> -       u16 *hw_csum;
> +       u32 csum_result;

   This is not against the patch currently under investigation. :-)

> +       u8 *hw_csum;
>  
>         /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>          * appended to packet data
>          */
> -       if (unlikely(skb->len < sizeof(__sum16)))
> +       if (unlikely(skb->len < sizeof(__wsum)))

   I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here
covered by that, not a 32-bit sum...

>                 return;
> -       hw_csum = (u16*)(skb_tail_pointer(skb) - sizeof(__sum16));
> +       hw_csum = skb_tail_pointer(skb) - sizeof(__wsum);
> +       csum_result = get_unaligned_le32(hw_csum);
>  
> -       if (*hw_csum == 0)
> +       if (csum_result == IPV4_RX_CSUM_OK || csum_result == IPV6_RX_CSUM_OK)

   I don't think there's a hard-and-fast way to differentiate the valid packet just from
the 2 16-bit checksums...

[...]
>>>>>>>> +
>>>>>>>> +	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 *.

   That won't do it, I'm afraid...

   From an IRC discuassion on IRC we concluded that we don't need to check the checksum's
value, we just need to store it for the upper layers to catch the invalid sums...

[...]

MBR, Sergey

Sergey Shtylyov Oct. 8, 2021, 8:13 p.m. UTC | #9

On 10/8/21 8:59 PM, Sergey Shtylyov wrote:
> On 10/8/21 9:46 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 <[e-mail]>
>>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>>> <[e-mail]>[...]
>>>>>>>>> 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);
> [...]
>>>>> 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?
> 
>> What I meant here is some thing like below, please let me know if you have any issues with
>> this, otherwise I would like to send the patch with below changes.
>>
>> Further improvements can happen later.
>>
>> Please let me know.
>>
>> +/* Hardware checksum status */
>> +#define IPV4_RX_CSUM_OK                0x00000000
>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> 
>    Mhm, this should prolly come from the IP headers...
> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index bbb42e5328e4..d9201fbbd472 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct net_device *ndev)
>>  
>>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)
>>  {
>> -       u16 *hw_csum;
>> +       u32 csum_result;
> 
>    This is not against the patch currently under investigation. :-)
> 
>> +       u8 *hw_csum;
>>  
>>         /* The hardware checksum is contained in sizeof(__sum16) (2) bytes
>>          * appended to packet data
>>          */
>> -       if (unlikely(skb->len < sizeof(__sum16)))
>> +       if (unlikely(skb->len < sizeof(__wsum)))
> 
>    I think this usage of __wsum is valid (I remember that I suggested it). We have 2 16-bit checksums here

   I meant "I don't think", of course. :-)

[...]

MBR, Sergey

Biju Das Oct. 9, 2021, 8:27 a.m. UTC | #10

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 10/8/21 8:59 PM, Sergey Shtylyov wrote:
> > On 10/8/21 9:46 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 <[e-mail]>
> >>>>>>>>> Reviewed-by: Lad Prabhakar
> >>>>>>>>> <[e-mail]>[...]
> >>>>>>>>> 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);
> > [...]
> >>>>> 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?
> >
> >> What I meant here is some thing like below, please let me know if you
> >> have any issues with this, otherwise I would like to send the patch
> with below changes.
> >>
> >> Further improvements can happen later.
> >>
> >> Please let me know.
> >>
> >> +/* Hardware checksum status */
> >> +#define IPV4_RX_CSUM_OK                0x00000000
> >> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> >
> >    Mhm, this should prolly come from the IP headers...
> >
> > [...]
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >> b/drivers/net/ethernet/renesas/ravb_main.c
> >> index bbb42e5328e4..d9201fbbd472 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
> >> net_device *ndev)
> >>
> >>  static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
> >> -       u16 *hw_csum;
> >> +       u32 csum_result;
> >
> >    This is not against the patch currently under investigation. :-)
> >
> >> +       u8 *hw_csum;
> >>
> >>         /* The hardware checksum is contained in sizeof(__sum16) (2)
> bytes
> >>          * appended to packet data
> >>          */
> >> -       if (unlikely(skb->len < sizeof(__sum16)))
> >> +       if (unlikely(skb->len < sizeof(__wsum)))
> >
> >    I think this usage of __wsum is valid (I remember that I suggested
> it). We have 2 16-bit checksums here
> 
>    I meant "I don't think", of course. :-)

Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.

All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
and only non-error cases will be set as CHECKSUM_UNNCESSARY.

Does it sounds good to you?

Regards,
Biju



> 
> [...]
> 
> MBR, Sergey

Sergey Shtylyov Oct. 9, 2021, 8:34 a.m. UTC | #11

On 09.10.2021 11:27, 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 <[e-mail]>
>>>>>>>>>>> Reviewed-by: Lad Prabhakar
>>>>>>>>>>> <[e-mail]>[...]
>>>>>>>>>>> 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);
>>> [...]
>>>>>>> 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?
>>>
>>>> What I meant here is some thing like below, please let me know if you
>>>> have any issues with this, otherwise I would like to send the patch
>> with below changes.
>>>>
>>>> Further improvements can happen later.
>>>>
>>>> Please let me know.
>>>>
>>>> +/* Hardware checksum status */
>>>> +#define IPV4_RX_CSUM_OK                0x00000000
>>>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
>>>
>>>     Mhm, this should prolly come from the IP headers...
>>>
>>> [...]
>>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>>> index bbb42e5328e4..d9201fbbd472 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
>>>> net_device *ndev)
>>>>
>>>>   static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
>>>> -       u16 *hw_csum;
>>>> +       u32 csum_result;
>>>
>>>     This is not against the patch currently under investigation. :-)
>>>
>>>> +       u8 *hw_csum;
>>>>
>>>>          /* The hardware checksum is contained in sizeof(__sum16) (2)
>> bytes
>>>>           * appended to packet data
>>>>           */
>>>> -       if (unlikely(skb->len < sizeof(__sum16)))
>>>> +       if (unlikely(skb->len < sizeof(__wsum)))
>>>
>>>     I think this usage of __wsum is valid (I remember that I suggested
>> it). We have 2 16-bit checksums here
>>
>>     I meant "I don't think", of course. :-)
> 
> Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and TCP/UDP/ICMP csum result.

    I'm not sure how to deal with the later...

> All error condition/unsupported cases will be passed to stack with CHECKSUM_NONE
> and only non-error cases will be set as CHECKSUM_UNNCESSARY.
> 
> Does it sounds good to you?

    No. The networking stack needs to know about the bad checksums too.

> Regards,
> Biju

>> [...]

MBR, Sergey

Biju Das Oct. 9, 2021, 9:41 a.m. UTC | #12

Hi Sergey,

> Subject: Re: [RFC 07/12] ravb: Fillup ravb_rx_gbeth() stub
> 
> On 09.10.2021 11:27, 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 <[e-mail]>
> >>>>>>>>>>> Reviewed-by: Lad Prabhakar
> >>>>>>>>>>> <[e-mail]>[...]
> >>>>>>>>>>> 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);
> >>> [...]
> >>>>>>> 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?
> >>>
> >>>> What I meant here is some thing like below, please let me know if
> >>>> you have any issues with this, otherwise I would like to send the
> >>>> patch
> >> with below changes.
> >>>>
> >>>> Further improvements can happen later.
> >>>>
> >>>> Please let me know.
> >>>>
> >>>> +/* Hardware checksum status */
> >>>> +#define IPV4_RX_CSUM_OK                0x00000000
> >>>> +#define IPV6_RX_CSUM_OK                0xFFFF0000
> >>>
> >>>     Mhm, this should prolly come from the IP headers...
> >>>
> >>> [...]
> >>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> >>>> b/drivers/net/ethernet/renesas/ravb_main.c
> >>>> index bbb42e5328e4..d9201fbbd472 100644
> >>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >>>> @@ -722,16 +722,18 @@ static void ravb_get_tx_tstamp(struct
> >>>> net_device *ndev)
> >>>>
> >>>>   static void ravb_rx_csum_gbeth(struct sk_buff *skb)  {
> >>>> -       u16 *hw_csum;
> >>>> +       u32 csum_result;
> >>>
> >>>     This is not against the patch currently under investigation. :-)
> >>>
> >>>> +       u8 *hw_csum;
> >>>>
> >>>>          /* The hardware checksum is contained in sizeof(__sum16)
> >>>> (2)
> >> bytes
> >>>>           * appended to packet data
> >>>>           */
> >>>> -       if (unlikely(skb->len < sizeof(__sum16)))
> >>>> +       if (unlikely(skb->len < sizeof(__wsum)))
> >>>
> >>>     I think this usage of __wsum is valid (I remember that I
> >>> suggested
> >> it). We have 2 16-bit checksums here
> >>
> >>     I meant "I don't think", of course. :-)
> >
> > Ok will use 2 * sizeof(__sum16) instead and extract IPV4 header csum and
> TCP/UDP/ICMP csum result.
> 
>     I'm not sure how to deal with the later...
> 
> > All error condition/unsupported cases will be passed to stack with
> > CHECKSUM_NONE and only non-error cases will be set as
> CHECKSUM_UNNCESSARY.

> >
> > Does it sounds good to you?
> 
>     No. The networking stack needs to know about the bad checksums too.

Currently some of the drivers is doing this way only. It doesn't pass bad checksum.
Non-error case sets CHECKSUM_UNNCESSARY and other case sets CHECKSUM_NONE to handle
It by stack.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-mac.c#L1147
[2] https://elixir.bootlin.com/linux/latest/source/drivers/staging/octeon/ethernet-rx.c#L343

Regards,
Biju

> 
> > Regards,
> > Biju
> 
> >> [...]
> 
> MBR, Sergey

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 35710da808a6..8c7b2569c7dd 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1081,6 +1081,7 @@  struct ravb_private {
 	struct ravb_ex_rx_desc *rx_ring[NUM_RX_QUEUE];
 	struct ravb_tx_desc *tx_ring[NUM_TX_QUEUE];
 	void *tx_align[NUM_TX_QUEUE];
+	struct sk_buff *rx_1st_skb;
 	struct sk_buff **rx_skb[NUM_RX_QUEUE];
 	struct sk_buff **tx_skb[NUM_TX_QUEUE];
 	u32 rx_over_errors;
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);
+
+	if (*hw_csum == 0)
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+	else
+		skb->ip_summed = CHECKSUM_NONE;
+}
+
 static void ravb_rx_csum(struct sk_buff *skb)
 {
 	u8 *hw_csum;
@@ -735,15 +752,155 @@  static void ravb_rx_csum(struct sk_buff *skb)
 	skb_trim(skb, skb->len - sizeof(__sum16));
 }
 
+static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
+					  struct ravb_rx_desc *desc)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct sk_buff *skb;
+
+	skb = priv->rx_skb[RAVB_BE][entry];
+	priv->rx_skb[RAVB_BE][entry] = NULL;
+	dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
+			 ALIGN(GBETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
+
+	return skb;
+}
+
 /* Packet receive function for Gigabit Ethernet */
 static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 {
-	/* Place holder */
-	return true;
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+	struct net_device_stats *stats;
+	struct ravb_rx_desc *desc;
+	struct sk_buff *skb;
+	dma_addr_t dma_addr;
+	u8  desc_status;
+	int boguscnt;
+	u16 pkt_len;
+	u8  die_dt;
+	int entry;
+	int limit;
+
+	entry = priv->cur_rx[q] % priv->num_rx_ring[q];
+	boguscnt = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
+	stats = &priv->stats[q];
+
+	boguscnt = min(boguscnt, *quota);
+	limit = boguscnt;
+	desc = &priv->gbeth_rx_ring[entry];
+	while (desc->die_dt != DT_FEMPTY) {
+		/* Descriptor type must be checked before all other reads */
+		dma_rmb();
+		desc_status = desc->msc;
+		pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
+
+		if (--boguscnt < 0)
+			break;
+
+		/* We use 0-byte descriptors to mark the DMA mapping errors */
+		if (!pkt_len)
+			continue;
+
+		if (desc_status & MSC_MC)
+			stats->multicast++;
+
+		if (desc_status & (MSC_CRC | MSC_RFE | MSC_RTSF | MSC_RTLF | MSC_CEEF)) {
+			stats->rx_errors++;
+			if (desc_status & MSC_CRC)
+				stats->rx_crc_errors++;
+			if (desc_status & MSC_RFE)
+				stats->rx_frame_errors++;
+			if (desc_status & (MSC_RTLF | MSC_RTSF))
+				stats->rx_length_errors++;
+			if (desc_status & MSC_CEEF)
+				stats->rx_missed_errors++;
+		} else {
+			die_dt = desc->die_dt & 0xF0;
+			switch (die_dt) {
+			case DT_FSINGLE:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_put(skb, pkt_len);
+				skb->protocol = eth_type_trans(skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_gbeth(skb);
+				napi_gro_receive(&priv->napi[q], skb);
+				stats->rx_packets++;
+				stats->rx_bytes += pkt_len;
+				break;
+			case DT_FSTART:
+				priv->rx_1st_skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				break;
+			case DT_FMID:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
+							       priv->rx_1st_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				dev_kfree_skb(skb);
+				break;
+			case DT_FEND:
+				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
+							       priv->rx_1st_skb->len,
+							       skb->data,
+							       pkt_len);
+				skb_put(priv->rx_1st_skb, pkt_len);
+				dev_kfree_skb(skb);
+				priv->rx_1st_skb->protocol =
+					eth_type_trans(priv->rx_1st_skb, ndev);
+				if (ndev->features & NETIF_F_RXCSUM)
+					ravb_rx_csum_gbeth(skb);
+				napi_gro_receive(&priv->napi[q],
+						 priv->rx_1st_skb);
+				stats->rx_packets++;
+				stats->rx_bytes += priv->rx_1st_skb->len;
+				break;
+			}
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->gbeth_rx_ring[entry];
+	}
+
+	/* Refill the RX ring buffers. */
+	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
+		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->gbeth_rx_ring[entry];
+		desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
+
+		if (!priv->rx_skb[q][entry]) {
+			skb = netdev_alloc_skb(ndev, info->max_rx_len);
+			if (!skb)
+				break;
+			ravb_set_buffer_align(skb);
+			dma_addr = dma_map_single(ndev->dev.parent,
+						  skb->data,
+						  GBETH_RX_BUFF_MAX,
+						  DMA_FROM_DEVICE);
+			skb_checksum_none_assert(skb);
+			/* We just set the data size to 0 for a failed mapping
+			 * which should prevent DMA  from happening...
+			 */
+			if (dma_mapping_error(ndev->dev.parent, dma_addr))
+				desc->ds_cc = cpu_to_le16(0);
+			desc->dptr = cpu_to_le32(dma_addr);
+			priv->rx_skb[q][entry] = skb;
+		}
+		/* Descriptor type must be set after all the above writes */
+		dma_wmb();
+		desc->die_dt = DT_FEMPTY;
+	}
+
+	*quota -= limit - (++boguscnt);
+
+	return boguscnt <= 0;
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rcar_rx(struct net_device *ndev, int *quota, int q)
+static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
@@ -2269,7 +2426,7 @@  static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
-	.receive = ravb_rcar_rx,
+	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
 	.dmac_init = ravb_dmac_init_rcar,
@@ -2294,7 +2451,7 @@  static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.rx_ring_free = ravb_rx_ring_free_rcar,
 	.rx_ring_format = ravb_rx_ring_format_rcar,
 	.alloc_rx_desc = ravb_alloc_rx_desc_rcar,
-	.receive = ravb_rcar_rx,
+	.receive = ravb_rx_rcar,
 	.set_rate = ravb_set_rate_rcar,
 	.set_feature = ravb_set_features_rcar,
 	.dmac_init = ravb_dmac_init_rcar,