| Message ID | 20190130005548.2212-2-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: management mode for bcm_sf2 | expand |
On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: > Some Ethernet switches might not be able to support disabling multicast > flooding globally when e.g: several bridges span the same physical > device, propagate the return value of br_mc_disabled_update() such that > this propagates correctly to user-space. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > net/bridge/br_multicast.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index 3aeff0895669..aff5e003d34f 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -813,20 +813,22 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t) > } > #endif > > -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 switchdev_attr attr = { > .orig_dev = dev, > .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, > - .flags = SWITCHDEV_F_DEFER, > + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, Actually, since the operation is deferred I don't think the return value from the driver is ever checked. Can you test it? I think it would be good to convert the attributes to use the switchdev notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del SDO with a notification") did for objects. Then you can have your listener veto the operation in the same context it is happening. > .u.mc_disabled = !value, > }; > > - switchdev_port_attr_set(dev, &attr); > + return switchdev_port_attr_set(dev, &attr); > } > > int br_multicast_add_port(struct net_bridge_port *port) > { > + int ret; > + > port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY; > > timer_setup(&port->multicast_router_timer, > @@ -837,8 +839,11 @@ 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)); > + ret = br_mc_disabled_update(port->dev, > + br_opt_get(port->br, > + BROPT_MULTICAST_ENABLED)); > + if (ret) > + return ret; > > port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats); > if (!port->mcast_stats) > @@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br, > int br_multicast_toggle(struct net_bridge *br, unsigned long val) > { > struct net_bridge_port *port; > + 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); > + if (err) > + goto unlock; > + > br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val); > if (!br_opt_get(br, BROPT_MULTICAST_ENABLED)) > goto unlock; > @@ -1957,7 +1966,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val) > unlock: > spin_unlock_bh(&br->multicast_lock); > > - return 0; > + return err; > } > > bool br_multicast_enabled(const struct net_device *dev) > -- > 2.17.1 >
On 1/29/19 11:36 PM, Ido Schimmel wrote: > On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: >> Some Ethernet switches might not be able to support disabling multicast >> flooding globally when e.g: several bridges span the same physical >> device, propagate the return value of br_mc_disabled_update() such that >> this propagates correctly to user-space. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> net/bridge/br_multicast.c | 23 ++++++++++++++++------- >> 1 file changed, 16 insertions(+), 7 deletions(-) >> >> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c >> index 3aeff0895669..aff5e003d34f 100644 >> --- a/net/bridge/br_multicast.c >> +++ b/net/bridge/br_multicast.c >> @@ -813,20 +813,22 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t) >> } >> #endif >> >> -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 switchdev_attr attr = { >> .orig_dev = dev, >> .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, >> - .flags = SWITCHDEV_F_DEFER, >> + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, > > Actually, since the operation is deferred I don't think the return value > from the driver is ever checked. Can you test it? You are right, you get a WARN() from switchdev_attr_port_set_now(), but this does not propagate back to the caller, so you can still create a bridge device and enslave a device successfully despite getting warnings on the console. > > I think it would be good to convert the attributes to use the switchdev > notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del > SDO with a notification") did for objects. Then you can have your > listener veto the operation in the same context it is happening. Alright, working on it. Would you do that just for the attr_set, or for attr_get as well (to be symmetrical)?
On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote: > On 1/29/19 11:36 PM, Ido Schimmel wrote: > > On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: > >> -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 switchdev_attr attr = { > >> .orig_dev = dev, > >> .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, > >> - .flags = SWITCHDEV_F_DEFER, > >> + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, > > > > Actually, since the operation is deferred I don't think the return value > > from the driver is ever checked. Can you test it? > > You are right, you get a WARN() from switchdev_attr_port_set_now(), but > this does not propagate back to the caller, so you can still create a > bridge device and enslave a device successfully despite getting warnings > on the console. > > > > > I think it would be good to convert the attributes to use the switchdev > > notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del > > SDO with a notification") did for objects. Then you can have your > > listener veto the operation in the same context it is happening. > > Alright, working on it. Would you do that just for the attr_set, or for > attr_get as well (to be symmetrical)? Yes, then we can get rid of switchdev_ops completely.
On 1/30/19 11:50 PM, Ido Schimmel wrote: > On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote: >> On 1/29/19 11:36 PM, Ido Schimmel wrote: >>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: >>>> -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 switchdev_attr attr = { >>>> .orig_dev = dev, >>>> .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, >>>> - .flags = SWITCHDEV_F_DEFER, >>>> + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, >>> >>> Actually, since the operation is deferred I don't think the return value >>> from the driver is ever checked. Can you test it? >> >> You are right, you get a WARN() from switchdev_attr_port_set_now(), but >> this does not propagate back to the caller, so you can still create a >> bridge device and enslave a device successfully despite getting warnings >> on the console. >> >>> >>> I think it would be good to convert the attributes to use the switchdev >>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del >>> SDO with a notification") did for objects. Then you can have your >>> listener veto the operation in the same context it is happening. >> >> Alright, working on it. Would you do that just for the attr_set, or for >> attr_get as well (to be symmetrical)? > > Yes, then we can get rid of switchdev_ops completely. > OK, so here is what I have so far: https://github.com/ffainelli/linux/pull/new/switchdev-attr although I am seeing some invalid context operations with DSA that I am debugging. Does this look like the right way to go from your perspective?
On Thu, Jan 31, 2019 at 05:19:25PM -0800, Florian Fainelli wrote: > On 1/30/19 11:50 PM, Ido Schimmel wrote: > > On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote: > >> On 1/29/19 11:36 PM, Ido Schimmel wrote: > >>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: > >>>> -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 switchdev_attr attr = { > >>>> .orig_dev = dev, > >>>> .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, > >>>> - .flags = SWITCHDEV_F_DEFER, > >>>> + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, > >>> > >>> Actually, since the operation is deferred I don't think the return value > >>> from the driver is ever checked. Can you test it? > >> > >> You are right, you get a WARN() from switchdev_attr_port_set_now(), but > >> this does not propagate back to the caller, so you can still create a > >> bridge device and enslave a device successfully despite getting warnings > >> on the console. > >> > >>> > >>> I think it would be good to convert the attributes to use the switchdev > >>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del > >>> SDO with a notification") did for objects. Then you can have your > >>> listener veto the operation in the same context it is happening. > >> > >> Alright, working on it. Would you do that just for the attr_set, or for > >> attr_get as well (to be symmetrical)? > > > > Yes, then we can get rid of switchdev_ops completely. > > > > OK, so here is what I have so far: > > https://github.com/ffainelli/linux/pull/new/switchdev-attr > > although I am seeing some invalid context operations with DSA that I am > debugging. Does this look like the right way to go from your perspective? That was quick :) I think I owe you some explanation as to why I even came up with the idea. But before that, I have another idea how to solve your immediate problem. You can employ a similar trick to the one used to set bridge port flags in br_switchdev_set_port_flag(). Like br_mc_disabled_update() it is also called from an atomic context, so in order to allow drivers to veto unsupported flags we introduced a new switchdev attribute: 'SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT' The attribute is only used in get operations and never deferred. You can introduce 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED_SUPPORT' and use it before invoking 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED'. Regarding the whole notifier business and switchdev in general. Using the device chain to propagate various bridge port attributes is wrong. We saw it multiple times in the past already. First with routes that were converted to use notifiers because the switch needs to offload the entire routing table and not only routes whose nexthop device is a switch port. Then with FDB entries and recently also with VLANs in a VLAN-aware bridge. See merge commit 02e1dbe402de ("Merge branch 'Pass-extack-to-SWITCHDEV_PORT_OBJ_ADD'") for more details. This is also true for switchdev attributes since we might want to support toggling of bridge port flags on a VXLAN device in the future. For example, to allow selective flooding. Others might have more use cases. I want to use the opportunity to pick your brain (and others') about more issues I see with switchdev and maybe reach some agreement and form a plan. Just to be clear, it is not related to your patchset. The prepare-commit model probably made sense in the beginning, but a few years later I think we know better. At least in mlxsw we have multiple places where we perform all the work in the prepare phase simply because that without doing all the work we don't have a way to guarantee that the commit phase will succeed. I'm not aware of other instances of this model in the networking code, so I wonder why we need it in switchdev and if we can simply remove it and simplify things. Another issue is that I believe we can completely remove the switchdev infrastructure as it basically boils down to bridge-specific notifiers. If you look at where switchdev is actually used in the networking stack while excluding the obvious suspects you get this: $ git grep -i -n -e 'switchdev' -- 'net/*' ':!net/bridge/*' ':!net/dsa/*' ':!net/switchdev/' net/8021q/vlan_dev.c:34:#include <net/switchdev.h> net/Kconfig:240:source "net/switchdev/Kconfig" net/Makefile:81:ifneq ($(CONFIG_NET_SWITCHDEV),) net/Makefile:82:obj-y += switchdev/ net/core/net-sysfs.c:15:#include <net/switchdev.h> net/core/net-sysfs.c:504: struct switchdev_attr attr = { net/core/net-sysfs.c:506: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, net/core/net-sysfs.c:507: .flags = SWITCHDEV_F_NO_RECURSE, net/core/net-sysfs.c:510: ret = switchdev_port_attr_get(netdev, &attr); net/core/rtnetlink.c:49:#include <net/switchdev.h> net/core/rtnetlink.c:1150: struct switchdev_attr attr = { net/core/rtnetlink.c:1152: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, net/core/rtnetlink.c:1153: .flags = SWITCHDEV_F_NO_RECURSE, net/core/rtnetlink.c:1156: err = switchdev_port_attr_get(dev, &attr); net/core/skbuff.c:4925:#ifdef CONFIG_NET_SWITCHDEV net/ipv4/ip_forward.c:72:#ifdef CONFIG_NET_SWITCHDEV net/ipv4/ipmr.c:70:#include <net/switchdev.h> net/ipv4/ipmr.c:841: struct switchdev_attr attr = { net/ipv4/ipmr.c:842: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, net/ipv4/ipmr.c:923: if (!switchdev_port_attr_get(dev, &attr)) { net/ipv4/ipmr.c:1802:#ifdef CONFIG_NET_SWITCHDEV net/ipv6/ip6_output.c:381:#ifdef CONFIG_NET_SWITCHDEV These are all instances related to the use of parent ID. Why not add a new ndo that will allow various users to query the parent ID of a netdev? bond and team can return an error if their slaves don't all have the same parent ID. You then end up with basic constructs like notifiers and ndo invocations and not with complex objects and attributes that are propagated via the device chain with a prepare-commit model. WDYT? > -- > Florian
On 2/2/19 7:47 AM, Ido Schimmel wrote: > On Thu, Jan 31, 2019 at 05:19:25PM -0800, Florian Fainelli wrote: >> On 1/30/19 11:50 PM, Ido Schimmel wrote: >>> On Wed, Jan 30, 2019 at 05:00:57PM -0800, Florian Fainelli wrote: >>>> On 1/29/19 11:36 PM, Ido Schimmel wrote: >>>>> On Tue, Jan 29, 2019 at 04:55:37PM -0800, Florian Fainelli wrote: >>>>>> -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 switchdev_attr attr = { >>>>>> .orig_dev = dev, >>>>>> .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, >>>>>> - .flags = SWITCHDEV_F_DEFER, >>>>>> + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, >>>>> >>>>> Actually, since the operation is deferred I don't think the return value >>>>> from the driver is ever checked. Can you test it? >>>> >>>> You are right, you get a WARN() from switchdev_attr_port_set_now(), but >>>> this does not propagate back to the caller, so you can still create a >>>> bridge device and enslave a device successfully despite getting warnings >>>> on the console. >>>> >>>>> >>>>> I think it would be good to convert the attributes to use the switchdev >>>>> notifier like commit d17d9f5e5143 ("switchdev: Replace port obj add/del >>>>> SDO with a notification") did for objects. Then you can have your >>>>> listener veto the operation in the same context it is happening. >>>> >>>> Alright, working on it. Would you do that just for the attr_set, or for >>>> attr_get as well (to be symmetrical)? >>> >>> Yes, then we can get rid of switchdev_ops completely. >>> >> >> OK, so here is what I have so far: >> >> https://github.com/ffainelli/linux/pull/new/switchdev-attr >> >> although I am seeing some invalid context operations with DSA that I am >> debugging. Does this look like the right way to go from your perspective? > > That was quick :) > > I think I owe you some explanation as to why I even came up with the > idea. But before that, I have another idea how to solve your immediate > problem. > > You can employ a similar trick to the one used to set bridge port flags > in br_switchdev_set_port_flag(). Like br_mc_disabled_update() it is also > called from an atomic context, so in order to allow drivers to veto > unsupported flags we introduced a new switchdev attribute: > 'SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS_SUPPORT' Yes that is a great idea, for some reason I completely missed your email here, but this is how I am going to approach it now. Another way to look at the problem could be to question whether we really need to be in atomic context when such attributes are pushed? AFAICT, we are executing with BH disabled because we want to protect the bridge master's bridge port list, right? > > The attribute is only used in get operations and never deferred. You can > introduce 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED_SUPPORT' and use it > before invoking 'SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED'. > > Regarding the whole notifier business and switchdev in general. Using > the device chain to propagate various bridge port attributes is wrong. > We saw it multiple times in the past already. First with routes that > were converted to use notifiers because the switch needs to offload the > entire routing table and not only routes whose nexthop device is a > switch port. Then with FDB entries and recently also with VLANs in a > VLAN-aware bridge. See merge commit 02e1dbe402de ("Merge branch > 'Pass-extack-to-SWITCHDEV_PORT_OBJ_ADD'") for more details. > > This is also true for switchdev attributes since we might want to > support toggling of bridge port flags on a VXLAN device in the future. > For example, to allow selective flooding. Others might have more use > cases. > > I want to use the opportunity to pick your brain (and others') about > more issues I see with switchdev and maybe reach some agreement and form > a plan. Just to be clear, it is not related to your patchset. > > The prepare-commit model probably made sense in the beginning, but a few > years later I think we know better. At least in mlxsw we have multiple > places where we perform all the work in the prepare phase simply because > that without doing all the work we don't have a way to guarantee that > the commit phase will succeed. I'm not aware of other instances of this > model in the networking code, so I wonder why we need it in switchdev > and if we can simply remove it and simplify things. If we can at least re-purpose the prepare + commit model such that the prepare phase is: can you support that operation (without allocating resources) and do that operation in the caller context, while the commit is deferred, then that would solve some of my issues but it could create more for mlxsw possibly? I have to wonder though, if we have a driver which is constructed such that: - all HW programming is done - a SW resource (e.g.: a rule object) is allocated last, then if the allocation fails, we should be able to easily rollback the HW programming we just did Would that work? > > Another issue is that I believe we can completely remove the switchdev > infrastructure as it basically boils down to bridge-specific notifiers. > If you look at where switchdev is actually used in the networking stack > while excluding the obvious suspects you get this: > > $ git grep -i -n -e 'switchdev' -- 'net/*' ':!net/bridge/*' ':!net/dsa/*' ':!net/switchdev/' > net/8021q/vlan_dev.c:34:#include <net/switchdev.h> > net/Kconfig:240:source "net/switchdev/Kconfig" > net/Makefile:81:ifneq ($(CONFIG_NET_SWITCHDEV),) > net/Makefile:82:obj-y += switchdev/ > net/core/net-sysfs.c:15:#include <net/switchdev.h> > net/core/net-sysfs.c:504: struct switchdev_attr attr = { > net/core/net-sysfs.c:506: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, > net/core/net-sysfs.c:507: .flags = SWITCHDEV_F_NO_RECURSE, > net/core/net-sysfs.c:510: ret = switchdev_port_attr_get(netdev, &attr); > net/core/rtnetlink.c:49:#include <net/switchdev.h> > net/core/rtnetlink.c:1150: struct switchdev_attr attr = { > net/core/rtnetlink.c:1152: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, > net/core/rtnetlink.c:1153: .flags = SWITCHDEV_F_NO_RECURSE, > net/core/rtnetlink.c:1156: err = switchdev_port_attr_get(dev, &attr); > net/core/skbuff.c:4925:#ifdef CONFIG_NET_SWITCHDEV > net/ipv4/ip_forward.c:72:#ifdef CONFIG_NET_SWITCHDEV > net/ipv4/ipmr.c:70:#include <net/switchdev.h> > net/ipv4/ipmr.c:841: struct switchdev_attr attr = { > net/ipv4/ipmr.c:842: .id = SWITCHDEV_ATTR_ID_PORT_PARENT_ID, > net/ipv4/ipmr.c:923: if (!switchdev_port_attr_get(dev, &attr)) { > net/ipv4/ipmr.c:1802:#ifdef CONFIG_NET_SWITCHDEV > net/ipv6/ip6_output.c:381:#ifdef CONFIG_NET_SWITCHDEV > > These are all instances related to the use of parent ID. Why not add a > new ndo that will allow various users to query the parent ID of a > netdev? bond and team can return an error if their slaves don't all have > the same parent ID. This should now be addressed as you might have seen. > > You then end up with basic constructs like notifiers and ndo invocations > and not with complex objects and attributes that are propagated via the > device chain with a prepare-commit model. > > WDYT? > >> -- >> Florian
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 3aeff0895669..aff5e003d34f 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -813,20 +813,22 @@ static void br_ip6_multicast_port_query_expired(struct timer_list *t) } #endif -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 switchdev_attr attr = { .orig_dev = dev, .id = SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED, - .flags = SWITCHDEV_F_DEFER, + .flags = SWITCHDEV_F_DEFER | SWITCHDEV_F_SKIP_EOPNOTSUPP, .u.mc_disabled = !value, }; - switchdev_port_attr_set(dev, &attr); + return switchdev_port_attr_set(dev, &attr); } int br_multicast_add_port(struct net_bridge_port *port) { + int ret; + port->multicast_router = MDB_RTR_TYPE_TEMP_QUERY; timer_setup(&port->multicast_router_timer, @@ -837,8 +839,11 @@ 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)); + ret = br_mc_disabled_update(port->dev, + br_opt_get(port->br, + BROPT_MULTICAST_ENABLED)); + if (ret) + return ret; port->mcast_stats = netdev_alloc_pcpu_stats(struct bridge_mcast_stats); if (!port->mcast_stats) @@ -1937,12 +1942,16 @@ static void br_multicast_start_querier(struct net_bridge *br, int br_multicast_toggle(struct net_bridge *br, unsigned long val) { struct net_bridge_port *port; + 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); + if (err) + goto unlock; + br_opt_toggle(br, BROPT_MULTICAST_ENABLED, !!val); if (!br_opt_get(br, BROPT_MULTICAST_ENABLED)) goto unlock; @@ -1957,7 +1966,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val) unlock: spin_unlock_bh(&br->multicast_lock); - return 0; + return err; } bool br_multicast_enabled(const struct net_device *dev)
Some Ethernet switches might not be able to support disabling multicast flooding globally when e.g: several bridges span the same physical device, propagate the return value of br_mc_disabled_update() such that this propagates correctly to user-space. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/bridge/br_multicast.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)