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 <[email protected]>
---
v2:
Added Fixes tag
drivers/net/ethernet/cadence/macb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 7a2fe63..2e418b8 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -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;
/* length of header */
--
2.7.4
From: Harini Katakam <[email protected]>
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 <[email protected]>
> ---
> 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:
> @@ -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.
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.
A lot of things about this patch do not add up.
Hi David,
> > -----Original Message-----
> > From: David Miller [mailto:[email protected]]
> > Sent: Tuesday, February 4, 2020 3:07 PM
> > To: Harini Katakam <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Michal Simek <[email protected]>; [email protected]
> > Subject: Re: [PATCH v2 1/2] net: macb: Remove unnecessary alignment check
> > for TSO
> >
> > From: Harini Katakam <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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
From: Harini Katakam <[email protected]>
Date: Tue, 4 Feb 2020 15:52:55 +0530
>> > 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.
Looks good with the above correction and a rework of your commit message
to explain things more clearly.
Thank you.