Message-ID: <20230130125813.asx5qtm6ttuwdobo@skbuf>
Date:   Mon, 30 Jan 2023 14:58:13 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Frank Wunderlich <frank-w@...lic-files.de>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Landen Chao <Landen.Chao@...iatek.com>,
        Sean Wang <sean.wang@...iatek.com>,
        DENG Qingfang <dqfext@...il.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Daniel Golle <daniel@...rotopia.org>
Subject: Re: [BUG] vlan-aware bridge breaks vlan on another port on same gmac

Hi Frank,

On Sat, Jan 21, 2023 at 03:12:50PM +0100, Frank Wunderlich wrote:
> Hi
> 
> > Gesendet: Samstag, 21. Januar 2023 um 14:35 Uhr
> > Von: "Vladimir Oltean" <olteanv@...il.com>
> > An: "Frank Wunderlich" <frank-w@...lic-files.de>
> > Cc: "Andrew Lunn" <andrew@...n.ch>, "Florian Fainelli" <f.fainelli@...il.com>, "David S. Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>, "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, "Landen Chao" <Landen.Chao@...iatek.com>, "Sean Wang" <sean.wang@...iatek.com>, "DENG Qingfang" <dqfext@...il.com>, "Matthias Brugger" <matthias.bgg@...il.com>, "Daniel Golle" <daniel@...rotopia.org>
> > Betreff: Re: [BUG] vlan-aware bridge breaks vlan on another port on same gmac
> >
> > On Sat, Jan 21, 2023 at 01:32:42PM +0100, Frank Wunderlich wrote:
> > > so first patch fixes the behaviour on bpi-r3 (mt7531)...but maybe mt7530 need the tagging on cpu-port
> > >
> > > > Can you try the second patch instead of the first one? Without digging
> > > > deeply into mt7530 hardware docs, that's the best chance of making
> > > > things work without changing how the hardware operates.
> > >
> > > second patch works for wan, but vlan on bridge is broken, no packets receiving my laptop (also no untagged ones).
> >
> > It's hard for me to understand how applying only patch "tag_mtk only
> > combine VLAN tag with MTK tag is user port is VLAN aware" can produce
> > the results you describe... For packets sent to port lan0, nothing
> > should have been changed by that patch, because dsa_port_is_vlan_filtering(dp)
> > should return true.
> >
> > If you can confirm there isn't any mistake in the testing procedure,
> > I'll take a look later today at the hardware documentation and try to
> > figure out why the CPU port is configured the way it is.
> 
> ok, booted again the kernel with first patch ("mt7530 don't make the CPU port a VLAN user port")
> and yes lan0-vlan is broken...
> seems i need to reboot after each lan/wan test to at least clean arp-cache.
> 
> but patch2 ("tag_mtk only combine VLAN tag with MTK tag is user port is VLAN aware") still not
> works on lanbridge vlan (no packet received on target).
> 
> regards Frank

Sorry for the delay and thanks again for testing.

I simply didn't have time to sit down with the hardware documentation
and (re)understand the concepts governing this switch.

I now have the patch below which should have everything working. Would
you mind testing it?

>From 9110460832d99c3b3e86ffcda472a27a52cdf259 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Mon, 30 Jan 2023 14:31:17 +0200
Subject: [PATCH] net: dsa: mt7530: don't change PVC_EG_TAG when CPU port
 becomes VLAN-aware

Frank reports that in a mt7530 setup where some ports are standalone and
some are in a VLAN-aware bridge, 8021q uppers of the standalone ports
lose their VLAN tag on xmit, as seen by the link partner.

This seems to occur because once the other ports join the VLAN-aware
bridge, mt7530_port_vlan_filtering() also calls
mt7530_port_set_vlan_aware(ds, cpu_dp->index), and this affects the way
that the switch processes the traffic of the standalone port.

Relevant is the PVC_EG_TAG bit. The MT7530 documentation says about it:

EG_TAG: Incoming Port Egress Tag VLAN Attribution
0: disabled (system default)
1: consistent (keep the original ingress tag attribute)

My interpretation is that this setting applies on the ingress port, and
"disabled" is basically the normal behavior, where the egress tag format
of the packet (tagged or untagged) is decided by the VLAN table
(MT7530_VLAN_EGRESS_UNTAG or MT7530_VLAN_EGRESS_TAG).

But there is also an option of overriding the system default behavior,
and for the egress tagging format of packets to be decided not by the
VLAN table, but simply by copying the ingress tag format (if ingress was
tagged, egress is tagged; if ingress was untagged, egress is untagged;
aka "consistent). This is useful in 2 scenarios:

- VLAN-unaware bridge ports will always encounter a miss in the VLAN
  table. They should forward a packet as-is, though. So we use
  "consistent" there. See commit e045124e9399 ("net: dsa: mt7530: fix
  tagged frames pass-through in VLAN-unaware mode").

- Traffic injected from the CPU port. The operating system is in god
  mode; if it wants a packet to exit as VLAN-tagged, it sends it as
  VLAN-tagged. Otherwise it sends it as VLAN-untagged*.

*This is true only if we don't consider the bridge TX forwarding offload
feature, which mt7530 doesn't support.

So for now, make the CPU port always stay in "consistent" mode to allow
software VLANs to be forwarded to their egress ports with the VLAN tag
intact, and not stripped.

Link: https://lore.kernel.org/netdev/trinity-e6294d28-636c-4c40-bb8b-b523521b00be-1674233135062@3c-app-gmx-bs36/
Fixes: e045124e9399 ("net: dsa: mt7530: fix tagged frames pass-through in VLAN-unaware mode")
Reported-by: Frank Wunderlich <frank-w@...lic-files.de>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/dsa/mt7530.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 616b21c90d05..3a15015bc409 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1302,14 +1302,26 @@ mt7530_port_set_vlan_aware(struct dsa_switch *ds, int port)
 		if (!priv->ports[port].pvid)
 			mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
 				   MT7530_VLAN_ACC_TAGGED);
-	}
 
-	/* Set the port as a user port which is to be able to recognize VID
-	 * from incoming packets before fetching entry within the VLAN table.
-	 */
-	mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK | PVC_EG_TAG_MASK,
-		   VLAN_ATTR(MT7530_VLAN_USER) |
-		   PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));
+		/* Set the port as a user port which is to be able to recognize
+		 * VID from incoming packets before fetching entry within the
+		 * VLAN table.
+		 */
+		mt7530_rmw(priv, MT7530_PVC_P(port),
+			   VLAN_ATTR_MASK | PVC_EG_TAG_MASK,
+			   VLAN_ATTR(MT7530_VLAN_USER) |
+			   PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));
+	} else {
+		/* Also set CPU ports to the "user" VLAN port attribute, to
+		 * allow VLAN classification, but keep the EG_TAG attribute as
+		 * "consistent" (i.o.w. don't change its value) for packets
+		 * received by the switch from the CPU, so that tagged packets
+		 * are forwarded to user ports as tagged, and untagged as
+		 * untagged.
+		 */
+		mt7530_rmw(priv, MT7530_PVC_P(port), VLAN_ATTR_MASK,
+			   VLAN_ATTR(MT7530_VLAN_USER));
+	}
 }
 
 static void
-- 
2.34.1

Powered by blists - more mailing lists