diff mbox series

[RESEND,net-next,v4,2/3] net: dsa: rzn1-a5psw: add support for .port_bridge_flags

Message ID 20230314163651.242259-3-clement.leger@bootlin.com
State New
Headers
Series net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags |

Commit Message

Clément Léger March 14, 2023, 4:36 p.m. UTC 
  When running vlan test (bridge_vlan_aware/unaware.sh), there were some
failure due to the lack .port_bridge_flag function to disable port
flooding. Implement this operation for BR_LEARNING, BR_FLOOD,
BR_MCAST_FLOOD and BR_BCAST_FLOOD.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/rzn1_a5psw.c | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Vladimir Oltean March 14, 2023, 10:56 p.m. UTC | #1 
On Tue, Mar 14, 2023 at 05:36:50PM +0100, Clément Léger wrote:
> When running vlan test (bridge_vlan_aware/unaware.sh), there were some
> failure due to the lack .port_bridge_flag function to disable port
> flooding. Implement this operation for BR_LEARNING, BR_FLOOD,
> BR_MCAST_FLOOD and BR_BCAST_FLOOD.
> 
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Vladimir Oltean March 14, 2023, 11:08 p.m. UTC | #2 
On Tue, Mar 14, 2023 at 05:36:50PM +0100, Clément Léger wrote:
> +static int a5psw_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> +				       struct switchdev_brport_flags flags,
> +				       struct netlink_ext_ack *extack)
> +{
> +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> +			   BR_BCAST_FLOOD))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +a5psw_port_bridge_flags(struct dsa_switch *ds, int port,
> +			struct switchdev_brport_flags flags,
> +			struct netlink_ext_ack *extack)
> +{
> +	struct a5psw *a5psw = ds->priv;
> +	u32 val;
> +
> +	if (flags.mask & BR_LEARNING) {
> +		val = flags.val & BR_LEARNING ? 0 : A5PSW_INPUT_LEARN_DIS(port);
> +		a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN,
> +			      A5PSW_INPUT_LEARN_DIS(port), val);
> +	}

2 issues.

1: does this not get overwritten by a5psw_port_stp_state_set()?
2: What is the hardware default value for A5PSW_INPUT_LEARN? Please make
   sure that standalone ports have learning disabled by default, when
   the driver probes.

> +
> +	if (flags.mask & BR_FLOOD) {
> +		val = flags.val & BR_FLOOD ? BIT(port) : 0;
> +		a5psw_reg_rmw(a5psw, A5PSW_UCAST_DEF_MASK, BIT(port), val);
> +	}
> +
> +	if (flags.mask & BR_MCAST_FLOOD) {
> +		val = flags.val & BR_MCAST_FLOOD ? BIT(port) : 0;
> +		a5psw_reg_rmw(a5psw, A5PSW_MCAST_DEF_MASK, BIT(port), val);
> +	}
> +
> +	if (flags.mask & BR_BCAST_FLOOD) {
> +		val = flags.val & BR_BCAST_FLOOD ? BIT(port) : 0;
> +		a5psw_reg_rmw(a5psw, A5PSW_BCAST_DEF_MASK, BIT(port), val);
> +	}

Humm, there's a (huge) problem with this flooding mask.

a5psw_flooding_set_resolution() - called from a5psw_port_bridge_join()
and a5psw_port_bridge_leave() - touches the same registers as
a5psw_port_bridge_flags(). Which means that your bridge forwarding
domain controls are the same as your flooding controls.

Which is bad news, because
dsa_port_bridge_leave()
-> dsa_port_switchdev_unsync_attrs()
   -> dsa_port_clear_brport_flags()
      -> dsa_port_bridge_flags()
         -> a5psw_port_bridge_flags()

enables flooding on the port after calling a5psw_port_bridge_leave().
So the port which has left a bridge is standalone, but it still forwards
packets to the other bridged ports!

