| Message ID | 20210414143413.1786981-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: bridge: propagate error code and extack from br_mc_disabled_update | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: bridge@lists.linux-foundation.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 3 this patch: 3 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 105 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3 this patch: 3 |
| netdev/header_inline | success | Link |
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); > >
On Wed, Apr 14, 2021 at 05:58:04PM +0300, Nikolay Aleksandrov wrote: > > @@ -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 Thanks, this is a good point. I think I should just do this instead: if (err == -EOPNOTSUPP) err = 0; if (err) ... And I haven't run the bridge selftests. You are talking about: tools/testing/selftests/net/forwarding/bridge_{igmp,mld}.sh right?
On 14/04/2021 18:13, Vladimir Oltean wrote: > On Wed, Apr 14, 2021 at 05:58:04PM +0300, Nikolay Aleksandrov wrote: >>> @@ -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 > > Thanks, this is a good point. I think I should just do this instead: > if (err == -EOPNOTSUPP) > err = 0; > if (err) > ... > > And I haven't run the bridge selftests. You are talking about: > tools/testing/selftests/net/forwarding/bridge_{igmp,mld}.sh > right? > Yeah, but it's nice to run all of the bridge selftests in there, sometimes unexpected breakages can show up.
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 9d265447d654..7f861a6eb348 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -1593,7 +1593,8 @@ static void br_multicast_port_group_rexmit(struct timer_list *t) spin_unlock(&br->multicast_lock); } -static void br_mc_disabled_update(struct net_device *dev, bool value) +static int br_mc_disabled_update(struct net_device *dev, bool value, + struct netlink_ext_ack *extack) { struct switchdev_attr attr = { .orig_dev = dev, @@ -1602,11 +1603,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value) .u.mc_disabled = !value, }; - switchdev_port_attr_set(dev, &attr, NULL); + return switchdev_port_attr_set(dev, &attr, extack); } int br_multicast_add_port(struct net_bridge_port *port) { + int err; + port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY; port->multicast_eht_hosts_limit = BR_MCAST_DEFAULT_EHT_HOSTS_LIMIT; @@ -1618,8 +1621,12 @@ int br_multicast_add_port(struct net_bridge_port *port) timer_setup(&port->ip6_own_query.timer, br_ip6_multicast_port_query_expired, 0); #endif - br_mc_disabled_update(port->dev, - br_opt_get(port->br, BROPT_MULTICAST_ENABLED)); + err = br_mc_disabled_update(port->dev, + br_opt_get(port->br, + BROPT_MULTICAST_ENABLED), + NULL); + if (err) + return err; port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats); if (!port->mcast_stats) @@ -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; } 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);