[RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Oleksij Rempel]

Current driver version is able to handle only one bridge at time.
Configuring two bridges on two different ports would end up shorting this
bridges by HW. To reproduce it:

	ip l a name br0 type bridge
	ip l a name br1 type bridge
	ip l s dev br0 up
	ip l s dev br1 up
	ip l s lan1 master br0
	ip l s dev lan1 up
	ip l s lan2 master br1
	ip l s dev lan2 up

	Ping on lan1 and get response on lan2, which should not happen.

This happened, because current driver version is storing one global "Port VLAN
Membership" and applying it to all ports which are members of any
bridge.
To solve this issue, we need to handle each port separately.

This patch is dropping the global port member storage and calculating
membership dynamically depending on STP state and bridge participation.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz8795.c    | 53 ++++-----------------
 drivers/net/dsa/microchip/ksz9477.c    | 64 ++++----------------------
 drivers/net/dsa/microchip/ksz_common.c | 56 +++++++++++++---------
 drivers/net/dsa/microchip/ksz_common.h |  1 -
 4 files changed, 51 insertions(+), 123 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 43fc3087aeb3..b5f6dff70c89 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1008,51 +1008,27 @@ static void ksz8_cfg_port_member(struct ksz_device *dev, int port, u8 member)
 static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 {
 	struct ksz_device *dev = ds->priv;
-	int forward = dev->member;
 	struct ksz_port *p;
-	int member = -1;
 	u8 data;
 
-	p = &dev->ports[port];
-
 	ksz_pread8(dev, port, P_STP_CTRL, &data);
 	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
 
 	switch (state) {
 	case BR_STATE_DISABLED:
 		data |= PORT_LEARN_DISABLE;
-		if (port < dev->phy_port_cnt)
-			member = 0;
 		break;
 	case BR_STATE_LISTENING:
 		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
-		if (port < dev->phy_port_cnt &&
-		    p->stp_state == BR_STATE_DISABLED)
-			member = dev->host_mask | p->vid_member;
 		break;
 	case BR_STATE_LEARNING:
 		data |= PORT_RX_ENABLE;
 		break;
 	case BR_STATE_FORWARDING:
 		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
-
-		/* This function is also used internally. */
-		if (port == dev->cpu_port)
-			break;
-
-		/* Port is a member of a bridge. */
-		if (dev->br_member & BIT(port)) {
-			dev->member |= BIT(port);
-			member = dev->member;
-		} else {
-			member = dev->host_mask | p->vid_member;
-		}
 		break;
 	case BR_STATE_BLOCKING:
 		data |= PORT_LEARN_DISABLE;
-		if (port < dev->phy_port_cnt &&
-		    p->stp_state == BR_STATE_DISABLED)
-			member = dev->host_mask | p->vid_member;
 		break;
 	default:
 		dev_err(ds->dev, "invalid STP state: %d\n", state);
@@ -1060,22 +1036,11 @@ static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 	}
 
 	ksz_pwrite8(dev, port, P_STP_CTRL, data);
+
+	p = &dev->ports[port];
 	p->stp_state = state;
-	/* Port membership may share register with STP state. */
-	if (member >= 0 && member != p->member)
-		ksz8_cfg_port_member(dev, port, (u8)member);
-
-	/* Check if forwarding needs to be updated. */
-	if (state != BR_STATE_FORWARDING) {
-		if (dev->br_member & BIT(port))
-			dev->member &= ~BIT(port);
-	}
 
-	/* When topology has changed the function ksz_update_port_member
-	 * should be called to modify port forwarding behavior.
-	 */
-	if (forward != dev->member)
-		ksz_update_port_member(dev, port);
+	ksz_update_port_member(dev, port);
 }
 
 static void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
@@ -1341,7 +1306,7 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port)
 
 static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
-	struct ksz_port *p = &dev->ports[port];
+	struct dsa_switch *ds = dev->ds;
 	struct ksz8 *ksz8 = dev->priv;
 	const u32 *masks;
 	u8 member;
@@ -1364,14 +1329,15 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	/* enable 802.1p priority */
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
-	if (cpu_port) {
+	if (dsa_is_cpu_port(ds, port)) {
 		if (!ksz_is_ksz88x3(dev))
 			ksz8795_cpu_interface_select(dev, port);
 
-		member = dev->port_mask;
+		member = dsa_user_ports(ds);
 	} else {
-		member = dev->host_mask | p->vid_member;
+		member = BIT(dsa_upstream_port(ds, port));
 	}
+
 	ksz8_cfg_port_member(dev, port, member);
 }
 
@@ -1392,11 +1358,9 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
 	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
 
 	p = &dev->ports[dev->cpu_port];
-	p->vid_member = dev->port_mask;
 	p->on = 1;
 
 	ksz8_port_setup(dev, dev->cpu_port, true);
-	dev->member = dev->host_mask;
 
 	for (i = 0; i < dev->phy_port_cnt; i++) {
 		p = &dev->ports[i];
@@ -1404,7 +1368,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
 		/* Initialize to non-zero so that ksz_cfg_port_member() will
 		 * be called.
 		 */
-		p->vid_member = BIT(i);
 		p->member = dev->port_mask;
 		ksz8_port_stp_state_set(ds, i, BR_STATE_DISABLED);
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 854e25f43fa7..3e7df6c0dc72 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -400,8 +400,6 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 	struct ksz_device *dev = ds->priv;
 	struct ksz_port *p = &dev->ports[port];
 	u8 data;
-	int member = -1;
-	int forward = dev->member;
 
 	ksz_pread8(dev, port, P_STP_CTRL, &data);
 	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
@@ -409,40 +407,18 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 	switch (state) {
 	case BR_STATE_DISABLED:
 		data |= PORT_LEARN_DISABLE;
-		if (port != dev->cpu_port)
-			member = 0;
 		break;
 	case BR_STATE_LISTENING:
 		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
-		if (port != dev->cpu_port &&
-		    p->stp_state == BR_STATE_DISABLED)
-			member = dev->host_mask | p->vid_member;
 		break;
 	case BR_STATE_LEARNING:
 		data |= PORT_RX_ENABLE;
 		break;
 	case BR_STATE_FORWARDING:
 		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
-
-		/* This function is also used internally. */
-		if (port == dev->cpu_port)
-			break;
-
-		member = dev->host_mask | p->vid_member;
-		mutex_lock(&dev->dev_mutex);
-
-		/* Port is a member of a bridge. */
-		if (dev->br_member & (1 << port)) {
-			dev->member |= (1 << port);
-			member = dev->member;
-		}
-		mutex_unlock(&dev->dev_mutex);
 		break;
 	case BR_STATE_BLOCKING:
 		data |= PORT_LEARN_DISABLE;
-		if (port != dev->cpu_port &&
-		    p->stp_state == BR_STATE_DISABLED)
-			member = dev->host_mask | p->vid_member;
 		break;
 	default:
 		dev_err(ds->dev, "invalid STP state: %d\n", state);
@@ -451,23 +427,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
 
 	ksz_pwrite8(dev, port, P_STP_CTRL, data);
 	p->stp_state = state;
-	mutex_lock(&dev->dev_mutex);
-	/* Port membership may share register with STP state. */
-	if (member >= 0 && member != p->member)
-		ksz9477_cfg_port_member(dev, port, (u8)member);
-
-	/* Check if forwarding needs to be updated. */
-	if (state != BR_STATE_FORWARDING) {
-		if (dev->br_member & (1 << port))
-			dev->member &= ~(1 << port);
-	}
 
-	/* When topology has changed the function ksz_update_port_member
-	 * should be called to modify port forwarding behavior.
-	 */
-	if (forward != dev->member)
-		ksz_update_port_member(dev, port);
-	mutex_unlock(&dev->dev_mutex);
+	ksz_update_port_member(dev, port);
 }
 
 static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
@@ -1168,10 +1129,10 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port)
 
 static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
