Remove a pair of ports from the port matrix when both ports have the
isolated flag set.
Signed-off-by: Matthias Schiffer <[email protected]>
---
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
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 <[email protected]>
> ---
> 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ç
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 <[email protected]>
>> ---
>> 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ç
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 <[email protected]>
>> ---
>> 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