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

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 1/26/24 1:15 AM, Biju Das wrote:

> Hi Sergey Shtylyov,

   Hi! :-)

> Thanks for the feedback.

   Not at all!

>> -----Original Message-----
>> From: Sergey Shtylyov <s.shtylyov@omp.ru>
>> Sent: Thursday, January 25, 2024 8:42 PM
>> Subject: Re: [PATCH net-next v2 1/2] ravb: Add Rx checksum offload support

[...]

>>> We can test this functionality by the below commands
>>>
>>> ethtool -K eth0 rx on --> to turn on Rx checksum offload ethtool -K
>>> eth0 rx off --> to turn off Rx checksum offload
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>> b/drivers/net/ethernet/renesas/ravb.h
>>> index e0f8276cffed..a2c494a85d12 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -44,6 +44,9 @@
>>>  #define RAVB_RXTSTAMP_TYPE_ALL	0x00000006
>>>  #define RAVB_RXTSTAMP_ENABLED	0x00000010	/* Enable RX timestamping
>> */
>>>
>>> +/* GbEthernet TOE hardware checksum values */
>>> +#define TOE_RX_CSUM_OK	0x0000
>>
>>    As I said before, this is hardly needed...
> 
> It is needed to match with the Checksum status as mentioned in the hardware manual.

   I think you can just compare to 0... ISTR that checksumming result
should indeed be 0 for a good IP header, so this # does not seem device
specific...

>> [...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index 0e3731f50fc2..59c7bedacef6 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
>>> +{
>>> +	bool rx_enable = ndev->features & NETIF_F_RXCSUM;
>>> +	u32 csr0;
>>> +
>>> +	if (!rx_enable)
>>> +		return;
>>> +
>>> +	csr0 = ravb_read(ndev, CSR0);
>>
>>    Why read it here, if we'll write a constant to this reg at the end of
>> ravb_emac_init_gbeth()?
> 
> The correct flow is
> 
> Disable tx/rx
> Enable Checksum
> Reenable Tx/rx if it is already enabled.

   Yeah, I figured. :-) However I can't find this flow in the RZ/G2L[C]
user's manual...

>>> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
>>
>>    We can just write 0 here, no?
> 
> See above.

   I have to repeat: either don't do read/modife/write CSR0 accesses here
or remove the below line from ravb_emac_init_gbeth():

	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);

   Well, I think this line should be removed in any case -- we shouldn't
enable RX/TX checksumming in CSR0 if we don't also set up CSR1/2...

[...]
>>> +
>>> +	ravb_write(ndev, csr0, CSR0);
>>
>>     I think we should move:
>>
>> 	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
>>
>> from ravb_emac_init_gbeth() here...
> 
> I am providing flexible options here.

   I don't get it... what flexibility do you mean?

[...]
>>> @@ -734,6 +755,32 @@ static void ravb_get_tx_tstamp(struct net_device
>> *ndev)
>>>  	}
>>>  }
>>>
>>> +static void ravb_rx_csum_gbeth(struct sk_buff *skb) {
>>> +	__wsum csum_ip_hdr, csum_proto;
>>> +	u8 *hw_csum;
>>> +
>>> +	/* The hardware checksum status is contained in sizeof(__sum16) * 2
>> = 4
>>> +	 * bytes appended to packet data. First 2 bytes is ip header csum
>> and
>>> +	 * last 2 bytes is protocol csum.
>>> +	 */
>>> +	if (unlikely(skb->len < sizeof(__sum16) * 2))
>>> +		return;
>>> +
>>> +	hw_csum = skb_tail_pointer(skb) - sizeof(__sum16);
>>> +	csum_proto = csum_unfold((__force
>>> +__sum16)get_unaligned_le16(hw_csum));
>>> +
>>> +	hw_csum -= sizeof(__sum16);
>>> +	csum_ip_hdr = csum_unfold((__force
>> __sum16)get_unaligned_le16(hw_csum));
>>> +	skb_trim(skb, skb->len - 2 * sizeof(__sum16));
>>> +
>>> +	/* TODO: IPV6 Rx csum */
>>> +	if (skb->protocol == htons(ETH_P_IP) && csum_ip_hdr ==
>> TOE_RX_CSUM_OK &&
>>> +	    csum_proto == TOE_RX_CSUM_OK)
>>> +		/* Hardware validated our checksum */
>>> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>>    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?

>> [...]
>>> @@ -2337,8 +2388,32 @@ static void ravb_set_rx_csum(struct net_device
>>> *ndev, bool enable)  static int ravb_set_features_gbeth(struct
>> net_device *ndev,
>>>  				   netdev_features_t features)
>>>  {
>>> -	/* Place holder */
>>> -	return 0;
>>> +	netdev_features_t changed = ndev->features ^ features;
>>> +	struct ravb_private *priv = netdev_priv(ndev);
>>> +	unsigned long flags;
>>> +	u32 csr0;
>>> +	int ret;
>>> +
>>> +	spin_lock_irqsave(&priv->lock, flags);
>>> +	csr0 = ravb_read(ndev, CSR0);
>>> +	ravb_write(ndev, csr0 & ~(CSR0_RPE | CSR0_TPE), CSR0);
>>> +	ret = ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0);
>>> +	if (ret)
>>> +		goto err_wait;
>>
>>    I don't understand: why do you clear the CSR0 bits even if (changed &
>> NETIF_F_RXCSUM) is 0? This looks very wrong...
> 
> I made the code simple. Can you please suggest a much simpler way than this?

   Of course, I can. I don't think clearing CSR0.TPE makes sense if you
only modify CSR2, and clearing CSR0.RPE makes sense if you only modify CSR1...

>>> +
>>> +	if (changed & NETIF_F_RXCSUM) {
>>> +		if (features & NETIF_F_RXCSUM)
>>> +			ravb_write(ndev, CSR2_ALL, CSR2);
>>> +		else
>>> +			ravb_write(ndev, 0, CSR2);
>>> +	}
>>
>>    I think you should put that into a separate function, like is done for
>> the EhterAVB...
> 
> you mean add this if else block to separate function?? Can you please elaborate??

   No, you need to 1st clear CSR0.{RPE|TPE}, then set up CSR1/2, then restore
CSR0... something like that.

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

> Cheers,
> Biju

MBR, Sergey