netdev - Re: [PATCH/RFC net-next] ravb: RX checksum offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <cc6f731d-2192-995f-4194-5dd6600c4d5b@cogentembedded.com>
Date:   Thu, 28 Sep 2017 22:56:32 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Simon Horman <horms@...ge.net.au>
Cc:     David Miller <davem@...emloft.net>,
        Magnus Damm <magnus.damm@...il.com>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH/RFC net-next] ravb: RX checksum offload

Hello!

On 09/28/2017 01:49 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.

    Then why you talk of 1's complement here?

>>> 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@...ge.net.au>
>>
>>     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);

    Yes, exactly! :-)

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

    Yes, I think it should work...

MBR, Sergei