-	u8 data8;
-	u8 member;
-	u16 data16;
 	struct ksz_port *p = &dev->ports[port];
+	struct dsa_switch *ds = dev->ds;
+	u8 data8, member;
+	u16 data16;
 
 	/* enable tag tail for host port */
 	if (cpu_port)
@@ -1250,12 +1211,12 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);
 		p->phydev.duplex = 1;
 	}
-	mutex_lock(&dev->dev_mutex);
-	if (cpu_port)
-		member = dev->port_mask;
+
+	if (dsa_is_cpu_port(ds, port))
+		member = dsa_user_ports(ds);
 	else
-		member = dev->host_mask | p->vid_member;
-	mutex_unlock(&dev->dev_mutex);
+		member = BIT(dsa_upstream_port(ds, port));
+
 	ksz9477_cfg_port_member(dev, port, member);
 
 	/* clear pending interrupts */
@@ -1276,8 +1237,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			const char *prev_mode;
 
 			dev->cpu_port = i;
-			dev->host_mask = (1 << dev->cpu_port);
-			dev->port_mask |= dev->host_mask;
 			p = &dev->ports[i];
 
 			/* Read from XMII register to determine host port
@@ -1312,13 +1271,10 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 
 			/* enable cpu port */
 			ksz9477_port_setup(dev, i, true);
-			p->vid_member = dev->port_mask;
 			p->on = 1;
 		}
 	}
 
