Port STP state after removing port from bridge

Port STP state after removing port from bridge

[Florian Fainelli]

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.
-- 
Florian

Re: Port STP state after removing port from bridge

[roopa]

On 2/18/15, 8:39 PM, Florian Fainelli 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.
>
Just curious, Are you only talking about hw state being left it DISABLED 
state in the switchdev context ?.
If yes, then cant the switch driver who is already listening to port 
leave msgs, clear the disabled state on the port. ?

Re: Port STP state after removing port from bridge

[Florian Fainelli]

2015-02-18 20:54 GMT-08:00 roopa <roopa@cumulusnetworks.com>:
> On 2/18/15, 8:39 PM, Florian Fainelli 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.
>>
> Just curious, Are you only talking about hw state being left it DISABLED
> state in the switchdev context ?.

Right, this is in the context of DSA using NET_SWITCHDEV.

> If yes, then cant the switch driver who is already listening to port leave
> msgs, clear the disabled state on the port. ?

I guess that is definitively possible, I am not seeing that being done
for rocker which I used as a model here, maybe that needs fixing there
as well?

Thanks!
-- 
Florian

Re: Port STP state after removing port from bridge

[roopa]

On 2/18/15, 9:00 PM, Florian Fainelli wrote:
> 2015-02-18 20:54 GMT-08:00 roopa <roopa@cumulusnetworks.com>:
>> On 2/18/15, 8:39 PM, Florian Fainelli 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.
>>>
>> Just curious, Are you only talking about hw state being left it DISABLED
>> state in the switchdev context ?.
> Right, this is in the context of DSA using NET_SWITCHDEV.
>
>> If yes, then cant the switch driver who is already listening to port leave
>> msgs, clear the disabled state on the port. ?
> I guess that is definitively possible, I am not seeing that being done
> for rocker which I used as a model here, maybe that needs fixing there
> as well?

o ok, yes, the switch drivers can do it. In this case rocker can/should. 
Our driver does do the cleanup.
But if all drivers end up having this logic, it would maybe make sense 
to consider it doing it in the bridge driver delete port code.

Thanks,
Roopa

Re: Port STP state after removing port from bridge

[Scott Feldman]

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

Re: Port STP state after removing port from bridge

[Jiri Pirko]

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? 

Re: Port STP state after removing port from bridge

[Scott Feldman]

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?

-scott

Re: Port STP state after removing port from bridge

[Florian Fainelli]

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);
-- 
Florian

Re: Port STP state after removing port from bridge

[Scott Feldman]

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

Re: Port STP state after removing port from bridge

[Florian Fainelli]

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!
-- 
Florian

Re: Port STP state after removing port from bridge

[Stephen Hemminger]

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.

Re: Port STP state after removing port from bridge

[Scott Feldman]

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