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?
