[Patch v2 net-next 0/3] Add support for mdb offload failure notification

[Patch v2 net-next 0/3] Add support for mdb offload failure notification

[Joseph Huang]

Currently the bridge does not provide real-time feedback to user space
on whether or not an attempt to offload an mdb entry was successful.

This patch set adds support to notify user space about failed offload
attempts, and is controlled by a new knob mdb_offload_fail_notification.

A break-down of the patches in the series:

Patch 1 adds offload failed flag to indicate that the offload attempt
has failed. The flag is reflected in netlink mdb entry flags.

Patch 2 adds the new bridge bool option mdb_offload_fail_notification.

Patch 3 notifies user space when the result is known, controlled by
mdb_offload_fail_notification setting.

Joseph Huang (3):
  net: bridge: mcast: Add offload failed mdb flag
  net: bridge: Add offload_fail_notification bopt
  net: bridge: mcast: Notify on mdb offload failure

 include/uapi/linux/if_bridge.h | 10 ++++++----
 net/bridge/br.c                |  5 +++++
 net/bridge/br_mdb.c            | 28 +++++++++++++++++++++++-----
 net/bridge/br_private.h        | 30 +++++++++++++++++++++++++-----
 net/bridge/br_switchdev.c      | 11 ++++++-----
 5 files changed, 65 insertions(+), 19 deletions(-)

---
v1: https://lore.kernel.org/netdev/20250318224255.143683-1-Joseph.Huang@garmin.com/
    iproute2 link:
    https://lore.kernel.org/netdev/20250318225026.145501-1-Joseph.Huang@garmin.com/
v2: Add br_multicast_pg_set_offload_flags helper to set offload flags
    Change multi-valued option mdb_notify_on_flag_change to bool option
    mdb_offload_fail_notification
    Change _br_mdb_notify to __br_mdb_notify
    Drop all #ifdef CONFIG_NET_SWITCHDEV
    Add br_mdb_should_notify helper and reorganize code in
    br_switch_mdb_complete
-- 
2.49.0

[Patch v2 net-next 1/3] net: bridge: mcast: Add offload failed mdb flag

[Joseph Huang]

