[PATCH RFC 0/3] net: bridge: set forwarding state when unbridging switchdev ports

[PATCH RFC 0/3] net: bridge: set forwarding state when unbridging switchdev ports

[Vivien Didelot]

When a port leaves a bridge, the bridge layer sets its STP state to Disabled.

But switchdev users seem to need to restore its state to Forwarding to keep the
communication with the CPU port functional (see DSA and Rocker for instance).

br_set_state() tries to set the switchdev port state, if correctly implemented.
So call it within del_nbp() to restore the Forwarding state on port removal.

Does that make sense or are there scenarios where we don't want this behavior?

Thanks,

Vivien Didelot (3):
  net: bridge: log state when setting it
  net: bridge: set forwarding state on port removal
  net: dsa: remove dsa_slave_stp_update

 net/bridge/br_if.c  |  5 +++++
 net/bridge/br_stp.c |  5 ++---
 net/dsa/slave.c     | 17 -----------------
 3 files changed, 7 insertions(+), 20 deletions(-)

-- 
2.7.1

[PATCH RFC 1/3] net: bridge: log state when setting it

[Vivien Didelot]

Every call to br_set_state is followed by a call to br_log_state.
Directly call it within br_set_state instead.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_stp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index b3cca12..077afca 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -52,6 +52,8 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
 	if (err && err != -EOPNOTSUPP)
 		br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
 				(unsigned int) p->port_no, p->dev->name);
+	else
+		br_log_state(p);
 }
 
 /* called under bridge lock */
@@ -126,7 +128,6 @@ static void br_root_port_block(const struct net_bridge *br,
 		  (unsigned int) p->port_no, p->dev->name);
 
 	br_set_state(p, BR_STATE_LISTENING);
-	br_log_state(p);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
 	if (br->forward_delay > 0)
@@ -407,7 +408,6 @@ static void br_make_blocking(struct net_bridge_port *p)
 			br_topology_change_detection(p->br);
 
 		br_set_state(p, BR_STATE_BLOCKING);
-		br_log_state(p);
 		br_ifinfo_notify(RTM_NEWLINK, p);
 
 		del_timer(&p->forward_delay_timer);
@@ -431,7 +431,6 @@ static void br_make_forwarding(struct net_bridge_port *p)
 	else
 		br_set_state(p, BR_STATE_LEARNING);
 
-	br_log_state(p);
 	br_ifinfo_notify(RTM_NEWLINK, p);
 
 	if (br->forward_delay != 0)
-- 
2.7.1

Re: [PATCH RFC 1/3] net: bridge: log state when setting it

[Ido Schimmel]

Fri, Feb 12, 2016 at 11:34:18PM IST, vivien.didelot@savoirfairelinux.com wrote:

Hi Vivien,

>Every call to br_set_state is followed by a call to br_log_state.
>Directly call it within br_set_state instead.
>
>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>---

Maybe I'm missing something, but there are other instances of this
pattern throughout the code, so why not convert them as well? See
br_set_port_state() for example.

Thanks.

> net/bridge/br_stp.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
>index b3cca12..077afca 100644
>--- a/net/bridge/br_stp.c
>+++ b/net/bridge/br_stp.c
>@@ -52,6 +52,8 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
> 	if (err && err != -EOPNOTSUPP)
> 		br_warn(p->br, "error setting offload STP state on port %u(%s)\n",
> 				(unsigned int) p->port_no, p->dev->name);
>+	else
>+		br_log_state(p);
> }
> 
> /* called under bridge lock */
>@@ -126,7 +128,6 @@ static void br_root_port_block(const struct net_bridge *br,
> 		  (unsigned int) p->port_no, p->dev->name);
> 
> 	br_set_state(p, BR_STATE_LISTENING);
>-	br_log_state(p);
> 	br_ifinfo_notify(RTM_NEWLINK, p);
> 
> 	if (br->forward_delay > 0)
>@@ -407,7 +408,6 @@ static void br_make_blocking(struct net_bridge_port *p)
> 			br_topology_change_detection(p->br);
> 
> 		br_set_state(p, BR_STATE_BLOCKING);
>-		br_log_state(p);
> 		br_ifinfo_notify(RTM_NEWLINK, p);
> 
> 		del_timer(&p->forward_delay_timer);
>@@ -431,7 +431,6 @@ static void br_make_forwarding(struct net_bridge_port *p)
> 	else
> 		br_set_state(p, BR_STATE_LEARNING);
> 
>-	br_log_state(p);
> 	br_ifinfo_notify(RTM_NEWLINK, p);
> 
> 	if (br->forward_delay != 0)
>-- 
>2.7.1
>

Re: [PATCH RFC 1/3] net: bridge: log state when setting it

[Vivien Didelot]

Hi Ido,

Ido Schimmel <idosch@mellanox.com> writes:

> Fri, Feb 12, 2016 at 11:34:18PM IST, vivien.didelot@savoirfairelinux.com wrote:
>
> Hi Vivien,
>
>>Every call to br_set_state is followed by a call to br_log_state.
>>Directly call it within br_set_state instead.
>>
>>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>---
>
> Maybe I'm missing something, but there are other instances of this
> pattern throughout the code, so why not convert them as well? See
> br_set_port_state() for example.

Good catch, I'll factorize that and send this as a unique patch then.

Thanks,
-v

[PATCH RFC 2/3] net: bridge: set forwarding state on port removal

[Vivien Didelot]

When a port leaves a bridge, the bridge layer puts its STP state to
Disabled. If the port is part of an hardware switch, the mode needs to
be set to Forwarding in order to restore communication with the CPU.

