On 8/20/19 2:01 AM, Nikolay Aleksandrov wrote:

On 8/20/19 12:10 AM, Vladimir Oltean wrote:
[snip]

quoted

It's good to know that it's there (like you said, it explains some
things) but I can't exactly say that removing it helps in any way.
In fact, removing it only overwrites the dsa_8021q VLANs with 1 during
bridge_join, while not actually doing anything upon a vlan_filtering
toggle.
So the sja1105 driver is in a way shielded by DSA from the bridge, for
the better.
It still appears to be true that the bridge doesn't think it's
necessary to notify through SWITCHDEV_OBJ_ID_PORT_VLAN again. So my
best bet is to restore the pvid on my own.
However I've already stumbled upon an apparent bug while trying to do
that. Does this look off? If it doesn't, I'll submit it as a patch:

commit 788f03991aa576fc0b4b26ca330af0f412c55582
Author: Vladimir Oltean [off-list ref]
Date:   Mon Aug 19 22:57:00 2019 +0300

    net: bridge: Keep the BRIDGE_VLAN_INFO_PVID flag in net_bridge_vlan

    Currently this simplified code snippet fails:

            br_vlan_get_pvid(netdev, &pvid);
            br_vlan_get_info(netdev, pvid, &vinfo);
            ASSERT(!(vinfo.flags & BRIDGE_VLAN_INFO_PVID));

    It is intuitive that the pvid of a netdevice should have the
    BRIDGE_VLAN_INFO_PVID flag set.

    However I can't seem to pinpoint a commit where this behavior was
    introduced. It seems like it's been like that since forever.

    Signed-off-by: Vladimir Oltean [off-list ref]

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 021cc9f66804..f49b2758bcab 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c

@@ -68,10 +68,13 @@ static bool __vlan_add_flags(struct

net_bridge_vlan *v, u16 flags)
     else
         vg = nbp_vlan_group(v->port);

-    if (flags & BRIDGE_VLAN_INFO_PVID)
+    if (flags & BRIDGE_VLAN_INFO_PVID) {
         ret = __vlan_add_pvid(vg, v->vid);
-    else
+        v->flags |= BRIDGE_VLAN_INFO_PVID;
+    } else {
         ret = __vlan_delete_pvid(vg, v->vid);
+        v->flags &= ~BRIDGE_VLAN_INFO_PVID;
+    }

     if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
         v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;

Hi Vladimir,
I know it looks logical to have it, but there are a few reasons why we don't
do it, most importantly because we need to have only one visible pvid at any single
time, even if it's stale - it must be just one. Right now that rule will not
be violated  by your change, but people will try using this flag and could see

Obviously, I'm talking about RCU pvid/vlan use cases similar to the dumps.
The locked cases are fine.

two pvids simultaneously. You can see that the pvid code is even using memory
barriers to propagate the new value faster and everywhere the pvid is read only once.
That is the reason the flag is set dynamically when dumping entries, too.
A second (weaker) argument against would be given the above we don't want
another way to do the same thing, specifically if it can provide us with

i.e. I would like to avoid explaining why this shouldn't be relied upon without locking

two pvids (e.g. if walking the vlan list) or if it can provide us with a pvid
different from the one set in the vg.
If you do need that flag, you can change br_vlan_get_info() to set the flag like
the other places do - if the vid matches the current vg pvid, or you could change
the whole pvid handling code to this new scheme as long as it can guarantee a single
pvid when walking the vlan list and fast pvid retrieval in the fast-path.

Cheers,
 Nik