On 4/4/25 18:25, Joseph Huang wrote: > On 4/4/2025 6:29 AM, Nikolay Aleksandrov wrote: >> On 4/4/25 02:44, Joseph Huang wrote: >>> Notify user space on mdb offload failure if mdb_offload_fail_notification >>> is set. >>> >>> Signed-off-by: Joseph Huang <joseph.hu...@garmin.com> >>> --- >>> net/bridge/br_mdb.c | 26 +++++++++++++++++++++----- >>> net/bridge/br_private.h | 9 +++++++++ >>> net/bridge/br_switchdev.c | 4 ++++ >>> 3 files changed, 34 insertions(+), 5 deletions(-) >>> >> >> The patch looks good, but one question - it seems we'll mark mdb entries with >> "offload failed" when we get -EOPNOTSUPP as an error as well. Is that >> intended? >> >> That is, if the option is enabled and we have mixed bridge ports, we'll mark >> mdbs >> to the non-switch ports as offload failed, but it is not due to a switch >> offload >> error. > > Good catch. No, that was not intended. > > What if we short-circuit and just return like you'd suggested initially if > err == -EOPNOTSUPP? > >>> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c >>> index 40f0b16e4df8..9b5005d0742a 100644 >>> --- a/net/bridge/br_switchdev.c >>> +++ b/net/bridge/br_switchdev.c >>> @@ -504,6 +504,7 @@ static void br_switchdev_mdb_complete(struct net_device >>> *dev, int err, void *pri >>> struct net_bridge_mdb_entry *mp; >>> struct net_bridge_port *port = data->port; >>> struct net_bridge *br = port->br; >>> + u8 old_flags; >>> > > + if (err == -EOPNOTSUPP) > + goto notsupp; > >>> spin_lock_bh(&br->multicast_lock); >>> mp = br_mdb_ip_get(br, &data->ip); >>> @@ -514,7 +515,10 @@ static void br_switchdev_mdb_complete(struct >>> net_device *dev, int err, void *pri >>> if (p->key.port != port) >>> continue; >>> + old_flags = p->flags; >>> br_multicast_set_pg_offload_flags(p, !err); >>> + if (br_mdb_should_notify(br, old_flags ^ p->flags)) >>> + br_mdb_flag_change_notify(br->dev, mp, p); >>> } >>> out: >>> spin_unlock_bh(&br->multicast_lock); >> > > + notsupp: > kfree(priv);
Looks good to me. Thanks!
