On 4/4/25 23:11, Joseph Huang wrote: > On 4/4/2025 4:04 PM, Nikolay Aleksandrov wrote: >> 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! > > Thanks for the review! > > And a logistic question. Now that part 1 and part 2 are ack'd (thanks again > for the review), when I send out v3, should I resend those (unmodified part 1 > and part 2) with my v3 patch series, or should I break this one off and only > send part 3 v3 as a separate patch? > > Thanks, > Joseph
You should send the whole set again as v3, but you should keep the acked-by tags in those patches. Cheers, Nik
