Re: [PATCH v2 net-next 1/8] net: bridge: vlan: notify switchdev only when something changed

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Feb 15, 2022 at 12:12:11PM +0200, Nikolay Aleksandrov wrote:
> On 15/02/2022 11:54, Vladimir Oltean wrote:
> > On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:
> >>> +/* return true if anything will change as a result of __vlan_add_flags,
> >>> + * false otherwise
> >>> + */
> >>> +static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
> >>> +{
> >>> +	struct net_bridge_vlan_group *vg;
> >>> +	u16 old_flags = v->flags;
> >>> +	bool pvid_changed;
> >>>  
> >>> -	return ret || !!(old_flags ^ v->flags);
> >>> +	if (br_vlan_is_master(v))
> >>> +		vg = br_vlan_group(v->br);
> >>> +	else
> >>> +		vg = nbp_vlan_group(v->port);
> >>> +
> >>> +	if (flags & BRIDGE_VLAN_INFO_PVID)
> >>> +		pvid_changed = (vg->pvid == v->vid);
> >>> +	else
> >>> +		pvid_changed = (vg->pvid != v->vid);
> >>> +
> >>> +	return pvid_changed || !!(old_flags ^ v->flags);
> >>>  }
> >>
> >> These two have to depend on each other, otherwise it's error-prone and
> >> surely in the future someone will forget to update both.
> >> How about add a "commit" argument to __vlan_add_flags and possibly rename
> >> it to __vlan_update_flags, then add 2 small helpers like __vlan_update_flags_precommit
> >> with commit == false and __vlan_update_flags_commit with commit == true.
> >> Or some other naming, the point is to always use the same flow and checks
> >> when updating the flags to make sure people don't forget.
> > 
> > You want to squash __vlan_flags_would_change() and __vlan_add_flags()
> > into a single function? But "would_change" returns bool, and "add"
> > returns void.
> > 
> 
> Hence the wrappers for commit == false and commit == true. You could name the precommit
> one __vlan_flags_would_change or something more appropriate. The point is to make
> sure we always update both when flags are changed.

I still have a little doubt that I understood you properly.
Do you mean like this?

diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 1402d5ca242d..0883c11897d6 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -34,36 +34,32 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
 	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
 }
 
-static bool __vlan_add_pvid(struct net_bridge_vlan_group *vg,
+static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
 			    const struct net_bridge_vlan *v)
 {
 	if (vg->pvid == v->vid)
-		return false;
+		return;
 
 	smp_wmb();
 	br_vlan_set_pvid_state(vg, v->state);
 	vg->pvid = v->vid;
-
-	return true;
 }
 
-static bool __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 {
 	if (vg->pvid != vid)
-		return false;
+		return;
 
 	smp_wmb();
 	vg->pvid = 0;
-
-	return true;
 }
 
-/* return true if anything changed, false otherwise */
-static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
+/* Commit a change of flags to @v. Do not use directly, instead go through
+ * __vlan_update_flags().
+ */
+static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 {
 	struct net_bridge_vlan_group *vg;
-	u16 old_flags = v->flags;
-	bool ret;
 
 	if (br_vlan_is_master(v))
 		vg = br_vlan_group(v->br);
@@ -71,16 +67,48 @@ static bool __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 		vg = nbp_vlan_group(v->port);
 
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		ret = __vlan_add_pvid(vg, v);
+		__vlan_add_pvid(vg, v);
 	else
-		ret = __vlan_delete_pvid(vg, v->vid);
+		__vlan_delete_pvid(vg, v->vid);
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
 	else
 		v->flags &= ~BRIDGE_VLAN_INFO_UNTAGGED;
+}
+
+/* Return true if anything will change as a result of __vlan_add_flags(),
+ * false otherwise. Do not use directly, instead go through
+ * __vlan_update_flags().
+ */
+static bool __vlan_flags_would_change(struct net_bridge_vlan *v, u16 flags)
+{
+	struct net_bridge_vlan_group *vg;
+	u16 old_flags = v->flags;
+	bool pvid_changed;
 
-	return ret || !!(old_flags ^ v->flags);
+	if (br_vlan_is_master(v))
+		vg = br_vlan_group(v->br);
+	else
+		vg = nbp_vlan_group(v->port);
+
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		pvid_changed = (vg->pvid != v->vid);
+	else
+		pvid_changed = (vg->pvid == v->vid);
+
+	return pvid_changed || !!(old_flags ^ v->flags);
+}
+
+static bool __vlan_update_flags(struct net_bridge_vlan *v, u16 flags,
+				bool commit)
+{
+	if (!commit)
+		return __vlan_flags_would_change(v, flags);
+
+	__vlan_add_flags(v, flags);
+
+	return false;
 }
 
 static int __vlan_vid_add(struct net_device *dev, struct net_bridge *br,
@@ -310,7 +338,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
 		goto out_fdb_insert;
 
 	__vlan_add_list(v);
-	__vlan_add_flags(v, flags);
+	__vlan_update_flags(v, flags, true);
 	br_multicast_toggle_one_vlan(v, true);
 
 	if (p)
@@ -670,11 +698,15 @@ static int br_vlan_add_existing(struct net_bridge *br,
 				u16 flags, bool *changed,
 				struct netlink_ext_ack *extack)
 {
+	bool would_change = __vlan_update_flags(vlan, flags, false);
 	int err;
 
-	err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags, extack);
-	if (err && err != -EOPNOTSUPP)
-		return err;
+	if (would_change) {
+		err = br_switchdev_port_vlan_add(br->dev, vlan->vid, flags,
+						 extack);
+		if (err && err != -EOPNOTSUPP)
+			return err;
+	}
 
 	if (!br_vlan_is_brentry(vlan)) {
 		/* Trying to change flags of non-existent bridge vlan */
@@ -696,7 +728,8 @@ static int br_vlan_add_existing(struct net_bridge *br,
 		br_multicast_toggle_one_vlan(vlan, true);
 	}
 
-	if (__vlan_add_flags(vlan, flags))
+	__vlan_update_flags(vlan, flags, true);
+	if (would_change)
 		*changed = true;
 
 	return 0;
@@ -1247,11 +1280,18 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
 	*changed = false;
 	vlan = br_vlan_find(nbp_vlan_group(port), vid);
 	if (vlan) {
-		/* Pass the flags to the hardware bridge */
-		ret = br_switchdev_port_vlan_add(port->dev, vid, flags, extack);
-		if (ret && ret != -EOPNOTSUPP)
-			return ret;
-		*changed = __vlan_add_flags(vlan, flags);
+		bool would_change = __vlan_update_flags(vlan, flags, false);
+
+		if (would_change) {
+			/* Pass the flags to the hardware bridge */
+			ret = br_switchdev_port_vlan_add(port->dev, vid,
+							 flags, extack);
+			if (ret && ret != -EOPNOTSUPP)
+				return ret;
+		}
+
+		__vlan_update_flags(vlan, flags, true);
+		*changed = would_change;
 
 		return 0;
 	}