Re: [PATCH v3 net-next 03/11] net: bridge: vlan: make __vlan_add_flags react only to PVID and UNTAGGED

On 16/02/2022 13:03, Nikolay Aleksandrov wrote:

On 15/02/2022 19:02, Vladimir Oltean wrote:

quoted

Currently there is a very subtle aspect to the behavior of
__vlan_add_flags(): it changes the struct net_bridge_vlan flags and
pvid, yet it returns true ("changed") even if none of those changed,
just a transition of br_vlan_is_brentry(v) took place from false to
true.

This can be seen in br_vlan_add_existing(), however we do not actually
rely on this subtle behavior, since the "if" condition that checks that
the vlan wasn't a brentry before had a useless (until now) assignment:

	*changed = true;

Make things more obvious by actually making __vlan_add_flags() do what's
written on the box, and be more specific about what is actually written
on the box. This is needed because further transformations will be done
to __vlan_add_flags().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: patch is new

 net/bridge/br_vlan.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 498cc297b492..89e2cfed7bdb 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c

@@ -58,7 +58,9 @@ static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 	return true;
 }
 
-/* return true if anything changed, false otherwise */
+/* Returns true if the BRIDGE_VLAN_INFO_PVID and BRIDGE_VLAN_INFO_UNTAGGED bits
+ * of @flags produced any change onto @v, false otherwise
+ */
 static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 {
 	struct net_bridge_vlan_group *vg;

@@ -80,7 +82,7 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 	else
 		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
 
-	return ret || !!(old_flags ^ v->flags);
+	return ret || !!((old_flags ^ v->flags) & BRIDGE_VLAN_INFO_UNTAGGED);
 }
 
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,

This patch is unnecessary and can be dropped given the next one.

OTOH being explicit is not bad, nevermind my comment. I'd just have put
this change after the refactoring, but it's ok either way.

Acked-by: Nikolay Aleksandrov <redacted>