| Message ID | 54E76912.3090203@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Feb 20, 2015 at 9:04 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 20/02/15 07:03, Scott Feldman wrote: >> On Fri, Feb 20, 2015 at 2:00 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>> Fri, Feb 20, 2015 at 05:45:01AM CET, sfeldma@gmail.com wrote: >>>> On Wed, Feb 18, 2015 at 11:39 PM, Florian Fainelli <f.fainelli@gmail.com> >>>> wrote: >>>> >>>>> Hi, >>>>> >>>>> It just occured to me that the following sequence: >>>>> >>>>> brctl addbr br0 >>>>> brctl addif br0 port0 >>>>> ... STP happens >>>>> brctl delif br0 port0 >>>>> >>>>> will leave port0 in STP disabled state, because the bridge code will >>>>> set the STP state to DISABLED, and only a down/up sequence can bring >>>>> it back to FORWARDING. >>>>> >>>>> Is this something that we should somehow fix? As an user it seems a >>>>> little convoluted having to do a down/up sequence to restore things. I >>>>> believe however that it is valid for the bridge layer to mark a port >>>>> as DISABLED when removing it. This is typically not noticed or even >>>>> remotely a problem with software bridges because we cannot enforce an >>>>> actual STP state at the HW level. >>>>> >>>>> Let me know your thoughts. >>>>> >>>>> >>>> The fix in rocker would be: >>>> >>>> diff --git a/drivers/net/ethernet/rocker/rocker.c >>>> b/drivers/net/ethernet/rocker/rocker.c >>>> index 34389b6a..e2004fb 100644 >>>> --- a/drivers/net/ethernet/rocker/rocker.c >>>> +++ b/drivers/net/ethernet/rocker/rocker.c >>>> @@ -4456,8 +4456,10 @@ static int rocker_port_bridge_leave(struct >>>> rocker_port *rocker_port) >>>> rocker_port_internal_vlan_id_get(rocker_port, >>>> rocker_port->dev->ifindex); >>>> err = rocker_port_vlan(rocker_port, 0, 0); >>>> + if (err) >>>> + return err; >>>> >>>> - return err; >>>> + return rocker_port_stp_update(rocker_port, BR_STATE_FORWARDING); >>>> } >>>> >>>> >>>> This will return the port back to it's initial state of >>>> BR_STATE_FORWARDING, after it's removed from the bridge. >>>> >>>> I'll include this patch in the rocker pile to be pushed later. >>>> >>>> -scott >>> >>> >>> I'm not sure, but wouldn't it be nicer it the bridge code would set >>> state to disabled before the port is removed from the bridge? >> >> When the port is removed from a bridge, for example with brctl delif, >> the bridge driver puts port in BR_STATE_DISABLED and then sends >> netdevice event NETDEV_CHANGEUPPER. In response to >> NETDEV_CHANGEUPPER, the rocker driver is returning port back to >> BR_STATE_FORWARDING (the initial state for an un-bridged port). So >> this preserves bridge behavior for non-switchdev uses. Does this >> answer the question, or did I miss understand your question? > > I think what we want is a solution at the bridge level, we have rocker > now updating the STP state to BR_STATE_FORWARDING when a given > rocker_port leaves a bridge, and I also had a similar change in DSA. > > Something like this maybe (untested): > > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index b087d278c679..d693a2a10b3c 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p) > > spin_lock_bh(&br->lock); > br_stp_disable_port(p); > + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD) > + br_set_state(p, BR_STATE_FORWARDING); > spin_unlock_bh(&br->lock); > > br_ifinfo_notify(RTM_DELLINK, p); Acked-by: Scott Feldman <sfeldma@gmail.com> I like it. I tested your version with rocker and it works great. (I removed my version). Would you push this one when things open up? -scott -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-02-21 11:43 GMT-08:00 Scott Feldman <sfeldma@gmail.com>: > On Fri, Feb 20, 2015 at 9:04 AM, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 20/02/15 07:03, Scott Feldman wrote: >>> On Fri, Feb 20, 2015 at 2:00 AM, Jiri Pirko <jiri@resnulli.us> wrote: >>>> Fri, Feb 20, 2015 at 05:45:01AM CET, sfeldma@gmail.com wrote: >>>>> On Wed, Feb 18, 2015 at 11:39 PM, Florian Fainelli <f.fainelli@gmail.com> >>>>> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> It just occured to me that the following sequence: >>>>>> >>>>>> brctl addbr br0 >>>>>> brctl addif br0 port0 >>>>>> ... STP happens >>>>>> brctl delif br0 port0 >>>>>> >>>>>> will leave port0 in STP disabled state, because the bridge code will >>>>>> set the STP state to DISABLED, and only a down/up sequence can bring >>>>>> it back to FORWARDING. >>>>>> >>>>>> Is this something that we should somehow fix? As an user it seems a >>>>>> little convoluted having to do a down/up sequence to restore things. I >>>>>> believe however that it is valid for the bridge layer to mark a port >>>>>> as DISABLED when removing it. This is typically not noticed or even >>>>>> remotely a problem with software bridges because we cannot enforce an >>>>>> actual STP state at the HW level. >>>>>> >>>>>> Let me know your thoughts. >>>>>> >>>>>> >>>>> The fix in rocker would be: >>>>> >>>>> diff --git a/drivers/net/ethernet/rocker/rocker.c >>>>> b/drivers/net/ethernet/rocker/rocker.c >>>>> index 34389b6a..e2004fb 100644 >>>>> --- a/drivers/net/ethernet/rocker/rocker.c >>>>> +++ b/drivers/net/ethernet/rocker/rocker.c >>>>> @@ -4456,8 +4456,10 @@ static int rocker_port_bridge_leave(struct >>>>> rocker_port *rocker_port) >>>>> rocker_port_internal_vlan_id_get(rocker_port, >>>>> rocker_port->dev->ifindex); >>>>> err = rocker_port_vlan(rocker_port, 0, 0); >>>>> + if (err) >>>>> + return err; >>>>> >>>>> - return err; >>>>> + return rocker_port_stp_update(rocker_port, BR_STATE_FORWARDING); >>>>> } >>>>> >>>>> >>>>> This will return the port back to it's initial state of >>>>> BR_STATE_FORWARDING, after it's removed from the bridge. >>>>> >>>>> I'll include this patch in the rocker pile to be pushed later. >>>>> >>>>> -scott >>>> >>>> >>>> I'm not sure, but wouldn't it be nicer it the bridge code would set >>>> state to disabled before the port is removed from the bridge? >>> >>> When the port is removed from a bridge, for example with brctl delif, >>> the bridge driver puts port in BR_STATE_DISABLED and then sends >>> netdevice event NETDEV_CHANGEUPPER. In response to >>> NETDEV_CHANGEUPPER, the rocker driver is returning port back to >>> BR_STATE_FORWARDING (the initial state for an un-bridged port). So >>> this preserves bridge behavior for non-switchdev uses. Does this >>> answer the question, or did I miss understand your question? >> >> I think what we want is a solution at the bridge level, we have rocker >> now updating the STP state to BR_STATE_FORWARDING when a given >> rocker_port leaves a bridge, and I also had a similar change in DSA. >> >> Something like this maybe (untested): >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index b087d278c679..d693a2a10b3c 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p) >> >> spin_lock_bh(&br->lock); >> br_stp_disable_port(p); >> + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD) >> + br_set_state(p, BR_STATE_FORWARDING); >> spin_unlock_bh(&br->lock); >> >> br_ifinfo_notify(RTM_DELLINK, p); > > Acked-by: Scott Feldman <sfeldma@gmail.com> > > I like it. I tested your version with rocker and it works great. (I > removed my version). Would you push this one when things open up? Ok, thanks for testing!
On Fri, 20 Feb 2015 09:04:18 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 20/02/15 07:03, Scott Feldman wrote: > > On Fri, Feb 20, 2015 at 2:00 AM, Jiri Pirko <jiri@resnulli.us> wrote: > >> Fri, Feb 20, 2015 at 05:45:01AM CET, sfeldma@gmail.com wrote: > >>> On Wed, Feb 18, 2015 at 11:39 PM, Florian Fainelli <f.fainelli@gmail.com> > >>> wrote: > >>> > >>>> Hi, > >>>> > >>>> It just occured to me that the following sequence: > >>>> > >>>> brctl addbr br0 > >>>> brctl addif br0 port0 > >>>> ... STP happens > >>>> brctl delif br0 port0 > >>>> > >>>> will leave port0 in STP disabled state, because the bridge code will > >>>> set the STP state to DISABLED, and only a down/up sequence can bring > >>>> it back to FORWARDING. > >>>> > >>>> Is this something that we should somehow fix? As an user it seems a > >>>> little convoluted having to do a down/up sequence to restore things. I > >>>> believe however that it is valid for the bridge layer to mark a port > >>>> as DISABLED when removing it. This is typically not noticed or even > >>>> remotely a problem with software bridges because we cannot enforce an > >>>> actual STP state at the HW level. > >>>> > >>>> Let me know your thoughts. > >>>> > >>>> > >>> The fix in rocker would be: > >>> > >>> diff --git a/drivers/net/ethernet/rocker/rocker.c > >>> b/drivers/net/ethernet/rocker/rocker.c > >>> index 34389b6a..e2004fb 100644 > >>> --- a/drivers/net/ethernet/rocker/rocker.c > >>> +++ b/drivers/net/ethernet/rocker/rocker.c > >>> @@ -4456,8 +4456,10 @@ static int rocker_port_bridge_leave(struct > >>> rocker_port *rocker_port) > >>> rocker_port_internal_vlan_id_get(rocker_port, > >>> rocker_port->dev->ifindex); > >>> err = rocker_port_vlan(rocker_port, 0, 0); > >>> + if (err) > >>> + return err; > >>> > >>> - return err; > >>> + return rocker_port_stp_update(rocker_port, BR_STATE_FORWARDING); > >>> } > >>> > >>> > >>> This will return the port back to it's initial state of > >>> BR_STATE_FORWARDING, after it's removed from the bridge. > >>> > >>> I'll include this patch in the rocker pile to be pushed later. > >>> > >>> -scott > >> > >> > >> I'm not sure, but wouldn't it be nicer it the bridge code would set > >> state to disabled before the port is removed from the bridge? > > > > When the port is removed from a bridge, for example with brctl delif, > > the bridge driver puts port in BR_STATE_DISABLED and then sends > > netdevice event NETDEV_CHANGEUPPER. In response to > > NETDEV_CHANGEUPPER, the rocker driver is returning port back to > > BR_STATE_FORWARDING (the initial state for an un-bridged port). So > > this preserves bridge behavior for non-switchdev uses. Does this > > answer the question, or did I miss understand your question? > > I think what we want is a solution at the bridge level, we have rocker > now updating the STP state to BR_STATE_FORWARDING when a given > rocker_port leaves a bridge, and I also had a similar change in DSA. > > Something like this maybe (untested): > > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index b087d278c679..d693a2a10b3c 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p) > > spin_lock_bh(&br->lock); > br_stp_disable_port(p); > + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD) > + br_set_state(p, BR_STATE_FORWARDING); > spin_unlock_bh(&br->lock); When port is deleted from bridge, it is no longer part of the bridge, so as far as it is concerned there should be no STP state! The next thing that is going to happen is free (after RCU grace period). This patch seems like it is setting something on an object that is destined for the trash heap. Anything doing following of bridge STP state should see the DELLINK event and respond appropriately. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 21, 2015 at 2:58 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Fri, 20 Feb 2015 09:04:18 -0800 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> On 20/02/15 07:03, Scott Feldman wrote: >> > On Fri, Feb 20, 2015 at 2:00 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> >> Fri, Feb 20, 2015 at 05:45:01AM CET, sfeldma@gmail.com wrote: >> >>> On Wed, Feb 18, 2015 at 11:39 PM, Florian Fainelli <f.fainelli@gmail.com> >> >>> wrote: >> >>> >> >>>> Hi, >> >>>> >> >>>> It just occured to me that the following sequence: >> >>>> >> >>>> brctl addbr br0 >> >>>> brctl addif br0 port0 >> >>>> ... STP happens >> >>>> brctl delif br0 port0 >> >>>> >> >>>> will leave port0 in STP disabled state, because the bridge code will >> >>>> set the STP state to DISABLED, and only a down/up sequence can bring >> >>>> it back to FORWARDING. >> >>>> >> >>>> Is this something that we should somehow fix? As an user it seems a >> >>>> little convoluted having to do a down/up sequence to restore things. I >> >>>> believe however that it is valid for the bridge layer to mark a port >> >>>> as DISABLED when removing it. This is typically not noticed or even >> >>>> remotely a problem with software bridges because we cannot enforce an >> >>>> actual STP state at the HW level. >> >>>> >> >>>> Let me know your thoughts. >> >>>> >> >>>> >> >>> The fix in rocker would be: >> >>> >> >>> diff --git a/drivers/net/ethernet/rocker/rocker.c >> >>> b/drivers/net/ethernet/rocker/rocker.c >> >>> index 34389b6a..e2004fb 100644 >> >>> --- a/drivers/net/ethernet/rocker/rocker.c >> >>> +++ b/drivers/net/ethernet/rocker/rocker.c >> >>> @@ -4456,8 +4456,10 @@ static int rocker_port_bridge_leave(struct >> >>> rocker_port *rocker_port) >> >>> rocker_port_internal_vlan_id_get(rocker_port, >> >>> rocker_port->dev->ifindex); >> >>> err = rocker_port_vlan(rocker_port, 0, 0); >> >>> + if (err) >> >>> + return err; >> >>> >> >>> - return err; >> >>> + return rocker_port_stp_update(rocker_port, BR_STATE_FORWARDING); >> >>> } >> >>> >> >>> >> >>> This will return the port back to it's initial state of >> >>> BR_STATE_FORWARDING, after it's removed from the bridge. >> >>> >> >>> I'll include this patch in the rocker pile to be pushed later. >> >>> >> >>> -scott >> >> >> >> >> >> I'm not sure, but wouldn't it be nicer it the bridge code would set >> >> state to disabled before the port is removed from the bridge? >> > >> > When the port is removed from a bridge, for example with brctl delif, >> > the bridge driver puts port in BR_STATE_DISABLED and then sends >> > netdevice event NETDEV_CHANGEUPPER. In response to >> > NETDEV_CHANGEUPPER, the rocker driver is returning port back to >> > BR_STATE_FORWARDING (the initial state for an un-bridged port). So >> > this preserves bridge behavior for non-switchdev uses. Does this >> > answer the question, or did I miss understand your question? >> >> I think what we want is a solution at the bridge level, we have rocker >> now updating the STP state to BR_STATE_FORWARDING when a given >> rocker_port leaves a bridge, and I also had a similar change in DSA. >> >> Something like this maybe (untested): >> >> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c >> index b087d278c679..d693a2a10b3c 100644 >> --- a/net/bridge/br_if.c >> +++ b/net/bridge/br_if.c >> @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p) >> >> spin_lock_bh(&br->lock); >> br_stp_disable_port(p); >> + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD) >> + br_set_state(p, BR_STATE_FORWARDING); >> spin_unlock_bh(&br->lock); > > When port is deleted from bridge, it is no longer part of the > bridge, so as far as it is concerned there should be no STP state! Your point is valid. It seems we're back to the port driver doing what it needs to do so the port can pass traffic once it leaves the bridge. It looks like this needs to be port-driver specific. -scott -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index b087d278c679..d693a2a10b3c 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -242,6 +242,8 @@ static void del_nbp(struct net_bridge_port *p) spin_lock_bh(&br->lock); br_stp_disable_port(p); + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD) + br_set_state(p, BR_STATE_FORWARDING); spin_unlock_bh(&br->lock); br_ifinfo_notify(RTM_DELLINK, p);