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

Commit Message

Alexis Lothoré (eBPF Foundation) Aug. 10, 2023, 9:36 a.m. UTC

From: Clément Léger <[e-mail]>

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.

Since .port_bridge_flags affects the bits disabling learning for a port,
ensure that any other modification on the same register done by
a5psw_port_stp_state_set is in sync by using the port learning state to
enable/disable learning on the port.

Signed-off-by: Clément Léger <[e-mail]>
Signed-off-by: Alexis Lothoré <[e-mail]>
---
Changes since v4:
- ensure that learning and flooding masks are not updated if port does not
  belong to bridge
- remove reviewed-by since patch is modified
---
 drivers/net/dsa/rzn1_a5psw.c | 60 ++++++++++++++++++++++++++++++++++--
 1 file changed, 58 insertions(+), 2 deletions(-)

Comments

Vladimir Oltean Aug. 11, 2023, 10:03 a.m. UTC | #1

Hi Alexis,

On Thu, Aug 10, 2023 at 11:36:50AM +0200, [e-mail] wrote:
> +	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);
> +	}

These 3 port masks will only do what you expect while the bridge has
vlan_filtering=0, correct? When vlan_filtering=1, packets classified to
a VLAN which don't hit any FDB entry will be always flooded to all ports
in that VLAN, correct?

Maybe you could restrict transitions to flooding disabled on ports with
vlan_filtering 1, and restrict transitions to vlan_filtering 1 on ports
with flooding disabled. Or at least add some comments about the
limitations. I wouldn't want subtle incompatibilities between the
hardware design and Linux' expectations to go under the radar like this.

Alexis Lothoré (eBPF Foundation) Aug. 11, 2023, 2:42 p.m. UTC | #2

Hello Vladimir,
On 8/11/23 12:03, Vladimir Oltean wrote:
> Hi Alexis,
> 
> On Thu, Aug 10, 2023 at 11:36:50AM +0200, [e-mail] wrote:
>> +	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);
>> +	}
> 
> These 3 port masks will only do what you expect while the bridge has
> vlan_filtering=0, correct? When vlan_filtering=1, packets classified to
> a VLAN which don't hit any FDB entry will be always flooded to all ports
> in that VLAN, correct?

After thoroughly reading the A5PSW doc again, I feel that this sentence is not
exactly true. If I refer to section 4.5.3.9, paragraph 3.c:

The VLAN table is used for both, VLAN domain verification [...] as well as VLAN
resolution. Once the frame has passed any VLAN domain verification (i.e. will
not be discarded by the verification function already), the forwarding
resolution applies.
[...]
- If the destination MAC address (Unicast or Multicast) is not found in the MAC
address table, or if the destination address is the Broadcast address, the frame
is forwarded according to the following rules:
  - The destination port mask is loaded from the respective register
U/M/BCAST_DEFAULT_MASK depending on unicast, multicast or broadcast. Then the
following filtering on this mask applies.
    - If the frame carries a VLAN tag, the VLAN resolution table is searched for
a matching VLAN ID and the frame is sent only to ports that are associated with
the VLAN ID.
    - If the frame carries a VLAN tag and the VLAN ID does not match any entry
in the VLAN Resolution Table, or the frame does not carry a VLAN tag, the frame
is forwarded to all ports that are enabled by the default mask.
    - If it cannot be associated with any VLAN group and if the default group
has been set to all zero, the frame is discarded.
[...]

