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 11:54:05AM +0200, 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.

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?