-	dev->member = dev->host_mask;
-
 	for (i = 0; i < dev->port_cnt; i++) {
 		if (i == dev->cpu_port)
 			continue;
@@ -1327,8 +1283,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 		/* Initialize to non-zero so that ksz_cfg_port_member() will
 		 * be called.
 		 */
-		p->vid_member = (1 << i);
-		p->member = dev->port_mask;
 		ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
 		p->on = 1;
 		if (i < dev->phy_port_cnt)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7c2968a639eb..84b7ef511ff4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -22,21 +22,46 @@
 
 void ksz_update_port_member(struct ksz_device *dev, int port)
 {
-	struct ksz_port *p;
+	struct ksz_port *p = &dev->ports[port];
+	struct dsa_switch *ds = dev->ds;
+	const struct dsa_port *dp;
+	u8 port_member = 0;
 	int i;
 
-	for (i = 0; i < dev->port_cnt; i++) {
-		if (i == port || i == dev->cpu_port)
+	if (!dsa_is_user_port(ds, port))
+		return;
+
+	dp = dsa_to_port(ds, port);
+
+	for (i = 0; i < ds->num_ports; i++) {
+		const struct dsa_port *dpi = dsa_to_port(ds, i);
+		struct ksz_port *pi = &dev->ports[i];
+		u8 val = 0;
+
+		if (!dsa_is_user_port(ds, i))
 			continue;
-		p = &dev->ports[i];
-		if (!(dev->member & (1 << i)))
+		if (port == i)
 			continue;
+		if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev)
+			continue;
+
+		pi = &dev->ports[i];
+		if (pi->stp_state != BR_STATE_DISABLED)
+			val |= BIT(dsa_upstream_port(ds, i));
 
-		/* Port is a member of the bridge and is forwarding. */
-		if (p->stp_state == BR_STATE_FORWARDING &&
-		    p->member != dev->member)
-			dev->dev_ops->cfg_port_member(dev, i, dev->member);
+		if (pi->stp_state == BR_STATE_FORWARDING &&
+		    p->stp_state == BR_STATE_FORWARDING) {
+			val |= BIT(port);
+			port_member |= BIT(i);
+		}
+
+		dev->dev_ops->cfg_port_member(dev, i, val);
 	}
+
+	if (p->stp_state != BR_STATE_DISABLED)
+		port_member |= BIT(dsa_upstream_port(ds, port));
+
+	dev->dev_ops->cfg_port_member(dev, port, port_member);
 }
 EXPORT_SYMBOL_GPL(ksz_update_port_member);
 
@@ -175,12 +200,6 @@ EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br)
 {
-	struct ksz_device *dev = ds->priv;
-
-	mutex_lock(&dev->dev_mutex);
-	dev->br_member |= (1 << port);
-	mutex_unlock(&dev->dev_mutex);
-
 	/* port_stp_state_set() will be called after to put the port in
 	 * appropriate state so there is no need to do anything.
 	 */
@@ -192,13 +211,6 @@ EXPORT_SYMBOL_GPL(ksz_port_bridge_join);
 void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
 			   struct net_device *br)
 {
-	struct ksz_device *dev = ds->priv;
-
-	mutex_lock(&dev->dev_mutex);
-	dev->br_member &= ~(1 << port);
-	dev->member &= ~(1 << port);
-	mutex_unlock(&dev->dev_mutex);
-
 	/* port_stp_state_set() will be called after to put the port in
 	 * forwarding state so there is no need to do anything.
 	 */
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 1597c63988b4..18c9b2e34cd1 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -26,7 +26,6 @@ struct ksz_port_mib {
 
 struct ksz_port {
 	u16 member;
-	u16 vid_member;
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
 	int stp_state;
 	struct phy_device phydev;
-- 
2.30.2

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Vladimir Oltean]

On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote:
> Current driver version is able to handle only one bridge at time.
> Configuring two bridges on two different ports would end up shorting this
> bridges by HW. To reproduce it:
> 
> 	ip l a name br0 type bridge
> 	ip l a name br1 type bridge
> 	ip l s dev br0 up
> 	ip l s dev br1 up
> 	ip l s lan1 master br0
> 	ip l s dev lan1 up
> 	ip l s lan2 master br1
> 	ip l s dev lan2 up
> 
> 	Ping on lan1 and get response on lan2, which should not happen.
> 
> This happened, because current driver version is storing one global "Port VLAN
> Membership" and applying it to all ports which are members of any
> bridge.
> To solve this issue, we need to handle each port separately.
> 
> This patch is dropping the global port member storage and calculating
> membership dynamically depending on STP state and bridge participation.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Because there wasn't any restriction in the driver against multiple
bridges, I would be tempted to send to the "net" tree and provide a
Fixes: tag.

>  drivers/net/dsa/microchip/ksz8795.c    | 53 ++++-----------------
>  drivers/net/dsa/microchip/ksz9477.c    | 64 ++++----------------------
>  drivers/net/dsa/microchip/ksz_common.c | 56 +++++++++++++---------
>  drivers/net/dsa/microchip/ksz_common.h |  1 -
>  4 files changed, 51 insertions(+), 123 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 43fc3087aeb3..b5f6dff70c89 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -1008,51 +1008,27 @@ static void ksz8_cfg_port_member(struct ksz_device *dev, int port, u8 member)
>  static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>  {
>  	struct ksz_device *dev = ds->priv;
> -	int forward = dev->member;
>  	struct ksz_port *p;
> -	int member = -1;
>  	u8 data;
>  
> -	p = &dev->ports[port];
> -
>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
>  
>  	switch (state) {
>  	case BR_STATE_DISABLED:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port < dev->phy_port_cnt)
> -			member = 0;
>  		break;
>  	case BR_STATE_LISTENING:
>  		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> -		if (port < dev->phy_port_cnt &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	case BR_STATE_LEARNING:
>  		data |= PORT_RX_ENABLE;
>  		break;
>  	case BR_STATE_FORWARDING:
>  		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> -
> -		/* This function is also used internally. */
> -		if (port == dev->cpu_port)
> -			break;
> -
> -		/* Port is a member of a bridge. */
> -		if (dev->br_member & BIT(port)) {
> -			dev->member |= BIT(port);
> -			member = dev->member;
> -		} else {
> -			member = dev->host_mask | p->vid_member;
> -		}
>  		break;
>  	case BR_STATE_BLOCKING:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port < dev->phy_port_cnt &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	default:
>  		dev_err(ds->dev, "invalid STP state: %d\n", state);
> @@ -1060,22 +1036,11 @@ static void ksz8_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>  	}
>  
>  	ksz_pwrite8(dev, port, P_STP_CTRL, data);
> +
> +	p = &dev->ports[port];
>  	p->stp_state = state;
> -	/* Port membership may share register with STP state. */
> -	if (member >= 0 && member != p->member)
> -		ksz8_cfg_port_member(dev, port, (u8)member);
> -
> -	/* Check if forwarding needs to be updated. */
> -	if (state != BR_STATE_FORWARDING) {
> -		if (dev->br_member & BIT(port))
> -			dev->member &= ~BIT(port);
> -	}
>  
> -	/* When topology has changed the function ksz_update_port_member
> -	 * should be called to modify port forwarding behavior.
> -	 */
> -	if (forward != dev->member)
> -		ksz_update_port_member(dev, port);
> +	ksz_update_port_member(dev, port);
>  }
>  
>  static void ksz8_flush_dyn_mac_table(struct ksz_device *dev, int port)
> @@ -1341,7 +1306,7 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port)
>  
>  static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
> -	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
>  	struct ksz8 *ksz8 = dev->priv;
>  	const u32 *masks;
>  	u8 member;
> @@ -1364,14 +1329,15 @@ static void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  	/* enable 802.1p priority */
>  	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
>  
> -	if (cpu_port) {
> +	if (dsa_is_cpu_port(ds, port)) {
>  		if (!ksz_is_ksz88x3(dev))
>  			ksz8795_cpu_interface_select(dev, port);
>  
> -		member = dev->port_mask;
> +		member = dsa_user_ports(ds);
>  	} else {
> -		member = dev->host_mask | p->vid_member;
> +		member = BIT(dsa_upstream_port(ds, port));
>  	}
> +
>  	ksz8_cfg_port_member(dev, port, member);
>  }
>  
> @@ -1392,11 +1358,9 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
>  	ksz_cfg(dev, regs[S_TAIL_TAG_CTRL], masks[SW_TAIL_TAG_ENABLE], true);
>  
>  	p = &dev->ports[dev->cpu_port];
> -	p->vid_member = dev->port_mask;
>  	p->on = 1;
>  
>  	ksz8_port_setup(dev, dev->cpu_port, true);
> -	dev->member = dev->host_mask;
>  
>  	for (i = 0; i < dev->phy_port_cnt; i++) {
>  		p = &dev->ports[i];
> @@ -1404,7 +1368,6 @@ static void ksz8_config_cpu_port(struct dsa_switch *ds)
>  		/* Initialize to non-zero so that ksz_cfg_port_member() will
>  		 * be called.
>  		 */
> -		p->vid_member = BIT(i);
>  		p->member = dev->port_mask;
>  		ksz8_port_stp_state_set(ds, i, BR_STATE_DISABLED);
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 854e25f43fa7..3e7df6c0dc72 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -400,8 +400,6 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  	struct ksz_device *dev = ds->priv;
>  	struct ksz_port *p = &dev->ports[port];
>  	u8 data;
> -	int member = -1;
> -	int forward = dev->member;
>  
>  	ksz_pread8(dev, port, P_STP_CTRL, &data);
>  	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> @@ -409,40 +407,18 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  	switch (state) {
>  	case BR_STATE_DISABLED:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port != dev->cpu_port)
> -			member = 0;
>  		break;
>  	case BR_STATE_LISTENING:
>  		data |= (PORT_RX_ENABLE | PORT_LEARN_DISABLE);
> -		if (port != dev->cpu_port &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	case BR_STATE_LEARNING:
>  		data |= PORT_RX_ENABLE;
>  		break;
>  	case BR_STATE_FORWARDING:
>  		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
> -
> -		/* This function is also used internally. */
> -		if (port == dev->cpu_port)
> -			break;
> -
> -		member = dev->host_mask | p->vid_member;
> -		mutex_lock(&dev->dev_mutex);
> -
> -		/* Port is a member of a bridge. */
> -		if (dev->br_member & (1 << port)) {
> -			dev->member |= (1 << port);
> -			member = dev->member;
> -		}
> -		mutex_unlock(&dev->dev_mutex);
>  		break;
>  	case BR_STATE_BLOCKING:
>  		data |= PORT_LEARN_DISABLE;
> -		if (port != dev->cpu_port &&
> -		    p->stp_state == BR_STATE_DISABLED)
> -			member = dev->host_mask | p->vid_member;
>  		break;
>  	default:
>  		dev_err(ds->dev, "invalid STP state: %d\n", state);
> @@ -451,23 +427,8 @@ static void ksz9477_port_stp_state_set(struct dsa_switch *ds, int port,
>  
>  	ksz_pwrite8(dev, port, P_STP_CTRL, data);
>  	p->stp_state = state;
> -	mutex_lock(&dev->dev_mutex);
> -	/* Port membership may share register with STP state. */
> -	if (member >= 0 && member != p->member)
> -		ksz9477_cfg_port_member(dev, port, (u8)member);
> -
> -	/* Check if forwarding needs to be updated. */
> -	if (state != BR_STATE_FORWARDING) {
> -		if (dev->br_member & (1 << port))
> -			dev->member &= ~(1 << port);
> -	}
>  
> -	/* When topology has changed the function ksz_update_port_member
> -	 * should be called to modify port forwarding behavior.
> -	 */
> -	if (forward != dev->member)
> -		ksz_update_port_member(dev, port);
> -	mutex_unlock(&dev->dev_mutex);
> +	ksz_update_port_member(dev, port);
>  }
>  
>  static void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
> @@ -1168,10 +1129,10 @@ static void ksz9477_phy_errata_setup(struct ksz_device *dev, int port)
>  
>  static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  {
> -	u8 data8;
> -	u8 member;
> -	u16 data16;
>  	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
> +	u8 data8, member;
> +	u16 data16;
>  
>  	/* enable tag tail for host port */
>  	if (cpu_port)
> @@ -1250,12 +1211,12 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_1, data8);
>  		p->phydev.duplex = 1;
>  	}
> -	mutex_lock(&dev->dev_mutex);
> -	if (cpu_port)
> -		member = dev->port_mask;
> +
> +	if (dsa_is_cpu_port(ds, port))
> +		member = dsa_user_ports(ds);
>  	else
> -		member = dev->host_mask | p->vid_member;
> -	mutex_unlock(&dev->dev_mutex);
> +		member = BIT(dsa_upstream_port(ds, port));
> +
>  	ksz9477_cfg_port_member(dev, port, member);
>  
>  	/* clear pending interrupts */
> @@ -1276,8 +1237,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  			const char *prev_mode;
>  
>  			dev->cpu_port = i;
> -			dev->host_mask = (1 << dev->cpu_port);
> -			dev->port_mask |= dev->host_mask;
>  			p = &dev->ports[i];
>  
>  			/* Read from XMII register to determine host port
> @@ -1312,13 +1271,10 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  
>  			/* enable cpu port */
>  			ksz9477_port_setup(dev, i, true);
> -			p->vid_member = dev->port_mask;
>  			p->on = 1;
>  		}
>  	}
>  
> -	dev->member = dev->host_mask;
> -
>  	for (i = 0; i < dev->port_cnt; i++) {
>  		if (i == dev->cpu_port)
>  			continue;
> @@ -1327,8 +1283,6 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
>  		/* Initialize to non-zero so that ksz_cfg_port_member() will
>  		 * be called.
>  		 */
> -		p->vid_member = (1 << i);
> -		p->member = dev->port_mask;
>  		ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
>  		p->on = 1;
>  		if (i < dev->phy_port_cnt)
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 7c2968a639eb..84b7ef511ff4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -22,21 +22,46 @@
>  
>  void ksz_update_port_member(struct ksz_device *dev, int port)
>  {
> -	struct ksz_port *p;
> +	struct ksz_port *p = &dev->ports[port];
> +	struct dsa_switch *ds = dev->ds;
> +	const struct dsa_port *dp;
> +	u8 port_member = 0;
>  	int i;
>  
> -	for (i = 0; i < dev->port_cnt; i++) {
> -		if (i == port || i == dev->cpu_port)
> +	if (!dsa_is_user_port(ds, port))
> +		return;
> +
> +	dp = dsa_to_port(ds, port);
> +
> +	for (i = 0; i < ds->num_ports; i++) {
> +		const struct dsa_port *dpi = dsa_to_port(ds, i);

Other drivers name this "other_dp", I don't think that name is too bad.
Also, you can use "dsa_switch_for_each_user_port", which is also more
efficient, although you can't if you target 'stable' with this change,
since it has been introduced rather recently.

> +		struct ksz_port *pi = &dev->ports[i];

and this could be "other_p" rather than "pi".

> +		u8 val = 0;
> +
> +		if (!dsa_is_user_port(ds, i))
>  			continue;
> -		p = &dev->ports[i];
> -		if (!(dev->member & (1 << i)))
> +		if (port == i)
>  			continue;
> +		if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev)
> +			continue;
> +
> +		pi = &dev->ports[i];
> +		if (pi->stp_state != BR_STATE_DISABLED)
> +			val |= BIT(dsa_upstream_port(ds, i));
>  

This is saying:
For each {user port, other port} pair, if the other port isn't DISABLED,
then allow the user port to forward towards the CPU port of the other port.
What sense does that make? You don't support multiple CPU ports, so this
port's CPU port is that port's CPU port, and you have one more (broken)
forwarding rule towards the CPU port below.

> -		/* Port is a member of the bridge and is forwarding. */
> -		if (p->stp_state == BR_STATE_FORWARDING &&
> -		    p->member != dev->member)
> -			dev->dev_ops->cfg_port_member(dev, i, dev->member);
> +		if (pi->stp_state == BR_STATE_FORWARDING &&
> +		    p->stp_state == BR_STATE_FORWARDING) {
> +			val |= BIT(port);
> +			port_member |= BIT(i);
> +		}
> +
> +		dev->dev_ops->cfg_port_member(dev, i, val);
>  	}
> +
> +	if (p->stp_state != BR_STATE_DISABLED)
> +		port_member |= BIT(dsa_upstream_port(ds, port));

Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
data packet forwarding, not control packet forwarding, right?

> +
> +	dev->dev_ops->cfg_port_member(dev, port, port_member);
>  }
>  EXPORT_SYMBOL_GPL(ksz_update_port_member);
>  
> @@ -175,12 +200,6 @@ EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
>  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>  			 struct net_device *br)
>  {
> -	struct ksz_device *dev = ds->priv;
> -
> -	mutex_lock(&dev->dev_mutex);
> -	dev->br_member |= (1 << port);
> -	mutex_unlock(&dev->dev_mutex);
> -
>  	/* port_stp_state_set() will be called after to put the port in
>  	 * appropriate state so there is no need to do anything.
>  	 */
> @@ -192,13 +211,6 @@ EXPORT_SYMBOL_GPL(ksz_port_bridge_join);
>  void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
>  			   struct net_device *br)
>  {
> -	struct ksz_device *dev = ds->priv;
> -
> -	mutex_lock(&dev->dev_mutex);
> -	dev->br_member &= ~(1 << port);
> -	dev->member &= ~(1 << port);
> -	mutex_unlock(&dev->dev_mutex);
> -
>  	/* port_stp_state_set() will be called after to put the port in
>  	 * forwarding state so there is no need to do anything.
>  	 */
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index 1597c63988b4..18c9b2e34cd1 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -26,7 +26,6 @@ struct ksz_port_mib {
>  
>  struct ksz_port {
>  	u16 member;
> -	u16 vid_member;
>  	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
>  	int stp_state;
>  	struct phy_device phydev;
> -- 
> 2.30.2
> 

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Oleksij Rempel]

On Wed, Nov 10, 2021 at 02:36:40PM +0200, Vladimir Oltean wrote:
> On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote:
> > Current driver version is able to handle only one bridge at time.
> > Configuring two bridges on two different ports would end up shorting this
> > bridges by HW. To reproduce it:
> > 
> > 	ip l a name br0 type bridge
> > 	ip l a name br1 type bridge
> > 	ip l s dev br0 up
> > 	ip l s dev br1 up
> > 	ip l s lan1 master br0
> > 	ip l s dev lan1 up
> > 	ip l s lan2 master br1
> > 	ip l s dev lan2 up
> > 
> > 	Ping on lan1 and get response on lan2, which should not happen.
> > 
> > This happened, because current driver version is storing one global "Port VLAN
> > Membership" and applying it to all ports which are members of any
> > bridge.
> > To solve this issue, we need to handle each port separately.
> > 
> > This patch is dropping the global port member storage and calculating
> > membership dynamically depending on STP state and bridge participation.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> 
> Because there wasn't any restriction in the driver against multiple
> bridges, I would be tempted to send to the "net" tree and provide a
> Fixes: tag.

This patch looks too intrusive to me. It will be hard to backport it to
older versions. How about have two patches: add limit to one bridge for
net and add support for multiple bridges for net-next?

> > +	dp = dsa_to_port(ds, port);
> > +
> > +	for (i = 0; i < ds->num_ports; i++) {
> > +		const struct dsa_port *dpi = dsa_to_port(ds, i);
> 
> Other drivers name this "other_dp", I don't think that name is too bad.
> Also, you can use "dsa_switch_for_each_user_port", which is also more
> efficient, although you can't if you target 'stable' with this change,
> since it has been introduced rather recently.

ok

> > +		struct ksz_port *pi = &dev->ports[i];
> 
> and this could be "other_p" rather than "pi".

ok

> > +		u8 val = 0;
> > +
> > +		if (!dsa_is_user_port(ds, i))
> >  			continue;
> > -		p = &dev->ports[i];
> > -		if (!(dev->member & (1 << i)))
> > +		if (port == i)
> >  			continue;
> > +		if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev)
> > +			continue;
> > +
> > +		pi = &dev->ports[i];
> > +		if (pi->stp_state != BR_STATE_DISABLED)
> > +			val |= BIT(dsa_upstream_port(ds, i));
> >  
> 
> This is saying:
> For each {user port, other port} pair, if the other port isn't DISABLED,
> then allow the user port to forward towards the CPU port of the other port.
> What sense does that make? You don't support multiple CPU ports, so this
> port's CPU port is that port's CPU port, and you have one more (broken)
> forwarding rule towards the CPU port below.

Ok, understand.

> > -		/* Port is a member of the bridge and is forwarding. */
> > -		if (p->stp_state == BR_STATE_FORWARDING &&
> > -		    p->member != dev->member)
> > -			dev->dev_ops->cfg_port_member(dev, i, dev->member);
> > +		if (pi->stp_state == BR_STATE_FORWARDING &&
> > +		    p->stp_state == BR_STATE_FORWARDING) {
> > +			val |= BIT(port);
> > +			port_member |= BIT(i);
> > +		}
> > +
> > +		dev->dev_ops->cfg_port_member(dev, i, val);
> >  	}
> > +
> > +	if (p->stp_state != BR_STATE_DISABLED)
> > +		port_member |= BIT(dsa_upstream_port(ds, port));
> 
> Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
> data packet forwarding, not control packet forwarding, right?

No. According to the KSZ9477S datasheet:
"The processor should program the “Static MAC Table” with the entries that it
needs to receive (for example, BPDU packets). The “overriding” bit should be set
so that the switch will forward those specific packets to the processor. The
processor may send packets to the port(s) in this state. Address learning is
disabled on the port in this state."

This part is not implemented.

In current driver implementation (before or after this patch), all
packets are forwarded. It looks like, current STP implementation in this driver
is not complete. If I create a loop, the bridge will permanently toggle one of
ports between blocking and listening. 

Currently I do not know how to proceed with it. Remove stp callback and
make proper, straightforward bride_join/leave? Implement common soft STP
for all switches without HW STP support?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Vladimir Oltean]

