[PATCH net-next v5 0/3] net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags

[PATCH net-next v5 0/3] net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags

[alexis.lothore]

From: Alexis Lothoré <alexis.lothore@bootlin.com>

Hello,
this series enables vlan support in Renesas RZN1 internal ethernet switch,
and is a follow up to the work initiated by Clement Leger a few months ago,
who handed me over the topic.
This new revision aims to iron the last few points raised by Vladimir to
ensure that the driver is in line with switch drivers expectations, and is
based on the lengthy discussion in [1] (thanks Vladimir for the valuable
explanations)

[1] https://lore.kernel.org/netdev/20230314163651.242259-1-clement.leger@bootlin.com/

----
V5:
 - ensure that flooding can be enabled only if port belongs to a bridge
 - enable learning in a5psw_port_stp_state_set() only if port has learning
   enabled
 - toggle vlan tagging on vlan filtering in
 - removed reviewed-by on second patch since its modified

RESEND V4:
 - Resent due to net-next being closed

V4:
 - Fix missing CPU port bit in a5psw->bridged_ports
 - Use unsigned int for vlan_res_id parameters
 - Rename a5psw_get_vlan_res_entry() to a5psw_new_vlan_res_entry()
 - In a5psw_port_vlan_add(), return -ENOSPC when no VLAN entry is found
 - In a5psw_port_vlan_filtering(), compute "val" from "mask"

V3:
 - Target net-next tree and correct version...

V2:
 - Fixed a few formatting errors
 - Add .port_bridge_flags implementation

Clément Léger (3):
  net: dsa: rzn1-a5psw: use a5psw_reg_rmw() to modify flooding
    resolution
  net: dsa: rzn1-a5psw: add support for .port_bridge_flags
  net: dsa: rzn1-a5psw: add vlan support

 drivers/net/dsa/rzn1_a5psw.c | 240 +++++++++++++++++++++++++++++++++--
 drivers/net/dsa/rzn1_a5psw.h |   8 +-
 2 files changed, 237 insertions(+), 11 deletions(-)

-- 
2.41.0

[PATCH net-next v5 1/3] net: dsa: rzn1-a5psw: use a5psw_reg_rmw() to modify flooding resolution

[alexis.lothore]

From: Clément Léger <clement.leger@bootlin.com>

.port_bridge_flags will be added and allows to modify the flood mask
independently for each port. Keeping the existing bridged_ports write
in a5psw_flooding_set_resolution() would potentially messed up this.
Use a read-modify-write to set that value and move bridged_ports
handling in bridge_port_join/leave.

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

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index c37d2e537230..302529edb4e0 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -331,13 +331,9 @@ static void a5psw_flooding_set_resolution(struct a5psw *a5psw, int port,
 			A5PSW_MCAST_DEF_MASK};
 	int i;
 
-	if (set)
-		a5psw->bridged_ports |= BIT(port);
-	else
-		a5psw->bridged_ports &= ~BIT(port);
-
 	for (i = 0; i < ARRAY_SIZE(offsets); i++)
