Re: [PATCH net-next v2 6/7] net: dsa: vsc73xx: Add vlan filtering

[Vladimir Oltean]

On Thu, Jun 29, 2023 at 10:18:08PM +0200, Paweł Dembicki wrote:
> niedz., 25 cze 2023 o 17:05 Vladimir Oltean <olteanv@xxxxxxxxx> napisał(a):
> > Why do you need ports to be double VLAN aware when vlan_filtering=0?
> > Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> > incoming packets, and set up the PVIDs of user ports as egress-tagged on
> > the CPU port?
> 
> Because I want to forward tagged and untagged frames when
> vlan_filtering is off.  If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
> all (tagged and untagged) traffic into the outer vlan, called by the
> datasheet as "MAN space". In QinQ mode, it is possible to ignore what
> goes from a particular port but it is possible to separate traffic
> from different ports.

I think we may have some problem in finding common terminology.

Opening the manual and seeing the table "Customer Port Sample Configuration",
I think it's indeed what you need. But I wouldn't call it "double VLAN aware".
The port is actually configured to be VLAN *unaware* from the perspective of
classification, and always encapsulate all packets in one more VLAN (the
port PVID).

This switch's analyzer is always aware only of the outer VLAN header, and
that's not "double VLAN aware" (it can perform no action based on the
inner VLAN, if that exists), but it's fine for what is needed of it.

You might be mixing these with MAC_CFG::VLAN_AWR and MAC_CFG::VLAN_DBLAWR,
which essentially are only there to allow single- and double-VLAN-tagged
frames to be longer by 4 and 8 bytes, respectively, than the max frame
size. I don't think that these 2 fields have any reason to depend upon
the bridge VLAN awareness state of the port. They can be unconditionally
enabled. After all, Linux only cares about MTU, and that is the size of
the L2 payload, excluding any VLAN headers, if present.

I would suggest that if you exclude the MAC_CFG registers from
vsc73xx_port_set_vlan_conf(), you end up with not as many VLAN awareness
modes as you think. 2, to be precise: on or off. So you don't need the
enum.

Also, AFAIU, I don't see a reason to modify CAT_VLAN_MISC::VLAN_KEEP_TAG_ENA
from the value of 1 at all. You could always keep frames in the queue
system with the VID attached, and strip that VID on egress, if necessary,
via TXUPDCFG.

Not sure if you're noticed this, but drivers/net/ethernet/mscc/ and
drivers/net/dsa/ocelot/ contain a driver for a newer generation of
hardware than the VSC73xx, but many of the concepts apply. Maybe you
can take a look at how some things were done there.

> > > +
> > > +     for (i = 0; i <= 3072; i++) {
> > > +             ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> >
> > What is the purpose of this?
> 
> I want to be sure that the table is cleared when vlan awareness is changed.

Yes, but why? That should specifically not be done, since there is no
code in the kernel to replay the port_vlan_add() and tag_8021q_vlan_add()
calls for you when the VLAN awareness state changes. If you delete the
VLANs, they're gone, even though in software they're still there.