On 14/04/2021 17:34, Vladimir Oltean wrote:
> From: Florian Fainelli <f.faine...@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.faine...@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@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);
>
>
