On 9/9/2020 9:31 AM, Vladimir Oltean wrote:
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.
The logic that writes to B53_VLAN_PORT_DEF_TAG does not update the
shadow copy in dev->ports[port].pvid which is how they are out of sync.
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?
The CPU port gets also programmed with 0 in B53_VLAN_PORT_DEF_TAG.
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.
How do you make sure that the CPU port sees the frame untagged which
would be necessary for a VLAN-unaware bridge? Do you have a special
remapping rule?
Initially the concern I had was with the use case described above which
was a 802.1Q separation, but in hindsight MAC address learning would
result in the frames going to the appropriate ports/VLANs anyway.
Tangentially, maybe we should finally add support for programming the CPU
port's VLAN membership independently from the other ports.
How?
Something like this:
https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/
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.
That's the plan, thanks!
--
Florian
