Date:   Wed, 9 Sep 2020 19:31:05 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     davem@...emloft.net, vivien.didelot@...il.com, andrew@...n.ch,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: dsa: set
 configure_vlan_while_not_filtering to true by default

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

Powered by blists - more mailing lists