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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Sergey Shtylyov <s.shtylyov@omp.ru>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>,
	nikita.yoush <nikita.yoush@cogentembedded.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	biju.das.au <biju.das.au@gmail.com>
Subject: RE: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support
Date: Tue, 30 Jan 2024 17:00:09 +0000	[thread overview]
Message-ID: <TYCPR01MB112695FD89B16249FD803D1E7867D2@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <05cbba41-2927-e4fd-cb00-d3f40d92830f@omp.ru>

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