Re: [Patch v2 net-next 3/3] net: bridge: mcast: Notify on mdb offload failure

[Nikolay Aleksandrov <razor@blackwall.org>]

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.Huang@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