You should be able to see that this is the case, if you put the ports
under a dummy bridge, then run tools/testing/selftests/drivers/net/dsa/no_forwarding.sh.
Clément Léger March 16, 2023, 11:53 a.m. UTC | #3 
Le Wed, 15 Mar 2023 01:08:21 +0200,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Tue, Mar 14, 2023 at 05:36:50PM +0100, Clément Léger wrote:
> > +static int a5psw_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > +				       struct switchdev_brport_flags flags,
> > +				       struct netlink_ext_ack *extack)
> > +{
> > +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> > +			   BR_BCAST_FLOOD))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +a5psw_port_bridge_flags(struct dsa_switch *ds, int port,
> > +			struct switchdev_brport_flags flags,
> > +			struct netlink_ext_ack *extack)
> > +{
> > +	struct a5psw *a5psw = ds->priv;
> > +	u32 val;
> > +
> > +	if (flags.mask & BR_LEARNING) {
> > +		val = flags.val & BR_LEARNING ? 0 : A5PSW_INPUT_LEARN_DIS(port);
> > +		a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN,
> > +			      A5PSW_INPUT_LEARN_DIS(port), val);
> > +	}  
> 
> 2 issues.
> 
> 1: does this not get overwritten by a5psw_port_stp_state_set()?

Hum indeed. How is this kind of thing supposed to be handled ? Should I
remove the handling of BR_LEARNING to forbid modifying it ? Ot should I
allow it only if STP isn't enabled (which I'm not sure how to do it) ?

> 2: What is the hardware default value for A5PSW_INPUT_LEARN? Please make
>    sure that standalone ports have learning disabled by default, when
>    the driver probes.
> 
> > +
> > +	if (flags.mask & BR_FLOOD) {
> > +		val = flags.val & BR_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_UCAST_DEF_MASK, BIT(port), val);
> > +	}
> > +
> > +	if (flags.mask & BR_MCAST_FLOOD) {
> > +		val = flags.val & BR_MCAST_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_MCAST_DEF_MASK, BIT(port), val);
> > +	}
> > +
> > +	if (flags.mask & BR_BCAST_FLOOD) {
> > +		val = flags.val & BR_BCAST_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_BCAST_DEF_MASK, BIT(port), val);
> > +	}  
> 
> Humm, there's a (huge) problem with this flooding mask.
> 
> a5psw_flooding_set_resolution() - called from a5psw_port_bridge_join()
> and a5psw_port_bridge_leave() - touches the same registers as
> a5psw_port_bridge_flags(). Which means that your bridge forwarding
> domain controls are the same as your flooding controls.
> 
> Which is bad news, because
> dsa_port_bridge_leave()
> -> dsa_port_switchdev_unsync_attrs()
>    -> dsa_port_clear_brport_flags()
>       -> dsa_port_bridge_flags()
>          -> a5psw_port_bridge_flags()  
> 
> enables flooding on the port after calling a5psw_port_bridge_leave().
> So the port which has left a bridge is standalone, but it still forwards
> packets to the other bridged ports!

Actually not this way because the port is configured in a specific mode
which only forward packet to the CPU ports. Indeed, we set a specific
rule using the PATTERN_CTRL register with the MGMTFWD bit set:
When set, the frame is forwarded to the management port only
(suppressing destination address lookup).

However, the port will received packets *from* the other ports (which is
wrong... I can handle that by not setting the flooding attributes if
the port is not in bridge. Doing so would definitely fix the various
problems that could happen.

BTW, the same goes with the learning bit that would be reenabled after
leaving the bridge and you mentionned it should be disabled for a
standalone port.

> 
> You should be able to see that this is the case, if you put the ports
> under a dummy bridge, then run tools/testing/selftests/drivers/net/dsa/no_forwarding.sh.

Yes, makes sense.

Thanks,
Vladimir Oltean March 24, 2023, 10:10 p.m. UTC | #4 
On Thu, Mar 16, 2023 at 12:53:29PM +0100, Clément Léger wrote:
> Le Wed, 15 Mar 2023 01:08:21 +0200,
> Vladimir Oltean <olteanv@gmail.com> a écrit :
> 
> > On Tue, Mar 14, 2023 at 05:36:50PM +0100, Clément Léger wrote:
> > > +static int a5psw_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > > +				       struct switchdev_brport_flags flags,
> > > +				       struct netlink_ext_ack *extack)
> > > +{
> > > +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> > > +			   BR_BCAST_FLOOD))
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +a5psw_port_bridge_flags(struct dsa_switch *ds, int port,
> > > +			struct switchdev_brport_flags flags,
> > > +			struct netlink_ext_ack *extack)
> > > +{
> > > +	struct a5psw *a5psw = ds->priv;
> > > +	u32 val;
> > > +
> > > +	if (flags.mask & BR_LEARNING) {
> > > +		val = flags.val & BR_LEARNING ? 0 : A5PSW_INPUT_LEARN_DIS(port);
> > > +		a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN,
> > > +			      A5PSW_INPUT_LEARN_DIS(port), val);
> > > +	}  
> > 
> > 2 issues.
> > 
> > 1: does this not get overwritten by a5psw_port_stp_state_set()?
> 
> Hum indeed. How is this kind of thing supposed to be handled ? Should I
> remove the handling of BR_LEARNING to forbid modifying it ? Ot should I
> allow it only if STP isn't enabled (which I'm not sure how to do it) ?

