lkml.org 
[lkml]   [2022]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets
Date
Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, May 12, 2022 9:15 PM
> To: Harini Katakam <harinik@xilinx.com>
> Cc: Harini Katakam <harinik@xilinx.com>; Nicolas Ferre
> <nicolas.ferre@microchip.com>; David Miller <davem@davemloft.net>;
> Claudiu Beznea <claudiu.beznea@microchip.com>; dumazet@google.com;
> Paolo Abeni <pabeni@redhat.com>; netdev <netdev@vger.kernel.org>;
> Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Michal Simek
> <michals@xilinx.com>; Radhey Shyam Pandey <radheys@xilinx.com>
> Subject: Re: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented
> packets
>
> On Thu, 12 May 2022 12:26:15 +0530 Harini Katakam wrote:
> > On Thu, May 12, 2022 at 4:10 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Tue, 10 May 2022 21:58:09 +0530 Harini Katakam wrote:
> > > > data_len in skbuff represents bytes resident in fragment lists or
> > > > unmapped page buffers. For such packets, when data_len is
> > > > non-zero, skb_put cannot be used - this will throw a kernel bug.
> > > > Hence do not use macb_pad_and_fcs for such fragments.
> > > >
> > > > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs
> > > > computation")
> > > > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > > > Signed-off-by: Radhey Shyam Pandey
> > > > <radhey.shyam.pandey@xilinx.com>
> > > > Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> > >
> > > I'm confused. When do we *have to* compute the FCS?
> > >
> > > This commit seems to indicate that we can't put the FCS so it's okay
> > > to ask the HW to do it. But that's backwards. We should ask the HW
> > > to compute the FCS whenever possible, to save the CPU cycles.
> > >
> > > Is there an unstated HW limitation here?
> >
> > Thanks for the review. The top level summary is that there CSUM
> > offload is enabled by via NETIF_F_HW_CSUM (and universally in IP
> > registers) and then selectively disabled for certain packets (using
> > NOCRC bit in buffer descriptors) where the application intentionally
> > performs CSUM and HW should not replace it, for ex. forwarding usecases.
> > I'm modifying this list of exceptions with this patch.
> >
> > This was due to HW limitation (see
> > https://www.spinics.net/lists/netdev/msg505065.html).
> > Further to this, Claudiu added macb_pad_and_fcs support. Please see
> > comment starting with "It was reported in" below:
> > https://lists.openwall.net/netdev/2018/10/30/76
> >
> > Hope this helps.
> > I'll fix the nit and send another version.
>
> So the NOCRC bit controls both ethernet and transport protocol checksums?
> The CRC in the name is a little confusing.

Yes

>
> Are you sure commit 403dc16796f5 ("cadence: force nonlinear buffers to be
> cloned") does not fix the case you're trying to address?

Thanks for this pointer. Yes, this patch already addresses the bug on linear skb buffers
and solves the issue I was hitting.

Apologies for the inconvenience. Please ignore this patch.

I had to unfortunately work on an tightly coupled older kernel and app to reproduce this
particular issue and hence could only verify whether my patch *works* on 5.18, not whether
it was *needed*. I backported 403dc16796f5 ("cadence: force nonlinear buffers to be cloned")
and it works as expected.

Regards,
Harini

\
 
 \ /
  Last update: 2022-05-12 19:11    [W:0.052 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site