Messages in this thread | | | From | Lino Sanfilippo <> | Subject | Aw: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum | Date | Thu, 15 Jul 2021 13:16:12 +0200 |
| |
Hi Vladimir,
> Gesendet: Donnerstag, 15. Juli 2021 um 08:54 Uhr > Von: "Vladimir Oltean" <olteanv@gmail.com> > An: "Andrew Lunn" <andrew@lunn.ch> > Cc: "Lino Sanfilippo" <LinoSanfilippo@gmx.de>, woojung.huh@microchip.com, UNGLinuxDriver@microchip.com, vivien.didelot@gmail.com, f.fainelli@gmail.com, davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org > Betreff: Re: [PATCH 2/2] net: dsa: tag_ksz: dont let the hardware process the layer 4 checksum > > On Wed, Jul 14, 2021 at 10:15:20PM +0200, Andrew Lunn wrote: > > On Wed, Jul 14, 2021 at 10:48:12PM +0300, Vladimir Oltean wrote: > > > Hi Lino, > > > > > > On Wed, Jul 14, 2021 at 09:17:23PM +0200, Lino Sanfilippo wrote: > > > > If the checksum calculation is offloaded to the network device (e.g due to > > > > NETIF_F_HW_CSUM inherited from the DSA master device), the calculated > > > > layer 4 checksum is incorrect. This is since the DSA tag which is placed > > > > after the layer 4 data is seen as a part of the data portion and thus > > > > errorneously included into the checksum calculation. > > > > To avoid this, always calculate the layer 4 checksum in software. > > > > > > > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > > > > --- > > > > > > This needs to be solved more generically for all tail taggers. Let me > > > try out a few things tomorrow and come with a proposal. > > > > Maybe the skb_linearize() is also a generic problem, since many of the > > tag drivers are using skb_put()? It looks like skb_linearize() is > > cheap because checking if the skb is already linear is cheap. So maybe > > we want to do it unconditionally? > > Yeah, but we should let the stack deal with both issues in validate_xmit_skb(). > There is a skb_needs_linearize() call which returns false because the > DSA interface inherits NETIF_F_SG from the master via dsa_slave_create(): > > slave_dev->features = master->vlan_features | NETIF_F_HW_TC; > > Arguably that's the problem right there, we shouldn't inherit neither > NETIF_F_HW_CSUM nor NETIF_F_SG from the list of features inheritable by > 8021q uppers. > > - If we inherit NETIF_F_SG we obligate ourselves to call skb_linearize() > for tail taggers on xmit. DSA probably doesn't break anything for > header taggers though even if the xmit skb is paged, since the DSA > header would always be part of the skb head, so this is a feature we > could keep for them. > - If we inherit NETIF_F_HW_CSUM from the master for tail taggers, it is > actively detrimential to keep this feature enabled, as proven my Lino. > As for header taggers, I fail to see how this would be helpful, since > the DSA master would always fail to see the real IP header (it has > been pushed to the right by the DSA tag), and therefore, the DSA > master offload would be effectively bypassed. So no point, really, in > inheriting it in the first place in any situation. > > Lino, to fix these bugs by letting validate_xmit_skb() know what works > for DSA and what doesn't, could you please: > > (a) move the current slave_dev->features assignment to > dsa_slave_setup_tagger()? We now support changing the tagging > protocol at runtime, and everything that depends on what the tagging > protocol is (in this case, tag_ops->needed_headroom vs > tag_ops->needed_tailroom) should be put in that function. > (b) unconditionally clear NETIF_F_HW_CSUM from slave_dev->features, > after inheriting the vlan_features from the master? > (c) clear NETIF_F_SG from slave_dev->features if we have a non-zero > tag_ops->needed_tailroom? >
Sure, I will test this solution. But I think NETIF_F_FRAGLIST should also be cleared in this case, right?
Regards, Lino
| |