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

[Biju Das <biju.das.jz@bp.renesas.com>]

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 <biju.das.jz@bp.renesas.com>
> >>>>>>>>>>> Reviewed-by: Lad Prabhakar
> >>>>>>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com>[...]
> >>>>>>>>>>> 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