On Fri, Nov 12, 2021 at 08:58:23AM +0100, Oleksij Rempel wrote:
> On Wed, Nov 10, 2021 at 02:36:40PM +0200, Vladimir Oltean wrote:
> > On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote:
> > > Current driver version is able to handle only one bridge at time.
> > > Configuring two bridges on two different ports would end up shorting this
> > > bridges by HW. To reproduce it:
> > > 
> > > 	ip l a name br0 type bridge
> > > 	ip l a name br1 type bridge
> > > 	ip l s dev br0 up
> > > 	ip l s dev br1 up
> > > 	ip l s lan1 master br0
> > > 	ip l s dev lan1 up
> > > 	ip l s lan2 master br1
> > > 	ip l s dev lan2 up
> > > 
> > > 	Ping on lan1 and get response on lan2, which should not happen.
> > > 
> > > This happened, because current driver version is storing one global "Port VLAN
> > > Membership" and applying it to all ports which are members of any
> > > bridge.
> > > To solve this issue, we need to handle each port separately.
> > > 
> > > This patch is dropping the global port member storage and calculating
> > > membership dynamically depending on STP state and bridge participation.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > 
> > Because there wasn't any restriction in the driver against multiple
> > bridges, I would be tempted to send to the "net" tree and provide a
> > Fixes: tag.
> 
> This patch looks too intrusive to me. It will be hard to backport it to
> older versions. How about have two patches: add limit to one bridge for
> net and add support for multiple bridges for net-next?

