Re: [PATCH net] ravb: expand rx descriptor data to accommodate hw checksum

[Simon Horman <horms@verge.net.au>]

On Fri, Jan 11, 2019 at 06:22:12PM +0300, Sergei Shtylyov wrote:
> On 01/11/2019 04:35 PM, Simon Horman wrote:
> 
> >>> EtherAVB may provide a checksum of packet data appended to packet data. In
> >>> order to allow this checksum to be received by the host descriptor data
> >>> needs to be enlarged by 2 bytes to accommodate the checksum.
> >>>
> >>> In the case of MTU-sized packets without a VLAN tag the
> >>> checksum were already accommodated by virtue of the space reserved for the
> >>> VLAN tag. However, a packet of MTU-size with a  VLAN tag consumed all
> >>> packet data space provided by a descriptor leaving no space for the
> >>> trailing checksum.
> >>
> >>    Wait! The gen3 manual is rather clear about the auto-checksumming not working
> >> right in the presence of the VLAN tag. Where do you check for that case?
> > 
> > In my testing on E3 this works correctly. Which portion of
> > the manual are you referring to?
> 
>    Section 50.4.1 in R-Car Series, 3rd Generation User’s Manual: Hardware, Rev 1.00,
> section 45A.3.14.1 in R-Car Series, 2nd Generation User’s Manual: Hardware, Rev 2.00.
>    The problem is that the checksum is always calculated starting at byte
> 14, i.e.  amidst the VLAN header (if present).

Yes, I understand that. What I'm unclear on is why that is a problem.
Empirically I have observed that RX csum offload works in the presence
of a VLAN tag and offers a significant benefit in terms of reduced CPU
utilisation.

> >>> This was not detected by the driver which incorrectly used the last two
> >>> bytes of packet data as the checksum and truncate the packet by two bytes.
> >>> This resulted all such packets being dropped.
> >>>
> >>> A work around is to disable rx checksum offload
> >>>  # ethtool -K eth0 rx off
> >>>
> >>> This patch resolves this problem by increasing the size available for
> >>> packet data in rx descriptors by two bytes. It also introduces
> >>> RAVB_CSUM_LEN to make things a little clearer than "2" sprinkled lightly
> >>> over the driver.
> >>
> >>    What about using sizeof(__sum16) instead? That type is declared in
> >> <linux/types.h> and used in 'struct iphdr'...
> > 
> > As in the following?
> > 
> > #define RAVB_CSUM_LEN sizeof(__sum16)
> 
>    No, as I said, I'd prefer to avoid any driver specific #define's.

If that is your preference I can update the patch accordingly.

> >>> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
> >>>
> >>> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> >>> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >> [...]
> 
> MBR, Sergei
>