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@xxxxxx>
>> 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@xxxxxxxxxxxxxx>
>> [...]
>>
>>> 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
