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.

> -----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
> 
> On 1/24/24 1:21 PM, Biju Das wrote:
> 
> > TOE has hw support for calculating IP header and TCP/UDP/ICMP checksum
> 
>    s/hw/hardware/.

Ok.

> 
> > for both IPV4 and IPV6.
> 
>    Those are usually called IPv4 and IPv6, no?
Agreed.
> 
> > Add Rx checksum offload supported by TOE for IPV4 and TCP/UDP protocols.
> >
> > For Rx, the result of checksum calculation is attached to last 4byte
> > of ethernet frames.
> 
>    "For Rx, the 4-byte result of checksum calculation is attached to the
> Ethernet frames", you wanted to say?

Yes.

> 
> > First 2bytes is result of IPV4 header checksum and next 2 bytes is
> > TCP/UDP/ICMP.
> 
>    TCP/UDP/ICMP checksum, you mean? Also, you alternatively say
> TCP/UDP/ICMP and just TCP/UDP -- which one is correct?


As per the hardware manual, it supports TCP/UDP/ICMP checksum.
So you are correct, it is TCP/UDP/ICMP checksum.

> 
> > If frame does not have error "0000" attached to checksum calculation
> 
>    "If a frame does not have checksum error, 0x0000 is attached as a
> checksum calculation result", you wanted to say?

Ok.

> 
> > result. For unsupported frames "ffff" is attached to checksum
> > calculation
> 
>    s/to/as/?

Correct.

> 
> > result. Cases like IPV6, IPV4 header is always set to "FFFF".
> 
>    "In case of an IPv6 packet, IPv4 checksum is always set to 0xFFFF", you
> wanted to say?

Correct.

> 
> > 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.

> 
> [...]
> > 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
> > @@ -522,6 +522,26 @@ static int ravb_ring_init(struct net_device *ndev,
> int q)
> >  	return -ENOMEM;
> >  }
> >
> > +static void ravb_csum_offload_init_gbeth(struct net_device *ndev)
> 
>    I'd leave out _offload...

Ok.

> 
> > +{
> > +	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.

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

See above.

> 
> > +	if (ravb_wait(ndev, CSR0, CSR0_RPE | CSR0_TPE, 0)) {
> > +		netdev_err(ndev, "Timeout Enabling HW CSUM failed\n");
> 
>    "Timeout enabling hardware checksum\n", perhaps?

OK.

> 
> [...]
> > +
> > +	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.

> 
> > +}
> > +
> [...]
> > @@ -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?

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

> 
> > +
> > +	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??


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

Cheers,
Biju