[PATCH net-next 1/2] net: dsa: mt7530: factor out bridge join/leave logic

[PATCH net-next 1/2] net: dsa: mt7530: factor out bridge join/leave logic

[Matthias Schiffer]

As preparation for implementing bridge port isolation, move the logic to
add and remove bits in the port matrix into a new helper
mt7530_update_port_member(), which is called from
mt7530_port_bridge_join() and mt7530_port_bridge_leave().

No functional change intended.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 drivers/net/dsa/mt7530.c | 103 +++++++++++++++++----------------------
 1 file changed, 46 insertions(+), 57 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 598434d8d6e4..ecacaefdd694 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1302,6 +1302,50 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 		   FID_PST(FID_BRIDGED, stp_state));
 }
 
+static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
+				      const struct net_device *bridge_dev, bool join)
+	__must_hold(&priv->reg_mutex)
+{
+	struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
+	struct mt7530_port *p = &priv->ports[port], *other_p;
+	struct dsa_port *cpu_dp = dp->cpu_dp;
+	u32 port_bitmap = BIT(cpu_dp->index);
+	int other_port;
+
+	dsa_switch_for_each_user_port(other_dp, priv->ds) {
+		other_port = other_dp->index;
+		other_p = &priv->ports[other_port];
+
+		if (dp == other_dp)
+			continue;
+
+		/* Add/remove this port to/from the port matrix of the other
+		 * ports in the same bridge. If the port is disabled, port
+		 * matrix is kept and not being setup until the port becomes
+		 * enabled.
+		 */
+		if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
+			continue;
+
+		if (join) {
+			other_p->pm |= PCR_MATRIX(BIT(port));
+			port_bitmap |= BIT(other_port);
+		} else {
+			other_p->pm &= ~PCR_MATRIX(BIT(port));
+		}
+
+		if (other_p->enable)
+			mt7530_rmw(priv, MT7530_PCR_P(other_port),
+				   PCR_MATRIX_MASK, other_p->pm);
+	}
+
+	/* Add/remove the all other ports to this port matrix. */
+	p->pm = PCR_MATRIX(port_bitmap);
+	if (priv->ports[port].enable)
+		mt7530_rmw(priv, MT7530_PCR_P(port),
+			   PCR_MATRIX_MASK, p->pm);
+}
+
 static int
 mt7530_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 			     struct switchdev_brport_flags flags,
@@ -1345,39 +1389,11 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 			struct dsa_bridge bridge, bool *tx_fwd_offload,
 			struct netlink_ext_ack *extack)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
-	struct dsa_port *cpu_dp = dp->cpu_dp;
-	u32 port_bitmap = BIT(cpu_dp->index);
 	struct mt7530_priv *priv = ds->priv;
 
 	mutex_lock(&priv->reg_mutex);
 
-	dsa_switch_for_each_user_port(other_dp, ds) {
-		int other_port = other_dp->index;
-
-		if (dp == other_dp)
-			continue;
-
-		/* Add this port to the port matrix of the other ports in the
-		 * same bridge. If the port is disabled, port matrix is kept
-		 * and not being setup until the port becomes enabled.
-		 */
-		if (!dsa_port_offloads_bridge(other_dp, &bridge))
-			continue;
-
-		if (priv->ports[other_port].enable)
-			mt7530_set(priv, MT7530_PCR_P(other_port),
-				   PCR_MATRIX(BIT(port)));
-		priv->ports[other_port].pm |= PCR_MATRIX(BIT(port));
-
-		port_bitmap |= BIT(other_port);
-	}
-
-	/* Add the all other ports to this port matrix. */
-	if (priv->ports[port].enable)
-		mt7530_rmw(priv, MT7530_PCR_P(port),
-			   PCR_MATRIX_MASK, PCR_MATRIX(port_bitmap));
-	priv->ports[port].pm |= PCR_MATRIX(port_bitmap);
+	mt7530_update_port_member(priv, port, bridge.dev, true);
 
 	/* Set to fallback mode for independent VLAN learning */
 	mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK,
@@ -1478,38 +1494,11 @@ static void
 mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 			 struct dsa_bridge bridge)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port), *other_dp;
-	struct dsa_port *cpu_dp = dp->cpu_dp;
 	struct mt7530_priv *priv = ds->priv;
 
 	mutex_lock(&priv->reg_mutex);
 