Add MDB_FLAGS_OFFLOAD_FAILED and MDB_PG_FLAGS_OFFLOAD_FAILED to indicate
that an attempt to offload the MDB entry to switchdev has failed.

Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 include/uapi/linux/if_bridge.h |  9 +++++----
 net/bridge/br_mdb.c            |  2 ++
 net/bridge/br_private.h        | 20 +++++++++++++++-----
 net/bridge/br_switchdev.c      |  7 ++-----
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index a5b743a2f775..f2a6de424f3f 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -699,10 +699,11 @@ struct br_mdb_entry {
 #define MDB_TEMPORARY 0
 #define MDB_PERMANENT 1
 	__u8 state;
-#define MDB_FLAGS_OFFLOAD	(1 << 0)
-#define MDB_FLAGS_FAST_LEAVE	(1 << 1)
-#define MDB_FLAGS_STAR_EXCL	(1 << 2)
-#define MDB_FLAGS_BLOCKED	(1 << 3)
+#define MDB_FLAGS_OFFLOAD		(1 << 0)
+#define MDB_FLAGS_FAST_LEAVE		(1 << 1)
+#define MDB_FLAGS_STAR_EXCL		(1 << 2)
+#define MDB_FLAGS_BLOCKED		(1 << 3)
+#define MDB_FLAGS_OFFLOAD_FAILED	(1 << 4)
 	__u8 flags;
 	__u16 vid;
 	struct {
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 1a52a0bca086..0639691cd19b 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -144,6 +144,8 @@ static void __mdb_entry_fill_flags(struct br_mdb_entry *e, unsigned char flags)
 		e->flags |= MDB_FLAGS_STAR_EXCL;
 	if (flags & MDB_PG_FLAGS_BLOCKED)
 		e->flags |= MDB_FLAGS_BLOCKED;
+	if (flags & MDB_PG_FLAGS_OFFLOAD_FAILED)
+		e->flags |= MDB_FLAGS_OFFLOAD_FAILED;
 }
 
 static void __mdb_entry_to_br_ip(struct br_mdb_entry *entry, struct br_ip *ip,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1054b8a88edc..5f9d6075017e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -306,11 +306,12 @@ struct net_bridge_fdb_flush_desc {
 	u16				vlan_id;
 };
 
-#define MDB_PG_FLAGS_PERMANENT	BIT(0)
-#define MDB_PG_FLAGS_OFFLOAD	BIT(1)
-#define MDB_PG_FLAGS_FAST_LEAVE	BIT(2)
-#define MDB_PG_FLAGS_STAR_EXCL	BIT(3)
-#define MDB_PG_FLAGS_BLOCKED	BIT(4)
+#define MDB_PG_FLAGS_PERMANENT		BIT(0)
+#define MDB_PG_FLAGS_OFFLOAD		BIT(1)
+#define MDB_PG_FLAGS_FAST_LEAVE		BIT(2)
+#define MDB_PG_FLAGS_STAR_EXCL		BIT(3)
+#define MDB_PG_FLAGS_BLOCKED		BIT(4)
+#define MDB_PG_FLAGS_OFFLOAD_FAILED	BIT(5)
 
 #define PG_SRC_ENT_LIMIT	32
 
@@ -1343,6 +1344,15 @@ br_multicast_ctx_matches_vlan_snooping(const struct net_bridge_mcast *brmctx)
 
 	return !!(vlan_snooping_enabled == br_multicast_ctx_is_vlan(brmctx));
 }
+
+static inline void
+br_multicast_set_pg_offload_flags(struct net_bridge_port_group *p,
+				  bool offloaded)
+{
+	p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
+	p->flags |= (offloaded ? MDB_PG_FLAGS_OFFLOAD :
+		MDB_PG_FLAGS_OFFLOAD_FAILED);
+}
 #else
 static inline int br_multicast_rcv(struct net_bridge_mcast **brmctx,
 				   struct net_bridge_mcast_port **pmctx,
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 7b41ee8740cb..40f0b16e4df8 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -505,9 +505,6 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
 	struct net_bridge_port *port = data->port;
 	struct net_bridge *br = port->br;
 
-	if (err)
-		goto err;
-
 	spin_lock_bh(&br->multicast_lock);
 	mp = br_mdb_ip_get(br, &data->ip);
 	if (!mp)
@@ -516,11 +513,11 @@ static void br_switchdev_mdb_complete(struct net_device *dev, int err, void *pri
 	     pp = &p->next) {
 		if (p->key.port != port)
 			continue;
-		p->flags |= MDB_PG_FLAGS_OFFLOAD;
+
+		br_multicast_set_pg_offload_flags(p, !err);
 	}
 out:
 	spin_unlock_bh(&br->multicast_lock);
-err:
 	kfree(priv);
 }
 
-- 
2.49.0

[Patch v2 net-next 2/3] net: bridge: Add offload_fail_notification bopt

[Joseph Huang]

Add BR_BOOLOPT_MDB_OFFLOAD_FAIL_NOTIFICATION bool option.

Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
---
 include/uapi/linux/if_bridge.h | 1 +
 net/bridge/br.c                | 5 +++++
 net/bridge/br_private.h        | 1 +
 3 files changed, 7 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index f2a6de424f3f..73876c0e2bba 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -831,6 +831,7 @@ enum br_boolopt_id {
 	BR_BOOLOPT_NO_LL_LEARN,
 	BR_BOOLOPT_MCAST_VLAN_SNOOPING,
 	BR_BOOLOPT_MST_ENABLE,
+	BR_BOOLOPT_MDB_OFFLOAD_FAIL_NOTIFICATION,
 	BR_BOOLOPT_MAX
 };
 
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 183fcb362f9e..25dda554ca5b 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -284,6 +284,9 @@ int br_boolopt_toggle(struct net_bridge *br, enum br_boolopt_id opt, bool on,
 	case BR_BOOLOPT_MST_ENABLE:
 		err = br_mst_set_enabled(br, on, extack);
 		break;
+	case BR_BOOLOPT_MDB_OFFLOAD_FAIL_NOTIFICATION:
+		br_opt_toggle(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION, on);
+		break;
 	default:
 		/* shouldn't be called with unsupported options */
 		WARN_ON(1);
@@ -302,6 +305,8 @@ int br_boolopt_get(const struct net_bridge *br, enum br_boolopt_id opt)
 		return br_opt_get(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED);
 	case BR_BOOLOPT_MST_ENABLE:
 		return br_opt_get(br, BROPT_MST_ENABLED);
+	case BR_BOOLOPT_MDB_OFFLOAD_FAIL_NOTIFICATION:
+		return br_opt_get(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION);
 	default:
 		/* shouldn't be called with unsupported options */
 		WARN_ON(1);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 5f9d6075017e..02188b7ff8e6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -484,6 +484,7 @@ enum net_bridge_opts {
 	BROPT_VLAN_BRIDGE_BINDING,
 	BROPT_MCAST_VLAN_SNOOPING_ENABLED,
 	BROPT_MST_ENABLED,
+	BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION,
 };
 
 struct net_bridge {
-- 
2.49.0

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

[Joseph Huang]

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(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 0639691cd19b..5f53f387d251 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -519,16 +519,17 @@ static size_t rtnl_mdb_nlmsg_size(const struct net_bridge_port_group *pg)
 	       rtnl_mdb_nlmsg_pg_size(pg);
 }
 
-void br_mdb_notify(struct net_device *dev,
-		   struct net_bridge_mdb_entry *mp,
-		   struct net_bridge_port_group *pg,
-		   int type)
+static void __br_mdb_notify(struct net_device *dev,
+			    struct net_bridge_mdb_entry *mp,
+			    struct net_bridge_port_group *pg,
+			    int type, bool notify_switchdev)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
 
-	br_switchdev_mdb_notify(dev, mp, pg, type);
+	if (notify_switchdev)
+		br_switchdev_mdb_notify(dev, mp, pg, type);
 
 	skb = nlmsg_new(rtnl_mdb_nlmsg_size(pg), GFP_ATOMIC);
 	if (!skb)
@@ -546,6 +547,21 @@ void br_mdb_notify(struct net_device *dev,
 	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
 }
 
+void br_mdb_notify(struct net_device *dev,
+		   struct net_bridge_mdb_entry *mp,
+		   struct net_bridge_port_group *pg,
+		   int type)
+{
+	__br_mdb_notify(dev, mp, pg, type, true);
+}
+
+void br_mdb_flag_change_notify(struct net_device *dev,
+			       struct net_bridge_mdb_entry *mp,
+			       struct net_bridge_port_group *pg)
+{
+	__br_mdb_notify(dev, mp, pg, RTM_NEWMDB, false);
+}
+
 static int nlmsg_populate_rtr_fill(struct sk_buff *skb,
 				   struct net_device *dev,
 				   int ifindex, u16 vid, u32 pid,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 02188b7ff8e6..fc43ccc06ccb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -1005,6 +1005,8 @@ int br_mdb_hash_init(struct net_bridge *br);
 void br_mdb_hash_fini(struct net_bridge *br);
 void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
 		   struct net_bridge_port_group *pg, int type);
+void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
+			       struct net_bridge_port_group *pg);
 void br_rtr_notify(struct net_device *dev, struct net_bridge_mcast_port *pmctx,
 		   int type);
 void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
@@ -1354,6 +1356,13 @@ br_multicast_set_pg_offload_flags(struct net_bridge_port_group *p,
 	p->flags |= (offloaded ? MDB_PG_FLAGS_OFFLOAD :
 		MDB_PG_FLAGS_OFFLOAD_FAILED);
 }
+
+static inline bool
+br_mdb_should_notify(const struct net_bridge *br, u8 changed_flags)
+{
+	return br_opt_get(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION) &&
+		(changed_flags & MDB_PG_FLAGS_OFFLOAD_FAILED);
+}
 #else
 static inline int br_multicast_rcv(struct net_bridge_mcast **brmctx,
 				   struct net_bridge_mcast_port **pmctx,
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;
 
 	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);
-- 
2.49.0

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

[Nikolay Aleksandrov]

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.

> diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
> index 0639691cd19b..5f53f387d251 100644
> --- a/net/bridge/br_mdb.c
> +++ b/net/bridge/br_mdb.c
> @@ -519,16 +519,17 @@ static size_t rtnl_mdb_nlmsg_size(const struct net_bridge_port_group *pg)
>  	       rtnl_mdb_nlmsg_pg_size(pg);
>  }
>  
> -void br_mdb_notify(struct net_device *dev,
> -		   struct net_bridge_mdb_entry *mp,
> -		   struct net_bridge_port_group *pg,
> -		   int type)
> +static void __br_mdb_notify(struct net_device *dev,
> +			    struct net_bridge_mdb_entry *mp,
> +			    struct net_bridge_port_group *pg,
> +			    int type, bool notify_switchdev)
>  {
>  	struct net *net = dev_net(dev);
>  	struct sk_buff *skb;
>  	int err = -ENOBUFS;
>  
> -	br_switchdev_mdb_notify(dev, mp, pg, type);
> +	if (notify_switchdev)
> +		br_switchdev_mdb_notify(dev, mp, pg, type);
>  
>  	skb = nlmsg_new(rtnl_mdb_nlmsg_size(pg), GFP_ATOMIC);
>  	if (!skb)
> @@ -546,6 +547,21 @@ void br_mdb_notify(struct net_device *dev,
>  	rtnl_set_sk_err(net, RTNLGRP_MDB, err);
>  }
>  
> +void br_mdb_notify(struct net_device *dev,
> +		   struct net_bridge_mdb_entry *mp,
> +		   struct net_bridge_port_group *pg,
> +		   int type)
> +{
> +	__br_mdb_notify(dev, mp, pg, type, true);
> +}
> +
> +void br_mdb_flag_change_notify(struct net_device *dev,
> +			       struct net_bridge_mdb_entry *mp,
> +			       struct net_bridge_port_group *pg)
> +{
> +	__br_mdb_notify(dev, mp, pg, RTM_NEWMDB, false);
> +}
> +
>  static int nlmsg_populate_rtr_fill(struct sk_buff *skb,
>  				   struct net_device *dev,
>  				   int ifindex, u16 vid, u32 pid,
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 02188b7ff8e6..fc43ccc06ccb 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1005,6 +1005,8 @@ int br_mdb_hash_init(struct net_bridge *br);
>  void br_mdb_hash_fini(struct net_bridge *br);
>  void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
>  		   struct net_bridge_port_group *pg, int type);
> +void br_mdb_flag_change_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
> +			       struct net_bridge_port_group *pg);
>  void br_rtr_notify(struct net_device *dev, struct net_bridge_mcast_port *pmctx,
>  		   int type);
>  void br_multicast_del_pg(struct net_bridge_mdb_entry *mp,
> @@ -1354,6 +1356,13 @@ br_multicast_set_pg_offload_flags(struct net_bridge_port_group *p,
>  	p->flags |= (offloaded ? MDB_PG_FLAGS_OFFLOAD :
>  		MDB_PG_FLAGS_OFFLOAD_FAILED);
>  }
> +
> +static inline bool
> +br_mdb_should_notify(const struct net_bridge *br, u8 changed_flags)
> +{
> +	return br_opt_get(br, BROPT_MDB_OFFLOAD_FAIL_NOTIFICATION) &&
> +		(changed_flags & MDB_PG_FLAGS_OFFLOAD_FAILED);
> +}
>  #else
>  static inline int br_multicast_rcv(struct net_bridge_mcast **brmctx,
>  				   struct net_bridge_mcast_port **pmctx,
> 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;
>  
>  	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);

Re: [Patch v2 net-next 1/3] net: bridge: mcast: Add offload failed mdb flag

[Nikolay Aleksandrov]

On 4/4/25 02:44, Joseph Huang wrote:
> Add MDB_FLAGS_OFFLOAD_FAILED and MDB_PG_FLAGS_OFFLOAD_FAILED to indicate
> that an attempt to offload the MDB entry to switchdev has failed.
> 
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
>  include/uapi/linux/if_bridge.h |  9 +++++----
>  net/bridge/br_mdb.c            |  2 ++
>  net/bridge/br_private.h        | 20 +++++++++++++++-----
>  net/bridge/br_switchdev.c      |  7 ++-----
>  4 files changed, 24 insertions(+), 14 deletions(-)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [Patch v2 net-next 2/3] net: bridge: Add offload_fail_notification bopt

[Nikolay Aleksandrov]

On 4/4/25 02:44, Joseph Huang wrote:
> Add BR_BOOLOPT_MDB_OFFLOAD_FAIL_NOTIFICATION bool option.
> 
> Signed-off-by: Joseph Huang <Joseph.Huang@garmin.com>
> ---
>  include/uapi/linux/if_bridge.h | 1 +
>  net/bridge/br.c                | 5 +++++
>  net/bridge/br_private.h        | 1 +
>  3 files changed, 7 insertions(+)
> 

Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

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

[Joseph Huang]

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);

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

[Nikolay Aleksandrov]

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!

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

[Joseph Huang]

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

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

[Nikolay Aleksandrov]

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