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

On 15/02/2022 12:10, Vladimir Oltean wrote:

On Tue, Feb 15, 2022 at 11:54:05AM +0200, Vladimir Oltean wrote:

quoted

On Tue, Feb 15, 2022 at 10:54:26AM +0200, Nikolay Aleksandrov wrote:

quoted

quoted

+/* 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.

Plus, we have call paths that would bypass the "prepare" stage and jump
to commit, and for good reason. Do we want to change those as well, or
what would be the benefit?

It's not really prepare (doesn't have any effect), it's a precommit check.
You can keep the would change name, just make sure both wrappers use the
same function so people will be changing just 1 function and won't forget
to update anything. Would be nice to add a comment about it.