-		a5psw_reg_writel(a5psw, offsets[i], a5psw->bridged_ports);
+		a5psw_reg_rmw(a5psw, offsets[i], BIT(port),
+			      set ? BIT(port) : 0);
 }
 
 static void a5psw_port_set_standalone(struct a5psw *a5psw, int port,
@@ -365,6 +361,8 @@ static int a5psw_port_bridge_join(struct dsa_switch *ds, int port,
 	a5psw->br_dev = bridge.dev;
 	a5psw_port_set_standalone(a5psw, port, false);
 
+	a5psw->bridged_ports |= BIT(port);
+
 	return 0;
 }
 
@@ -373,6 +371,8 @@ static void a5psw_port_bridge_leave(struct dsa_switch *ds, int port,
 {
 	struct a5psw *a5psw = ds->priv;
 
+	a5psw->bridged_ports &= ~BIT(port);
+
 	a5psw_port_set_standalone(a5psw, port, true);
 
 	/* No more ports bridged */
@@ -992,6 +992,8 @@ static int a5psw_probe(struct platform_device *pdev)
 	if (IS_ERR(a5psw->base))
 		return PTR_ERR(a5psw->base);
 
+	a5psw->bridged_ports = BIT(A5PSW_CPU_PORT);
+
 	ret = a5psw_pcs_get(a5psw);
 	if (ret)
 		return ret;
-- 
2.41.0

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

[alexis.lothore]

From: Clément Léger <clement.leger@bootlin.com>

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 <clement.leger@bootlin.com>
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
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(-)

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

[PATCH net-next v5 3/3] net: dsa: rzn1-a5psw: add vlan support

[alexis.lothore]

From: Clément Léger <clement.leger@bootlin.com>

Add support for vlan operation (add, del, filtering) on the RZN1
driver. The a5psw switch supports up to 32 VLAN IDs with filtering,
tagged/untagged VLANs and PVID for each ports.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Changes since v4:
- ensure vlan port tagging is enabled/disabled only when enabling/disabling
  vlan-aware bridges (by configuring it in a5psw_port_vlan_filtering
  instead of a5psw_port_vlan_<add/del>)
---
 drivers/net/dsa/rzn1_a5psw.c | 166 +++++++++++++++++++++++++++++++++++
 drivers/net/dsa/rzn1_a5psw.h |   8 +-
 2 files changed, 171 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
index e4a93dad1d58..2bb458f2c1f8 100644
--- a/drivers/net/dsa/rzn1_a5psw.c
+++ b/drivers/net/dsa/rzn1_a5psw.c
@@ -639,6 +639,146 @@ static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port,
 	return ret;
 }
 
+static int a5psw_port_vlan_filtering(struct dsa_switch *ds, int port,
+				     bool vlan_filtering,
+				     struct netlink_ext_ack *extack)
+{
+	u32 mask = BIT(port + A5PSW_VLAN_VERI_SHIFT) |
+		   BIT(port + A5PSW_VLAN_DISC_SHIFT);
+	u32 val = vlan_filtering ? mask : 0;
+	struct a5psw *a5psw = ds->priv;
+
+	/* Disable/enable vlan tagging */
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE_ENA, BIT(port),
+		      vlan_filtering ? BIT(port) : 0);
+
+	/* Disable/enable vlan input filtering */
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_VERIFY, mask, val);
+
+	return 0;
+}
+
+static int a5psw_find_vlan_entry(struct a5psw *a5psw, u16 vid)
+{
+	u32 vlan_res;
+	int i;
+
+	/* Find vlan for this port */
+	for (i = 0; i < A5PSW_VLAN_COUNT; i++) {
+		vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i));
+		if (FIELD_GET(A5PSW_VLAN_RES_VLANID, vlan_res) == vid)
+			return i;
+	}
+
+	return -1;
+}
+
+static int a5psw_new_vlan_res_entry(struct a5psw *a5psw, u16 newvid)
+{
+	u32 vlan_res;
+	int i;
+
+	/* Find a free VLAN entry */
+	for (i = 0; i < A5PSW_VLAN_COUNT; i++) {
+		vlan_res = a5psw_reg_readl(a5psw, A5PSW_VLAN_RES(i));
+		if (!(FIELD_GET(A5PSW_VLAN_RES_PORTMASK, vlan_res))) {
+			vlan_res = FIELD_PREP(A5PSW_VLAN_RES_VLANID, newvid);
+			a5psw_reg_writel(a5psw, A5PSW_VLAN_RES(i), vlan_res);
+			return i;
+		}
+	}
+
+	return -1;
+}
+
+static void a5psw_port_vlan_tagged_cfg(struct a5psw *a5psw,
+				       unsigned int vlan_res_id, int port,
+				       bool set)
+{
+	u32 mask = A5PSW_VLAN_RES_WR_PORTMASK | A5PSW_VLAN_RES_RD_TAGMASK |
+		   BIT(port);
+	u32 vlan_res_off = A5PSW_VLAN_RES(vlan_res_id);
+	u32 val = A5PSW_VLAN_RES_WR_TAGMASK, reg;
+
+	if (set)
+		val |= BIT(port);
+
+	/* Toggle tag mask read */
+	a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK);
+	reg = a5psw_reg_readl(a5psw, vlan_res_off);
+	a5psw_reg_writel(a5psw, vlan_res_off, A5PSW_VLAN_RES_RD_TAGMASK);
+
+	reg &= ~mask;
+	reg |= val;
+	a5psw_reg_writel(a5psw, vlan_res_off, reg);
+}
+
+static void a5psw_port_vlan_cfg(struct a5psw *a5psw, unsigned int vlan_res_id,
+				int port, bool set)
+{
+	u32 mask = A5PSW_VLAN_RES_WR_TAGMASK | BIT(port);
+	u32 reg = A5PSW_VLAN_RES_WR_PORTMASK;
+
+	if (set)
+		reg |= BIT(port);
+
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_RES(vlan_res_id), mask, reg);
+}
+
+static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan,
+			       struct netlink_ext_ack *extack)
+{
+	bool tagged = !(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+	struct a5psw *a5psw = ds->priv;
+	u16 vid = vlan->vid;
+	int vlan_res_id;
+
+	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
+		vid, port, tagged ? "tagged" : "untagged",
+		pvid ? "PVID" : "no PVID");
+
+	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
+	if (vlan_res_id < 0) {
+		vlan_res_id = a5psw_new_vlan_res_entry(a5psw, vid);
+		if (vlan_res_id < 0)
+			return -ENOSPC;
+	}
+
+	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, true);
+	if (tagged)
+		a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, true);
+
+	/* Configure port to tag with corresponding VID, but do not enable it
+	 * yet: wait for vlan filtering to be enabled to enable vlan port
+	 * tagging
+	 */
+	if (pvid)
+		a5psw_reg_writel(a5psw, A5PSW_SYSTEM_TAGINFO(port), vid);
+
+	return 0;
+}
+
+static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
+			       const struct switchdev_obj_port_vlan *vlan)
+{
+	struct a5psw *a5psw = ds->priv;
+	u16 vid = vlan->vid;
+	int vlan_res_id;
+
+	dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port);
+
+	vlan_res_id = a5psw_find_vlan_entry(a5psw, vid);
+	if (vlan_res_id < 0)
+		return -EINVAL;
+
+	a5psw_port_vlan_cfg(a5psw, vlan_res_id, port, false);
+	a5psw_port_vlan_tagged_cfg(a5psw, vlan_res_id, port, false);
+
+	return 0;
+}
+
 static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port)
 {
 	u32 reg_lo, reg_hi;
@@ -756,6 +896,27 @@ static void a5psw_get_eth_ctrl_stats(struct dsa_switch *ds, int port,
 	ctrl_stats->MACControlFramesReceived = stat;
 }
 
+static void a5psw_vlan_setup(struct a5psw *a5psw, int port)
+{
+	u32 reg;
+
+	/* Enable TAG always mode for the port, this is actually controlled
+	 * by VLAN_IN_MODE_ENA field which will be used for PVID insertion
+	 */
+	reg = A5PSW_VLAN_IN_MODE_TAG_ALWAYS;
+	reg <<= A5PSW_VLAN_IN_MODE_PORT_SHIFT(port);
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_IN_MODE, A5PSW_VLAN_IN_MODE_PORT(port),
+		      reg);
+
+	/* Set transparent mode for output frame manipulation, this will depend
+	 * on the VLAN_RES configuration mode
+	 */
+	reg = A5PSW_VLAN_OUT_MODE_TRANSPARENT;
+	reg <<= A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port);
+	a5psw_reg_rmw(a5psw, A5PSW_VLAN_OUT_MODE,
+		      A5PSW_VLAN_OUT_MODE_PORT(port), reg);
+}
+
 static int a5psw_setup(struct dsa_switch *ds)
 {
 	struct a5psw *a5psw = ds->priv;
@@ -830,6 +991,8 @@ static int a5psw_setup(struct dsa_switch *ds)
 		/* Enable standalone mode for user ports */
 		if (dsa_port_is_user(dp))
 			a5psw_port_set_standalone(a5psw, port, true);
+
+		a5psw_vlan_setup(a5psw, port);
 	}
 
 	return 0;
