Skip to contentSkip to conversation

mlxsw: Set port STP state on bridge enslavement

PATCH Applied Petr Machata 3 participants 5 messages Aug 8, 2023 – Aug 9, 2023

Subsystems (4)
lore
Share & tools
lore

Petr Machata

author maintainer
#0 lore raw

cover mlxsw: Set port STP state on bridge enslavement

When the first port joins a LAG that already has a bridge upper, an instance of struct mlxsw_sp_bridge_port is created for the LAG to keep track of it as a bridge port. The bridge_port's STP state is initialized to BR_STATE_DISABLED. This made sense previously, because mlxsw would only ever allow a port to join a LAG if the LAG had no uppers. Thus if a bridge_port was instantiated, it must have been because the LAG as such is joining a bridge, and the STP state is correspondingly disabled.

However as of commit 2c5ffe8d7226 ("mlxsw: spectrum: Permit enslavement to netdevices with uppers"), mlxsw allows a port to join a LAG that is already a member of a bridge. The STP state may be different than disabled in that case. Initialize it properly by querying the actual state.

This bug may cause an issue as traffic on ports attached to a bridged LAG gets dropped on ingress with discard_ingress_general counter bumped.

The above fix in patch #1. Patch #2 contains a selftest that would sporadically reproduce the issue.

Petr Machata (2):
mlxsw: Set port STP state on bridge enslavement
selftests: mlxsw: router_bridge_lag: Add a new selftest

 .../mellanox/mlxsw/spectrum_switchdev.c       |  2 +-
 .../drivers/net/mlxsw/router_bridge_lag.sh    | 50 +++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/mlxsw/router_bridge_lag.sh
Signature · 1 line
2.41.0
author maintainer
#1 lore raw

PATCH 1/2 mlxsw: Set port STP state on bridge enslavement

When the first port joins a LAG that already has a bridge upper, an instance of struct mlxsw_sp_bridge_port is created for the LAG to keep track of it as a bridge port. The bridge_port's STP state is initialized to BR_STATE_DISABLED. This made sense previously, because mlxsw would only ever allow a port to join a LAG if the LAG had no uppers. Thus if a bridge_port was instantiated, it must have been because the LAG as such is joining a bridge, and the STP state is correspondingly disabled.

However as of commit 2c5ffe8d7226 ("mlxsw: spectrum: Permit enslavement to netdevices with uppers"), mlxsw allows a port to join a LAG that is already a member of a bridge. The STP state may be different than disabled in that case. Initialize it properly by querying the actual state.

This bug may cause an issue as traffic on ports attached to a bridged LAG gets dropped on ingress with discard_ingress_general counter bumped.

Fixes c6514f3627a0 ("Merge branch 'mlxsw-enslavement'")
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c +1 -1
index 5376d4af5f91..ad90f7f5eeb7 100644
@@ -490,7 +490,7 @@ mlxsw_sp_bridge_port_create(struct mlxsw_sp_bridge_device *bridge_device,
 		bridge_port->system_port = mlxsw_sp_port->local_port;
 	bridge_port->dev = brport_dev;
 	bridge_port->bridge_device = bridge_device;
-	bridge_port->stp_state = BR_STATE_DISABLED;
+	bridge_port->stp_state = br_port_get_stp_state(brport_dev);
 	bridge_port->flags = BR_LEARNING | BR_FLOOD | BR_LEARNING_SYNC |
 			     BR_MCAST_FLOOD;
 	INIT_LIST_HEAD(&bridge_port->vlans_list);
Signature · 1 line
2.41.0
author maintainer
#2 lore raw

PATCH 2/2 selftests: mlxsw: router_bridge_lag: Add a new selftest

In reply to Petr Machata

Add a selftest to verify enslavement to a LAG with upper after fresh devlink reload.

---
 .../drivers/net/mlxsw/router_bridge_lag.sh    | 50 +++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/mlxsw/router_bridge_lag.sh
tools/testing/selftests/drivers/net/mlxsw/router_bridge_lag.sh new file +50 -0
new file mode 100755
index 000000000000..6ce317cfaf9b
@@ -0,0 +1,50 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# Test enslavement to LAG with a clean slate.
+# See $lib_dir/router_bridge_lag.sh for further details.
+
+ALL_TESTS="
+	config_devlink_reload
+	config_enslave_h1
+	config_enslave_h2
+	config_enslave_h3
+	config_enslave_h4
+	config_enslave_swp1
+	config_enslave_swp2
+	config_enslave_swp3
+	config_enslave_swp4
+	config_wait
+	ping_ipv4
+	ping_ipv6
+"
+
+config_devlink_reload()
+{
+	log_info "Devlink reload"
+	devlink_reload
+}
+
+config_enslave_h1()
+{
+	config_enslave $h1 lag1
+}
+
+config_enslave_h2()
+{
+	config_enslave $h2 lag4
+}
+
+config_enslave_h3()
+{
+	config_enslave $h3 lag4
+}
+
+config_enslave_h4()
+{
+	config_enslave $h4 lag1
+}
+
+lib_dir=$(dirname $0)/../../../net/forwarding
+EXTRA_SOURCE="source $lib_dir/devlink_lib.sh"
+source $lib_dir/router_bridge_lag.sh
Signature · 1 line
2.41.0
reviewer Reviewed-by on the v1 series
#3 lore raw
In reply to Petr Machata
Quoted 16 lines · Petr Machata

When the first port joins a LAG that already has a bridge upper, an instance of struct mlxsw_sp_bridge_port is created for the LAG to keep track of it as a bridge port. The bridge_port's STP state is initialized to BR_STATE_DISABLED. This made sense previously, because mlxsw would only ever allow a port to join a LAG if the LAG had no uppers. Thus if a bridge_port was instantiated, it must have been because the LAG as such is joining a bridge, and the STP state is correspondingly disabled.

However as of commit 2c5ffe8d7226 ("mlxsw: spectrum: Permit enslavement to netdevices with uppers"), mlxsw allows a port to join a LAG that is already a member of a bridge. The STP state may be different than disabled in that case. Initialize it properly by querying the actual state.

This bug may cause an issue as traffic on ports attached to a bridged LAG gets dropped on ingress with discard_ingress_general counter bumped.

Quoted 6 lines

The above fix in patch #1. Patch #2 contains a selftest that would sporadically reproduce the issue.

Petr Machata (2):
mlxsw: Set port STP state on bridge enslavement
selftests: mlxsw: router_bridge_lag: Add a new selftest

#4 lore raw
In reply to Petr Machata
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Tue, 8 Aug 2023 15:18:14 +0200 you wrote:

When the first port joins a LAG that already has a bridge upper, an instance of struct mlxsw_sp_bridge_port is created for the LAG to keep track of it as a bridge port. The bridge_port's STP state is initialized to BR_STATE_DISABLED. This made sense previously, because mlxsw would only ever allow a port to join a LAG if the LAG had no uppers. Thus if a bridge_port was instantiated, it must have been because the LAG as such is joining a bridge, and the STP state is correspondingly disabled.

[...]

Here is the summary with links:
- [net-next,1/2] mlxsw: Set port STP state on bridge enslavement

    https://git.kernel.org/netdev/net-next/c/a76ca8afd45a
  - [net-next,2/2] selftests: mlxsw: router_bridge_lag: Add a new selftest
    https://git.kernel.org/netdev/net-next/c/aae5bb8d18d8

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html