-	dsa_switch_for_each_user_port(other_dp, ds) {
-		int other_port = other_dp->index;
-
-		if (dp == other_dp)
-			continue;
-
-		/* Remove this port from the port matrix of the other ports
-		 * in the same bridge. If the port is disabled, port matrix
-		 * is kept and not being setup until the port becomes enabled.
-		 */
-		if (!dsa_port_offloads_bridge(other_dp, &bridge))
-			continue;
-
-		if (priv->ports[other_port].enable)
-			mt7530_clear(priv, MT7530_PCR_P(other_port),
-				     PCR_MATRIX(BIT(port)));
-		priv->ports[other_port].pm &= ~PCR_MATRIX(BIT(port));
-	}
-
-	/* Set the cpu port to be the only one in the port matrix of
-	 * this port.
-	 */
-	if (priv->ports[port].enable)
-		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
-			   PCR_MATRIX(BIT(cpu_dp->index)));
-	priv->ports[port].pm = PCR_MATRIX(BIT(cpu_dp->index));
+	mt7530_update_port_member(priv, port, bridge.dev, false);
 
 	/* When a port is removed from the bridge, the port would be set up
 	 * back to the default as is at initial boot which is a VLAN-unaware
-- 
2.45.2

[PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Matthias Schiffer]

Remove a pair of ports from the port matrix when both ports have the
isolated flag set.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---
 drivers/net/dsa/mt7530.c | 21 ++++++++++++++++++---
 drivers/net/dsa/mt7530.h |  1 +
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ecacaefdd694..44939379aba8 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1303,7 +1303,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 }
 
 static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
-				      const struct net_device *bridge_dev, bool join)
+				      const struct net_device *bridge_dev,
+				      bool join)
 	__must_hold(&priv->reg_mutex)
 {
 	struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
@@ -1311,6 +1312,7 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
 	struct dsa_port *cpu_dp = dp->cpu_dp;
 	u32 port_bitmap = BIT(cpu_dp->index);
 	int other_port;
+	bool isolated;
 
 	dsa_switch_for_each_user_port(other_dp, priv->ds) {
 		other_port = other_dp->index;
@@ -1327,7 +1329,9 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
 		if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
 			continue;
 
-		if (join) {
+		isolated = p->isolated && other_p->isolated;
+
+		if (join && !isolated) {
 			other_p->pm |= PCR_MATRIX(BIT(port));
 			port_bitmap |= BIT(other_port);
 		} else {
@@ -1352,7 +1356,7 @@ mt7530_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 			     struct netlink_ext_ack *extack)
 {
 	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
-			   BR_BCAST_FLOOD))
+			   BR_BCAST_FLOOD | BR_ISOLATED))
 		return -EINVAL;
 
 	return 0;
@@ -1381,6 +1385,17 @@ mt7530_port_bridge_flags(struct dsa_switch *ds, int port,
 		mt7530_rmw(priv, MT753X_MFC, BC_FFP(BIT(port)),
 			   flags.val & BR_BCAST_FLOOD ? BC_FFP(BIT(port)) : 0);
 
+	if (flags.mask & BR_ISOLATED) {
+		struct dsa_port *dp = dsa_to_port(ds, port);
+		struct net_device *bridge_dev = dsa_port_bridge_dev_get(dp);
+
+		priv->ports[port].isolated = !!(flags.val & BR_ISOLATED);
+
+		mutex_lock(&priv->reg_mutex);
+		mt7530_update_port_member(priv, port, bridge_dev, true);
+		mutex_unlock(&priv->reg_mutex);
+	}
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 2ea4e24628c6..28592123070b 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -721,6 +721,7 @@ struct mt7530_fdb {
  */
 struct mt7530_port {
 	bool enable;
+	bool isolated;
 	u32 pm;
 	u16 pvid;
 	struct phylink_pcs *sgmii_pcs;
-- 
2.45.2

Re: [PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Arınç ÜNAL]

On 15/06/2024 01:21, Matthias Schiffer wrote:
> Remove a pair of ports from the port matrix when both ports have the
> isolated flag set.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   drivers/net/dsa/mt7530.c | 21 ++++++++++++++++++---
>   drivers/net/dsa/mt7530.h |  1 +
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ecacaefdd694..44939379aba8 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1303,7 +1303,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>   }
>   
>   static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
> -				      const struct net_device *bridge_dev, bool join)
> +				      const struct net_device *bridge_dev,
> +				      bool join)

Run git clang-format on this patch as well please.

