Re: [PATCH net-next 06/11] net: dsa: ocelot: create a template for the DSA tags on xmit

[Vladimir Oltean <olteanv@gmail.com>]

Hi Andrew,

On Thu, 28 May 2020 at 17:51, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, May 28, 2020 at 02:41:08AM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > With this patch we try to kill 2 birds with 1 stone.
> >
> > First of all, some switches that use tag_ocelot.c don't have the exact
> > same bitfield layout for the DSA tags. The destination ports field is
> > different for Seville VSC9953 for example. So the choices are to either
> > duplicate tag_ocelot.c into a new tag_seville.c (sub-optimal) or somehow
> > take into account a supposed ocelot->dest_ports_offset when packing this
> > field into the DSA injection header (again not ideal).
> >
> > Secondly, tag_ocelot.c already needs to memset a 128-bit area to zero
> > and call some packing() functions of dubious performance in the
> > fastpath. And most of the values it needs to pack are pretty much
> > constant (BYPASS=1, SRC_PORT=CPU, DEST=port index). So it would be good
> > if we could improve that.
> >
> > The proposed solution is to allocate a memory area per port at probe
> > time, initialize that with the statically defined bits as per chip
> > hardware revision, and just perform a simpler memcpy in the fastpath.
>
> Hi Vladimir
>
> We try to keep the taggers independent of the DSA drivers. I think
> tag_ocelot.c is the only one that breaks this.
>
> tag drivers are kernel modules. They have all the options of a kernel
> module, such as init and exit functions. You could create these
> templates in the module init function, and clean them up in the exit
> function. You can also register multiple taggers in one
> driver. tag_brcm.c does this as an example. So you can have a Seville
> tagger which uses different templates to ocelot.
>
>        Andrew

I don't particularly like that tag_brcm.c is riddled with #if /
#endif, they make it difficult to follow.

And if I allocate/free the xmit template in the
dsa_tag_driver_module_init / dsa_tag_driver_module_exit, how can I
reach the pointer to the correct per-switch-per-port template in the
ocelot_xmit function?

Please note that ocelot_xmit is already stateful, and it _needs_ to be
stateful: for 1588, it saves and increments the TX timestamp ID which
will be matched to the data that is received in felix_irq_handler.

And sja1105 also breaks the tagger/driver separation, and in even
"worse" ways - see sja1105_xmit_tpid which transmits a different frame
depending on which state the driver is in; also sja1105_decode_subvlan
which on RX looks up a table populated by the driver.

Generally speaking, I don't see any good reason why keeping the tagger
and the driver separated should be a design goal, especially when the
hotpath depends on stateful information (and the tagging driver can't
do anything at all without a backing switch driver anyway). Separation
could be done only in the simplest of cases, but as more advanced
features are necessary (not arguing that the template I'm adding here
is "advanced" stuff), this becomes practically impossible. Please also
see this tag_ocelot.c patch which needs to take the classified VLAN
from the DSA tag, or not, depending on the VLAN awareness state of the
port:
https://patchwork.ozlabs.org/project/netdev/patch/20200506074900.28529-7-xiaoliang.yang_1@nxp.com/

Thanks,
-Vladimir