On Wed, Sep 13, 2017 at 08:54:00PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 09/12/2017 04:04 PM, Simon Horman wrote:
>
> >Add support for RX checksum offload. This is enabled by default and
> >may be disabled and re-enabled using ethtool:
> >
> > # ethtool -K eth0 rx off
> > # ethtool -K eth0 rx on
> >
> >The RAVB provides a simple checksumming scheme which appears to be
> >completely compatible with CHECKSUM_COMPLETE: a 1's complement sum of
>
> Hm, the gen2/3 manuals say calculation doesn't involve bit inversion...
Yes, I believe that matches my observation of the values supplied by
the hardware. Empirically this appears to be what the kernel expects.
> >all packet data after the L2 header is appended to packet data; this may
> >be trivially read by the driver and used to update the skb accordingly.
> >
> >In terms of performance throughput is close to gigabit line-rate both with
> >and without RX checksum offload enabled. Perf output, however, appears to
> >indicate that significantly less time is spent in do_csum(). This is as
> >expected.
>
> [...]
>
> >By inspection this also appears to be compatible with the ravb found
> >on R-Car Gen 2 SoCs, however, this patch is currently untested on such
> >hardware.
>
> I probably won't be able to test it on gen2 too...
>
> >Signed-off-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
>
> I'm generally OK with the patch but have some questions/comments below...
Thanks, I will try to address them.
> >---
> > drivers/net/ethernet/renesas/ravb_main.c | 58 +++++++++++++++++++++++++++++++-
> > 1 file changed, 57 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >index fdf30bfa403b..7c6438cd7de7 100644
> >--- a/drivers/net/ethernet/renesas/ravb_main.c
> >+++ b/drivers/net/ethernet/renesas/ravb_main.c
> [...]
> >@@ -1842,6 +1859,41 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> > return phy_mii_ioctl(phydev, req, cmd);
> > }
> >+static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
> >+{
> >+ struct ravb_private *priv = netdev_priv(ndev);
> >+ unsigned long flags;
> >+
> >+ spin_lock_irqsave(&priv->lock, flags);
> >+
> >+ /* Disable TX and RX */
> >+ ravb_rcv_snd_disable(ndev);
> >+
> >+ /* Modify RX Checksum setting */
> >+ if (enable)
> >+ ravb_modify(ndev, ECMR, 0, ECMR_RCSC);
>
> Please use ECMR_RCSC as the 3rd argument too to conform the common driver
> style.
>
> >+ else
> >+ ravb_modify(ndev, ECMR, ECMR_RCSC, 0);
>
> This *if* can easily be folded into a single ravb_modify() call...
Thanks, something like this?
ravb_modify(ndev, ECMR, ECMR_RCSC, enable ? ECMR_RCSC : 0);
> [...]
> >@@ -2004,6 +2057,9 @@ static int ravb_probe(struct platform_device *pdev)
> > if (!ndev)
> > return -ENOMEM;
> >+ ndev->features |= NETIF_F_RXCSUM;
> >+ ndev->hw_features |= ndev->features;
>
> Hum, both fields are 0 before this? Then why not use '=' instead of '|='?
> Even if not, why not just use the same value as both the rvalues?
I don't feel strongly about this, how about?
ndev->features = NETIF_F_RXCSUM;
ndev->hw_features = NETIF_F_RXCSUM;
