On Mon, Jan 24, 2022 at 09:01:20AM -0800, Florian Fainelli wrote:

On 1/24/2022 8:55 AM, Vladimir Oltean wrote:

quoted

On Mon, Jan 24, 2022 at 08:46:49AM -0800, Jakub Kicinski wrote:

quoted

I thought for drivers setting the legacy NETIF_F_IP*_CSUM feature
it's driver's responsibility to validate the geometry of the packet
will work with the parser the device has. Or at least I think that's
what Tom was pushing for when he was cleaning up the checksumming last
(and wrote the long comment on the subject in skbuff.h).

Sorry Jakub, I don't understand what you mean to say when applied to the
context discussed here?

I believe what Jakub meant to say is that if a DSA conduit device driver
advertises any of the NETIF_F_IP*_CSUM feature bits, then the driver's
transmit path has the responsibility of checking that the payload being
transmitted has a chance of being checksummed properly by the hardware. The
problem here is not so much the geometry itself (linear or not, number/size
of fragments, etc.) as much as the placement of the L2/L3 headers usually.

DSA conduit network device drivers do not have the ability today to
determine what type of DSA tagging is being applied onto the DSA master but
they do know whether DSA tagging is in use or not which may be enough to be
overly compatible.

It is not clear to me whether we can solve this generically within the DSA
framework or even if this is desirable, but once we have identified a
problematic association of DSA tagger and DSA conduit, we can always have
the DSA conduit driver do something like:

if (netdev_uses_dsa(dev))
	skb_checksum_help()

or have a fix_features callback which does reject the enabling of
NETIF_F_IP*_CSUM if netdev_uses_dsa() becomes true.

Yes, but as you point out, the DSA master driver doesn't know what
header/trailer format it's dealing with. We could use netdev_uses_dsa()
as a very rough approximation, and that might work when we know that the
particular Ethernet controller is used only in conjunction with a single
type of DSA switch [from the same vendor], but I think we're just
delaying the inevitable, which is to treat the case where an Ethernet
controller can be a DSA master for more than one switch type, and it
understands some protocols but not others.
Also, scattering "if (netdev_uses_dsa(dev)) skb_checksum_help()" in
DSA-unaware drivers (the common case) seems like the improper approach.
We might end up seeing this pattern quite a lot, so DSA-unaware drivers
won't be DSA-unaware any longer.
It's still possible I'm misunderstanding something...