That would work.

> > > +	dp = dsa_to_port(ds, port);
> > > +
> > > +	for (i = 0; i < ds->num_ports; i++) {
> > > +		const struct dsa_port *dpi = dsa_to_port(ds, i);
> > 
> > Other drivers name this "other_dp", I don't think that name is too bad.
> > Also, you can use "dsa_switch_for_each_user_port", which is also more
> > efficient, although you can't if you target 'stable' with this change,
> > since it has been introduced rather recently.
> 
> ok
> 
> > > +		struct ksz_port *pi = &dev->ports[i];
> > 
> > and this could be "other_p" rather than "pi".
> 
> ok
> 
> > > +		u8 val = 0;
> > > +
> > > +		if (!dsa_is_user_port(ds, i))
> > >  			continue;
> > > -		p = &dev->ports[i];
> > > -		if (!(dev->member & (1 << i)))
> > > +		if (port == i)
> > >  			continue;
> > > +		if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev)
> > > +			continue;
> > > +
> > > +		pi = &dev->ports[i];
> > > +		if (pi->stp_state != BR_STATE_DISABLED)
> > > +			val |= BIT(dsa_upstream_port(ds, i));
> > >  
> > 
> > This is saying:
> > For each {user port, other port} pair, if the other port isn't DISABLED,
> > then allow the user port to forward towards the CPU port of the other port.
> > What sense does that make? You don't support multiple CPU ports, so this
> > port's CPU port is that port's CPU port, and you have one more (broken)
> > forwarding rule towards the CPU port below.
> 
> Ok, understand.
> 
> > > -		/* Port is a member of the bridge and is forwarding. */
> > > -		if (p->stp_state == BR_STATE_FORWARDING &&
> > > -		    p->member != dev->member)
> > > -			dev->dev_ops->cfg_port_member(dev, i, dev->member);
> > > +		if (pi->stp_state == BR_STATE_FORWARDING &&
> > > +		    p->stp_state == BR_STATE_FORWARDING) {
> > > +			val |= BIT(port);
> > > +			port_member |= BIT(i);
> > > +		}
> > > +
> > > +		dev->dev_ops->cfg_port_member(dev, i, val);
> > >  	}
> > > +
> > > +	if (p->stp_state != BR_STATE_DISABLED)
> > > +		port_member |= BIT(dsa_upstream_port(ds, port));
> > 
> > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
> > data packet forwarding, not control packet forwarding, right?
> 
> No. According to the KSZ9477S datasheet:
> "The processor should program the “Static MAC Table” with the entries that it
> needs to receive (for example, BPDU packets). The “overriding” bit should be set
> so that the switch will forward those specific packets to the processor. The
> processor may send packets to the port(s) in this state. Address learning is
> disabled on the port in this state."
> 
> This part is not implemented.
> 
> In current driver implementation (before or after this patch), all
> packets are forwarded. It looks like, current STP implementation in this driver
> is not complete. If I create a loop, the bridge will permanently toggle one of
> ports between blocking and listening. 
> 
> Currently I do not know how to proceed with it. Remove stp callback and
> make proper, straightforward bride_join/leave? Implement common soft STP
> for all switches without HW STP support?

