Re: [PATCH net-next] net: bridge: propagate error code and extack from br_mc_disabled_update

[Nikolay Aleksandrov <nikolay@nvidia.com>]

On 14/04/2021 17:34, Vladimir Oltean wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> 
> Some Ethernet switches might only be able to support disabling multicast
> flooding globally, which is an issue for example when several bridges
> span the same physical device and request contradictory settings.
> 
> Propagate the return value of br_mc_disabled_update() such that this
> limitation is transmitted correctly to user-space.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/bridge/br_multicast.c | 26 +++++++++++++++++++-------
>  net/bridge/br_netlink.c   |  4 +++-
>  net/bridge/br_private.h   |  3 ++-
>  net/bridge/br_sysfs_br.c  |  8 +-------
>  4 files changed, 25 insertions(+), 16 deletions(-)
> 

Hi,
One comment below.

[snip]
> @@ -3560,16 +3567,21 @@ static void br_multicast_start_querier(struct net_bridge *br,
>  	rcu_read_unlock();
>  }
>  
> -int br_multicast_toggle(struct net_bridge *br, unsigned long val)
> +int br_multicast_toggle(struct net_bridge *br, unsigned long val,
> +			struct netlink_ext_ack *extack)
>  {
>  	struct net_bridge_port *port;
>  	bool change_snoopers = false;
> +	int err = 0;
>  
>  	spin_lock_bh(&br->multicast_lock);
>  	if (!!br_opt_get(br, BROPT_MULTICAST_ENABLED) == !!val)
>  		goto unlock;
>  
> -	br_mc_disabled_update(br->dev, val);
> +	err = br_mc_disabled_update(br->dev, val, extack);
> +	if (err && err != -EOPNOTSUPP)
> +		goto unlock;
> +
>  	br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val);
>  	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED)) {
>  		change_snoopers = true;
> @@ -3607,7 +3619,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val)
>  			br_multicast_leave_snoopers(br);
>  	}
>  
> -	return 0;
> +	return err;

Here won't you return EOPNOTSUPP even though everything above was successful ?
I mean if br_mc_disabled_update() returns -EOPNOTSUPP it will just be returned
and the caller would think there was an error.

Did you try running the bridge selftests with this patch ?

Thanks,
 Nik

>  }
>  
>  bool br_multicast_enabled(const struct net_device *dev)
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index f2b1343f8332..0456593aceec 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -1293,7 +1293,9 @@ static int br_changelink(struct net_device *brdev, struct nlattr *tb[],
>  	if (data[IFLA_BR_MCAST_SNOOPING]) {
>  		u8 mcast_snooping = nla_get_u8(data[IFLA_BR_MCAST_SNOOPING]);
>  
> -		br_multicast_toggle(br, mcast_snooping);
> +		err = br_multicast_toggle(br, mcast_snooping, extack);
> +		if (err)
> +			return err;
>  	}
>  
>  	if (data[IFLA_BR_MCAST_QUERY_USE_IFADDR]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ecb91e13d777..947c724c26b2 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -812,7 +812,8 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
>  			struct sk_buff *skb, bool local_rcv, bool local_orig);
>  int br_multicast_set_router(struct net_bridge *br, unsigned long val);
>  int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val);
> -int br_multicast_toggle(struct net_bridge *br, unsigned long val);
> +int br_multicast_toggle(struct net_bridge *br, unsigned long val,
> +			struct netlink_ext_ack *extack);
>  int br_multicast_set_querier(struct net_bridge *br, unsigned long val);
>  int br_multicast_set_hash_max(struct net_bridge *br, unsigned long val);
>  int br_multicast_set_igmp_version(struct net_bridge *br, unsigned long val);
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 072e29840082..381467b691d5 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -409,17 +409,11 @@ static ssize_t multicast_snooping_show(struct device *d,
>  	return sprintf(buf, "%d\n", br_opt_get(br, BROPT_MULTICAST_ENABLED));
>  }
>  
> -static int toggle_multicast(struct net_bridge *br, unsigned long val,
> -			    struct netlink_ext_ack *extack)
> -{
> -	return br_multicast_toggle(br, val);
> -}
> -
>  static ssize_t multicast_snooping_store(struct device *d,
>  					struct device_attribute *attr,
>  					const char *buf, size_t len)
>  {
> -	return store_bridge_parm(d, buf, len, toggle_multicast);
> +	return store_bridge_parm(d, buf, len, br_multicast_toggle);
>  }
>  static DEVICE_ATTR_RW(multicast_snooping);
>  
>