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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---

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.

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 <redacted>
---

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!

Hi Ido,

Ido Schimmel [off-list ref] writes:

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

Hi Vivien,

quoted

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 <redacted>
---

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

Hi Ido,

Ido Schimmel [off-list ref] writes:

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

Hi Vivien,

quoted

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 <redacted>
---

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