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@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);
> >>> [...]
> >>>>>>> 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
