Date: Fri, 15 Dec 2023 15:45:47 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Romain Gantois <romain.gantois@...tlin.com>,
	Andrew Lunn <andrew@...n.ch>,
	Corinna Vinschen <vinschen@...hat.com>
Cc: Florian Fainelli <f.fainelli@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Jose Abreu <joabreu@...opsys.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: DSA tags seem to break checksum offloading on DWMAC100

On Fri, Dec 15, 2023 at 11:30:47AM +0100, Andrew Lunn wrote:
> > So it seems like a solution is needed to prevent checksum offloading by Ethernet
> > drivers when DSA tags are in used. I'm not sure how this would be done, since
> > DSA is purposefully kept quite separate from CPU port drivers. What are your
> > thoughts on this?
> 
> It is not as simple as that, because some Ethernet drivers do know how
> to correctly calculate checksums when there is a DSA
> header. e.g. Marvell and Broadcom devices can do this, when combined
> with Marvell/Broadcom switches. I don't know how the Broadcom driver
> does this, but on the Marvell Ethernet drivers, there is a value you
> set in the transmit descriptor to indicate how big the headers are
> before the IP header. Its normally used to skip over the VLAN tag, but
> it can also be used to skip over the DSA header.
> 
> So i would suggest you look at the data sheet and see if there is
> anything similar, a way to tell the hardware where the IP header
> actually is in the frame. If you can do that, you can then actually
> make use of the hardware checksum, rather than disable it.
> 
>      Andrew

There's a chance that the DWMAC cannot be told what is the offset of the
IP header - it finds it by itself. Looking at DWMAC documentation for
the TDES3_CHECKSUM_INSERTION_SHIFT bit, it seems to me that this is the
case. Table "Transmit Checksum Offload Engine Functions for Different
Packet Types" also describes for which packet types this will work, and
for which it won't. My guess is that the DWMAC would classify DSA-tagged
packets as "Non-IPv4 or IPv6 packet", but they need checksums for the
inner transport layer nonetheless. I think that transport checksumming
of IP over IP tunnels is also broken, because stmmac says to the stack
it will checksum them, but this table says it won't.

We say this in Documentation/networking/dsa/dsa.rst:

  If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM or
  NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the
  offload hardware already expects that specific tag (perhaps due to
  matching vendors). DSA user ports inherit those flags from the
  conduit, and it is up to the driver to correctly fall back to software
  checksum when the IP header is not where the hardware expects. If that
  check is ineffective, the packets might go to the network without a
  proper checksum (the checksum field will have the pseudo IP header
  sum).

TL;DR: the simplest fix would be to revert commit 6b2c6e4a938f ("net:
stmmac: propagate feature flags to vlan") in 'net', and rework
stmmac_xmit() in 'net-next' to only use csum_insertion for those packets
where it will work, and fall back to skb_checksum_help() otherwise.
The existing check with coe_unsupported per TX queue is insufficient.

Powered by blists - more mailing lists