Re: [PATCH net 1/2] net: switchdev: Add helper to check if an object event is pending

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Jan 31, 2024 at 03:48:05PM +0100, Tobias Waldekranz wrote:
> >> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> >> +				    enum switchdev_notifier_type nt,
> >> +				    const struct switchdev_obj *obj);
> >
> > I think this is missing a shim definition for when CONFIG_NET_SWITCHDEV
> > is disabled.
> 
> Even though the only caller is br_switchdev.c, which is guarded behind
> CONFIG_NET_SWITCHDEV?

My mistake, please disregard.

> >>  int switchdev_port_obj_add(struct net_device *dev,
> >>  			   const struct switchdev_obj *obj,
> >>  			   struct netlink_ext_ack *extack);
> >> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> >> index 5b045284849e..40bb17c7fdbf 100644
> >> --- a/net/switchdev/switchdev.c
> >> +++ b/net/switchdev/switchdev.c
> >> @@ -19,6 +19,35 @@
> >>  #include <linux/rtnetlink.h>
> >>  #include <net/switchdev.h>
> >>  
> >> +static bool switchdev_obj_eq(const struct switchdev_obj *a,
> >> +			     const struct switchdev_obj *b)
> >> +{
> >> +	const struct switchdev_obj_port_vlan *va, *vb;
> >> +	const struct switchdev_obj_port_mdb *ma, *mb;
> >> +
> >> +	if (a->id != b->id || a->orig_dev != b->orig_dev)
> >> +		return false;
> >> +
> >> +	switch (a->id) {
> >> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> >> +		va = SWITCHDEV_OBJ_PORT_VLAN(a);
> >> +		vb = SWITCHDEV_OBJ_PORT_VLAN(b);
> >> +		return va->flags == vb->flags &&
> >> +			va->vid == vb->vid &&
> >> +			va->changed == vb->changed;
> >> +	case SWITCHDEV_OBJ_ID_PORT_MDB:
> >> +	case SWITCHDEV_OBJ_ID_HOST_MDB:
> >> +		ma = SWITCHDEV_OBJ_PORT_MDB(a);
> >> +		mb = SWITCHDEV_OBJ_PORT_MDB(b);
> >> +		return ma->vid == mb->vid &&
> >> +			!memcmp(ma->addr, mb->addr, sizeof(ma->addr));
> >
> > ether_addr_equal().
> >
> >> +	default:
> >> +		break;
> >
> > Does C allow you to not return anything here?
> 
> No warnings or errors are generated by my compiler (GCC 12.2.0).
> 
> My guess is that the expansion of BUG() ends with
> __builtin_unreachable() or similar.

Interesting, I didn't know that. Although checkpatch says: "WARNING: Do
not crash the kernel unless it is absolutely unavoidable--use
WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or
variants". So I'm conflicted about what I just learned and how it can be
applied in a way that checkpatch doesn't dislike.

> 
> >> +	}
> >> +
> >> +	BUG();
> >> +}
> >> +
> >>  static LIST_HEAD(deferred);
> >>  static DEFINE_SPINLOCK(deferred_lock);
> >>  
> >> @@ -307,6 +336,38 @@ int switchdev_port_obj_del(struct net_device *dev,
> >>  }
> >>  EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
> >>  
> >> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> >> +				    enum switchdev_notifier_type nt,
> >> +				    const struct switchdev_obj *obj)
> >
> > A kernel-doc comment would be great. It looks like it's not returning
> > whether the port object is deferred, but whether the _action_ given by
> > @nt on the @obj is deferred. This further distinguishes between deferred
> > additions and deferred removals.
> >
> 
> Fair, so should the name change as well? I guess you'd want something
> like switchdev_port_obj_notification_is_deferred, but that sure is
> awfully long.

switchdev_port_obj_act_is_deferred() for action, maybe?