@@ -859,6 +1022,9 @@ static const struct dsa_switch_ops a5psw_switch_ops = {
 	.port_bridge_flags = a5psw_port_bridge_flags,
 	.port_stp_state_set = a5psw_port_stp_state_set,
 	.port_fast_age = a5psw_port_fast_age,
+	.port_vlan_filtering = a5psw_port_vlan_filtering,
+	.port_vlan_add = a5psw_port_vlan_add,
+	.port_vlan_del = a5psw_port_vlan_del,
 	.port_fdb_add = a5psw_port_fdb_add,
 	.port_fdb_del = a5psw_port_fdb_del,
 	.port_fdb_dump = a5psw_port_fdb_dump,
diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h
index b869192eef3f..d54acedac194 100644
--- a/drivers/net/dsa/rzn1_a5psw.h
+++ b/drivers/net/dsa/rzn1_a5psw.h
@@ -51,7 +51,9 @@
 #define A5PSW_VLAN_IN_MODE_TAG_ALWAYS		0x2
 
 #define A5PSW_VLAN_OUT_MODE		0x2C
-#define A5PSW_VLAN_OUT_MODE_PORT(port)	(GENMASK(1, 0) << ((port) * 2))
+#define A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port)	((port) * 2)
+#define A5PSW_VLAN_OUT_MODE_PORT(port)	(GENMASK(1, 0) << \
+					A5PSW_VLAN_OUT_MODE_PORT_SHIFT(port))
 #define A5PSW_VLAN_OUT_MODE_DIS		0x0
 #define A5PSW_VLAN_OUT_MODE_STRIP	0x1
 #define A5PSW_VLAN_OUT_MODE_TAG_THROUGH	0x2
