Messages in this thread | | | From | Harini Katakam <> | Subject | RE: [PATCH v2] net: macb: Disable macb pad and fcs for fragmented packets | Date | Thu, 12 May 2022 17:09:17 +0000 |
| |
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
| |