| Message ID | 20190116200102.2749-2-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: management mode for bcm_sf2 | expand |
On Wed, Jan 16, 2019 at 12:00:49PM -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 | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index 3aeff0895669..09fc92541873 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -813,7 +813,7 @@ 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, > @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value) > .u.mc_disabled = !value, > }; > > - switchdev_port_attr_set(dev, &attr); > + return switchdev_port_attr_set(dev, &attr); Did you test this for veth? If switchdev ops are not defined, this returns -EOPNOTSUPP. See __br_vlan_filter_toggle() for reference > } > > 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; > > 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; You never return the error > -- > 2.17.1 >
On 1/17/19 5:47 AM, Ido Schimmel wrote: > On Wed, Jan 16, 2019 at 12:00:49PM -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 | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c >> index 3aeff0895669..09fc92541873 100644 >> --- a/net/bridge/br_multicast.c >> +++ b/net/bridge/br_multicast.c >> @@ -813,7 +813,7 @@ 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, >> @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value) >> .u.mc_disabled = !value, >> }; >> >> - switchdev_port_attr_set(dev, &attr); >> + return switchdev_port_attr_set(dev, &attr); > > Did you test this for veth? If switchdev ops are not defined, this > returns -EOPNOTSUPP. See __br_vlan_filter_toggle() for reference I did not, good catch, I will add a check on -EOPNOTSUPP and silence that error code: if (err && err != -ENOTSUPP) return err; > >> } >> >> 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; >> >> 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; > > You never return the error Huh, indeed, thanks!
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 3aeff0895669..09fc92541873 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -813,7 +813,7 @@ 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, @@ -822,11 +822,13 @@ static void br_mc_disabled_update(struct net_device *dev, bool value) .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; 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;
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 | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)