Messages in this thread |  | | From | Harini Katakam <> | Date | Tue, 4 Feb 2020 15:52:55 +0530 | Subject | Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check for TSO |
| |
Hi David,
> > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Tuesday, February 4, 2020 3:07 PM > > To: Harini Katakam <harinik@xilinx.com> > > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com; > > kuba@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > > Michal Simek <michals@xilinx.com>; harinikatakamlinux@gmail.com > > Subject: Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check > > for TSO > > > > From: Harini Katakam <harini.katakam@xilinx.com> > > Date: Mon, 3 Feb 2020 18:48:01 +0530 > > > > > The IP TSO implementation does NOT require the length to be a multiple > > > of 8. That is only a requirement for UFO as per IP documentation. > > > > > > Fixes: 1629dd4f763c ("cadence: Add LSO support.") > > > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com> > > > --- > > > v2: > > > Added Fixes tag > > > > Several problems with this. > > > > The subject talks about alignemnt check, but you are not changing the alignment > > check. Instead you are modifying the linear buffer > > check:
Thanks for the review. Everything below that line becomes unused when alignment check is removed. More details below.
> > > > > @@ -1792,7 +1792,7 @@ static netdev_features_t > > macb_features_check(struct sk_buff *skb, > > > /* Validate LSO compatibility */ > > > > > > /* there is only one buffer */ > > > - if (!skb_is_nonlinear(skb)) > > > + if (!skb_is_nonlinear(skb) || (ip_hdr(skb)->protocol != > > > +IPPROTO_UDP)) > > > return features; > > > > So either your explanation is wrong or the code change is wrong.
Alignment check is not required for TSO and is ONLY required for UFO. So, if NOT(UDP), just return.
macb_features_check() { If existing linear check (or) if !UDP no need to change features, just return
Alignment check implementation which is only necessary for UDP. }
> > > > Furthermore, if you add this condition then there is now dead code below this. > > The code that checks for example: > > > > /* length of header */ > > hdrlen = skb_transport_offset(skb); > > if (ip_hdr(skb)->protocol == IPPROTO_TCP) > > hdrlen += tcp_hdrlen(skb); > > > > will never trigger this IPPROTO_TCP condition after your change.
Yes, this is dead code now. I'll remove it.
> > > > A lot of things about this patch do not add up.
Please let me know if you have any further concerns.
Regards, Harini
|  |