>   	__must_hold(&priv->reg_mutex)
>   {
>   	struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
> @@ -1311,6 +1312,7 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>   	struct dsa_port *cpu_dp = dp->cpu_dp;
>   	u32 port_bitmap = BIT(cpu_dp->index);
>   	int other_port;
> +	bool isolated;
>   
>   	dsa_switch_for_each_user_port(other_dp, priv->ds) {
>   		other_port = other_dp->index;
> @@ -1327,7 +1329,9 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>   		if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
>   			continue;
>   
> -		if (join) {
> +		isolated = p->isolated && other_p->isolated;
> +
> +		if (join && !isolated) {
>   			other_p->pm |= PCR_MATRIX(BIT(port));
>   			port_bitmap |= BIT(other_port);
>   		} else {

Why must other_p->isolated be true as well? If I understand correctly, when
a user port is isolated, non isolated ports can't communicate with it
whilst the CPU port can. If I were to isolate a port which is the only
isolated one at the moment, the isolated flag would not be true. Therefore,
the isolated port would not be removed from the port matrix of other user
ports. Why not only check for p->isolated?

Arınç

Re: [PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Arınç ÜNAL]

On 16/06/2024 09:52, Arınç ÜNAL wrote:
> On 15/06/2024 01:21, Matthias Schiffer wrote:
>> Remove a pair of ports from the port matrix when both ports have the
>> isolated flag set.
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
>>   drivers/net/dsa/mt7530.c | 21 ++++++++++++++++++---
>>   drivers/net/dsa/mt7530.h |  1 +
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ecacaefdd694..44939379aba8 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -1303,7 +1303,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>>   }
>>   static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>> -                      const struct net_device *bridge_dev, bool join)
>> +                      const struct net_device *bridge_dev,
>> +                      bool join)
> 
> Run git clang-format on this patch as well please.
> 
>>       __must_hold(&priv->reg_mutex)
>>   {
>>       struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
>> @@ -1311,6 +1312,7 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>>       struct dsa_port *cpu_dp = dp->cpu_dp;
>>       u32 port_bitmap = BIT(cpu_dp->index);
>>       int other_port;
>> +    bool isolated;
>>       dsa_switch_for_each_user_port(other_dp, priv->ds) {
>>           other_port = other_dp->index;
>> @@ -1327,7 +1329,9 @@ static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>>           if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
>>               continue;
>> -        if (join) {
>> +        isolated = p->isolated && other_p->isolated;
>> +
>> +        if (join && !isolated) {
>>               other_p->pm |= PCR_MATRIX(BIT(port));
>>               port_bitmap |= BIT(other_port);
>>           } else {
> 
> Why must other_p->isolated be true as well? If I understand correctly, when
> a user port is isolated, non isolated ports can't communicate with it
> whilst the CPU port can. If I were to isolate a port which is the only
> isolated one at the moment, the isolated flag would not be true. Therefore,
> the isolated port would not be removed from the port matrix of other user
> ports. Why not only check for p->isolated?

The concept of port isolation is that the isolated port can only
communicate with non-isolated ports so the current implementation looks ok.

Which switch models did you test this on; MT7530, MT7531, MT7988 SoC
switch? I will test it on MT7530 and MT7531 tomorrow evening.

Arınç

Re: [PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 2997 bytes --]

On 16/06/2024 08:52, Arınç ÜNAL wrote:
> On 15/06/2024 01:21, Matthias Schiffer wrote:
>> Remove a pair of ports from the port matrix when both ports have the
>> isolated flag set.
>>
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>> ---
>>   drivers/net/dsa/mt7530.c | 21 ++++++++++++++++++---
>>   drivers/net/dsa/mt7530.h |  1 +
>>   2 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ecacaefdd694..44939379aba8 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -1303,7 +1303,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int 
>> port, u8 state)
>>   }
>>   static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
>> -                      const struct net_device *bridge_dev, bool join)
>> +                      const struct net_device *bridge_dev,
>> +                      bool join)
> 
> Run git clang-format on this patch as well please.

Oops, will do.


> 
>>       __must_hold(&priv->reg_mutex)
>>   {
>>       struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
>> @@ -1311,6 +1312,7 @@ static void mt7530_update_port_member(struct 
>> mt7530_priv *priv, int port,
>>       struct dsa_port *cpu_dp = dp->cpu_dp;
>>       u32 port_bitmap = BIT(cpu_dp->index);
>>       int other_port;
>> +    bool isolated;
>>       dsa_switch_for_each_user_port(other_dp, priv->ds) {
>>           other_port = other_dp->index;
>> @@ -1327,7 +1329,9 @@ static void mt7530_update_port_member(struct 
>> mt7530_priv *priv, int port,
>>           if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
>>               continue;
>> -        if (join) {
>> +        isolated = p->isolated && other_p->isolated;
>> +
>> +        if (join && !isolated) {
>>               other_p->pm |= PCR_MATRIX(BIT(port));
>>               port_bitmap |= BIT(other_port);
>>           } else {
> 
> Why must other_p->isolated be true as well? If I understand correctly, when
> a user port is isolated, non isolated ports can't communicate with it
> whilst the CPU port can. If I were to isolate a port which is the only
> isolated one at the moment, the isolated flag would not be true. Therefore,
> the isolated port would not be removed from the port matrix of other user
> ports. Why not only check for p->isolated?

As far as I can tell, the rules are:

- non-isolated ports can communicate with every port
- isolated ports can't communicate with other isolated ports
- communication is symmetric

You'll find that the logic works the same for non-offloaded bridge 
forwarding (see should_deliver() in net/bridge/br_forward.c and 
br_skb_isolated() in net/bridge/br_private.h).

Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Andrew Lunn]

> As far as I can tell, the rules are:
> 
> - non-isolated ports can communicate with every port
> - isolated ports can't communicate with other isolated ports
> - communication is symmetric

It is a bit more subtle than that.

By default, all ports should be isolated. They can exchange packets
with the CPU port, but nothing else. This goes back to the model of
switches just look like a bunch of netdev interfaces. By default,
linux netdev interfaces are standalone. You need to add a bridge
before packets can flow between ports.

Once you add a bridge, ports within that bridge can exchange
packets. However, there can be multiple bridges. So a port needs to be
isolated from ports in another bridge, but non-isolated to ports
within the same bridge.

       Andrew

Re: [PATCH net-next 2/2] net: dsa: mt7530: add support for bridge port isolation

[Matthias Schiffer]

[-- Attachment #1.1: Type: text/plain, Size: 1086 bytes --]

On 16/06/2024 17:35, Andrew Lunn wrote:
>> As far as I can tell, the rules are:
>>
>> - non-isolated ports can communicate with every port
>> - isolated ports can't communicate with other isolated ports
>> - communication is symmetric
> 
> It is a bit more subtle than that.
> 
> By default, all ports should be isolated. They can exchange packets
> with the CPU port, but nothing else. This goes back to the model of
> switches just look like a bunch of netdev interfaces. By default,
> linux netdev interfaces are standalone. You need to add a bridge
> before packets can flow between ports.
> 
> Once you add a bridge, ports within that bridge can exchange
> packets. However, there can be multiple bridges. So a port needs to be
> isolated from ports in another bridge, but non-isolated to ports
> within the same bridge.
> 
>         Andrew

Right, I was talking about ports in the same bridge (which is already 
handled as expected by the mt7530 driver, just the option to set the 
isolation flag for individual bridge ports was unsupported).

Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH net-next 1/2] net: dsa: mt7530: factor out bridge join/leave logic

[Arınç ÜNAL]

On 15/06/2024 01:21, Matthias Schiffer wrote:
> As preparation for implementing bridge port isolation, move the logic to
> add and remove bits in the port matrix into a new helper
> mt7530_update_port_member(), which is called from
> mt7530_port_bridge_join() and mt7530_port_bridge_leave().
> 
> No functional change intended.
> 
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
> ---
>   drivers/net/dsa/mt7530.c | 103 +++++++++++++++++----------------------
>   1 file changed, 46 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 598434d8d6e4..ecacaefdd694 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1302,6 +1302,50 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state)
>   		   FID_PST(FID_BRIDGED, stp_state));
>   }
>   
> +static void mt7530_update_port_member(struct mt7530_priv *priv, int port,
> +				      const struct net_device *bridge_dev, bool join)
> +	__must_hold(&priv->reg_mutex)

Please run git clang-format on the patch.

> +{
> +	struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
> +	struct mt7530_port *p = &priv->ports[port], *other_p;
> +	struct dsa_port *cpu_dp = dp->cpu_dp;
> +	u32 port_bitmap = BIT(cpu_dp->index);
> +	int other_port;
> +
> +	dsa_switch_for_each_user_port(other_dp, priv->ds) {
> +		other_port = other_dp->index;
> +		other_p = &priv->ports[other_port];
> +
> +		if (dp == other_dp)
> +			continue;
> +
> +		/* Add/remove this port to/from the port matrix of the other
> +		 * ports in the same bridge. If the port is disabled, port
> +		 * matrix is kept and not being setup until the port becomes
> +		 * enabled.
> +		 */
> +		if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))

Would be helpful to mention in the patch log that
dsa_port_offloads_bridge_dev() is now being used instead of
dsa_port_offloads_bridge().

> +			continue;
> +
> +		if (join) {
> +			other_p->pm |= PCR_MATRIX(BIT(port));
> +			port_bitmap |= BIT(other_port);
> +		} else {
> +			other_p->pm &= ~PCR_MATRIX(BIT(port));
> +		}
> +
> +		if (other_p->enable)
> +			mt7530_rmw(priv, MT7530_PCR_P(other_port),
> +				   PCR_MATRIX_MASK, other_p->pm);
> +	}
> +
> +	/* Add/remove the all other ports to this port matrix. */

I would add to this comment: When removing, only the CPU port will remain
in the port matrix of this port.

To not omit the original comment:

	/* Set the cpu port to be the only one in the port matrix of
	 * this port.
	 */

> +	p->pm = PCR_MATRIX(port_bitmap);
> +	if (priv->ports[port].enable)
> +		mt7530_rmw(priv, MT7530_PCR_P(port),
> +			   PCR_MATRIX_MASK, p->pm);

I see changes to have the code streamlined. Overall, I would appreciate a
more verbose patch log.

Arınç