What does "soft STP" mean? You need to have a port state in which data
plane packets are blocked, but BPDUs can pass. Unless you trap all
packets to the CPU and make the selection in software (therefore,
including the forwarding, I don't know if that is so desirable), you
don't have much of a choice except to do what you've said above, program
the static MAC table with entries for 01-80-c2-00-00-0x which trap those
link-local multicast addresses to the CPU and set the STP state override
bit for them and for them only.

BTW, see the "bridge link set" section in "man bridge" for a list of
what you should do in each STP state.

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Oleksij Rempel]

On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote:

.....

> > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
> > > data packet forwarding, not control packet forwarding, right?
> > 
> > No. According to the KSZ9477S datasheet:
> > "The processor should program the “Static MAC Table” with the entries that it
> > needs to receive (for example, BPDU packets). The “overriding” bit should be set
> > so that the switch will forward those specific packets to the processor. The
> > processor may send packets to the port(s) in this state. Address learning is
> > disabled on the port in this state."
> > 
> > This part is not implemented.
> > 
> > In current driver implementation (before or after this patch), all
> > packets are forwarded. It looks like, current STP implementation in this driver
> > is not complete. If I create a loop, the bridge will permanently toggle one of
> > ports between blocking and listening. 
> > 
> > Currently I do not know how to proceed with it. Remove stp callback and
> > make proper, straightforward bride_join/leave? Implement common soft STP
> > for all switches without HW STP support?
> 
> What does "soft STP" mean?

Some HW seems to provide configuration bits for ports STP states. For
example by enabling it, I can just set listening state and it will only pass
BPDU packets without need to program static MAC table. (At least, this
would be my expectation)

For example like this:
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/mt7530.c#L1121

If this HW really exist and works as expected, how should I name it?

> You need to have a port state in which data  plane packets are blocked,
> but BPDUs can pass.

ack.

