Date:   Mon, 24 Jan 2022 17:31:47 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        Alvin Šipraga <ALSI@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple
 cpu ports, non cpu extint

On Sat, Jan 22, 2022 at 05:12:28PM -0300, Luiz Angelo Daros de Luca wrote:
> I'm new to DSA but I think that a solution like that might not scale
> well. For every possible master network driver, it needs to know if
> its offload feature will handle every different tag.

Correct, with the sensible default being that no checksum offloading in
the presence of DSA tags is supported.

> Imagining that both new offload HW and new switch tags will still
> appear in the kernel, it might be untreatable.

You don't see DSA masters understanding DSA tagging protocols every day,
I think you're overstating this. We'd have to cover Marvell-on-Marvell,
Broadcom-on-Broadcom, Mediatek-on-Mediatek, and the rest will have to
add their support when they add the hardware.

> I know dsa properties are not the solution for everything (and I'm
> still adapting to where that border is) but, in this case, it is a
> device specific arrangement between the ethernet device and the
> switch. Wouldn't it be better to allow the
> one writing the device-tree description inform if a master feature
> cannot be copied to slave devices?

Assuming an ultra-generic Ethernet controller with advanced soft parser
capabilities, you'd have to teach it the format of each DSA tagging
protocol you intend it to understand anyway, so this doesn't appear the
kind of thing best described in the device tree, since it may easily be
out of sync with what the driver is able to tell the hardware to do.

> 
> I checked DSA doc again and it says:
> 
> "Since tagging protocols in category 1 and 2 break software (and most
> often also hardware) packet dissection on the DSA master, features
> such as RPS (Receive Packet Steering) on the DSA master would be
> broken. The DSA framework deals with this by hooking into the flow
> dissector and shifting the offset at which the IP header is to be
> found in the tagged frame as seen by the DSA master. This behavior is
> automatic based on the overhead value of the tagging protocol. If not
> all packets are of equal size, the tagger can implement the
> flow_dissect method of the struct dsa_device_ops and override this
> default behavior by specifying the correct offset incurred by each
> individual RX packet. Tail taggers do not cause issues to the flow
> dissector."
> 
> It makes me think that it is the master network driver that does not
> implement that IP header location shift. Anyway, I believe it also
> depends on HW capabilities to inform that shift, right?
> 
> I'm trying to think as a DSA newbie (which is exactly what I am).
> Differently from an isolated ethernet driver, with DSA, the system
> does have control of "something" after the offload should be applied:
> the dsa switch. Can't we have a generic way to send a packet to the
> switch and make it bounce back to the CPU (passing through the offload
> engine)? Would it work if I set the destination port as the CPU port?
> This way, we could simply detect if the offload worked and disable
> those features that did not work. It could work with a generic
> implementation or, if needed, a specialized optional ds_switch_ops
> function just to setup that temporary lookback forwarding rule.

To be clear, do you consider this simpler than an ndo operation that
returns true or false if a certain DSA master can offload a certain
netdev feature in the presence of a certain DSA tag?

Ignoring the fact that there are subtly different ways in which various
hardware manufacturers implement packet injection from the CPU (and this
is reflected in the various struct dsa_device_ops :: xmit operation),
plus the fact that dsa_device_ops :: xmit takes a slave net_device as
argument, for which there is none to represent the CPU port. These
points mean that you'd need to implement a separate, hardware-specific
loopback xmit for each tagging protocol. But again, ignoring that for a
second.

When would be a good time to probe for DSA master features? The DSA
master might be down when DSA switches probe. What should we do with
packets sent on a DSA port until we've finished probing for DSA master
capabilities?

> > So the sad news for you is that this is pretty much "net-next" material,
> > even if it fixes what is essentially a design shortcoming. If we're
> > quick, we could start doing this right as net-next reopens, and that
> > would give other developers maximum opportunity to fix up the
> > performance regressions caused by lack of TX checksumming.
> 
> No problem. I'm already playing the long game. I'm just trying to fix
> a device I own using my free time and I don't have any manager with
> impossible deadlines.
> However, any solution with a performance regression would break the
> kernel API. I would rather add a new device-tree option :-)

Feel free to do whatever you want in OpenWRT, but as a general rule of
thumb, if something can be solved without involving the device tree,
then involving the device tree is probably the wrong approach.

Powered by blists - more mailing lists