RE: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support

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

Hi Sergey Shtylyov,

Thanks for the feedback.

> >>    Don't we need to set skb->csum_level?
> >
> > As per my knowledge, it is not needed. I may be wrong. Why do you
> > think it is needed?
> 
>  *   CHECKSUM_UNNECESSARY is applicable to following protocols:
>  *     TCP: IPv6 and IPv4.
>  *     UDP: IPv4 and IPv6. A device may apply CHECKSUM_UNNECESSARY to a
>  *       zero UDP checksum for either IPv4 or IPv6, the networking stack
>  *       may perform further validation in this case.
>  *     GRE: only if the checksum is present in the header.
>  *     SCTP: indicates the CRC in SCTP header has been validated.
>  *     FCOE: indicates the CRC in FC frame has been validated.
>  *
>  *   skb->csum_level indicates the number of consecutive checksums found
> in
>  *   the packet minus one that have been verified as CHECKSUM_UNNECESSARY.
>  *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
>  *   and a device is able to verify the checksums for UDP (possibly zero),
>  *   GRE (checksum flag is set) and TCP, skb->csum_level would be set to
>  *   two. If the device were only able to verify the UDP checksum and not
>  *   GRE, either because it doesn't support GRE checksum or because GRE
>  *   checksum is bad, skb->csum_level would be set to zero (TCP checksum
> is
>  *   not considered in this case).
> 
>    It would seem we should set this field to 1 if the TCP/UDP checksum was
> successfully verified?

I guess it is for encapsulated packets. For just IP and UDP/TCP
Checksum it is not required.

See

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/3com/3c59x.c#L2669
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/amazon/ena/ena_netdev.c#L1626
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/atheros/alx/main.c#L272
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/am65-cpsw-nuss.c#L711

> 
> >> [...]
> >>> @@ -2518,6 +2593,8 @@ static const struct ravb_hw_info gbeth_hw_info =
> {
> >>>  	.emac_init = ravb_emac_init_gbeth,
> >>>  	.gstrings_stats = ravb_gstrings_stats_gbeth,
> >>>  	.gstrings_size = sizeof(ravb_gstrings_stats_gbeth),
> >>> +	.net_hw_features = NETIF_F_RXCSUM,
> >>> +	.net_features = NETIF_F_RXCSUM,
> >>
> >>    What about NETIF_F_IP_CSUM, BTW?
> >
> > Why is it needed? Can you please clarify?
> 
>    Ignore me -- this one seems to be used for the TX path.
>    I just had to learn how the checksum offloading works while reviewing
> your patches... :-)

OK.

For other comments. I will test and respond.

Cheers,
Biju