On 8/20/19 2:01 AM, Nikolay Aleksandrov wrote:
> On 8/20/19 12:10 AM, Vladimir Oltean wrote:
> [snip]
>> 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 <olte...@gmail.com>
>> 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 <olte...@gmail.com>
>>
>> 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
>
>