Call br_set_state() in del_nbp(), which only affects switchdev users.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/bridge/br_if.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c367b3e..93e7b24 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -251,6 +251,11 @@ static void del_nbp(struct net_bridge_port *p)
 
 	nbp_vlan_flush(p);
 	br_fdb_delete_by_port(br, p, 0, 1);
+
+	/* If the port is part of an hardware switch, set its STP state to
+	 * forwarding to restore communication with the CPU port.
+	 */
+	br_set_state(p, BR_STATE_FORWARDING);
 	switchdev_deferred_process();
 
 	nbp_update_port_count(br);
-- 
2.7.1

Re: [PATCH RFC 2/3] net: bridge: set forwarding state on port removal

[Ido Schimmel]

Fri, Feb 12, 2016 at 11:34:19PM IST, vivien.didelot@savoirfairelinux.com wrote:

Hi Vivien,

>When a port leaves a bridge, the bridge layer puts its STP state to
>Disabled. If the port is part of an hardware switch, the mode needs to
>be set to Forwarding in order to restore communication with the CPU.
>
>Call br_set_state() in del_nbp(), which only affects switchdev users.
>
>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>---

If you have a LAG device in the bridge and one of the ports leaves the
LAG, then the bridge's teardown sequence isn't invoked and you need to
do the cleanup yourself, by calling your dsa_slave_bridge_port_leave()
function (just an example, I know you don't currently support this).

With this change, you'll have to call both the
dsa_slave_bridge_port_leave() function and dsa_slave_stp_update()
instead of always calling the same one when leaving a bridge.

Also, I think this kind of things should be done by drivers, as it's
specific to them. For example, in the case of a physical port, this
wouldn't benefit mlxsw at all, as STP state is a per-VLAN setting and
since VLANs were already flushed in nbp_vlan_flush(), then nothing would
happen.

Thanks!

> net/bridge/br_if.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
>diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>index c367b3e..93e7b24 100644
>--- a/net/bridge/br_if.c
>+++ b/net/bridge/br_if.c
>@@ -251,6 +251,11 @@ static void del_nbp(struct net_bridge_port *p)
> 
> 	nbp_vlan_flush(p);
> 	br_fdb_delete_by_port(br, p, 0, 1);
>+
>+	/* If the port is part of an hardware switch, set its STP state to
>+	 * forwarding to restore communication with the CPU port.
>+	 */
>+	br_set_state(p, BR_STATE_FORWARDING);
> 	switchdev_deferred_process();
> 
> 	nbp_update_port_count(br);
>-- 
>2.7.1
>

Re: [PATCH RFC 2/3] net: bridge: set forwarding state on port removal

[Vivien Didelot]

Hi Ido,

Ido Schimmel <idosch@mellanox.com> writes:

> Fri, Feb 12, 2016 at 11:34:19PM IST, vivien.didelot@savoirfairelinux.com wrote:
>
> Hi Vivien,
>
>>When a port leaves a bridge, the bridge layer puts its STP state to
>>Disabled. If the port is part of an hardware switch, the mode needs to
>>be set to Forwarding in order to restore communication with the CPU.
>>
>>Call br_set_state() in del_nbp(), which only affects switchdev users.
>>
>>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>>---
>
> If you have a LAG device in the bridge and one of the ports leaves the
> LAG, then the bridge's teardown sequence isn't invoked and you need to
> do the cleanup yourself, by calling your dsa_slave_bridge_port_leave()
> function (just an example, I know you don't currently support this).
>
> With this change, you'll have to call both the
> dsa_slave_bridge_port_leave() function and dsa_slave_stp_update()
> instead of always calling the same one when leaving a bridge.
>
> Also, I think this kind of things should be done by drivers, as it's
> specific to them. For example, in the case of a physical port, this
> wouldn't benefit mlxsw at all, as STP state is a per-VLAN setting and
> since VLANs were already flushed in nbp_vlan_flush(), then nothing would
> happen.

Thanks for the explanation! I drop this RFC then.

Best,
-v

[PATCH RFC 3/3] net: dsa: remove dsa_slave_stp_update

[Vivien Didelot]

When a port leaves a bridge, the bridge layer now restores its STP state
to Forwarding, if the device belong to a switchdev user.

Thus remove this redundant action from the DSA layer.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 net/dsa/slave.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 40b9ca7..2b3774e 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -410,18 +410,6 @@ static u32 dsa_slave_br_port_mask(struct dsa_switch *ds,
 	return mask;
 }
 
-static int dsa_slave_stp_update(struct net_device *dev, u8 state)
-{
-	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct dsa_switch *ds = p->parent;
-	int ret = -EOPNOTSUPP;
-
-	if (ds->drv->port_stp_update)
-		ret = ds->drv->port_stp_update(ds, p->port, state);
-
-	return ret;
-}
-
 static int dsa_slave_port_attr_set(struct net_device *dev,
 				   const struct switchdev_attr *attr,
 				   struct switchdev_trans *trans)
@@ -552,11 +540,6 @@ static int dsa_slave_bridge_port_leave(struct net_device *dev)
 
 	p->bridge_dev = NULL;
 
-	/* Port left the bridge, put in BR_STATE_DISABLED by the bridge layer,
-	 * so allow it to be in BR_STATE_FORWARDING to be kept functional
-	 */
-	dsa_slave_stp_update(dev, BR_STATE_FORWARDING);
-
 	return ret;
 }
 
-- 
2.7.1