> Unless you trap all packets to the CPU and make the selection in software
> (therefore, including the forwarding, I don't know if that is so desirable),

Yes, this is my point, on plain linux bridge with two simple USB ethernet
adapters, I'm able to use STP without any HW offloading.

If my HW do not provide straightforward way to trap BPDU packets to CPU,
i should be able to reuse functionality already provided by the linux
bridge. Probably I need to signal it some how from dsa driver, to let linux
bridge make proper decision and reduce logging noise.

For example:
- Have flag like: ds->sta_without_bpdu_trap = true;
- If no .port_mdb_add/.port_fdb_add callbacks are implemented, handle
  all incoming packet by the linux bridge without making lots of noise,
  and use .port_bridge_join/.port_bridge_leave to separate ports.
- If .port_mdb_add/.port_fdb_add are implemented, program the static MAC table.

> you don't have much of a choice except to do what you've said above, program
> the static MAC table with entries for 01-80-c2-00-00-0x which trap those
> link-local multicast addresses to the CPU and set the STP state override
> bit for them and for them only.

Hm... Microchip documentation do not describes it as STP state override. Only
as "port state override". And since STP state is not directly configurable
on this switch, it probably means receive/transmit enable state of the port.
So, packets with matching MAC should be forwarded even if port is in the
receive disabled state. Correct?

> BTW, see the "bridge link set" section in "man bridge" for a list of
> what you should do in each STP state.

ack. Thank you.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Vladimir Oltean]

On Tue, Nov 16, 2021 at 09:39:03AM +0100, Oleksij Rempel wrote:
> On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote:
> 
> .....
> 
> > > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
> > > > data packet forwarding, not control packet forwarding, right?
> > > 
> > > No. According to the KSZ9477S datasheet:
> > > "The processor should program the “Static MAC Table” with the entries that it
> > > needs to receive (for example, BPDU packets). The “overriding” bit should be set
> > > so that the switch will forward those specific packets to the processor. The
> > > processor may send packets to the port(s) in this state. Address learning is
> > > disabled on the port in this state."
> > > 
> > > This part is not implemented.
> > > 
> > > In current driver implementation (before or after this patch), all
> > > packets are forwarded. It looks like, current STP implementation in this driver
> > > is not complete. If I create a loop, the bridge will permanently toggle one of
> > > ports between blocking and listening. 
> > > 
> > > Currently I do not know how to proceed with it. Remove stp callback and
> > > make proper, straightforward bride_join/leave? Implement common soft STP
> > > for all switches without HW STP support?
> > 
> > What does "soft STP" mean?
> 
> Some HW seems to provide configuration bits for ports STP states. For
> example by enabling it, I can just set listening state and it will only pass
> BPDU packets without need to program static MAC table. (At least, this
> would be my expectation)
> 
> For example like this:
> https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/mt7530.c#L1121
> 
> If this HW really exist and works as expected, how should I name it?

You are simply talking about the kind of registers that a switch exposes.
That doesn't really matter, the end result should be the same.
For example, sja1105 doesn't have named STP states, but it allows the
driver to specify whether data plane packets can be received and sent on
a given port.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L2030
On other switches it's even more interesting, you need to calibrate STP
states from the port forwarding mask.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/ethernet/mscc/ocelot.c#L1476
In any case, the switch should be able (somehow) to distinguish
link-local multicast packets and send them to the CPU port regardless of
the source port's STP state, and also be able to inject a link-local
multicast packet into a port regardless of its STP state.
For Micrel/Microchip switches, at least BPDU injection should be
supported, I believe that's what this piece of code does:
https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_ksz.c#L127

> > You need to have a port state in which data  plane packets are blocked,
> > but BPDUs can pass.
> 
> ack.
> 
> > Unless you trap all packets to the CPU and make the selection in software
> > (therefore, including the forwarding, I don't know if that is so desirable),
> 
> Yes, this is my point, on plain linux bridge with two simple USB ethernet
> adapters, I'm able to use STP without any HW offloading.
> 
> If my HW do not provide straightforward way to trap BPDU packets to CPU,
> i should be able to reuse functionality already provided by the linux
> bridge.

At least on net-next you can return -EOPNOTSUPP in ->port_bridge_join
and DSA will leave your port to operate in standalone mode and what will
happen is exactly what you describe. But if lack of STP support is what
you're trying to fix, there might be better ways of fixing it than doing
software bridging.

> Probably I need to signal it some how from dsa driver, to let linux
> bridge make proper decision and reduce logging noise.

What logging noise?

> For example:
> - Have flag like: ds->sta_without_bpdu_trap = true;

:-/ what would this flag do?

> - If no .port_mdb_add/.port_fdb_add callbacks are implemented, handle
>   all incoming packet by the linux bridge without making lots of noise,
>   and use .port_bridge_join/.port_bridge_leave to separate ports.

You can already let forwarding be partially done in software. For
example qca8k sets up the CPU port as the only destination for multicast
and for flooding:
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/qca8k.c#L1157
Notice how its tagging protocol driver does not call
dsa_default_offload_fwd_mark(), in order to let the bridge forward the
packets in software:
https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_qca.c#L51

> - If .port_mdb_add/.port_fdb_add are implemented, program the static MAC table.

You want DSA to program the static MAC table for link-local traffic?
Why? DSA doesn't care how you trap your link-local traffic to the CPU,
it might vary wildly between one switch and another. Also,
->port_mdb_add() is for data plane multicast packets (aka not BPDUs),
and ->port_fdb_add() is for unicast data plane packets (again not
BPDUs). Your driver wouldn't even be the first one that traps link-local
traffic privately.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/hirschmann/hellcreek.c#L1050
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L868
There isn't any good way for user space to have visibility into which
packets a switch will trap. There is devlink-trap which AFAIK allows you
to see but not modify the traps that are built-in to the hardware/driver:
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-trap.html
There are also traps which you can add using the tc-trap action
(amazingly I could not find documentation for this). But I don't think
it would surprise anyone if you would trap BPDUs by default in the
driver - as mentioned, some switches already do this with no way to disable it.

> > you don't have much of a choice except to do what you've said above, program
> > the static MAC table with entries for 01-80-c2-00-00-0x which trap those
> > link-local multicast addresses to the CPU and set the STP state override
> > bit for them and for them only.
> 
> Hm... Microchip documentation do not describes it as STP state override. Only
> as "port state override".

Potato, patato, it should be the same thing.

> And since STP state is not directly configurable on this switch, it
> probably means receive/transmit enable state of the port.  So, packets
> with matching MAC should be forwarded even if port is in the receive
> disabled state. Correct?

In the context we've been discussing so far, "forwarding" has a pretty
specific meaning, which is autonomously redirecting from one front port
to another. For link-local packets, what you want is "trapping", i.e.
send to the CPU and to the CPU only.

> 
> > BTW, see the "bridge link set" section in "man bridge" for a list of
> > what you should do in each STP state.
> 
> ack. Thank you.
> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Oleksij Rempel]

