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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sergey Shtylyov,

Thanks for the feedback.

> -----Original Message-----
> From: Sergey Shtylyov <s.shtylyov@xxxxxx>
> Sent: Monday, February 5, 2024 7:01 PM
> Subject: Re: [PATCH v4 net-next 1/2] ravb: Add Rx checksum offload support
> for GbEth
> 
> On 2/3/24 5:25 PM, Biju Das wrote:
> 
> > TOE has hardware support for calculating IP header and TCP/UDP/ICMP
> > checksum for both IPv4 and IPv6.
> >
> > Add Rx checksum offload supported by TOE for IPv4 and TCP/UDP protocols.
> >
> > For Rx, the 4-byte result of checksum calculation is attached to the
> > Ethernet frames.First 2-bytes is result of IPv4 header checksum and
> > next 2-bytes is TCP/UDP/ICMP checksum.
> >
> > If a frame does not have checksum error, 0x0000 is attached as
> > checksum calculation result. For unsupported frames 0xFFFF is attached
> > as checksum calculation result. In case of an IPv6 packet, IPv4
> > checksum is always set to 0xFFFF.
> >
> > 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..64bf29d01ad0 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -205,7 +205,10 @@ enum ravb_reg {
> >  	TLFRCR	= 0x0758,
> >  	RFCR	= 0x0760,
> >  	MAFCR	= 0x0778,
> > -	CSR0    = 0x0800,	/* RZ/G2L only */
> > +
> > +	/* RZ/G2L TOE registers */
> 
>    Thanks. Though I think I'd prefer /* TOE registers (RZ/G2L only) */...

OK.

> 
> [...]
> > @@ -978,6 +981,21 @@ enum CSR0_BIT {
> >  	CSR0_RPE	= 0x00000020,
> >  };
> >
> > +enum CSR2_BIT {
> > +	CSR2_RIP4	= 0x00000001,
> > +	CSR2_RTCP4	= 0x00000010,
> > +	CSR2_RUDP4	= 0x00000020,
> > +	CSR2_RICMP4	= 0x00000040,
> > +	CSR2_RTCP6	= 0x00100000,
> > +	CSR2_RUDP6	= 0x00200000,
> > +	CSR2_RICMP6	= 0x00400000,
> > +	CSR2_RHOP	= 0x01000000,
> > +	CSR2_RROUT	= 0x02000000,
> > +	CSR2_RAHD	= 0x04000000,
> > +	CSR2_RDHD	= 0x08000000,
> > +	CSR2_ALL	= 0x0F700071,
> 
>   I doubt we really need CSR2_ALL...

Will remove it.
> 
> [...]
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > b/drivers/net/ethernet/renesas/ravb_main.c
> > index 0e3731f50fc2..4f310bcee7c0 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -522,6 +522,24 @@ static int ravb_ring_init(struct net_device *ndev,
> int q)
> >  	return -ENOMEM;
> >  }
> >
> > +static void ravb_csum_init_gbeth(struct net_device *ndev) {
> > +	if (!(ndev->features & NETIF_F_RXCSUM))
> > +		goto done;
> > +
> > +	ravb_write(ndev, 0, CSR0);
> > +	if (ravb_wait(ndev, CSR0, CSR0_RPE, 0)) {
> > +		netdev_err(ndev, "Timeout enabling hardware checksum\n");
> > +		ndev->features &= ~NETIF_F_RXCSUM;
> > +	} else {
> > +		ravb_write(ndev, CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
> > +					      CSR2_RICMP6), CSR2);
> 
>    With these 3 bits being 0, the bits 24...27 are ignored anyway, the
> manual says. So I think I'd prefer:
> 
> 		ravb_write(ndev, CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 |
> CSR2_RICMP4,
> 			   CSR2);

OK.

> 
> > +	}
> > +
> > +done:
> > +	ravb_write(ndev, CSR0_TPE | CSR0_RPE, CSR0);
> 
>    I think we shouldn't set CSR0.TPE yet at this point, as we n't setup
> CSR1 yet...

This will cause regression. As board won't boot as tx fails. So CSR0.{TPE,RPE}
is must.

> 
> [...]
> > @@ -2334,11 +2381,48 @@ 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;
> > +	int ret = 0;
> > +	u32 val;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	if (changed & NETIF_F_RXCSUM) {
> > +		if (features & NETIF_F_RXCSUM)
> > +			val = CSR2_ALL & ~(CSR2_RTCP6 | CSR2_RUDP6 |
> CSR2_RICMP6);
> 
>    Likewise, I'd prefer:
> 
> 			val = CSR2_RIP4 | CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4;

Ok.

> 
> > +		else
> > +			val = 0;
> > +
> > +		ret = ravb_endisable_csum_gbeth(ndev, CSR2, val, CSR0_RPE);
> > +		if (ret)
> > +			goto done;
> > +	}
> > +
> > +	ndev->features = features;
> > +done:
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return ret;
> >  }
> >
> >  static int ravb_set_features_rcar(struct net_device *ndev,
> [...]
> 
>    Otherwise LGTM. We're close! :-)

I will send v5, based on above.

Cheers,
Biju




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux