[RFC net-next 0/2] mt7530 software fallback bridging fix

[RFC net-next 0/2] mt7530 software fallback bridging fix

[DENG Qingfang]

DSA core has gained software fallback support since commit 2f5dc00f7a3e,
but it does not work properly on mt7530. This patch series fixes the
issues.

DENG Qingfang (2):
  net: dsa: tag_mtk: skip address learning on transmit to standalone
    ports
  net: dsa: mt7530: trap packets from standalone ports to the CPU

 drivers/net/dsa/mt7530.c | 28 ++++++++++++++++++++
 drivers/net/dsa/mt7530.h | 56 ++++++++++++++++++++++++++++++++++++++++
 net/dsa/tag_mtk.c        |  6 ++---
 3 files changed, 87 insertions(+), 3 deletions(-)

-- 
2.25.1

[RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[DENG Qingfang]

Consider the following bridge configuration, where bond0 is not
offloaded:

         +-- br0 --+
        / /   |     \
       / /    |      \
      /  |    |     bond0
     /   |    |     /   \
   swp0 swp1 swp2 swp3 swp4
     .        .       .
     .        .       .
     A        B       C

Address learning is enabled on offloaded ports (swp0~2) and the CPU
port, so when client A sends a packet to C, the following will happen:

1. The switch learns that client A can be reached at swp0.
2. The switch probably already knows that client C can be reached at the
   CPU port, so it forwards the packet to the CPU.
3. The bridge core knows client C can be reached at bond0, so it
   forwards the packet back to the switch.
4. The switch learns that client A can be reached at the CPU port.
5. The switch forwards the packet to either swp3 or swp4, according to
   the packet's tag.

That makes client A's MAC address flap between swp0 and the CPU port. If
client B sends a packet to A, it is possible that the packet is
forwarded to the CPU. With offload_fwd_mark = 1, the bridge core won't
forward it back to the switch, resulting in packet loss.

To avoid that, skip address learning on the CPU port when the destination
port is standalone, which can be done by setting the SA_DIS bit of the
MTK tag, if bridge_dev of the destination port is not set.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 net/dsa/tag_mtk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index cc3ba864ad5b..8c361812e21b 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -15,8 +15,7 @@
 #define MTK_HDR_XMIT_TAGGED_TPID_8100	1
 #define MTK_HDR_XMIT_TAGGED_TPID_88A8	2
 #define MTK_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
-#define MTK_HDR_XMIT_DP_BIT_MASK	GENMASK(5, 0)
-#define MTK_HDR_XMIT_SA_DIS		BIT(6)
+#define MTK_HDR_XMIT_SA_DIS_SHIFT	6
 
 static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
@@ -50,7 +49,8 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	 * whether that's a combined special tag with 802.1Q header.
 	 */
 	mtk_tag[0] = xmit_tpid;
-	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
+	mtk_tag[1] = BIT(dp->index) |
+		     (!dp->bridge_dev << MTK_HDR_XMIT_SA_DIS_SHIFT);
 
 	/* Tag control information is kept for 802.1Q */
 	if (xmit_tpid == MTK_HDR_XMIT_UNTAGGED) {
-- 
2.25.1

[RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

Consider the following bridge configuration, where bond0 is not
offloaded:

         +-- br0 --+
        / /   |     \
       / /    |      \
      /  |    |     bond0
     /   |    |     /   \
   swp0 swp1 swp2 swp3 swp4
     .        .       .
     .        .       .
     A        B       C

Ideally, when the switch receives a packet from swp3 or swp4, it should
forward the packet to the CPU, according to the port matrix and unknown
unicast flood settings.

But packet loss will happen if the destination address is at one of the
offloaded ports (swp0~2). For example, when client C sends a packet to
A, the FDB lookup will indicate that it should be forwarded to swp0, but
the port matrix of swp3 and swp4 is configured to only allow the CPU to
be its destination, so it is dropped.

MT7530's FDB has 8 filter IDs, but they are only available for shared
VLAN learning, and all VLAN-unaware ports use 0 as the default filter
ID.

Fortunately, MT7530 supports ACL, and the ACL action happens before the
FDB lookup. So we install an ACL rule which traps all packets to the
CPU, and enable it for standalone ports. This way, the packet loss can
be avoided.

Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c | 28 ++++++++++++++++++++
 drivers/net/dsa/mt7530.h | 56 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 69f21b71614c..6b5c85446e6f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1228,6 +1228,8 @@ mt7530_port_bridge_join(struct dsa_switch *ds, int port,
 		mt7530_rmw(priv, MT7530_PCR_P(port),
 			   PCR_MATRIX_MASK, PCR_MATRIX(port_bitmap));
 	priv->ports[port].pm |= PCR_MATRIX(port_bitmap);
+	/* Don't trap frames to the CPU port */
+	mt7530_clear(priv, MT7530_PCR_P(port), PORT_ACL_EN);
 
 	mutex_unlock(&priv->reg_mutex);
 
@@ -1328,6 +1330,8 @@ mt7530_port_bridge_leave(struct dsa_switch *ds, int port,
 		mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
 			   PCR_MATRIX(BIT(MT7530_CPU_PORT)));
 	priv->ports[port].pm = PCR_MATRIX(BIT(MT7530_CPU_PORT));
+	/* Trap all frames to the CPU port */
+	mt7530_set(priv, MT7530_PCR_P(port), PORT_ACL_EN);
 
 	mutex_unlock(&priv->reg_mutex);
 }
@@ -2037,6 +2041,24 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
 	return ret;
 }
 
+static void
+mt7530_setup_acl(struct mt7530_priv *priv)
+{
+	u32 action;
+
+	/* Set ACL pattern mask to 0 to match unconditionally */
+	mt7530_write(priv, MT7530_VAWD1, 0);
+	mt7530_write(priv, MT7530_VAWD2, 0);
+	mt7530_vlan_cmd(priv, MT7530_VTCR_WR_ACL_MASK, 0);
+
+	/* Set ACL action to forward frames to the CPU port */
+	action = ACL_PORT_EN | ACL_PORT(BIT(MT7530_CPU_PORT)) |
+		 ACL_EG_TAG(MT7530_VLAN_EG_CONSISTENT);
+	mt7530_write(priv, MT7530_VAWD1, action);
+	mt7530_write(priv, MT7530_VAWD2, 0);
+	mt7530_vlan_cmd(priv, MT7530_VTCR_WR_ACL_ACTION, 0);
+}
+
 static int
 mt7530_setup(struct dsa_switch *ds)
 {
@@ -2133,6 +2155,8 @@ mt7530_setup(struct dsa_switch *ds)
 
 			/* Disable learning by default on all user ports */
 			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
+			/* Trap all frames to the CPU port */
+			mt7530_set(priv, MT7530_PCR_P(i), PORT_ACL_EN);
 		}
 		/* Enable consistent egress tag */
 		mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK,
@@ -2300,6 +2324,8 @@ mt7531_setup(struct dsa_switch *ds)
 
 			/* Disable learning by default on all user ports */
 			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
+			/* Trap all frames to the CPU port */
+			mt7530_set(priv, MT7530_PCR_P(i), PORT_ACL_EN);
 		}
 
 		/* Enable consistent egress tag */
@@ -3005,6 +3031,8 @@ mt753x_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	mt7530_setup_acl(priv);
+
 	ret = mt7530_setup_irq(priv);
 	if (ret)
 		return ret;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index b19b389ff10a..10cb278d7c36 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -137,6 +137,15 @@ enum mt7530_vlan_cmd {
 	 */
 	MT7530_VTCR_RD_VID = 0,
 	MT7530_VTCR_WR_VID = 1,
+	/* Read/Write the specified ACL rule pattern */
+	MT7530_VTCR_RD_ACL_PATTERN = 4,
+	MT7530_VTCR_WR_ACL_PATTERN = 5,
+	/* Read/Write the specified ACL rule mask */
+	MT7530_VTCR_RD_ACL_MASK = 8,
+	MT7530_VTCR_WR_ACL_MASK = 9,
+	/* Read/Write the specified ACL rule action */
+	MT7530_VTCR_RD_ACL_ACTION = 10,
+	MT7530_VTCR_WR_ACL_ACTION = 11,
 };
 
 /* Register for setup vlan and acl write data */
@@ -153,6 +162,35 @@ enum mt7530_vlan_cmd {
 #define  PORT_MEM_SHFT			16
 #define  PORT_MEM_MASK			0xff
 
+/* ACL rule pattern */
+#define  BIT_CMP(x)			(((x) & 0xffff) << 16)
+#define  CMP_PAT(x)			((x) & 0xffff)
+
+/* ACL rule action */
+#define  ACL_MANG			BIT(29)
+#define  ACL_INT_EN			BIT(28)
+#define  ACL_CNT_EN			BIT(27)
+#define  ACL_CNT_IDX(x)			(((x) & 0x7) << 24)
+#define  VLAN_PORT_EN			BIT(23)
+#define  DA_SWAP			BIT(22)
+#define  SA_SWAP			BIT(21)
+#define  PPP_RM				BIT(20)
+#define  LKY_VLAN			BIT(19)
+#define  ACL_EG_TAG(x)			(((x) & 0x7) << 16)
+#define  ACL_PORT(x)			(((x) & 0xff) << 8)
+#define  ACL_PORT_EN			BIT(7)
+#define  PRI_USER(x)			(((x) & 0x7) << 4)
+#define  ACL_MIR_EN			BIT(3)
+#define  ACL_PORT_FW(x)			((x) & 0x7)
+
+enum mt7530_to_cpu_port_fw {
+	PORT_FW_DEFAULT,
+	PORT_FW_EXCLUDE_CPU = 4,
+	PORT_FW_INCLUDE_CPU,
+	PORT_FW_CPU_ONLY,
+	PORT_FW_DROP,
+};
+
 #define MT7530_VAWD2			0x98
 /* Egress Tag Control */
 #define  ETAG_CTRL_P(p, x)		(((x) & 0x3) << ((p) << 1))
@@ -164,6 +202,23 @@ enum mt7530_vlan_egress_attr {
 	MT7530_VLAN_EGRESS_STACK = 3,
 };
 
+/* ACL rule pattern */
+#define  ACL_TABLE_EN			BIT(19)
+#define  OFST_TP(x)			(((x) & 0x7) << 16)
+#define  ACL_SP(x)			(((x) & 0xff) << 8)
+#define  WORD_OFST(x)			(((x) & 0x7f) << 1)
+#define  CMP_SEL			BIT(0)
+
+enum mt7530_acl_offset_type {
+	MT7530_ACL_MAC_HEADER,
+	MT7530_ACL_L2_PAYLOAD,
+	MT7530_ACL_IP_HEADER,
+	MT7530_ACL_IP_DATAGRAM,
+	MT7530_ACL_TCP_UDP_HEADER,
+	MT7530_ACL_TCP_UDP_DATAGRAM,
+	MT7530_ACL_IPV6_HEADER,
+};
+
 /* Register for address age control */
 #define MT7530_AAC			0xa0
 /* Disable ageing */
@@ -192,6 +247,7 @@ enum mt7530_stp_state {
 
 /* Register for port control */
 #define MT7530_PCR_P(x)			(0x2004 + ((x) * 0x100))
+#define  PORT_ACL_EN			BIT(10)
 #define  PORT_TX_MIR			BIT(9)
 #define  PORT_RX_MIR			BIT(8)
 #define  PORT_VLAN(x)			((x) & 0x3)
-- 
2.25.1

Re: [RFC net-next 0/2] mt7530 software fallback bridging fix

[Vladimir Oltean]

On Thu, Jul 29, 2021 at 01:53:24AM +0800, DENG Qingfang wrote:
> DSA core has gained software fallback support since commit 2f5dc00f7a3e,
> but it does not work properly on mt7530. This patch series fixes the
> issues.

I haven't looked at the patches, just read the commit messages. Your
approach makes sense considering that mt7530 supports ACL rules. For
switches that don't, I was thinking that we could add a check within DSA
that bridging with software uppers such as LAGs can be allowed only as
long as the bridge is VLAN-aware. If it is, then the classified VLAN for
packets on standalone ports can be made == 0, and if independent VLAN
learning is used, then the FDB entries learned on bridged ports will
always have a VLAN ID != 0, so the standalone switch port won't attempt
to shortcircuit the forwarding process towards the bridge port.
Anyway, we can have both solutions, yours and the generic DSA restriction.
I was just not expecting to see a fix for this already, it makes me
think that the DSA restriction for VLAN-unaware software bridging should
not be unconditional, but we should guard it behind a new bool option
like ds->fdb_shared_across_all_ports = true or something like that.
What do you think?

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Thu, Jul 29, 2021 at 01:53:25AM +0800, DENG Qingfang wrote:
> Consider the following bridge configuration, where bond0 is not
> offloaded:
> 
>          +-- br0 --+
>         / /   |     \
>        / /    |      \
>       /  |    |     bond0
>      /   |    |     /   \
>    swp0 swp1 swp2 swp3 swp4
>      .        .       .
>      .        .       .
>      A        B       C
> 
> Address learning is enabled on offloaded ports (swp0~2) and the CPU
> port, so when client A sends a packet to C, the following will happen:
> 
> 1. The switch learns that client A can be reached at swp0.
> 2. The switch probably already knows that client C can be reached at the
>    CPU port, so it forwards the packet to the CPU.
> 3. The bridge core knows client C can be reached at bond0, so it
>    forwards the packet back to the switch.
> 4. The switch learns that client A can be reached at the CPU port.
> 5. The switch forwards the packet to either swp3 or swp4, according to
>    the packet's tag.
> 
> That makes client A's MAC address flap between swp0 and the CPU port. If
> client B sends a packet to A, it is possible that the packet is
> forwarded to the CPU. With offload_fwd_mark = 1, the bridge core won't
> forward it back to the switch, resulting in packet loss.
> 
> To avoid that, skip address learning on the CPU port when the destination
> port is standalone, which can be done by setting the SA_DIS bit of the
> MTK tag, if bridge_dev of the destination port is not set.
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
>  net/dsa/tag_mtk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> index cc3ba864ad5b..8c361812e21b 100644
> --- a/net/dsa/tag_mtk.c
> +++ b/net/dsa/tag_mtk.c
> @@ -15,8 +15,7 @@
>  #define MTK_HDR_XMIT_TAGGED_TPID_8100	1
>  #define MTK_HDR_XMIT_TAGGED_TPID_88A8	2
>  #define MTK_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
> -#define MTK_HDR_XMIT_DP_BIT_MASK	GENMASK(5, 0)
> -#define MTK_HDR_XMIT_SA_DIS		BIT(6)
> +#define MTK_HDR_XMIT_SA_DIS_SHIFT	6
>  
>  static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>  				    struct net_device *dev)
> @@ -50,7 +49,8 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>  	 * whether that's a combined special tag with 802.1Q header.
>  	 */
>  	mtk_tag[0] = xmit_tpid;
> -	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;

Why stop AND-ing with MTK_HDR_XMIT_DP_BIT_MASK if you were doing that
before? If it's not needed (probably isn't), it would be nice to split
that up.

> +	mtk_tag[1] = BIT(dp->index) |
> +		     (!dp->bridge_dev << MTK_HDR_XMIT_SA_DIS_SHIFT);
>  
>  	/* Tag control information is kept for 802.1Q */
>  	if (xmit_tpid == MTK_HDR_XMIT_UNTAGGED) {
> -- 
> 2.25.1
> 

Otherwise this is as correct as can be without implementing TX
forwarding offload for the bridge (which you've explained why it doesn't
map 1:1 with what your hw can do). But just because a port is under a bridge
doesn't mean that the only packets it sends belong to that bridge. Think
AF_PACKET sockets, PTP etc. The bridge also has a no_linklocal_learn
option that maybe should be taken into consideration for drivers that
can do something meaningful about it. Anyway, food for thought.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[Vladimir Oltean]

On Thu, Jul 29, 2021 at 01:53:26AM +0800, DENG Qingfang wrote:
>  /* Register for setup vlan and acl write data */
> @@ -153,6 +162,35 @@ enum mt7530_vlan_cmd {
>  #define  PORT_MEM_SHFT			16
>  #define  PORT_MEM_MASK			0xff
>  
> +/* ACL rule pattern */
> +#define  BIT_CMP(x)			(((x) & 0xffff) << 16)
> +#define  CMP_PAT(x)			((x) & 0xffff)

not used

> +
> +/* ACL rule action */
> +#define  ACL_MANG			BIT(29)
> +#define  ACL_INT_EN			BIT(28)
> +#define  ACL_CNT_EN			BIT(27)
> +#define  ACL_CNT_IDX(x)			(((x) & 0x7) << 24)
> +#define  VLAN_PORT_EN			BIT(23)
> +#define  DA_SWAP			BIT(22)
> +#define  SA_SWAP			BIT(21)
> +#define  PPP_RM				BIT(20)
> +#define  LKY_VLAN			BIT(19)
> +#define  ACL_EG_TAG(x)			(((x) & 0x7) << 16)
> +#define  ACL_PORT(x)			(((x) & 0xff) << 8)
> +#define  ACL_PORT_EN			BIT(7)
> +#define  PRI_USER(x)			(((x) & 0x7) << 4)
> +#define  ACL_MIR_EN			BIT(3)
> +#define  ACL_PORT_FW(x)			((x) & 0x7)
> +
> +enum mt7530_to_cpu_port_fw {
> +	PORT_FW_DEFAULT,
> +	PORT_FW_EXCLUDE_CPU = 4,
> +	PORT_FW_INCLUDE_CPU,
> +	PORT_FW_CPU_ONLY,
> +	PORT_FW_DROP,
> +};

not used

> +
>  #define MT7530_VAWD2			0x98
>  /* Egress Tag Control */
>  #define  ETAG_CTRL_P(p, x)		(((x) & 0x3) << ((p) << 1))
> @@ -164,6 +202,23 @@ enum mt7530_vlan_egress_attr {
>  	MT7530_VLAN_EGRESS_STACK = 3,
>  };
>  
> +/* ACL rule pattern */
> +#define  ACL_TABLE_EN			BIT(19)
> +#define  OFST_TP(x)			(((x) & 0x7) << 16)
> +#define  ACL_SP(x)			(((x) & 0xff) << 8)
> +#define  WORD_OFST(x)			(((x) & 0x7f) << 1)
> +#define  CMP_SEL			BIT(0)

not used

> +
> +enum mt7530_acl_offset_type {
> +	MT7530_ACL_MAC_HEADER,
> +	MT7530_ACL_L2_PAYLOAD,
> +	MT7530_ACL_IP_HEADER,
> +	MT7530_ACL_IP_DATAGRAM,
> +	MT7530_ACL_TCP_UDP_HEADER,
> +	MT7530_ACL_TCP_UDP_DATAGRAM,
> +	MT7530_ACL_IPV6_HEADER,
> +};

not used

> +
>  /* Register for address age control */
>  #define MT7530_AAC			0xa0
>  /* Disable ageing */
> @@ -192,6 +247,7 @@ enum mt7530_stp_state {
>  
>  /* Register for port control */
>  #define MT7530_PCR_P(x)			(0x2004 + ((x) * 0x100))
> +#define  PORT_ACL_EN			BIT(10)
>  #define  PORT_TX_MIR			BIT(9)
>  #define  PORT_RX_MIR			BIT(8)
>  #define  PORT_VLAN(x)			((x) & 0x3)
> -- 
> 2.25.1
> 

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[Vladimir Oltean]

On Thu, Jul 29, 2021 at 01:53:26AM +0800, DENG Qingfang wrote:
> MT7530's FDB has 8 filter IDs, but they are only available for shared
> VLAN learning, and all VLAN-unaware ports use 0 as the default filter
> ID.

Actually, on second thought...
If MT7530 supports 8 FIDs and it has 7 ports, then you can assign one
FID to each standalone port or VLAN-unaware bridge it is a member of.
The drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c driver has a
similar concept, only instead of FID, it manages FDB IDs - each port is
assigned to an FDB ID and it learns and looks up MAC addresses only
within that FDB ID. Every standalone port uses its own fdb_id, and every
port under the same bridge shares the same fdb_id (the fdb_id associated
with a bridge is equal with the fdb_id of the first standalone port that
joins that bridge; every other standalone port that joins a bridge will
change its fdb_id to that of the bridge). When a port leaves a bridge
and becomes standalone again, its fdb_id will again change to the first
unused value. My point is that if you search for "fdb_id" in that driver
you will maybe find some inspiration for how things like this can be
managed. I know it's not 100% the same as your situation (the FDBs in
the dpaa2-switch are selected by the VLAN table, and for that reason,
the dpaa2-switch can only operate as VLAN-aware, and with shared VLAN
learning per FDB), but the same concept can be reused, I think. With
each port and VLAN-unaware bridge having its own FID, I think you will
not have the shortcircuit issue any longer (and with VLAN-aware bridges
you shouldn't have it anyway).

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

On Thu, Jul 29, 2021 at 11:28 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> Actually, on second thought...
> If MT7530 supports 8 FIDs and it has 7 ports, then you can assign one
> FID to each standalone port or VLAN-unaware bridge it is a member of.

The problem is, there is no way to do that..

According to the reference manual:
Filter ID is learned automatically from VLAN Table. 0 is the default value if
VLAN Table is not applicable.

So it is always 0 in VLAN-unaware mode.

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[Vladimir Oltean]

On Fri, Jul 30, 2021 at 12:11:45AM +0800, DENG Qingfang wrote:
> On Thu, Jul 29, 2021 at 11:28 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > Actually, on second thought...
> > If MT7530 supports 8 FIDs and it has 7 ports, then you can assign one
> > FID to each standalone port or VLAN-unaware bridge it is a member of.
> 
> The problem is, there is no way to do that..
> 
> According to the reference manual:
> Filter ID is learned automatically from VLAN Table. 0 is the default value if
> VLAN Table is not applicable.
> 
> So it is always 0 in VLAN-unaware mode.

I have the MT7621 GSW, and sadly this reference manual isn't the best in
explaining what is and what is not possible. For example, I am still not
clear what is meant by "VID1" and "VID0". Is "VID1" the inner (customer)
VLAN tag, and "VID0" the outer (service) VLAN tag, or "VID1" means the
actual VLAN ID 1?

And the bits 3:1 of VAWD1 (VLAN table access register) indicate a FID
field per VLAN. I cannot find the piece that you quoted in this manual.
But what I expect to happen for a Transparent Port is that the packets
are always classified to that port's PVID, and the VLAN Table is looked
up with that PVID. There, it will find the FID, which this driver
currently always configures as zero. In my manual's description, in the
"Transparent Port" chapter, it does explicitly say:

	VID0 and VID1 will store PVID as the default VID which is used
	to look up the VLAN table.

So I get the impression that the phrase "the VLAN table is not applicable"
is not quite correct, but I might be wrong...

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

On Fri, Jul 30, 2021 at 12:50 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> I have the MT7621 GSW, and sadly this reference manual isn't the best in
> explaining what is and what is not possible. For example, I am still not
> clear what is meant by "VID1" and "VID0". Is "VID1" the inner (customer)
> VLAN tag, and "VID0" the outer (service) VLAN tag, or "VID1" means the
> actual VLAN ID 1?
>
> And the bits 3:1 of VAWD1 (VLAN table access register) indicate a FID
> field per VLAN. I cannot find the piece that you quoted in this manual.
> But what I expect to happen for a Transparent Port is that the packets
> are always classified to that port's PVID, and the VLAN Table is looked
> up with that PVID. There, it will find the FID, which this driver
> currently always configures as zero. In my manual's description, in the
> "Transparent Port" chapter, it does explicitly say:
>
>         VID0 and VID1 will store PVID as the default VID which is used
>         to look up the VLAN table.
>
> So I get the impression that the phrase "the VLAN table is not applicable"
> is not quite correct, but I might be wrong...

Alright, I think I've made some progress.
In the current code, we only use two combinations to toggle user
ports' VLAN awareness: one is PCR.PORT_VLAN set to port matrix mode
with PVC.VLAN_ATTR set to transparent port, the other is PCR.PORT_VLAN
set to security mode with PVC.VLAN_ATTR set to user port.

It turns out that only PVC.VLAN_ATTR contributes to VLAN awareness.
Port matrix mode just skips the VLAN table lookup. The reference
manual is somehow misleading when describing PORT_VLAN modes (See Page
17 of MT7531 Reference Manual, available at
http://wiki.banana-pi.org/Banana_Pi_BPI-R64#Resources). It states that
PORT_MEM (VLAN port member) is used for destination if the VLAN table
lookup hits, but actually it uses **PORT_MEM & PORT_MATRIX** (bitwise
AND of VLAN port member and port matrix) instead, which means we can
have two or more separate VLAN-aware bridges with the same PVID and
traffic won't leak between them.

So I came up with a solution: Set PORT_VLAN to fallback mode when in
VLAN-unaware mode, this way, even VLAN-unaware bridges will use
independent VLAN filtering. Then assign all standalone ports to a
reserved VLAN.

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

On Fri, Jul 30, 2021 at 11:45:41PM +0800, DENG Qingfang wrote:
> So I came up with a solution: Set PORT_VLAN to fallback mode when in
> VLAN-unaware mode, this way, even VLAN-unaware bridges will use
> independent VLAN filtering. Then assign all standalone ports to a

independent VLAN *learning

> reserved VLAN.

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[Vladimir Oltean]

On Fri, Jul 30, 2021 at 11:45:41PM +0800, DENG Qingfang wrote:
> On Fri, Jul 30, 2021 at 12:50 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> > I have the MT7621 GSW, and sadly this reference manual isn't the best in
> > explaining what is and what is not possible. For example, I am still not
> > clear what is meant by "VID1" and "VID0". Is "VID1" the inner (customer)
> > VLAN tag, and "VID0" the outer (service) VLAN tag, or "VID1" means the
> > actual VLAN ID 1?
> >
> > And the bits 3:1 of VAWD1 (VLAN table access register) indicate a FID
> > field per VLAN. I cannot find the piece that you quoted in this manual.
> > But what I expect to happen for a Transparent Port is that the packets
> > are always classified to that port's PVID, and the VLAN Table is looked
> > up with that PVID. There, it will find the FID, which this driver
> > currently always configures as zero. In my manual's description, in the
> > "Transparent Port" chapter, it does explicitly say:
> >
> >         VID0 and VID1 will store PVID as the default VID which is used
> >         to look up the VLAN table.
> >
> > So I get the impression that the phrase "the VLAN table is not applicable"
> > is not quite correct, but I might be wrong...
> 
> Alright, I think I've made some progress.
> In the current code, we only use two combinations to toggle user
> ports' VLAN awareness: one is PCR.PORT_VLAN set to port matrix mode
> with PVC.VLAN_ATTR set to transparent port, the other is PCR.PORT_VLAN
> set to security mode with PVC.VLAN_ATTR set to user port.
> 
> It turns out that only PVC.VLAN_ATTR contributes to VLAN awareness.
> Port matrix mode just skips the VLAN table lookup. The reference
> manual is somehow misleading when describing PORT_VLAN modes (See Page
> 17 of MT7531 Reference Manual, available at
> http://wiki.banana-pi.org/Banana_Pi_BPI-R64#Resources). It states that
> PORT_MEM (VLAN port member) is used for destination if the VLAN table
> lookup hits, but actually it uses **PORT_MEM & PORT_MATRIX** (bitwise
> AND of VLAN port member and port matrix) instead, which means we can
> have two or more separate VLAN-aware bridges with the same PVID and
> traffic won't leak between them.

Ah, but it's not completely misleading. It does say:

	2'b01: Fallback mode

	Enable 802.1Q function for all the received frames.
	Do not discard received frames due to ingress membership violation.
	**Frames whose VID is missed on the VLAN table will be filtered
	by the Port Matrix Member**.

(emphasis mine on the last paragraph)

> So I came up with a solution: Set PORT_VLAN to fallback mode when in
> VLAN-unaware mode, this way, even VLAN-unaware bridges will use
> independent VLAN filtering.

If you did indeed test that the Port Matrix is still used to enforce
separation between ports if the VLAN table _does_ match and we're in
fallback mode, then we should be okay.

> Then assign all standalone ports to a reserved VLAN.

You mean all standalone ports to the same VLAN ID, like 4095, or each
standalone port to a separate reserved VLAN ID? As long as address
learning is disabled on the standalone ports, I guess using a single
VLAN ID like 4095 for all of them is just fine, the Port Matrix will
take care of the rest.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Wed, Jul 28, 2021 at 09:37:05PM +0300, Vladimir Oltean wrote:
> Otherwise this is as correct as can be without implementing TX
> forwarding offload for the bridge (which you've explained why it doesn't
> map 1:1 with what your hw can do). But just because a port is under a bridge
> doesn't mean that the only packets it sends belong to that bridge. Think
> AF_PACKET sockets, PTP etc. The bridge also has a no_linklocal_learn
> option that maybe should be taken into consideration for drivers that
> can do something meaningful about it. Anyway, food for thought.

Considering that you also have the option of setting
ds->assisted_learning_on_cpu_port = true and this will have less false
positives, what are the reasons why you did not choose that approach?

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

On Fri, Jul 30, 2021 at 07:18:52PM +0300, Vladimir Oltean wrote:
> > It turns out that only PVC.VLAN_ATTR contributes to VLAN awareness.
> > Port matrix mode just skips the VLAN table lookup. The reference
> > manual is somehow misleading when describing PORT_VLAN modes (See Page
> > 17 of MT7531 Reference Manual, available at
> > http://wiki.banana-pi.org/Banana_Pi_BPI-R64#Resources). It states that
> > PORT_MEM (VLAN port member) is used for destination if the VLAN table
> > lookup hits, but actually it uses **PORT_MEM & PORT_MATRIX** (bitwise
> > AND of VLAN port member and port matrix) instead, which means we can
> > have two or more separate VLAN-aware bridges with the same PVID and
> > traffic won't leak between them.
> 
> Ah, but it's not completely misleading. It does say:
> 
> 	2'b01: Fallback mode
> 
> 	Enable 802.1Q function for all the received frames.
> 	Do not discard received frames due to ingress membership violation.
> 	**Frames whose VID is missed on the VLAN table will be filtered
> 	by the Port Matrix Member**.
> 
> (emphasis mine on the last paragraph)
> 
> > So I came up with a solution: Set PORT_VLAN to fallback mode when in
> > VLAN-unaware mode, this way, even VLAN-unaware bridges will use
> > independent VLAN filtering.
> 
> If you did indeed test that the Port Matrix is still used to enforce
> separation between ports if the VLAN table _does_ match and we're in
> fallback mode, then we should be okay.

Yes, that's what I mean. Tested as well.

> 
> > Then assign all standalone ports to a reserved VLAN.
> 
> You mean all standalone ports to the same VLAN ID, like 4095, or each
> standalone port to a separate reserved VLAN ID? As long as address
> learning is disabled on the standalone ports, I guess using a single
> VLAN ID like 4095 for all of them is just fine, the Port Matrix will
> take care of the rest.

I just found a cleaner solution: Leaving standalone ports in port matrix
mode. As all bridges use independent VLAN learning, standalone ports'
FDB lookup with FID 0 won't hit.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[DENG Qingfang]

On Fri, Jul 30, 2021 at 07:24:03PM +0300, Vladimir Oltean wrote:
> Considering that you also have the option of setting
> ds->assisted_learning_on_cpu_port = true and this will have less false
> positives, what are the reasons why you did not choose that approach?

You're right. Hardware learning on CPU port does have some limitations.

I have been testing a multi CPU ports patch, and assisted learning has
to be used, because FDB entries should be installed like multicast
ones, which point to all CPU ports.

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[Vladimir Oltean]

On Sat, Jul 31, 2021 at 01:21:14AM +0800, DENG Qingfang wrote:
> I just found a cleaner solution: Leaving standalone ports in port matrix
> mode. As all bridges use independent VLAN learning, standalone ports'
> FDB lookup with FID 0 won't hit.

So standalone ports are completely VLAN-unaware and always use a FID of
0, ports under a VLAN-unaware bridge are in fallback mode (look up the
VLAN table but don't drop on miss), use a FID of 1-7, and ports under a
VLAN-aware bridge are in the security mode and use the CVID instead of
the FID for VLAN classification?

Make sure to test a mix of standalone, VLAN-unaware bridge and
VLAN-aware bridge with the same MAC address in all 3 domains. If that
works well this should be really good.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Sat, Jul 31, 2021 at 01:32:03AM +0800, DENG Qingfang wrote:
> On Fri, Jul 30, 2021 at 07:24:03PM +0300, Vladimir Oltean wrote:
> > Considering that you also have the option of setting
> > ds->assisted_learning_on_cpu_port = true and this will have less false
> > positives, what are the reasons why you did not choose that approach?
> 
> You're right. Hardware learning on CPU port does have some limitations.
> 
> I have been testing a multi CPU ports patch, and assisted learning has
> to be used, because FDB entries should be installed like multicast
> ones, which point to all CPU ports.

Ah, mt7530 is one of the switches which has multiple CPU ports, I had
forgotten that. In that case, then static FDB entries are pretty much
the only way to go indeed.

I am going to send a patch series soon to convert sja1105 to assisted
learning too. It doesn't support multiple CPU ports, and it does have
hardware learning on the CPU port, but it can be arranged in cross-chip
topologies where each switch has its own CPU port, so from DSA's
perspective, it is as though we are dealing with a multi-CPU port switch
(the DSA tree does have multiple CPUs, in fact).  I have been
obsessively testing this configuration for the past few weeks and I
think the assisted learning functionality works fairly well by now.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Fri, Jul 30, 2021 at 08:39:02PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 31, 2021 at 01:32:03AM +0800, DENG Qingfang wrote:
> > On Fri, Jul 30, 2021 at 07:24:03PM +0300, Vladimir Oltean wrote:
> > > Considering that you also have the option of setting
> > > ds->assisted_learning_on_cpu_port = true and this will have less false
> > > positives, what are the reasons why you did not choose that approach?
> > 
> > You're right. Hardware learning on CPU port does have some limitations.
> > 
> > I have been testing a multi CPU ports patch, and assisted learning has
> > to be used, because FDB entries should be installed like multicast
> > ones, which point to all CPU ports.
> 
> Ah, mt7530 is one of the switches which has multiple CPU ports, I had
> forgotten that. In that case, then static FDB entries are pretty much
> the only way to go indeed.

I forget which ones are the modes in which the multi-CPU feature on
mt7530 is supposed to be used: static assignment of user ports to CPU
ports, or LAG between the CPU ports, or a mix of both?

Re: [RFC net-next 2/2] net: dsa: mt7530: trap packets from standalone ports to the CPU

[DENG Qingfang]

On Fri, Jul 30, 2021 at 08:35:11PM +0300, Vladimir Oltean wrote:
> On Sat, Jul 31, 2021 at 01:21:14AM +0800, DENG Qingfang wrote:
> > I just found a cleaner solution: Leaving standalone ports in port matrix
> > mode. As all bridges use independent VLAN learning, standalone ports'
> > FDB lookup with FID 0 won't hit.
> 
> So standalone ports are completely VLAN-unaware and always use a FID of
> 0, ports under a VLAN-unaware bridge are in fallback mode (look up the
> VLAN table but don't drop on miss), use a FID of 1-7, and ports under a
> VLAN-aware bridge are in the security mode and use the CVID instead of
> the FID for VLAN classification?

No. Both VLAN-unaware and VLAN-aware bridges use independent VLAN learning
i.e. use CVID for FDB lookup.

> 
> Make sure to test a mix of standalone, VLAN-unaware bridge and
> VLAN-aware bridge with the same MAC address in all 3 domains. If that
> works well this should be really good.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[DENG Qingfang]

On Fri, Jul 30, 2021 at 08:41:35PM +0300, Vladimir Oltean wrote:
> On Fri, Jul 30, 2021 at 08:39:02PM +0300, Vladimir Oltean wrote:
> > Ah, mt7530 is one of the switches which has multiple CPU ports, I had
> > forgotten that. In that case, then static FDB entries are pretty much
> > the only way to go indeed.
> 
> I forget which ones are the modes in which the multi-CPU feature on
> mt7530 is supposed to be used: static assignment of user ports to CPU
> ports, or LAG between the CPU ports, or a mix of both?

MT7530 only supports static assignment, by changing the port matrix.

MT7531 also supports hardware LAG, but I don't think it's ideal because
its CPU ports have different speeds (one 1Gbps RGMII and the other 2.5Gbps
HSGMII).

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[DENG Qingfang]

On Fri, Jul 30, 2021 at 07:24:03PM +0300, Vladimir Oltean wrote:
> Considering that you also have the option of setting
> ds->assisted_learning_on_cpu_port = true and this will have less false
> positives, what are the reasons why you did not choose that approach?

After enabling it, I noticed .port_fdb_{add,del} are called with VID=0
(which it does not use now) unless I turn on VLAN filtering. Is that
normal?

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Sat, Jul 31, 2021 at 03:00:20AM +0800, DENG Qingfang wrote:
> On Fri, Jul 30, 2021 at 07:24:03PM +0300, Vladimir Oltean wrote:
> > Considering that you also have the option of setting
> > ds->assisted_learning_on_cpu_port = true and this will have less false
> > positives, what are the reasons why you did not choose that approach?
> 
> After enabling it, I noticed .port_fdb_{add,del} are called with VID=0
> (which it does not use now) unless I turn on VLAN filtering. Is that
> normal?

They are called with the VID from the learned packet.
If the bridge is VLAN-unaware, the MAC SA is learned with VID 0.
Generally, VID 0 is always used for VLAN-unaware bridging. You can
privately translate VID 0 to whatever VLAN ID you use in VLAN-unaware
mode.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[DENG Qingfang]

On Fri, Jul 30, 2021 at 10:07:06PM +0300, Vladimir Oltean wrote:
> > After enabling it, I noticed .port_fdb_{add,del} are called with VID=0
> > (which it does not use now) unless I turn on VLAN filtering. Is that
> > normal?
> 
> They are called with the VID from the learned packet.
> If the bridge is VLAN-unaware, the MAC SA is learned with VID 0.
> Generally, VID 0 is always used for VLAN-unaware bridging. You can
> privately translate VID 0 to whatever VLAN ID you use in VLAN-unaware
> mode.

Now the issue is PVID is always set to the bridge's vlan_default_pvid,
regardless of VLAN awareless.

Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports

[Vladimir Oltean]

On Sat, Jul 31, 2021 at 03:25:55AM +0800, DENG Qingfang wrote:
> On Fri, Jul 30, 2021 at 10:07:06PM +0300, Vladimir Oltean wrote:
> > > After enabling it, I noticed .port_fdb_{add,del} are called with VID=0
> > > (which it does not use now) unless I turn on VLAN filtering. Is that
> > > normal?
> > 
> > They are called with the VID from the learned packet.
> > If the bridge is VLAN-unaware, the MAC SA is learned with VID 0.
> > Generally, VID 0 is always used for VLAN-unaware bridging. You can
> > privately translate VID 0 to whatever VLAN ID you use in VLAN-unaware
> > mode.
> 
> Now the issue is PVID is always set to the bridge's vlan_default_pvid,
> regardless of VLAN awareless.

Then change that, sja1105 and ocelot/felix are good examples of how to
set a pvid in VLAN-unaware mode that is independent of what the bridge
asks for.