On Tue, Sep 08, 2020 at 05:02:06PM -0700, Florian Fainelli wrote:
> Found the problem, we do not allow the CPU port to be configured as
> untagged, and when we toggle vlan_filtering we actually incorrectly "move"
> the PVID from 1 to 0,
pvid 1 must be coming from the default_pvid of the bridge, I assume.
Where is pvid 0 (aka dev->ports[port].pvid) coming from? Is it simply
the cached value from B53_VLAN_PORT_DEF_TAG, from a previous
b53_vlan_filtering() call? Strange.
> which is incorrect, but since the CPU is also untagged in VID 0 this
> is why it "works" or rather two mistakes canceling it each other.
How does the CPU end up untagged in VLAN 0?
> I still need to confirm this, but the bridge in VLAN filtering mode seems to
> support receiving frames with the default_pvid as tagged, and it will untag
> it for the bridge master device transparently.
So it seems.
> The reason for not allowing the CPU port to be untagged
> (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be
> added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged,
> and port 4 is PVID 2 untagged. Back then there was no support for Broadcom
> tags, so the only way to differentiate traffic properly was to also add a
> pair of tagged VIDs to the DSA master.
> I am still trying to remember whether there were other concerns that
> prompted me to make that change and would appreciate some thoughts on that.
I think it makes some sense to always configure the VLANs on the CPU
port as tagged either way. I did the same in Felix and it's ok. But that
was due to a hardware limitation. On sja1105 I'm keeping the same flags
as on the user port, and that is ok too.
> Tangentially, maybe we should finally add support for programming the CPU
> port's VLAN membership independently from the other ports.
How?
> The following appears to work nicely now and allows us to get rid of the
> b53_vlan_filtering() logic, which would no longer work now because it
> assumed that toggling vlan_filtering implied that there would be no VLAN
> configuration when filtering was off.
>
> diff --git a/drivers/net/dsa/b53/b53_common.c
> b/drivers/net/dsa/b53/b53_common.c
> index 26fcff85d881..fac033730f4a 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up);
> int b53_vlan_filtering(struct dsa_switch *ds, int port, bool
> vlan_filtering)
> {
> struct b53_device *dev = ds->priv;
> - u16 pvid, new_pvid;
> -
> - b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid);
> - if (!vlan_filtering) {
> - /* Filtering is currently enabled, use the default PVID
> since
> - * the bridge does not expect tagging anymore
> - */
> - dev->ports[port].pvid = pvid;
> - new_pvid = b53_default_pvid(dev);
> - } else {
> - /* Filtering is currently disabled, restore the previous
> PVID */
> - new_pvid = dev->ports[port].pvid;
> - }
> -
> - if (pvid != new_pvid)
> - b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
> - new_pvid);
Yes, much simpler.
>
> b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering);
>
> @@ -1389,7 +1372,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port,
> untagged = true;
>
> vl->members |= BIT(port);
> - if (untagged && !dsa_is_cpu_port(ds, port))
> + if (untagged)
> vl->untag |= BIT(port);
> else
> vl->untag &= ~BIT(port);
> @@ -1427,7 +1410,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
> if (pvid == vid)
> pvid = b53_default_pvid(dev);
>
> - if (untagged && !dsa_is_cpu_port(ds, port))
> + if (untagged)
Ok, so you're removing this workaround now. A welcome simplification.
> vl->untag &= ~(BIT(port));
>
> b53_set_vlan_entry(dev, vid, vl);
> @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device
> *base,
> dev->priv = priv;
> dev->ops = ops;
> ds->ops = &b53_switch_ops;
> + ds->configure_vlan_while_not_filtering = true;
> + dev->vlan_enabled = ds->configure_vlan_while_not_filtering;
> mutex_init(&dev->reg_mutex);
> mutex_init(&dev->stats_mutex);
>
>
> --
> Florian
Looks good!
I'm going to hold off with my configure_vlan_while_not_filtering patch.
You can send this one before me.
Thanks,
-Vladimir
