Commit b3612ccdf284 ("net: dsa: microchip: implement multi-bridge support")
plugged a packet leak between ports that were members of different bridges.
Unfortunately, this broke another use case, namely that of more than two
ports that are members of the same bridge.

After that commit, when a port is added to a bridge, hardware bridging
between other member ports of that bridge will be cleared, preventing
packet exchange between them.

Fix by ensuring that the Port VLAN Membership bitmap includes any existing
ports in the bridge, not just the port being added.

Fixes: b3612ccdf284 ("net: dsa: microchip: implement multi-bridge support")
Signed-off-by: Svenning Sørensen <sss@secomea.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 55dbda04ea62..243f8ad6d06e 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -26,7 +26,7 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
 	struct dsa_switch *ds = dev->ds;
 	u8 port_member = 0, cpu_port;
 	const struct dsa_port *dp;
-	int i;
+	int i, j;
 
 	if (!dsa_is_user_port(ds, port))
 		return;
@@ -45,13 +45,33 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
 			continue;
 		if (!dsa_port_bridge_same(dp, other_dp))
 			continue;
+		if (other_p->stp_state != BR_STATE_FORWARDING)
+			continue;
 
-		if (other_p->stp_state == BR_STATE_FORWARDING &&
-		    p->stp_state == BR_STATE_FORWARDING) {
+		if (p->stp_state == BR_STATE_FORWARDING) {
 			val |= BIT(port);
 			port_member |= BIT(i);
 		}
 
+		/* Retain port [i]'s relationship to other ports than [port] */
+		for (j = 0; j < ds->num_ports; j++) {
+			const struct dsa_port *third_dp;
+			struct ksz_port *third_p;
+
+			if (j == i)
+				continue;
+			if (j == port)
+				continue;
+			if (!dsa_is_user_port(ds, j))
+				continue;
+			third_p = &dev->ports[j];
+			if (third_p->stp_state != BR_STATE_FORWARDING)
+				continue;
+			third_dp = dsa_to_port(ds, j);
+			if (dsa_port_bridge_same(other_dp, third_dp))
+				val |= BIT(j);
+		}
+
 		dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
 	}
 
-- 
2.20.1

On Fri, Feb 18, 2022 at 11:27:01AM +0000, Svenning Sørensen wrote:
> Commit b3612ccdf284 ("net: dsa: microchip: implement multi-bridge support")
> plugged a packet leak between ports that were members of different bridges.
> Unfortunately, this broke another use case, namely that of more than two
> ports that are members of the same bridge.
> 
> After that commit, when a port is added to a bridge, hardware bridging
> between other member ports of that bridge will be cleared, preventing
> packet exchange between them.
> 
> Fix by ensuring that the Port VLAN Membership bitmap includes any existing
> ports in the bridge, not just the port being added.
> 
> Fixes: b3612ccdf284 ("net: dsa: microchip: implement multi-bridge support")
> Signed-off-by: Svenning Sørensen <sss@secomea.com>

Thank you for your patch. You are right. I'm able to reproduce this issue on
ksz9477.

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

> ---
>  drivers/net/dsa/microchip/ksz_common.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 55dbda04ea62..243f8ad6d06e 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -26,7 +26,7 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
>  	struct dsa_switch *ds = dev->ds;
>  	u8 port_member = 0, cpu_port;
>  	const struct dsa_port *dp;
> -	int i;
> +	int i, j;
>  
>  	if (!dsa_is_user_port(ds, port))
>  		return;
> @@ -45,13 +45,33 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
>  			continue;
>  		if (!dsa_port_bridge_same(dp, other_dp))
>  			continue;
> +		if (other_p->stp_state != BR_STATE_FORWARDING)
> +			continue;
>  
> -		if (other_p->stp_state == BR_STATE_FORWARDING &&
> -		    p->stp_state == BR_STATE_FORWARDING) {
> +		if (p->stp_state == BR_STATE_FORWARDING) {
>  			val |= BIT(port);
>  			port_member |= BIT(i);
>  		}
>  
> +		/* Retain port [i]'s relationship to other ports than [port] */
> +		for (j = 0; j < ds->num_ports; j++) {
> +			const struct dsa_port *third_dp;
> +			struct ksz_port *third_p;
> +
> +			if (j == i)
> +				continue;
> +			if (j == port)
> +				continue;
> +			if (!dsa_is_user_port(ds, j))
> +				continue;
> +			third_p = &dev->ports[j];
> +			if (third_p->stp_state != BR_STATE_FORWARDING)
> +				continue;
> +			third_dp = dsa_to_port(ds, j);
> +			if (dsa_port_bridge_same(other_dp, third_dp))
> +				val |= BIT(j);
> +		}
> +
>  		dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
>  	}
>  
> -- 
> 2.20.1
> 
> 

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

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 18 Feb 2022 11:27:01 +0000 you wrote:
> Commit b3612ccdf284 ("net: dsa: microchip: implement multi-bridge support")
> plugged a packet leak between ports that were members of different bridges.
> Unfortunately, this broke another use case, namely that of more than two
> ports that are members of the same bridge.
> 
> After that commit, when a port is added to a bridge, hardware bridging
> between other member ports of that bridge will be cleared, preventing
> packet exchange between them.
> 
> [...]

Here is the summary with links:
  - net: dsa: microchip: fix bridging with more than two member ports
    https://git.kernel.org/netdev/net/c/3d00827a90db

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html