@@ -60,7 +62,7 @@
 #define A5PSW_VLAN_IN_MODE_ENA		0x30
 #define A5PSW_VLAN_TAG_ID		0x34
 
-#define A5PSW_SYSTEM_TAGINFO(port)	(0x200 + A5PSW_PORT_OFFSET(port))
+#define A5PSW_SYSTEM_TAGINFO(port)	(0x200 + 4 * (port))
 
 #define A5PSW_AUTH_PORT(port)		(0x240 + 4 * (port))
 #define A5PSW_AUTH_PORT_AUTHORIZED	BIT(0)
@@ -69,7 +71,7 @@
 #define A5PSW_VLAN_RES_WR_PORTMASK	BIT(30)
 #define A5PSW_VLAN_RES_WR_TAGMASK	BIT(29)
 #define A5PSW_VLAN_RES_RD_TAGMASK	BIT(28)
-#define A5PSW_VLAN_RES_ID		GENMASK(16, 5)
+#define A5PSW_VLAN_RES_VLANID		GENMASK(16, 5)
 #define A5PSW_VLAN_RES_PORTMASK		GENMASK(4, 0)
 
 #define A5PSW_RXMATCH_CONFIG(port)	(0x3e80 + 4 * (port))
-- 
2.41.0

Re: [PATCH net-next v5 0/3] net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags

[Simon Horman]

On Thu, Aug 10, 2023 at 11:36:48AM +0200, alexis.lothore@bootlin.com wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Hello,
> this series enables vlan support in Renesas RZN1 internal ethernet switch,
> and is a follow up to the work initiated by Clement Leger a few months ago,
> who handed me over the topic.
> This new revision aims to iron the last few points raised by Vladimir to
> ensure that the driver is in line with switch drivers expectations, and is
> based on the lengthy discussion in [1] (thanks Vladimir for the valuable
> explanations)
> 
> [1] https://lore.kernel.org/netdev/20230314163651.242259-1-clement.leger@bootlin.com/
> 

For series,

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Vladimir Oltean]

Hi Alexis,

On Thu, Aug 10, 2023 at 11:36:50AM +0200, alexis.lothore@bootlin.com 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.

Re: [PATCH net-next v5 3/3] net: dsa: rzn1-a5psw: add vlan support

[Vladimir Oltean]

On Thu, Aug 10, 2023 at 11:36:51AM +0200, alexis.lothore@bootlin.com wrote:
> +static int a5psw_port_vlan_add(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan *vlan,
> +			       struct netlink_ext_ack *extack)
> +{
> +	dev_dbg(a5psw->dev, "Add VLAN %d on port %d, %s, %s\n",
> +		vid, port, tagged ? "tagged" : "untagged",
> +		pvid ? "PVID" : "no PVID");
> +}
> +
> +static int a5psw_port_vlan_del(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan *vlan)
> +{
> +	dev_dbg(a5psw->dev, "Removing VLAN %d on port %d\n", vid, port);
> +}

These are unnecessary, we have trace points for VLANs in net/dsa/trace.h.

Re: [PATCH net-next v5 0/3] net: dsa: rzn1-a5psw: add support for vlan and .port_bridge_flags

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 10 Aug 2023 11:36:48 +0200 you wrote:
> From: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> Hello,
> this series enables vlan support in Renesas RZN1 internal ethernet switch,
> and is a follow up to the work initiated by Clement Leger a few months ago,
> who handed me over the topic.
> This new revision aims to iron the last few points raised by Vladimir to
> ensure that the driver is in line with switch drivers expectations, and is
> based on the lengthy discussion in [1] (thanks Vladimir for the valuable
> explanations)
> 
> [...]

Here is the summary with links:
  - [net-next,v5,1/3] net: dsa: rzn1-a5psw: use a5psw_reg_rmw() to modify flooding resolution
    https://git.kernel.org/netdev/net-next/c/6cf30fdd7b06
  - [net-next,v5,2/3] net: dsa: rzn1-a5psw: add support for .port_bridge_flags
    https://git.kernel.org/netdev/net-next/c/0d37f839836b
  - [net-next,v5,3/3] net: dsa: rzn1-a5psw: add vlan support
    https://git.kernel.org/netdev/net-next/c/7b3f77c428ad

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

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

[Alexis Lothoré]

Hello Vladimir,
On 8/11/23 12:03, Vladimir Oltean wrote:
> Hi Alexis,
> 
> On Thu, Aug 10, 2023 at 11:36:50AM +0200, alexis.lothore@bootlin.com 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.
> 

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Vladimir Oltean]

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.