It's handled correctly by only enabling learning in port_stp_state_set()
if dp->learning allows it. See sja1105_bridge_stp_state_set():

	case BR_STATE_LEARNING:
		mac[port].dyn_learn = dp->learning;
		break;
	case BR_STATE_FORWARDING:
		mac[port].dyn_learn = dp->learning;

ocelot_bridge_stp_state_set():

	if ((state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) &&
	    ocelot_port->learn_ena)
		learn_ena = ANA_PORT_PORT_CFG_LEARN_ENA;

ksz_port_stp_state_set():

	case BR_STATE_LEARNING:
		if (!p->learning)
			data |= PORT_LEARN_DISABLE;
		break;
	case BR_STATE_FORWARDING:
		if (!p->learning)
			data |= PORT_LEARN_DISABLE;

> > enables flooding on the port after calling a5psw_port_bridge_leave().
> > So the port which has left a bridge is standalone, but it still forwards
> > packets to the other bridged ports!
> 
> Actually not this way because the port is configured in a specific mode
> which only forward packet to the CPU ports. Indeed, we set a specific
> rule using the PATTERN_CTRL register with the MGMTFWD bit set:
> When set, the frame is forwarded to the management port only
> (suppressing destination address lookup).

Ah, cool, this answers one of my issues in the other thread.

> However, the port will received packets *from* the other ports (which is
> wrong... I can handle that by not setting the flooding attributes if
> the port is not in bridge. Doing so would definitely fix the various
> problems that could happen.

hmm.. I guess that could work?
diff mbox series

Patch

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 7dcca15e0b11..5059b2814cdd 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -342,6 +342,49 @@  static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
 		a5psw->br_dev = NULL;
 }
 
+static int a5psw_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+				       struct switchdev_brport_flags flags,
+				       struct netlink_ext_ack *extack)
+{
+	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
+			   BR_BCAST_FLOOD))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+a5psw_port_bridge_flags(struct dsa_switch *ds, int port,
+			struct switchdev_brport_flags flags,
+			struct netlink_ext_ack *extack)
+{
+	struct a5psw *a5psw = ds->priv;
+	u32 val;
+
+	if (flags.mask & BR_LEARNING) {
+		val = flags.val & BR_LEARNING ? 0 : A5PSW_INPUT_LEARN_DIS(port);
+		a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN,
+			      A5PSW_INPUT_LEARN_DIS(port), val);
+	}
+
+	if (flags.mask & BR_FLOOD) {
+		val = flags.val & BR_FLOOD ? BIT(port) : 0;
+		a5psw_reg_rmw(a5psw, A5PSW_UCAST_DEF_MASK, BIT(port), val);
+	}
+
+	if (flags.mask & BR_MCAST_FLOOD) {
+		val = flags.val & BR_MCAST_FLOOD ? BIT(port) : 0;
+		a5psw_reg_rmw(a5psw, A5PSW_MCAST_DEF_MASK, BIT(port), val);
+	}
+
+	if (flags.mask & BR_BCAST_FLOOD) {
+		val = flags.val & BR_BCAST_FLOOD ? BIT(port) : 0;
+		a5psw_reg_rmw(a5psw, A5PSW_BCAST_DEF_MASK, BIT(port), val);
+	}
+
+	return 0;
+}
+
 static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 {
 	u32 mask = A5PSW_INPUT_LEARN_DIS(port) | A5PSW_INPUT_LEARN_BLOCK(port);
@@ -754,6 +797,8 @@  static const struct dsa_switch_ops a5psw_switch_ops = {
 	.set_ageing_time = a5psw_set_ageing_time,
 	.port_bridge_join = a5psw_port_bridge_join,
 	.port_bridge_leave = a5psw_port_bridge_leave,
+	.port_pre_bridge_flags = a5psw_port_pre_bridge_flags,
+	.port_bridge_flags = a5psw_port_bridge_flags,
 	.port_stp_state_set = a5psw_port_stp_state_set,
 	.port_fast_age = a5psw_port_fast_age,
 	.port_fdb_add = a5psw_port_fdb_add,