On Tue, Nov 16, 2021 at 02:47:23PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 16, 2021 at 09:39:03AM +0100, Oleksij Rempel wrote:
> > On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote:
> > 
> > .....
 
> > Probably I need to signal it some how from dsa driver, to let linux
> > bridge make proper decision and reduce logging noise.
> 
> What logging noise?

I get this with current ksz driver:
[   40.185928] br0: port 2(lan2) entered blocking state
[   40.190924] br0: port 2(lan2) entered listening state
[   41.043186] br0: port 2(lan2) entered blocking state
[   55.512832] br0: port 1(lan1) entered learning state
[   61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
[   61.279192] br0: port 2(lan2) entered listening state
[   63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[   63.123314] br0: port 2(lan2) entered blocking state
[   68.953098] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[   70.872840] br0: port 1(lan1) entered forwarding state
[   70.878183] br0: topology change detected, propagating
[   70.883820] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[   83.672804] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
[   83.679181] br0: port 2(lan2) entered listening state
[   85.113244] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[   85.123313] br0: port 2(lan2) entered blocking state
[   97.753160] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[  103.513076] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[  105.432801] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
[  105.439221] br0: port 2(lan2) entered listening state
[  107.113238] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[  107.123326] br0: port 2(lan2) entered blocking state
[  127.192807] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
[  127.199220] br0: port 2(lan2) entered listening state
[  129.113249] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[  129.123378] br0: port 2(lan2) entered blocking state
[  149.592804] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
[  149.600308] br0: port 2(lan2) entered listening state
[  151.113276] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
[  151.125213] br0: port 2(lan2) entered blocking state

Probably I have wrong expectation... 

> > And since STP state is not directly configurable on this switch, it
> > probably means receive/transmit enable state of the port.  So, packets
> > with matching MAC should be forwarded even if port is in the receive
> > disabled state. Correct?
> 
> In the context we've been discussing so far, "forwarding" has a pretty
> specific meaning, which is autonomously redirecting from one front port
> to another. For link-local packets, what you want is "trapping", i.e.
> send to the CPU and to the CPU only.

Ok. Thank you!

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Andrew Lunn]

> > What logging noise?
> 
> I get this with current ksz driver:
> [   40.185928] br0: port 2(lan2) entered blocking state
> [   40.190924] br0: port 2(lan2) entered listening state
> [   41.043186] br0: port 2(lan2) entered blocking state
> [   55.512832] br0: port 1(lan1) entered learning state
> [   61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
> [   61.279192] br0: port 2(lan2) entered listening state
> [   63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)

I would guess that transmission from the CPU is broken in this
case. It could be looking up the destination address in the
translation table and not finding an entry. So it floods the packet
out all interfaces, including the CPU. So the CPU receives its own
packet and gives this warning.

Flooding should exclude where the frame came from.

	 Andrew

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Vladimir Oltean]

On Tue, Nov 16, 2021 at 02:40:06PM +0100, Andrew Lunn wrote:
> > > What logging noise?
> > 
> > I get this with current ksz driver:
> > [   40.185928] br0: port 2(lan2) entered blocking state
> > [   40.190924] br0: port 2(lan2) entered listening state
> > [   41.043186] br0: port 2(lan2) entered blocking state
> > [   55.512832] br0: port 1(lan1) entered learning state
> > [   61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
> > [   61.279192] br0: port 2(lan2) entered listening state
> > [   63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
> 
> I would guess that transmission from the CPU is broken in this
> case. It could be looking up the destination address in the
> translation table and not finding an entry. So it floods the packet
> out all interfaces, including the CPU. So the CPU receives its own
> packet and gives this warning.
> 
> Flooding should exclude where the frame came from.

I interpret this very differently. If Oleksij is looping lan1 with lan2
and he keeps the MAC addresses the way DSA sets them up by default, i.e.
equal and inherited from the DSA master, then receiving a packet with a
MAC SA (lan2) equal with the address of the receiving interface (lan1)
is absolutely natural. What is not natural is that the bridge attempts
to learn from this packet (the message is printed from br_fdb_update),
which in turn is caused by the fact that the port is allowed to proceed
to the LEARNING state despite there being a loop (which is not detected
by STP because STP is broken as Oleksij describes).

Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge support

[Andrew Lunn]

On Tue, Nov 16, 2021 at 03:53:35PM +0200, Vladimir Oltean wrote:
> On Tue, Nov 16, 2021 at 02:40:06PM +0100, Andrew Lunn wrote:
> > > > What logging noise?
> > > 
> > > I get this with current ksz driver:
> > > [   40.185928] br0: port 2(lan2) entered blocking state
> > > [   40.190924] br0: port 2(lan2) entered listening state
> > > [   41.043186] br0: port 2(lan2) entered blocking state
> > > [   55.512832] br0: port 1(lan1) entered learning state
> > > [   61.272802] br0: port 2(lan2) neighbor 8000.ae:1b:91:58:77:8b lost
> > > [   61.279192] br0: port 2(lan2) entered listening state
> > > [   63.113236] br0: received packet on lan1 with own address as source address (addr:00:0e:cd:00:cd:be, vlan:0)
> > 
> > I would guess that transmission from the CPU is broken in this
> > case. It could be looking up the destination address in the
> > translation table and not finding an entry. So it floods the packet
> > out all interfaces, including the CPU. So the CPU receives its own
> > packet and gives this warning.
> > 
> > Flooding should exclude where the frame came from.
> 
> I interpret this very differently. If Oleksij is looping lan1 with lan2
> and he keeps the MAC addresses the way DSA sets them up by default, i.e.
> equal and inherited from the DSA master, then receiving a packet with a
> MAC SA (lan2) equal with the address of the receiving interface (lan1)
> is absolutely natural. What is not natural is that the bridge attempts
> to learn from this packet (the message is printed from br_fdb_update),
> which in turn is caused by the fact that the port is allowed to proceed
> to the LEARNING state despite there being a loop (which is not detected
> by STP because STP is broken as Oleksij describes).

Ah, yes, that is more likely.

Sorry, should not of jumped in without reading all the context. If STP
is broken, odd things will happen.

   Andrew