Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering

[Paweł Dembicki]

śr., 27 wrz 2023 o 01:58 Vladimir Oltean <olteanv@xxxxxxxxx> napisał(a):
>
> On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote:
> > > > +             if (vsc->untagged_storage[port] < VLAN_N_VID &&
> > > > +                 !vid_is_dsa_8021q(vsc->untagged_storage[port]) &&
> > > > +                 !vid_is_dsa_8021q(vid) &&
> > >
> > > The problem here which led to these vid_is_dsa_8021q() checks is that
> > > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to
> > > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload
> > > those, correct?
> >
> > In my case, the major problem with tag8021q vlans is
> > "dsa_tag_8021q_bridge_join" function:
> > "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del".
> > I must disable pvid/untagged checking, because it will always fail. I
> > let kernel do the job,
> > it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
>
> I'm not sure that you described the problem in a way that I can understand, here.
>
> After dsa_tag_8021q_bridge_join():
> -> dsa_port_tag_8021q_vlan_add(dp, bridge_vid)
> -> dsa_port_tag_8021q_vlan_del(dp, standalone_vid)
>
> it's *expected* that there should be only one untagged/pvid per port: the bridge_vid.
>
> For context, consider the fact that you can run the following commands:
>
> bridge vlan add dev eth0 vid 10 pvid
> bridge vlan add dev eth0 vid 11 pvid
>
> and after the second command, vid 10 stops being a pvid.
>
> So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior
> is not correct. You need to implement the pvid overwriting behavior, since
> there's always only 1 pvid.
>

Yes, overwriting pvid is the only proper way. Kernel mechanism will
take care about the number of pvids. I will fixit in v4.

> So that leaves the "untagged" flag as being problematic, correct? Could
> you comment...
>
> >
> > > But when the port is VSC73XX_VLAN_IGNORE mode (and
> > > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus,
> > > *all* VLANs are egress-untagged VLANs, correct?
> > >
> > > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the
> > > first place, for tag_8021q VLANs, if you don't rely on the port's native
> > > VLAN for egress untagging?
>
> ... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag"
> is a configuration that can actually be supported by vsc73xx. You just
> need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q
> basically requests exactly that configuration on user ports (both the
> bridge_vid and the standalone_vid are egress-untagged). So your check is
> too restrictive, you are denying a configuration that would work.
> The problem only appears when you mix egress-tagged with egress-untagged
> VLANs on a port. Only then there can be at most 1 egress-untagged VID,
> because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the
> egress-tagged VIDs to work.

Should I make a local copy of the quantity of egress untagged and
tagged vlans per port to resolve this issue, shouldn't I?
And then I check how many vlans are egress tagged or untagged for a
properly restricted solution?

I see another problem. Even if I return an error value, the untagged
will be marked in 'bridge vlan' listing. I'm not sure how it should
work in this case.

>
> > > A comment would be good which states that the flipping between the
> > > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering()
> > > only gets called on actual changes to vlan_filtering, and thus, to
> > > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage
> > > needs to go to hardware, and what is in hardware needs to go to storage.
> > >
> > > It's an interesting implementation for sure.
> > >
> >
> > Thank you.
>
> I'm not sure if that was a compliment :)

Touché. :)

>At least in this form, it's
> certainly non-trivial to determine by looking at the code if it is
> correct or not, and it uses different patterns than the other VLAN
> implementations in DSA drivers. Generally, boring and obvious is
> preferable. But after I took the time to understand, it seems plausible
> that the approach might work.
>
> Let's see how the same idea looks, cleaned up a bit but not redesigned,
> in v4.

I try to at least clean pvid and untagged issues before v4.