Re: [net-next RFC PATCH 0/6] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 07, 2021 at 08:21:52PM +0100, Ansuel Smith wrote:
> On Tue, Dec 07, 2021 at 08:15:24PM +0100, Andrew Lunn wrote:
> > > The qca tag header provide a TYPE value that refer to a big list of
> > > Frame type. In all of this at value 2 we have the type that tells us
> > > that is a READ_WRITE_REG_ACK (aka a mdio rw Ethernet packet)
> > > 
> > > The idea of using the tagger is to skip parsing the packet 2 times
> > > considering the qca tag header is present at the same place in both
> > > normal packet and mdio rw Ethernet packet.
> > > 
> > > Your idea would be hook this before the tagger and parse it?
> > > I assume that is the only way if this has to be generilized. But I
> > > wonder if this would create some overhead by the double parsing.
> > 
> > So it seems i remembered this incorrectly. Marvell call this Remote
> > Management Unit, RMU. And RMU makes use of bits inside the Marvell
> > Tag. I was thinking it was outside of the tag.
> > 
> > So, yes, the tagger does need to be involved in this.
> > 
> > The initial design of DSA was that the tagger and main driver were
> > kept separate. This has been causing us problems recently, we have use
> > cases where we need to share information between the tagger and the
> > driver. This looks like it is going to be another case of that.
> > 
> > 	Andrew
> 
> I mean if you check the code this is still somewhat ""separate"".
> I ""abuse"" the dsa port priv to share the required data.
> (I allocate a different struct... i put it in qca8k_priv and i set every
> port priv to this struct)
> 
> Wonder if we can add something to share data between the driver and the
> port so the access that from the tagger. (something that doesn't use the
> port priv)

The one problem relevant to this submission among those referenced by
Andrew is that dp->priv needs to be allocated by the Ethernet switch
driver, although it is used by the tagging protocol driver. So they
aren't really 'separate', nor can they be, the way dp->priv is currently
designed, it can only be "abused", not really "used".

The DSA design allows in principle for any switch driver to return any
protocol it desires in ->get_tag_protocol(). I occasionally test various
tagger submissions by hacking dsa_loop to do just that. But your
tag_qca.c driver would have a pretty unpleasant surprise if it was to be
paired to any other switch driver than qca8k, because that other driver
would either not allocate memory for dp->priv, or (worse) allocate some
other type of structure, expected to be used differently etc.

An even bigger complication is created by the fact that we can
dynamically change tagging protocols in certain cases (dsa <-> edsa,
ocelot <-> ocelot-8021q), and the current design doesn't really scale:
if any tagging protocol required its own dp->priv format, we may end up
with bugs such as the driver not freeing the old dp->priv and setting up
the new one, when the tagging protocol changes. These mistakes are all
too easy to make currently.

Another potential issue, which I don't see present here, but still
worth watching out for, is that the tagger cannot use symbols exported
by the switch, and vice versa. Otherwise the tagger cannot be inserted
into the kernel when built as module, due to missing symbols provided by
the switch. And the switch will not probe until it has a tagger.

I'm afraid we need to make a decision now whether we keep supporting the
separation between taggers and switch drivers, especially since the
tagger could become a bus provider for the switch driver. We need to
weigh the pros and cons.

I thought about what would be needed and I think we'd need tagger-owned
per-switch-tree private data. But this implies that there needs to be a
hook in the tagging protocol driver that notifies it when a certain
struct dsa_switch_tree *dst binds and unbinds to a certain tagger.
Then it could pick and choose the ports that need dp->priv configured in
a certain way: some taggers need the dp->priv of user ports to hold
something per port, others need the dp->priv of _all_ user ports to
point to something shared, others (like yours) apparently need the
dp->priv of the CPU port to hold something. This would become something
handled and owned exclusively by the tagger.

Ansuel, would something like this help you in any way?