I understand from the second bullet that even when vlan filtering is enabled
(which occurs as first step), the first flooding filter (used in second step,
resolution) remains the flooding masks from unicast/multicast/broadcast default
mask registers. The vlan resolution is then applied over it as a second filter,
and only make the flooding more "restrictive", it does not bypass it (so if a
port is in the vlan which VID is in an incoming packet but the port is not also
defined in the U/M/B default mask, incoming packet won't be flooded to it).
> 
> Maybe you could restrict transitions to flooding disabled on ports with
> vlan_filtering 1, and restrict transitions to vlan_filtering 1 on ports
> with flooding disabled. Or at least add some comments about the
> limitations. I wouldn't want subtle incompatibilities between the
> hardware design and Linux' expectations to go under the radar like this.
>

Vladimir Oltean Aug. 17, 2023, 5:35 p.m. UTC | #3

Hi Alexis,

On Fri, Aug 11, 2023 at 04:42:18PM +0200, Alexis Lothoré wrote:
> > These 3 port masks will only do what you expect while the bridge has
> > vlan_filtering=0, correct? When vlan_filtering=1, packets classified to
> > a VLAN which don't hit any FDB entry will be always flooded to all ports
> > in that VLAN, correct?
> 
> After thoroughly reading the A5PSW doc again, I feel that this sentence is not
> exactly true. If I refer to section 4.5.3.9, paragraph 3.c:
> 
> The VLAN table is used for both, VLAN domain verification [...] as well as VLAN
> resolution. Once the frame has passed any VLAN domain verification (i.e. will
> not be discarded by the verification function already), the forwarding
> resolution applies.
> [...]
> - If the destination MAC address (Unicast or Multicast) is not found in the MAC
> address table, or if the destination address is the Broadcast address, the frame
> is forwarded according to the following rules:
>   - The destination port mask is loaded from the respective register
> U/M/BCAST_DEFAULT_MASK depending on unicast, multicast or broadcast. Then the
> following filtering on this mask applies.
>     - If the frame carries a VLAN tag, the VLAN resolution table is searched for
> a matching VLAN ID and the frame is sent only to ports that are associated with
> the VLAN ID.
>     - If the frame carries a VLAN tag and the VLAN ID does not match any entry
> in the VLAN Resolution Table, or the frame does not carry a VLAN tag, the frame
> is forwarded to all ports that are enabled by the default mask.
>     - If it cannot be associated with any VLAN group and if the default group
> has been set to all zero, the frame is discarded.
> [...]
> 
> I understand from the second bullet that even when vlan filtering is enabled
> (which occurs as first step), the first flooding filter (used in second step,
> resolution) remains the flooding masks from unicast/multicast/broadcast default
> mask registers. The vlan resolution is then applied over it as a second filter,
> and only make the flooding more "restrictive", it does not bypass it (so if a
> port is in the vlan which VID is in an incoming packet but the port is not also
> defined in the U/M/B default mask, incoming packet won't be flooded to it).

Thanks for the clarification. In this case, the code is fine. I must have left
with the wrong impression from the previous discussion with Clément.

Patch

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index 302529edb4e0..e4a93dad1d58 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -380,9 +380,63 @@  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 a port is set as standalone, we do not want to be able to
+	 * configure flooding nor learning which would result in joining the
+	 * unique bridge. This can happen when a port leaves the bridge, in
+	 * which case the DSA core will try to "clear" all flags for the
+	 * standalone port (ie enable flooding, disable learning). In that case
+	 * do not fail but do not apply the flags.
+	 */
+	if (!(a5psw->bridged_ports & BIT(port)))
+		return 0;
+
+	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)
 {
 	bool learning_enabled, rx_enabled, tx_enabled;
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct a5psw *a5psw = ds->priv;
 
 	switch (state) {
@@ -396,12 +450,12 @@  static void a5psw_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 	case BR_STATE_LEARNING:
 		rx_enabled = false;
 		tx_enabled = false;
-		learning_enabled = true;
+		learning_enabled = dp->learning;
 		break;
 	case BR_STATE_FORWARDING:
 		rx_enabled = true;
 		tx_enabled = true;
-		learning_enabled = true;
+		learning_enabled = dp->learning;
 		break;
 	default:
 		dev_err(ds->dev, "invalid STP state: %d\n", state);
@@ -801,6 +855,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,