[PATCH net-next v1 0/6] net: dsa: mt7628: add VLAN filtering support

[PATCH net-next v1 0/6] net: dsa: mt7628: add VLAN filtering support

[Joris Vaisvila]

Add bridge offload and VLAN filtering support to the MT7628 switch.

The MT7628 switch uses VLAN membership as its forwarding mechanism. This
series uses tag_8021q VLANs to emulate VLAN-unaware bridge forwarding,
while using the bridge's VLAN configuration directly when VLAN filtering
is enabled. A major limitation is that VIDs cannot be reused between
separate bridges, as this would allow cross-bridge forwarding. tag_8021q
VIDs are also reserved for the same reason.

The switch driver adds 6x "reserved" VLAN slots for use by tag_8021q
that are only programmed on VLAN-unaware ports, together with double
tagging, while the remaining slots are only programmed for VLAN-aware
ports.

The tagger is updated to preserve the existing VLAN tags on transmit and
receive.

Thanks,
Joris

Joris Vaisvila (6):
  net: dsa: mt7628: rework vlan block allocator
  net: dsa: mt7628: add port bridge offload support
  net: dsa: tag: mt7628: add bridge support
  net: dsa: mt7628: add VLAN filtering support
  net: dsa: tag: mt7628: add VLAN awareness support
  MAINTAINERS: add myself as MT7628 embedded switch maintainer

 MAINTAINERS              |   9 ++
 drivers/net/dsa/mt7628.c | 305 ++++++++++++++++++++++++++++++++-------
 net/dsa/tag_mt7628.c     |  66 +++++++--
 3 files changed, 313 insertions(+), 67 deletions(-)

-- 
2.55.0

[PATCH net-next v1 1/6] net: dsa: mt7628: rework vlan block allocator

[Joris Vaisvila]

Rework the VLAN hardware entry allocator to allow reserving entries for
tag_8021q bridge/isolation use. This is prerequisite work for VLAN
filtering support.

6 VLAN table entries are reserved for tag_8021q use and the rest are
left for the upcoming VLAN filtering support. No functional change
intended.

The active property is removed from VLAN entries in favor of vid == 0,
since tag_8021q VIDs cannot be 0.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 drivers/net/dsa/mt7628.c | 160 +++++++++++++++++++++++++--------------
 1 file changed, 104 insertions(+), 56 deletions(-)

diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
index fb63f6f644b9..990556cc2f65 100644
--- a/drivers/net/dsa/mt7628.c
+++ b/drivers/net/dsa/mt7628.c
@@ -138,6 +138,7 @@
 #define MT7628_ESW_PORTS_CPU BIT(6)
 #define MT7628_ESW_PORTS_ALL GENMASK(6, 0)
 
+#define MT7628_ESW_NUM_USER_PORTS 5
 #define MT7628_ESW_NUM_PORTS 7
 #define MT7628_NUM_VLANS 16
 
@@ -151,8 +152,14 @@ static const struct regmap_config mt7628_esw_regmap_cfg = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+enum {
+	MT7628_VLAN_TYPE_AWARE,
+	MT7628_VLAN_TYPE_UNAWARE,
+	MT7628_VLAN_TYPE_NUM,
+};
+
 struct mt7628_vlan {
-	bool active;
+	unsigned int type;
 	u8 members;
 	u8 untag;
 	u16 vid;
@@ -383,6 +390,82 @@ static void mt7628_esw_set_vub(struct mt7628_esw *esw, unsigned int vlan,
 			   MT7628_ESW_VUB_PREP(vlan, vub));
 }
 
+static struct mt7628_vlan *mt7628_find_vlan_block(struct dsa_switch *ds,
+						  u16 vid, unsigned int type)
+{
+	struct mt7628_esw *esw = ds->priv;
+	struct mt7628_vlan *vlan;
+	int i;
+
+	for (i = 0; i < MT7628_NUM_VLANS; i++) {
+		vlan = &esw->vlans[i];
+		if (vlan->vid == vid && vlan->type == type)
+			return vlan;
+	}
+	return NULL;
+}
+
+static struct mt7628_vlan *mt7628_alloc_vlan_block(struct dsa_switch *ds,
+						   u16 vid, unsigned int type)
+{
+	struct mt7628_esw *esw = ds->priv;
+	struct mt7628_vlan *vlan;
+	int i;
+
+	for (i = 0; i < MT7628_NUM_VLANS; i++) {
+		vlan = &esw->vlans[i];
+		if (vlan->vid)
+			continue;
+		if (vlan->type != type)
+			continue;
+		vlan->vid = vid;
+		return vlan;
+	}
+
+	return NULL;
+}
+
+static int mt7628_port_join_vlan_block(struct dsa_switch *ds, int port, u16 vid,
+				       unsigned int type, u16 flags)
+{
+	struct mt7628_vlan *vlan = mt7628_find_vlan_block(ds, vid, type);
+	struct mt7628_esw *esw = ds->priv;
+
+	if (!vlan)
+		vlan = mt7628_alloc_vlan_block(ds, vid, type);
+
+	if (!vlan)
+		return -ENOSPC;
+
+	vlan->members |= BIT(port);
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		esw->tag_8021q_pvid[port] = vid;
+	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
+		vlan->untag |= BIT(port);
+	return 0;
+}
+
+static int mt7628_port_leave_vlan_block(struct dsa_switch *ds, int port,
+					u16 vid, unsigned int type)
+{
+	struct mt7628_vlan *vlan = mt7628_find_vlan_block(ds, vid, type);
+	struct mt7628_esw *esw = ds->priv;
+
+	if (!vlan)
+		return -ENOENT;
+
+	if (esw->tag_8021q_pvid[port] == vid)
+		esw->tag_8021q_pvid[port] = 0;
+	vlan->members &= ~BIT(port);
+	vlan->untag &= ~BIT(port);
+	/*
+	 * Free the vlan if we're the last member of it.
+	 */
+	if (!vlan->members)
+		vlan->vid = 0;
+	return 0;
+}
+
 static void mt7628_vlan_sync(struct dsa_switch *ds)
 {
 	struct mt7628_esw *esw = ds->priv;
@@ -425,6 +508,17 @@ static int mt7628_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/*
+	 * Dedicate the first num_user_ports + 1 VLAN slots for tag_8021q.
+	 * Since bridges are only offloaded when they have at least one member
+	 * port, the worst case entry requirement is 1 per port. The extra slot
+	 * is needed because when changing the configuration, tag_8021q adds a
+	 * new VLAN before removing the old one. The rest of the VLAN slots can
+	 * be used for filtering.
+	 */
+	for (int i = 0; i < MT7628_ESW_NUM_USER_PORTS + 1; i++)
+		esw->vlans[i].type = MT7628_VLAN_TYPE_UNAWARE;
+
 	rtnl_lock();
 	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
 	rtnl_unlock();
@@ -479,67 +573,21 @@ static void mt7628_phylink_get_caps(struct dsa_switch *ds, int port,
 static int mt7628_dsa_8021q_vlan_add(struct dsa_switch *ds, int port,
 				     u16 vid, u16 flags)
 {
-	struct mt7628_esw *esw = ds->priv;
-	struct mt7628_vlan *vlan = NULL;
-	int i;
-
-	for (i = 0; i < MT7628_NUM_VLANS; i++) {
-		struct mt7628_vlan *check_vlan = &esw->vlans[i];
-
-		if (!check_vlan->active && !vlan)
-			vlan = check_vlan;
-
-		if (check_vlan->active && check_vlan->vid == vid) {
-			vlan = check_vlan;
-			break;
-		}
-	}
-
-	if (!vlan)
-		return -ENOSPC;
-
-	vlan->vid = vid;
-	vlan->active = true;
-	vlan->members |= BIT(port);
-
-	if (flags & BRIDGE_VLAN_INFO_PVID)
-		esw->tag_8021q_pvid[port] = vid;
-
-	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
-		vlan->untag |= BIT(port);
-
+	int ret =
+	    mt7628_port_join_vlan_block(ds, port, vid, MT7628_VLAN_TYPE_UNAWARE,
+					flags);
+	if (ret)
+		return ret;
 	mt7628_vlan_sync(ds);
 	return 0;
 }
 
 static int mt7628_dsa_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 {
-	struct mt7628_esw *esw = ds->priv;
-	struct mt7628_vlan *vlan = NULL;
-	int i;
-
-	for (i = 0; i < MT7628_NUM_VLANS; i++) {
-		struct mt7628_vlan *check_vlan = &esw->vlans[i];
-
-		if (!check_vlan->active || check_vlan->vid != vid)
-			continue;
-		vlan = check_vlan;
-		break;
-	}
-	if (!vlan)
-		return -ENOENT;
-
-	if (esw->tag_8021q_pvid[port] == vid)
-		esw->tag_8021q_pvid[port] = 0;
-
-	vlan->members &= ~BIT(port);
-	vlan->untag &= ~BIT(port);
-
-	if (!vlan->members) {
-		vlan->active = false;
-		vlan->vid = 0;
-	}
-
+	int ret = mt7628_port_leave_vlan_block(ds, port, vid,
+					       MT7628_VLAN_TYPE_UNAWARE);
+	if (ret)
+		return ret;
 	mt7628_vlan_sync(ds);
 	return 0;
 }
-- 
2.55.0

Re: [PATCH net-next v1 1/6] net: dsa: mt7628: rework vlan block allocator

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The tag_8021q VLAN-entry reservation in mt7628_setup()
  (drivers/net/dsa/mt7628.c:511-520) hard-codes…

--

net: dsa: mt7628: rework vlan block allocator

This reworks the MT7628 VLAN hardware entry allocator into
find/alloc/join/leave helpers, replaces the per-entry active flag with
vid == 0, and reserves the first six VLAN table entries for tag_8021q
use so the remaining entries can later serve VLAN filtering.

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> index fb63f6f644b9e..990556cc2f655 100644
> --- a/drivers/net/dsa/mt7628.c
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> @@ -383,6 +390,82 @@ static void mt7628_esw_set_vub(struct mt7628_esw *esw, unsigned int vlan,
> +static struct mt7628_vlan *mt7628_alloc_vlan_block(struct dsa_switch *ds,
> +						   u16 vid, unsigned int type)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	struct mt7628_vlan *vlan;
> +	int i;
> +
> +	for (i = 0; i < MT7628_NUM_VLANS; i++) {
> +		vlan = &esw->vlans[i];
> +		if (vlan->vid)
> +			continue;
> +		if (vlan->type != type)
> +			continue;
> +		vlan->vid = vid;
> +		return vlan;
> +	}
> +
> +	return NULL;
> +}

[ ... ]

> @@ -425,6 +508,17 @@ static int mt7628_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Dedicate the first num_user_ports + 1 VLAN slots for tag_8021q.
> +	 * Since bridges are only offloaded when they have at least one member
> +	 * port, the worst case entry requirement is 1 per port. The extra slot
> +	 * is needed because when changing the configuration, tag_8021q adds a
> +	 * new VLAN before removing the old one. The rest of the VLAN slots can
> +	 * be used for filtering.
> +	 */
> +	for (int i = 0; i < MT7628_ESW_NUM_USER_PORTS + 1; i++)
> +		esw->vlans[i].type = MT7628_VLAN_TYPE_UNAWARE;
> +

[Severity: Medium]
Is the worst case really one entry per local user port plus one?

tag_8021q VIDs are not only generated for this switch's own ports.
dsa_tag_8021q_bridge_join() adds the bridge VID with broadcast = true:

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_join() {
	...
	err = dsa_port_tag_8021q_vlan_add(dp, bridge_vid, true);
	if (err)
		return err;

	dsa_port_tag_8021q_vlan_del(dp, standalone_vid, false);
	...
}

which reaches dsa_broadcast(), documented as notifying all DSA trees in
the system, and the per-port match is:

net/dsa/tag_8021q.c:dsa_port_tag_8021q_vlan_match() {
	return dsa_port_is_dsa(dp) || dsa_port_is_cpu(dp) || dp == info->dp;
}

so this switch's CPU port installs an entry for every foreign bridge VID
and for every peer switch standalone VID in the same tree as well.

With the five local standalone VIDs from dsa_tag_8021q_port_setup()
occupying five slots and one foreign VID taking the sixth, does
mt7628_alloc_vlan_block() then return -ENOSPC for the next tag_8021q VID
even though ten hardware entries are still free?  The type match:

	if (vlan->type != type)
		continue;

prevents a tag_8021q request from ever falling back to the entries typed
MT7628_VLAN_TYPE_AWARE, and up to DSA_TAG_8021Q_MAX_NUM_BRIDGES == 7
distinct bridge VIDs can be broadcast.  Before this patch,
mt7628_dsa_8021q_vlan_add() could use any of the 16 entries, so the
reachable pool for these callbacks shrinks from 16 to 6.

Along the same lines, the commit message says:

    6 VLAN table entries are reserved for tag_8021q use and the rest are
    left for the upcoming VLAN filtering support. No functional change
    intended.

Given the reduced pool, is "no functional change intended" accurate for
configurations with a second tag_8021q tree or a cross-chip peer?

Would it make sense to derive the reservation from the ports DSA actually
instantiates (dsa_switch_for_each_user_port) and to let tag_8021q
requests fall back to unused entries of the other type, rather than
keying it off the constant MT7628_ESW_NUM_USER_PORTS while ds->num_ports
is MT7628_ESW_NUM_PORTS?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com

[PATCH net-next v1 2/6] net: dsa: mt7628: add port bridge offload support

[Joris Vaisvila]

Add STP support and hook up the generic tag_8021q bridge join and leave
functions to dsa_switch_ops to allow hardware-offloaded bridging.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 drivers/net/dsa/mt7628.c | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
index 990556cc2f65..7eabfad3e114 100644
--- a/drivers/net/dsa/mt7628.c
+++ b/drivers/net/dsa/mt7628.c
@@ -34,6 +34,7 @@
 #define MT7628_ESW_REG_VUB(vlan) (0x100 + 4 * ((vlan) / 4))
 #define MT7628_ESW_REG_SOCPC 0x8c
 #define MT7628_ESW_REG_POC0 0x90
+#define MT7628_ESW_REG_POC1 0x94
 #define MT7628_ESW_REG_POC2 0x98
 #define MT7628_ESW_REG_SGC 0x9c
 #define MT7628_ESW_REG_PCR0 0xc0
@@ -92,6 +93,9 @@
 
 #define MT7628_ESW_POC0_PORT_DISABLE GENMASK(29, 23)
 
+#define MT7628_ESW_POC1_PORT_BLOCKING_STATE GENMASK(22, 16)
+#define MT7628_ESW_POC1_PORT_DIS_LEARNING GENMASK(14, 8)
+
 #define MT7628_ESW_POC2_PER_VLAN_UNTAG_EN BIT(15)
 
 #define MT7628_ESW_SGC_AGING_INTERVAL GENMASK(3, 0)
@@ -519,6 +523,8 @@ static int mt7628_setup(struct dsa_switch *ds)
 	for (int i = 0; i < MT7628_ESW_NUM_USER_PORTS + 1; i++)
 		esw->vlans[i].type = MT7628_VLAN_TYPE_UNAWARE;
 
+	ds->max_num_bridges = DSA_TAG_8021Q_MAX_NUM_BRIDGES;
+
 	rtnl_lock();
 	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
 	rtnl_unlock();
@@ -599,6 +605,40 @@ static void mt7628_teardown(struct dsa_switch *ds)
 	rtnl_unlock();
 }
 
+static void mt7628_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+	struct mt7628_esw *esw = ds->priv;
+	bool forward_disable;
+	bool learn_disable;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+	case BR_STATE_BLOCKING:
+	case BR_STATE_LISTENING:
+		forward_disable = true;
+		learn_disable = true;
+		break;
+	case BR_STATE_LEARNING:
+		forward_disable = true;
+		learn_disable = false;
+		break;
+	case BR_STATE_FORWARDING:
+		forward_disable = false;
+		learn_disable = false;
+		break;
+	default:
+		dev_err(ds->dev, "invalid STP state: %d\n", state);
+		return;
+	}
+
+	regmap_assign_bits(esw->regmap, MT7628_ESW_REG_POC1,
+			   FIELD_PREP(MT7628_ESW_POC1_PORT_DIS_LEARNING,
+				      BIT(port)), learn_disable);
+	regmap_assign_bits(esw->regmap, MT7628_ESW_REG_POC1,
+			   FIELD_PREP(MT7628_ESW_POC1_PORT_BLOCKING_STATE,
+				      BIT(port)), forward_disable);
+}
+
 static const struct dsa_switch_ops mt7628_switch_ops = {
 	.get_tag_protocol = mt7628_get_tag_proto,
 	.setup = mt7628_setup,
@@ -608,6 +648,9 @@ static const struct dsa_switch_ops mt7628_switch_ops = {
 	.phylink_get_caps = mt7628_phylink_get_caps,
 	.tag_8021q_vlan_add = mt7628_dsa_8021q_vlan_add,
 	.tag_8021q_vlan_del = mt7628_dsa_8021q_vlan_del,
+	.port_bridge_join = dsa_tag_8021q_bridge_join,
+	.port_bridge_leave = dsa_tag_8021q_bridge_leave,
+	.port_stp_state_set = mt7628_stp_state_set,
 };
 
 static int mt7628_probe(struct platform_device *pdev)
-- 
2.55.0

Re: [PATCH net-next v1 2/6] net: dsa: mt7628: add port bridge offload support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 2 · Low: 1

- [High] Bridge TX forwarding offload is advertised but the MT7628 tagger
  never performs an imprecise/flooding transmit, so locally-originated…
- [Medium] STP/bridge offload is added without the paired hardware-FDB
  flush: mt7628_switch_ops gains .port_stp_state_set…
- [Medium] Newly reachable non-unwound partial failure of bridge join
  leaves the driver's private VLAN state permanently corrupted.
- [Low] At this commit only, bridged ports transmit with a VID that no
  longer exists in the hardware VLAN table.

--

net: dsa: mt7628: add port bridge offload support

This adds a .port_stp_state_set implementation that drives the POC1
blocking/learning bits, and wires the generic tag_8021q bridge join and
leave helpers into mt7628_switch_ops so bridges can be offloaded.

A few questions about the new hooks below.

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> index 990556cc2f655..7eabfad3e1144 100644
> --- a/drivers/net/dsa/mt7628.c
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> @@ -599,6 +605,40 @@ static void mt7628_teardown(struct dsa_switch *ds)
>  	rtnl_unlock();
>  }
>  
> +static void mt7628_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +	bool forward_disable;
> +	bool learn_disable;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		forward_disable = true;
> +		learn_disable = true;
> +		break;

[Severity: Medium]
Should this series also add a .port_fast_age implementation to go with
the new STP hook?

dsa_port_set_state() in net/dsa/port.c calls the driver's
port_stp_state_set() and then asks for fast ageing on every transition
out of LEARNING/FORWARDING:

net/dsa/port.c:dsa_port_set_state() {
	...
	if (!dsa_port_can_configure_learning(dp) ||
	    (do_fast_age && dp->learning)) {
		...
		dsa_port_fast_age(dp);
	}
}

mt7628_switch_ops has no .port_bridge_flags, so
dsa_port_can_configure_learning() returns false and the branch is always
taken. dsa_port_fast_age() then does nothing at all, including skipping
the SWITCHDEV_FDB_FLUSH_TO_BRIDGE notification:

net/dsa/port.c:dsa_port_fast_age() {
	...
	if (!ds->ops->port_fast_age)
		return;
	...
}

Since mt7628_stp_state_set() only sets MT7628_ESW_POC1_PORT_BLOCKING_STATE,
do the addresses the ASIC learned while the port was forwarding stay in
the L2 table after a topology change, a link down, or an
"ip link set swp0 nomaster"?  If so, does the hardware keep resolving
those MACs to the now blocked/standalone port and drop the frames until
the aging interval that mt7628_switch_init() programs (the comment there
documents it as 300 s) expires?

mt7530.c pairs the same offload with mt7530_port_fast_age() for this
case.

> +	case BR_STATE_LEARNING:
> +		forward_disable = true;
> +		learn_disable = false;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		forward_disable = false;
> +		learn_disable = false;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	regmap_assign_bits(esw->regmap, MT7628_ESW_REG_POC1,
> +			   FIELD_PREP(MT7628_ESW_POC1_PORT_DIS_LEARNING,
> +				      BIT(port)), learn_disable);
> +	regmap_assign_bits(esw->regmap, MT7628_ESW_REG_POC1,
> +			   FIELD_PREP(MT7628_ESW_POC1_PORT_BLOCKING_STATE,
> +				      BIT(port)), forward_disable);
> +}
> +
>  static const struct dsa_switch_ops mt7628_switch_ops = {
>  	.get_tag_protocol = mt7628_get_tag_proto,
>  	.setup = mt7628_setup,
> @@ -608,6 +648,9 @@ static const struct dsa_switch_ops mt7628_switch_ops = {
>  	.phylink_get_caps = mt7628_phylink_get_caps,
>  	.tag_8021q_vlan_add = mt7628_dsa_8021q_vlan_add,
>  	.tag_8021q_vlan_del = mt7628_dsa_8021q_vlan_del,
> +	.port_bridge_join = dsa_tag_8021q_bridge_join,
> +	.port_bridge_leave = dsa_tag_8021q_bridge_leave,

[Severity: High]
Does using dsa_tag_8021q_bridge_join() here promise more than the
mt7628 tagger delivers?

dsa_tag_8021q_bridge_join() sets tx_fwd_offload unconditionally:

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_join() {
	...
	*tx_fwd_offload = true;
	...
}

dsa_port_bridge_join() stores that in dp->bridge->tx_fwd_offload and
passes it to switchdev_bridge_port_offload(), so the bridge sets
BR_TX_FWD_OFFLOAD_BIT.  On egress the bridge then hands the driver only
one copy per hwdom:

net/bridge/br_switchdev.c:nbp_switchdev_allowed_egress() {
	return !test_bit(p->hwdom, &cb->fwd_hwdoms) &&
	       (!skb->offload_fwd_mark || cb->src_hwdom != p->hwdom);
}

But mt7628_tag_xmit() in net/dsa/tag_mt7628.c always builds a precise
single-port mask:

	xmit_tpid = ETH_P_8021Q |
		FIELD_PREP(MT7628_TAG_TX_PORT, dsa_xmit_port_mask(skb, dev));

and dsa_xmit_port_mask() in net/dsa/tag.h starts from
"unsigned long mask = BIT(dp->index);" and only ever adds HSR peers, so
the mask is never widened to the bridge members.

With two ports in one bridge, do flooded frames (ARP, DHCP, IPv6 ND,
mDNS, unknown unicast) originating from the host or from another hwdom
reach only one of the members, with the bridge suppressing the remaining
copies?  tag_sja1105.c has sja1105_imprecise_xmit() for this; is an
equivalent flooding transmit path needed in tag_mt7628.c?  The final
state of the series still selects only the bridge VID on
skb->offload_fwd_mark and leaves the destination port mask untouched.

[Severity: Low]
At this commit alone, does a bridged port transmit with a VID that is no
longer present in the hardware VLAN table?

dsa_tag_8021q_bridge_join() adds the bridge VID and then removes the
standalone one:

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_join() {
	...
	dsa_port_tag_8021q_vlan_del(dp, standalone_vid, false);
	...
}

That reaches mt7628_dsa_8021q_vlan_del() -> mt7628_port_leave_vlan_block(),
which clears the members and untag bits and, for the last member, the VID
itself:

	vlan->members &= ~BIT(port);
	vlan->untag &= ~BIT(port);
	if (!vlan->members)
		vlan->vid = 0;

and mt7628_vlan_sync() commits that to hardware.  The tagger at this
commit still does:

net/dsa/tag_mt7628.c:mt7628_tag_xmit() {
	...
	xmit_vlan = dsa_tag_8021q_standalone_vid(dp);
	...
}

The following patch in the series ("net: dsa: tag: mt7628: add bridge
support", 4ed08042206018344b61ac96d48a0c9a44800b8f) changes this to pick
dsa_tag_8021q_bridge_vid() when skb->offload_fwd_mark is set, so the end
of the series is fine.  Would it be worth merging the two so that
bisecting does not land on a commit where bridged ports do not pass CPU
traffic?

[Severity: Medium]
Can a failed bridge join leave the driver's private VLAN slots
permanently occupied?

dsa_tag_8021q_bridge_join() first programs the bridge VID on all ports.
dsa_switch_tag_8021q_vlan_add() walks the ports in index order, so the
user port is programmed first and mt7628_port_join_vlan_block() claims a
slot and records the PVID:

	vlan->members |= BIT(port);
	if (flags & BRIDGE_VLAN_INFO_PVID)
		esw->pvid[type][port] = vid;

The CPU port is handled afterwards and can fail before the driver op
runs:

net/dsa/tag_8021q.c:dsa_port_do_tag_8021q_vlan_add() {
	...
	v = kzalloc_obj(*v);
	if (!v)
		return -ENOMEM;
	...
}

net/dsa/tag_8021q.c:dsa_switch_tag_8021q_vlan_add() {
	...
			err = dsa_port_do_tag_8021q_vlan_add(dp, info->vid, flags);
			if (err)
				return err;
	...
}

The error propagates to dsa_port_bridge_join(), which takes the
out_rollback path calling only dsa_port_bridge_destroy() and never
broadcasting DSA_NOTIFIER_BRIDGE_LEAVE, so mt7628_port_leave_vlan_block()
is not called for the bridge VID.

Does that leave one of the MT7628_ESW_NUM_USER_PORTS + 1 reserved slots
with vlan->vid != 0 and members = BIT(port) forever, so
mt7628_alloc_vlan_block() can never reuse it and later joins can hit
-ENOSPC?  And with the port's hardware PVID pointing at a VLAN that has
no CPU member, does that port lose CPU connectivity with no path that
restores it?  The non-unwinding part is in the DSA core and shared with
other tag_8021q users, but this patch makes it reachable for mt7628;
would restoring the standalone VID on join failure be feasible here?
This is still the case at the end of the series.

> +	.port_stp_state_set = mt7628_stp_state_set,
>  };
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com

[PATCH net-next v1 3/6] net: dsa: tag: mt7628: add bridge support

[Joris Vaisvila]

Egress packets need to be transmitted on the same VLAN that a port is
in. Transmit on tag_8021q_bridge_vid if the port is in a bridge.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 net/dsa/tag_mt7628.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/dsa/tag_mt7628.c b/net/dsa/tag_mt7628.c
index 80b50ff08e53..fab9d4f04b7a 100644
--- a/net/dsa/tag_mt7628.c
+++ b/net/dsa/tag_mt7628.c
@@ -20,8 +20,8 @@
  * append a tag_8021q standalone VLAN tag for each port. That means we can
  * safely strip the outer VLAN tag after parsing it.
  *
- * A VLAN tag is constructed on egress to target the standalone VLAN and
- * destination port.
+ * A VLAN tag is constructed on egress to target the standalone or bridge
+ * VLAN and destination port.
  */
 
 #define MT7628_TAG_NAME "mt7628"
@@ -38,7 +38,9 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
 	__be16 *tag;
 
 	dp = dsa_user_to_port(dev);
-	xmit_vlan = dsa_tag_8021q_standalone_vid(dp);
+	xmit_vlan = skb->offload_fwd_mark ?
+	    dsa_tag_8021q_bridge_vid(dsa_port_bridge_num_get(dp)) :
+	    dsa_tag_8021q_standalone_vid(dp);
 
 	skb_push(skb, MT7628_TAG_LEN);
 	dsa_alloc_etype_header(skb, MT7628_TAG_LEN);
-- 
2.55.0

Re: [PATCH net-next v1 3/6] net: dsa: tag: mt7628: add bridge support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 2

- [High] mt7628_tag_xmit() (net/dsa/tag_mt7628.c:40-53) switches the VLAN
  of an `skb->offload_fwd_mark` frame to the tag_8021q bridge (VBID)…
- [Medium] mt7628_tag_xmit() (net/dsa/tag_mt7628.c:41-43) derives the
  egress VLAN from `dsa_port_bridge_num_get(dp)` solely on the basis of…
- [Low] mt7628_tag_xmit() selects the imprecise tag_8021q bridge VID for
  every skb with offload_fwd_mark set, without checking…
- [Low] Series ordering/bisectability: the preceding commit 'net: dsa:
  mt7628: add port bridge offload support'…

Pre-existing issues:
- [Medium] mt7628_tag_xmit() reads rtnl-protected bridge state
  (dsa_port_bridge_num_get(dp) -> dp->bridge->num) from the lockless TX…

--

net: dsa: tag: mt7628: add bridge support

The tagger now picks the tag_8021q bridge VLAN instead of the port's
standalone VLAN when skb->offload_fwd_mark is set, so that bridged egress
uses the VLAN the port is a member of.

> diff --git a/net/dsa/tag_mt7628.c b/net/dsa/tag_mt7628.c
> index 80b50ff08e53f..fab9d4f04b7a0 100644
> --- a/net/dsa/tag_mt7628.c
> +++ b/net/dsa/tag_mt7628.c

[ ... ]

> @@ -38,7 +38,9 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
>  	__be16 *tag;
>  
>  	dp = dsa_user_to_port(dev);
> -	xmit_vlan = dsa_tag_8021q_standalone_vid(dp);
> +	xmit_vlan = skb->offload_fwd_mark ?
> +	    dsa_tag_8021q_bridge_vid(dsa_port_bridge_num_get(dp)) :
> +	    dsa_tag_8021q_standalone_vid(dp);

[Severity: High]
The VLAN is switched to the bridge broadcast domain here, but the
destination port field of the tag is still the precise single-port bitmap:

net/dsa/tag_mt7628.c:mt7628_tag_xmit() {
	tag[0] = htons(ETH_P_8021Q |
		       FIELD_PREP(MT7628_TAG_TX_PORT,
				  dsa_xmit_port_mask(skb, dev)));
	tag[1] = htons(xmit_vlan);
}

and dsa_xmit_port_mask() in net/dsa/tag.h starts from a single bit:

	unsigned long mask = BIT(dp->index);

Can the switch replicate a flooded frame with that mask?

The driver uses dsa_tag_8021q_bridge_join() as .port_bridge_join, and that
function sets *tx_fwd_offload = true unconditionally, so the software bridge
stops cloning to the other ports of the same hwdom:

net/bridge/br_switchdev.c:nbp_switchdev_allowed_egress() {
	return !test_bit(p->hwdom, &cb->fwd_hwdoms) &&
		(!skb->offload_fwd_mark || cb->src_hwdom != p->hwdom);
}

Documentation/networking/dsa/dsa.rst describes the resulting contract:

	Additionally, replication of data plane packets (multicast, flooding)
	is handled in hardware and the bridge driver will transmit a single
	skb for each packet that may or may not need replication.

With the destination bitmap pinned to one port, do host-originated
broadcast, multicast and unknown unicast frames (ARP requests, DHCP, mDNS,
IPv6 NS) reach only a single bridge member port, with no software copies for
the remaining ports of the same switch?

For comparison, sja1105_imprecise_xmit() in net/dsa/tag_sja1105.c uses
dsa_tag_8021q_bridge_vid() without restricting the destination port.

This also looks unchanged at the end of the series: after "net: dsa: tag:
mt7628: add VLAN awareness support" both the VLAN-aware and VLAN-unaware
bridge paths still build xmit_tpid from dsa_xmit_port_mask().

[Severity: Medium]
Is skb->offload_fwd_mark on its own enough to conclude that the egress port
is bridged?

include/net/dsa.h:dsa_port_bridge_num_get() {
	return dp->bridge ? dp->bridge->num : 0;
}

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_vid() {
	/* The VBID value of 0 is reserved for precise TX, but it is also
	 * reserved/invalid for the bridge_num, so all is well.
	 */
	return DSA_8021Q_RSV | DSA_8021Q_VBID(bridge_num);
}

So a bridge_num of 0 yields 0xC00, which is also the standalone VID of
switch 0 / port 0.

mt7628_tag_rcv() sets the mark for every frame received on a bridged port:

net/dsa/tag.h:dsa_default_offload_fwd_mark() {
	skb->offload_fwd_mark = !!(dp->bridge);
}

and nothing clears it when a tc mirred egress redirect action hands that
same skb to a different user port. If that port is standalone, does the
frame get tagged with VID 0xC00 while the destination bitmap forces egress
on a port that is not a member of that VLAN, so the switch drops it under
PFC1 EN_VLAN? Before this change the egress port's own standalone VID was
used.

Would a fallback such as: if bridge_num is 0, use
dsa_tag_8021q_standalone_vid(dp), be reasonable here?

The same condition is reused at the end of the series, where this path
calls br_vlan_enabled(dsa_port_bridge_dev_get(dp)); with a standalone
egress port that is br_vlan_enabled(NULL), which reaches netdev_priv(NULL)
when CONFIG_BRIDGE_VLAN_FILTERING is enabled.

[Severity: Low]
This isn't a bug in the final tree, but the preceding commit 9369664c988d
("net: dsa: mt7628: add port bridge offload support") already installs
.port_bridge_join = dsa_tag_8021q_bridge_join, and that function drops the
port's standalone VLAN:

net/dsa/tag_8021q.c:dsa_tag_8021q_bridge_join() {
	err = dsa_port_tag_8021q_vlan_add(dp, bridge_vid, true);
	if (err)
		return err;

	dsa_port_tag_8021q_vlan_del(dp, standalone_vid, false);
}

Until the tagger change above, mt7628_tag_xmit() still tags egress frames
with dsa_tag_8021q_standalone_vid(dp), a VLAN the bridged port is no longer
a member of, so bridged transmit is broken at that intermediate commit and a
bisection landing there sees the breakage.

Could the driver and tagger changes be squashed, or the dependency mentioned
in the commit messages?

>  
>  	skb_push(skb, MT7628_TAG_LEN);
>  	dsa_alloc_etype_header(skb, MT7628_TAG_LEN);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com

[PATCH net-next v1 4/6] net: dsa: mt7628: add VLAN filtering support

[Joris Vaisvila]

Add support for VLAN filtering to the MT7628's built-in switch.

The only way to control forwarding on this switch is through VLANs.
Bridges and VLANs are represented by the same hardware, programmed
differently.

VLAN membership for tag_8021q and user-requested VLANs is stored in
software, but membership is only programmed for one type of VLAN, based
on port VLAN filtering state. If VLAN filtering is enabled on a port, it
is programmed only into "VLAN_AWARE" VLANs and double-tag is disabled.
Otherwise it is programmed into "VLAN_UNAWARE" VLANs and double-tag is
enabled to simulate VLAN-unaware bridge behaviour.

The primary limitation is that a VID must not be common between separate
bridges, as that would allow forwarding between bridge ports of separate
bridges. This means we can't use tag_8021q VIDs or VLANs used on other
bridges. This is checked in mt7628_port_vlan_add.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 drivers/net/dsa/mt7628.c | 122 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 114 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
index 7eabfad3e114..629dbf968a17 100644
--- a/drivers/net/dsa/mt7628.c
+++ b/drivers/net/dsa/mt7628.c
@@ -174,9 +174,10 @@ struct mt7628_esw {
 	struct reset_control *rst_esw;
 	struct regmap *regmap;
 	struct dsa_switch *ds;
-	u16 tag_8021q_pvid[MT7628_ESW_NUM_PORTS];
+	u16 pvid[MT7628_VLAN_TYPE_NUM][MT7628_ESW_NUM_PORTS];
 	struct mt7628_vlan vlans[MT7628_NUM_VLANS];
 	struct device *dev;
+	u8 vlan_filtering;
 };
 
 static int mt7628_mii_read(struct mii_bus *bus, int port, int regnum)
@@ -442,10 +443,16 @@ static int mt7628_port_join_vlan_block(struct dsa_switch *ds, int port, u16 vid,
 		return -ENOSPC;
 
 	vlan->members |= BIT(port);
+
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		esw->tag_8021q_pvid[port] = vid;
+		esw->pvid[type][port] = vid;
+	else if (esw->pvid[type][port] == vid)
+		esw->pvid[type][port] = 0;
+
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		vlan->untag |= BIT(port);
+	else
+		vlan->untag &= ~BIT(port);
 	return 0;
 }
 
@@ -458,8 +465,8 @@ static int mt7628_port_leave_vlan_block(struct dsa_switch *ds, int port,
 	if (!vlan)
 		return -ENOENT;
 
-	if (esw->tag_8021q_pvid[port] == vid)
-		esw->tag_8021q_pvid[port] = 0;
+	if (esw->pvid[type][port] == vid)
+		esw->pvid[type][port] = 0;
 	vlan->members &= ~BIT(port);
 	vlan->untag &= ~BIT(port);
 	/*
@@ -473,18 +480,42 @@ static int mt7628_port_leave_vlan_block(struct dsa_switch *ds, int port,
 static void mt7628_vlan_sync(struct dsa_switch *ds)
 {
 	struct mt7628_esw *esw = ds->priv;
+	struct dsa_port *dp;
 	int i;
 
 	for (i = 0; i < MT7628_NUM_VLANS; i++) {
 		struct mt7628_vlan *vlan = &esw->vlans[i];
+		u8 member_mask;
+
+		if (vlan->type == MT7628_VLAN_TYPE_AWARE)
+			member_mask = esw->vlan_filtering;
+		else
+			member_mask = ~esw->vlan_filtering;
+		member_mask |= MT7628_ESW_PORTS_CPU;
+		/*
+		 * Put VLAN filtering ports only into VLAN aware VLANs and
+		 * non VLAN filtering ports into VLAN unaware VLANs.
+		 *
+		 * CPU may not be removed from any VLAN, as VLAN filtering
+		 * applies only to user ports.
+		 */
 
-		mt7628_esw_set_vmsc(esw, i, vlan->members);
 		mt7628_esw_set_vlan_id(esw, i, vlan->vid);
-		mt7628_esw_set_vub(esw, i, vlan->untag);
+		mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
+		mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);
+
 	}
 
-	for (i = 0; i < ds->num_ports; i++)
-		mt7628_esw_set_pvid(esw, i, esw->tag_8021q_pvid[i]);
+	dsa_switch_for_each_user_port(dp, ds) {
+		unsigned int type = BIT(dp->index) & esw->vlan_filtering ?
+		    MT7628_VLAN_TYPE_AWARE : MT7628_VLAN_TYPE_UNAWARE;
+		mt7628_esw_set_pvid(esw, dp->index, esw->pvid[type][dp->index]);
+	}
+	regmap_update_bits(esw->regmap, MT7628_ESW_REG_SGC2,
+			   MT7628_ESW_SGC2_DOUBLE_TAG_EN,
+			   FIELD_PREP(MT7628_ESW_SGC2_DOUBLE_TAG_EN,
+				      MT7628_ESW_PORTS_NOCPU &
+				      ~esw->vlan_filtering));
 }
 
 static int mt7628_setup(struct dsa_switch *ds)
@@ -598,6 +629,78 @@ static int mt7628_dsa_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
 	return 0;
 }
 
+static int mt7628_port_vlan_filtering(struct dsa_switch *ds, int port,
+				      bool vlan_filtering,
+				      struct netlink_ext_ack *extack)
+{
+	struct mt7628_esw *esw = ds->priv;
+
+	if (vlan_filtering)
+		esw->vlan_filtering |= BIT(port);
+	else
+		esw->vlan_filtering &= ~BIT(port);
+	mt7628_vlan_sync(ds);
+	return 0;
+}
+
+static int mt7628_port_vlan_add(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan,
+				struct netlink_ext_ack *extack)
+{
+	struct mt7628_vlan *vlan_block;
+	struct dsa_port *other_dp;
+	struct dsa_port *dp;
+	int ret;
+
+	if (vid_is_dsa_8021q(vlan->vid)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Range 3072-4095 reserved for dsa_8021q operation");
+		return -EBUSY;
+	}
+
+	vlan_block =
+	    mt7628_find_vlan_block(ds, vlan->vid, MT7628_VLAN_TYPE_AWARE);
+	dp = dsa_to_port(ds, port);
+	/*
+	 * CPU port can be added to any VLANs, but user ports need to ensure,
+	 * that if the VLAN already exists it's not used by a bridge we're not
+	 * a member of, because VLANs are the only form of forwarding control
+	 * we have on this switch.
+	 */
+	if (vlan_block && !dsa_port_is_cpu(dp)) {
+		dsa_switch_for_each_user_port(other_dp, ds) {
+			if (other_dp == dp)
+				continue;
+			if (other_dp->bridge == dp->bridge)
+				continue;
+			if (!(vlan_block->members & BIT(other_dp->index)))
+				continue;
+			NL_SET_ERR_MSG_MOD(extack,
+					   "VLAN ID used on another bridge");
+			return -EBUSY;
+		}
+	}
+	ret =
+	    mt7628_port_join_vlan_block(ds, port, vlan->vid,
+					MT7628_VLAN_TYPE_AWARE, vlan->flags);
+	if (ret)
+		return ret;
+
+	mt7628_vlan_sync(ds);
+	return 0;
+}
+
+static int mt7628_port_vlan_del(struct dsa_switch *ds, int port,
+				const struct switchdev_obj_port_vlan *vlan)
+{
+	int ret = mt7628_port_leave_vlan_block(ds, port, vlan->vid,
+					       MT7628_VLAN_TYPE_AWARE);
+	if (ret)
+		return ret;
+	mt7628_vlan_sync(ds);
+	return 0;
+}
+
 static void mt7628_teardown(struct dsa_switch *ds)
 {
 	rtnl_lock();
@@ -651,6 +754,9 @@ static const struct dsa_switch_ops mt7628_switch_ops = {
 	.port_bridge_join = dsa_tag_8021q_bridge_join,
 	.port_bridge_leave = dsa_tag_8021q_bridge_leave,
 	.port_stp_state_set = mt7628_stp_state_set,
+	.port_vlan_filtering = mt7628_port_vlan_filtering,
+	.port_vlan_add = mt7628_port_vlan_add,
+	.port_vlan_del = mt7628_port_vlan_del,
 };
 
 static int mt7628_probe(struct platform_device *pdev)
-- 
2.55.0

Re: [PATCH net-next v1 4/6] net: dsa: mt7628: add VLAN filtering support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider:

Critical: 0 · High: 4 · Medium: 0 · Low: 2

- [High] mt7628_port_vlan_add() rejects with -EBUSY any VID that is
  already recorded in a VLAN-AWARE software block by a user port…
- [High] mt7628_port_vlan_add() accepts VLAN ID 0, but `vlan->vid == 0`
  is simultaneously the driver's 'slot is free' sentinel…
- [High] mt7628_vlan_sync() programs the per-VLAN egress-untag bitmap
  (VUB) with the CPU port bit set whenever a VLAN carries…
- [High] Two standalone ports can be incorrectly allowed to share the
  same VLAN ID, which breaks port isolation if they are later enslaved…
- [Low] Retained for policy combination only; my baseline verdict is
  FALSE POSITIVE.
- [Low] Retained for policy combination only; my baseline verdict is
  FALSE POSITIVE.

--

net: dsa: mt7628: add VLAN filtering support

This adds .port_vlan_filtering, .port_vlan_add and .port_vlan_del to the
MT7628 built-in switch driver.  Membership for tag_8021q and
user-requested VLANs is tracked in software, and only one of the two
VLAN types is programmed into hardware per port depending on that port's
VLAN filtering state.  A few questions below about the new
mt7628_vlan_sync() masking and about the input validation in
mt7628_port_vlan_add().

> diff --git a/drivers/net/dsa/mt7628.c b/drivers/net/dsa/mt7628.c
> index 7eabfad3e1144..629dbf968a171 100644
> --- a/drivers/net/dsa/mt7628.c
> +++ b/drivers/net/dsa/mt7628.c

[ ... ]

> @@ -442,10 +443,16 @@ static int mt7628_port_join_vlan_block(struct dsa_switch *ds, int port, u16 vid,
>  		return -ENOSPC;
>  
>  	vlan->members |= BIT(port);
> +
>  	if (flags & BRIDGE_VLAN_INFO_PVID)
> -		esw->tag_8021q_pvid[port] = vid;
> +		esw->pvid[type][port] = vid;
> +	else if (esw->pvid[type][port] == vid)
> +		esw->pvid[type][port] = 0;
> +
>  	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
>  		vlan->untag |= BIT(port);
> +	else
> +		vlan->untag &= ~BIT(port);
>  	return 0;
>  }

[ ... ]

> @@ -473,18 +480,42 @@ static int mt7628_port_leave_vlan_block(struct dsa_switch *ds, int port,
>  static void mt7628_vlan_sync(struct dsa_switch *ds)
>  {
>  	struct mt7628_esw *esw = ds->priv;
> +	struct dsa_port *dp;
>  	int i;
>  
>  	for (i = 0; i < MT7628_NUM_VLANS; i++) {
>  		struct mt7628_vlan *vlan = &esw->vlans[i];
> +		u8 member_mask;
> +
> +		if (vlan->type == MT7628_VLAN_TYPE_AWARE)
> +			member_mask = esw->vlan_filtering;
> +		else
> +			member_mask = ~esw->vlan_filtering;
> +		member_mask |= MT7628_ESW_PORTS_CPU;

[Severity: High]
Should the CPU bit be forced into the untag mask as well as the member
mask here?

member_mask always has MT7628_ESW_PORTS_CPU OR'ed in, and it is applied
to both values:

	mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
	mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);

The CPU port bit can be set in vlan->untag: when a port joins a bridge,
br_switchdev_vlan_replay() replays the bridge master's default_pvid with
BRIDGE_VLAN_INFO_UNTAGGED, and dsa_user_host_vlan_add() only clears
BRIDGE_VLAN_INFO_PVID:

net/dsa/user.c:dsa_user_host_vlan_add() {
	...
	/* Even though drivers often handle CPU membership in special ways,
	 * it doesn't make sense to program a PVID, so clear this flag.
	 */
	vlan.flags &= ~BRIDGE_VLAN_INFO_PVID;
	...
}

so mt7628_port_join_vlan_block() executes vlan->untag |= BIT(cpu), and
MT7628_ESW_VUB_M covers bit 6, so the CPU egress untag really lands in
hardware.

The MT7628 special tag lives in the VLAN tag itself, and the receive path
decodes it unconditionally:

net/dsa/tag_mt7628.c:mt7628_tag_rcv() {
	phdr = dsa_etype_header_pos_rx(skb);
	...
	skb_pull_rcsum(skb, MT7628_TAG_LEN);
	dsa_strip_etype_header(skb, MT7628_TAG_LEN);
}

If frames reach the conduit untagged, does mt7628_tag_rcv() then read the
frame's real EtherType as the source port and eat 4 bytes of payload?
For IPv4 (0x0800) the port field decodes as 0 and 4 bytes of the IP
header are removed; for ARP (0x0806) it decodes as 6 (the CPU port) and
dsa_conduit_find_user() returns NULL.

Before this patch tag_8021q passed flags = 0 for CPU and DSA ports, so
the CPU untag bit could never be set.  Would masking BIT(cpu) out of the
VUB value (keeping the CPU a tagged member of every VLAN) be the right
fix?

> +		/*
> +		 * Put VLAN filtering ports only into VLAN aware VLANs and
> +		 * non VLAN filtering ports into VLAN unaware VLANs.
> +		 *
> +		 * CPU may not be removed from any VLAN, as VLAN filtering
> +		 * applies only to user ports.
> +		 */
>  
> -		mt7628_esw_set_vmsc(esw, i, vlan->members);
>  		mt7628_esw_set_vlan_id(esw, i, vlan->vid);
> -		mt7628_esw_set_vub(esw, i, vlan->untag);
> +		mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
> +		mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);

[Severity: Low]
This one looks like it resolves itself later in the series, so it is more
of a note than a bug report.

Masking here removes VLAN-filtering ports from the membership and untag
masks of all UNAWARE (tag_8021q) blocks, while mt7628_port_vlan_add()
refuses dsa_8021q VIDs in AWARE blocks, so at this commit no VLAN entry
permits or untags the VID that mt7628_tag_xmit() inserts for such a port.

The later commit "net: dsa: tag: mt7628: add VLAN awareness support" adds
a VLAN-aware egress path that only rewrites the TPID:

net/dsa/tag_mt7628.c:mt7628_tag_xmit() {
	if (skb->offload_fwd_mark &&
	    br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
		tag = dsa_etype_header_pos_tx(skb);
		tag[0] = htons(xmit_tpid);
		return skb;
	}
	...
}

so tag_8021q membership is no longer needed for filtering ports.  Frames
injected directly on a bridged user netdev (offload_fwd_mark == 0, e.g.
STP BPDUs) still use the standalone tag_8021q VID though - is that path
expected to work on a filtering port?

> +
>  	}
>  
> -	for (i = 0; i < ds->num_ports; i++)
> -		mt7628_esw_set_pvid(esw, i, esw->tag_8021q_pvid[i]);
> +	dsa_switch_for_each_user_port(dp, ds) {
> +		unsigned int type = BIT(dp->index) & esw->vlan_filtering ?
> +		    MT7628_VLAN_TYPE_AWARE : MT7628_VLAN_TYPE_UNAWARE;
> +		mt7628_esw_set_pvid(esw, dp->index, esw->pvid[type][dp->index]);
> +	}
> +	regmap_update_bits(esw->regmap, MT7628_ESW_REG_SGC2,
> +			   MT7628_ESW_SGC2_DOUBLE_TAG_EN,
> +			   FIELD_PREP(MT7628_ESW_SGC2_DOUBLE_TAG_EN,
> +				      MT7628_ESW_PORTS_NOCPU &
> +				      ~esw->vlan_filtering));

[Severity: Low]
Also not a bug at the end of the series, just a bisectability note.

Clearing MT7628_ESW_SGC2_DOUBLE_TAG_EN for VLAN-filtering ports means the
switch no longer inserts an outer tag, so at this commit mt7628_tag_rcv()
decodes the source port from the wire TPID and strips 4 bytes.  The later
commit "net: dsa: tag: mt7628: add VLAN awareness support" handles that
case:

net/dsa/tag_mt7628.c:mt7628_tag_rcv() {
	...
	if (source_port == 0 && !is_dsa_8021q)
		skb->dev = dsa_find_designated_bridge_port_by_vid(dev, vid);
	else
		skb->dev = dsa_conduit_find_user(dev, 0, source_port);
	...
}

Since the driver change lands before the tagger change, is receive broken
for VLAN-filtering ports at this point in the series?  Would reordering
the two patches help bisection?

>  }
>  
>  static int mt7628_setup(struct dsa_switch *ds)

[ ... ]

> @@ -598,6 +629,78 @@ static int mt7628_dsa_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
>  	return 0;
>  }
>  
> +static int mt7628_port_vlan_filtering(struct dsa_switch *ds, int port,
> +				      bool vlan_filtering,
> +				      struct netlink_ext_ack *extack)
> +{
> +	struct mt7628_esw *esw = ds->priv;
> +
> +	if (vlan_filtering)
> +		esw->vlan_filtering |= BIT(port);
> +	else
> +		esw->vlan_filtering &= ~BIT(port);
> +	mt7628_vlan_sync(ds);
> +	return 0;
> +}
> +
> +static int mt7628_port_vlan_add(struct dsa_switch *ds, int port,
> +				const struct switchdev_obj_port_vlan *vlan,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct mt7628_vlan *vlan_block;
> +	struct dsa_port *other_dp;
> +	struct dsa_port *dp;
> +	int ret;
> +
> +	if (vid_is_dsa_8021q(vlan->vid)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Range 3072-4095 reserved for dsa_8021q operation");
> +		return -EBUSY;
> +	}

[Severity: High]
Does VLAN ID 0 need to be rejected (or ignored) here too?

VID 0 is the driver's "slot is free" sentinel:

drivers/net/dsa/mt7628.c:mt7628_find_vlan_block() {
	for (i = 0; i < MT7628_NUM_VLANS; i++) {
		vlan = &esw->vlans[i];
		if (vlan->vid == vid && vlan->type == type)
			return vlan;
	}
}

drivers/net/dsa/mt7628.c:mt7628_alloc_vlan_block() {
		if (vlan->vid)
			continue;
		if (vlan->type != type)
			continue;
		vlan->vid = vid;
		return vlan;
}

VID 0 arrives on its own for a VLAN-filtering port: dsa_port_vlan_filtering()
sets NETIF_F_HW_VLAN_CTAG_FILTER on the user netdev, and on the next
NETDEV_UP the 8021q core pushes VID 0 down:

net/8021q/vlan.c:vlan_vid0_add() {
	if (!(dev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
		return;

	pr_info("adding VLAN 0 to HW filter on device %s\n", dev->name);

	err = vlan_vid_add(dev, htons(ETH_P_8021Q), 0);
}

which reaches dsa_user_vlan_rx_add_vid() -> dsa_port_vlan_add() ->
ds->ops->port_vlan_add() with vid 0.  So "ip link set swp0 master br0"
(vlan_filtering 1) followed by "ip link set swp0 up" delivers vid 0 here.
mt7530, b53, mv88e6xxx and rtl8365mb all special-case vid 0 in their
.port_vlan_add for this reason.

With vid 0, mt7628_find_vlan_block() matches the first unused AWARE slot,
so mt7628_alloc_vlan_block() never runs and mt7628_port_join_vlan_block()
leaves vlan->vid at 0 while doing:

	vlan->members |= BIT(port);

Can the next real VLAN add then be handed that same still-vid-0 slot by
mt7628_alloc_vlan_block(), inheriting the stale members bitmap?  In that
case the block did not exist before the add, so the cross-bridge check
below is skipped, and mt7628_vlan_sync() would program a port of one
bridge as a hardware member of another bridge's VLAN.

> +
> +	vlan_block =
> +	    mt7628_find_vlan_block(ds, vlan->vid, MT7628_VLAN_TYPE_AWARE);
> +	dp = dsa_to_port(ds, port);
> +	/*
> +	 * CPU port can be added to any VLANs, but user ports need to ensure,
> +	 * that if the VLAN already exists it's not used by a bridge we're not
> +	 * a member of, because VLANs are the only form of forwarding control
> +	 * we have on this switch.
> +	 */
> +	if (vlan_block && !dsa_port_is_cpu(dp)) {
> +		dsa_switch_for_each_user_port(other_dp, ds) {
> +			if (other_dp == dp)
> +				continue;
> +			if (other_dp->bridge == dp->bridge)
> +				continue;
> +			if (!(vlan_block->members & BIT(other_dp->index)))
> +				continue;
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "VLAN ID used on another bridge");
> +			return -EBUSY;
> +		}
> +	}

[Severity: High]
Should this comparison require that the two ports are actually
bridged, rather than just having equal dp->bridge pointers?

For two standalone user ports both dp->bridge and other_dp->bridge are
NULL, so the test above evaluates NULL == NULL and skips the conflict
check - the two ports are treated as if they were members of the same
forwarding domain.  Standalone ports do reach .port_vlan_add: adding an
8021q upper ("ip link add link swp0 name swp0.10 type vlan id 10")
goes through dsa_user_vlan_rx_add_vid() -> dsa_port_vlan_add() ->
mt7628_port_vlan_add(), so doing that on swp0 and swp1 leaves the AWARE
block for VID 10 with members = BIT(swp0) | BIT(swp1).

Nothing re-validates that state later.  The membership only becomes
visible in hardware once esw->vlan_filtering gains the port bits, and
mt7628_port_vlan_filtering() just sets/clears the bit and calls
mt7628_vlan_sync(), which programs vlan->members & esw->vlan_filtering
into the VMSC entry.  So if the two ports are subsequently enslaved to
different VLAN-aware bridges, is the shared VID 10 block programmed with
both ports as members, giving exactly the cross-bridge forwarding this
check was written to prevent?

Treating a NULL bridge as its own isolation domain (i.e. only skipping
when both ports are bridged into the same dsa_bridge) would catch the
add-then-enslave order too, but it would also refuse the very common
case of the same VLAN ID on two independent standalone ports, which
works fine as long as neither is bridged.  Would it be better to keep
the permissive behaviour at add time and instead re-run the conflict
check from .port_bridge_join / .port_vlan_filtering, where the actual
isolation domains are known?

[Severity: High]
Does this check prevent a second bridge from ever being created on this
switch?

DSA sets ds->configure_vlan_while_not_filtering = true before ops->setup(),
so bridge VLANs are handed to the driver regardless of the bridge's
vlan_filtering state.  br_add_if() then unconditionally installs
br->default_pvid (1, with PVID|UNTAGGED|BRENTRY) and aborts the
enslavement on error:

net/bridge/br_if.c:br_add_if() {
	err = nbp_vlan_init(p, extack);
	if (err) {
		netdev_err(dev, "failed to initialize vlan filtering on this port\n");
		goto err6;
	}
}

__vlan_vid_add() propagates any switchdev error other than -EOPNOTSUPP,
so the -EBUSY above is fatal.  After "ip link set swp0 master br0" the
AWARE block for VID 1 has members = BIT(swp0); "ip link set swp1 master
br1" then finds that block, sees a different dp->bridge pointer, and
returns -EBUSY.  Before this patch .port_vlan_add was absent, so
dsa_switch_vlan_add() returned -EOPNOTSUPP and the join succeeded, which
makes the max_num_bridges = DSA_TAG_8021Q_MAX_NUM_BRIDGES support added
earlier in the series unreachable without first changing
vlan_default_pvid on every bridge.

Would it make sense to skip the rejection when the conflicting membership
is not actually programmed?  mt7628_vlan_sync() masks AWARE members with
esw->vlan_filtering:

	if (vlan->type == MT7628_VLAN_TYPE_AWARE)
		member_mask = esw->vlan_filtering;

so a non-filtering port is never written into an AWARE VMSC entry and no
cross-bridge forwarding could result from it.

> +	ret =
> +	    mt7628_port_join_vlan_block(ds, port, vlan->vid,
> +					MT7628_VLAN_TYPE_AWARE, vlan->flags);
> +	if (ret)
> +		return ret;
> +
> +	mt7628_vlan_sync(ds);
> +	return 0;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com

[PATCH net-next v1 5/6] net: dsa: tag: mt7628: add VLAN awareness support

[Joris Vaisvila]

Support VLAN-aware bridges in the MT7628 tagger.

VLAN-aware bridge traffic already contains the VLAN tag used for
forwarding, so modify the TPID to contain the destination port mask
instead of adding a new tag.

On receive, strip the MT7628 tag, use the VID to find the source port if
the switch reports port 0 on VLAN-aware traffic, then restore the VLAN
tag as a hardware-accelerated VLAN tag.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 net/dsa/tag_mt7628.c | 62 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 50 insertions(+), 12 deletions(-)

diff --git a/net/dsa/tag_mt7628.c b/net/dsa/tag_mt7628.c
index fab9d4f04b7a..7c71e8b08648 100644
--- a/net/dsa/tag_mt7628.c
+++ b/net/dsa/tag_mt7628.c
@@ -6,6 +6,7 @@
 
 #include <linux/etherdevice.h>
 #include <linux/dsa/8021q.h>
+#include <linux/if_vlan.h>
 #include <net/dsa.h>
 
 #include "tag.h"
@@ -15,13 +16,16 @@
  * On TX the lower 6 bits encode the destination port bitmask.
  * On RX the lower 3 bits encode the source port number.
  *
- * The switch hardware will not modify the TPID of an incoming packet if it is
- * already VLAN tagged. To work around this the switch is configured to always
- * append a tag_8021q standalone VLAN tag for each port. That means we can
- * safely strip the outer VLAN tag after parsing it.
+ * The switch can only use VLANs for forwarding control. VLAN-unaware bridges
+ * are simulated using tag_8021q and double tagging, while VLAN-aware bridges
+ * use the VLANs configured by the bridge directly.
  *
- * A VLAN tag is constructed on egress to target the standalone or bridge
- * VLAN and destination port.
+ * On egress, the tagger either adds a new MT7628 tag that contains the
+ * standalone or bridge tag_8021q VLAN and destination port mask, or modifies
+ * an existing VLAN tag to contain the destination port mask.
+ *
+ * On ingress, the VLAN tag is restored after stripping the MT7628 tag, if it
+ * is not a tag_8021q VLAN.
  */
 
 #define MT7628_TAG_NAME "mt7628"
@@ -34,10 +38,25 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
 				       struct net_device *dev)
 {
 	struct dsa_port *dp;
+	u16 xmit_tpid;
 	u16 xmit_vlan;
 	__be16 *tag;
 
+	xmit_tpid =
+	    ETH_P_8021Q | FIELD_PREP(MT7628_TAG_TX_PORT,
+				     dsa_xmit_port_mask(skb, dev));
 	dp = dsa_user_to_port(dev);
+	if (skb->offload_fwd_mark &&
+	    br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {
+		/*
+		 * On VLAN aware ports only modify the TPID to contain the
+		 * MT7628 egress port metadata, instead of adding a new vlan tag
+		 */
+		tag = dsa_etype_header_pos_tx(skb);
+		tag[0] = htons(xmit_tpid);
+		return skb;
+	}
+
 	xmit_vlan = skb->offload_fwd_mark ?
 	    dsa_tag_8021q_bridge_vid(dsa_port_bridge_num_get(dp)) :
 	    dsa_tag_8021q_standalone_vid(dp);
@@ -47,9 +66,7 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
 
 	tag = dsa_etype_header_pos_tx(skb);
 
-	tag[0] = htons(ETH_P_8021Q |
-		       FIELD_PREP(MT7628_TAG_TX_PORT,
-				  dsa_xmit_port_mask(skb, dev)));
+	tag[0] = htons(xmit_tpid);
 	tag[1] = htons(xmit_vlan);
 
 	return skb;
@@ -58,7 +75,10 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
 static struct sk_buff *mt7628_tag_rcv(struct sk_buff *skb,
 				      struct net_device *dev)
 {
+	unsigned int source_port;
+	bool is_dsa_8021q;
 	__be16 *phdr;
+	u16 tci, vid;
 
 	if (unlikely(!pskb_may_pull(skb, MT7628_TAG_LEN))) {
 		kfree_skb(skb);
@@ -66,9 +86,23 @@ static struct sk_buff *mt7628_tag_rcv(struct sk_buff *skb,
 	}
 
 	phdr = dsa_etype_header_pos_rx(skb);
-	skb->dev =
-	    dsa_conduit_find_user(dev, 0,
-				  FIELD_GET(MT7628_TAG_RX_PORT, ntohs(*phdr)));
+	source_port = FIELD_GET(MT7628_TAG_RX_PORT, ntohs(*phdr));
+	tci = ntohs(phdr[1]);
+	vid = tci & VLAN_VID_MASK;
+	is_dsa_8021q = vid_is_dsa_8021q(vid);
+
+	/*
+	 * The source port info is only encoded in the TPID field for packets
+	 * where the VLAN tag is inserted by the PVID mechanism. With VLAN
+	 * filtering enabled, VLAN-tagged ingress packets appear as if they're
+	 * originating on port 0. Use the VID to identify the bridge port in
+	 * this case.
+	 */
+	if (source_port == 0 && !is_dsa_8021q)
+		skb->dev = dsa_find_designated_bridge_port_by_vid(dev, vid);
+	else
+		skb->dev = dsa_conduit_find_user(dev, 0, source_port);
+
 	if (!skb->dev) {
 		kfree_skb(skb);
 		return NULL;
@@ -77,6 +111,10 @@ static struct sk_buff *mt7628_tag_rcv(struct sk_buff *skb,
 	skb_pull_rcsum(skb, MT7628_TAG_LEN);
 	dsa_strip_etype_header(skb, MT7628_TAG_LEN);
 	dsa_default_offload_fwd_mark(skb);
+
+	if (!is_dsa_8021q)
+		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), tci);
+
 	return skb;
 }
 
-- 
2.55.0

Re: [PATCH net-next v1 5/6] net: dsa: tag: mt7628: add VLAN awareness support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 3 · Medium: 2 · Low: 0

- [High] mt7628_tag_xmit() (net/dsa/tag_mt7628.c:49-58) overwrites the
  two bytes at dsa_etype_header_pos_tx(skb) (skb->data + 2*ETH_ALEN)…
- [High] For a port under a VLAN-aware (filtering) bridge,
  mt7628_tag_xmit() only takes the new "modify TPID in place" path when…
- [High] mt7628_tag_xmit() (net/dsa/tag_mt7628.c:49-50) passes the result
  of dsa_port_bridge_dev_get(dp) straight into br_vlan_enabled(), which…
- [Medium] Both new code paths hardcode ETH_P_8021Q.
- [Medium] mt7628_tag_rcv() (net/dsa/tag_mt7628.c:115-116) re-inserts the
  switch-inserted VLAN tag as a hwaccel tag for every non-tag_8021q VID,…

--

net: dsa: tag: mt7628: add VLAN awareness support

This teaches the MT7628 tagger to support VLAN-aware bridges by rewriting
the TPID of an already present VLAN tag on egress instead of pushing a new
MT7628 tag, and by restoring the stripped VLAN tag as a hardware
accelerated tag on ingress.  The receive path also falls back to a VID
based source port lookup when the switch reports port 0.

A few questions about the new paths below.

> diff --git a/net/dsa/tag_mt7628.c b/net/dsa/tag_mt7628.c
> index fab9d4f04b7a0..7c71e8b086481 100644
> --- a/net/dsa/tag_mt7628.c
> +++ b/net/dsa/tag_mt7628.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/etherdevice.h>
>  #include <linux/dsa/8021q.h>
> +#include <linux/if_vlan.h>
>  #include <net/dsa.h>
>  
>  #include "tag.h"

[ ... ]

> @@ -34,10 +38,25 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
>  				       struct net_device *dev)
>  {
>  	struct dsa_port *dp;
> +	u16 xmit_tpid;
>  	u16 xmit_vlan;
>  	__be16 *tag;
>  
> +	xmit_tpid =
> +	    ETH_P_8021Q | FIELD_PREP(MT7628_TAG_TX_PORT,
> +				     dsa_xmit_port_mask(skb, dev));
>  	dp = dsa_user_to_port(dev);
> +	if (skb->offload_fwd_mark &&
> +	    br_vlan_enabled(dsa_port_bridge_dev_get(dp))) {

[Severity: High]
Can br_vlan_enabled() be reached here with a NULL device?

dsa_port_bridge_dev_get() is documented to return NULL for a port with no
bridge:

include/net/dsa.h:
	static inline struct net_device *
	dsa_port_bridge_dev_get(const struct dsa_port *dp)
	{
		return dp->bridge ? dp->bridge->dev : NULL;
	}

and the real br_vlan_enabled() dereferences its argument without a check:

net/bridge/br_vlan.c:
	bool br_vlan_enabled(const struct net_device *dev)
	{
		struct net_bridge *br = netdev_priv(dev);

		return br_opt_get(br, BROPT_VLAN_ENABLED);
	}

skb->offload_fwd_mark is set on RX for any bridged user port by
dsa_default_offload_fwd_mark(), and it survives a redirect
(tcf_mirred_forward() -> dev_queue_xmit()).  A frame received on a bridged
port and redirected out a standalone port with
"tc ... action mirred egress redirect dev swpN" would therefore enter
mt7628_tag_xmit() with offload_fwd_mark == 1 and dp->bridge == NULL.  The
pre-patch code tolerated that because dsa_port_bridge_num_get() checks
dp->bridge itself.

Is the lockless read of dp->bridge also safe against teardown?
dsa_port_bridge_destroy() frees the bridge with a plain kfree():

net/dsa/port.c:
	struct dsa_bridge *bridge = dp->bridge;

	dp->bridge = NULL;

	if (!refcount_dec_and_test(&bridge->refcount))
		return;
	...
	kfree(bridge);

There is no call_rcu()/kfree_rcu() or synchronize_net() here, while the
tagger reads dp->bridge from softirq TX context, so "ip link set swpX
nomaster" with traffic flowing looks like it can make the reader observe
either NULL or a freed struct dsa_bridge and then dereference bridge->dev.

Note also that dp->bridge is read twice, once here and once via
dsa_port_bridge_num_get() below, so a concurrent join/leave can select this
branch based on one bridge and compute the VBID from another (or from
bridge_num 0, which is reserved).

Would reading the bridge device once into a local and testing it, the way
dsa_xmit_ll() does, address both?

net/dsa/tag_dsa.c:
	if (skb->protocol == htons(ETH_P_8021Q) &&
	    (!br_dev || br_vlan_enabled(br_dev))) {

> +		/*
> +		 * On VLAN aware ports only modify the TPID to contain the
> +		 * MT7628 egress port metadata, instead of adding a new vlan tag
> +		 */
> +		tag = dsa_etype_header_pos_tx(skb);
> +		tag[0] = htons(xmit_tpid);
> +		return skb;
> +	}
> +

[Severity: High]
Is there anything guaranteeing that the two bytes at
dsa_etype_header_pos_tx(skb) really are an 802.1Q TPID here?

skb->offload_fwd_mark only says the frame was/should be hardware forwarded;
it says nothing about the header layout.  A concrete sequence:

  - swp0 and swp1 in a VLAN-unaware bridge.  mt7628_tag_rcv() takes the
    is_dsa_8021q == true path, so no VLAN tag is restored and the frame is
    untagged, but dsa_default_offload_fwd_mark() still sets
    skb->offload_fwd_mark = 1.

  - a tc ingress rule on swp0 redirects to swp2, which belongs to a
    VLAN-aware bridge.

mt7628_tag_xmit() then takes this branch on an untagged frame: the real
EtherType (say 0x0800) is replaced by ETH_P_8021Q | port_mask and the next
two payload bytes are consumed by the switch as the VLAN TCI, so the frame
is forwarded on a VID taken from packet payload.

The sibling taggers gate the same optimisation on the frame actually being
tagged, for example tag_dsa.c:

	if (skb->protocol == htons(ETH_P_8021Q) &&
	    (!br_dev || br_vlan_enabled(br_dev))) {

and tag_sja1105.c uses skb_vlan_tag_present() /
__vlan_hwaccel_push_inside().  Would an eth_type_vlan(skb->protocol) or
skb_vlan_tag_present() test here, falling back to pushing a real MT7628
tag, keep this branch self-consistent?

[Severity: Medium]
Both new paths hardcode ETH_P_8021Q.  What happens with a bridge created as
"vlan_filtering 1 vlan_protocol 802.1ad"?

On egress the existing 0x88A8 TPID is overwritten with ETH_P_8021Q |
port_mask, and on ingress mt7628_tag_rcv() restores the tag with
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), tci), so the bridge sees
vlan_proto 0x8100 while it selected 0x88A8.

DSA has no SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL handler, so neither the
core nor drivers/net/dsa/mt7628.c can reject such a bridge, and the
registers the driver programs contain no configurable TPID.

sja1105 consults the bridge instead of hardcoding the value:

net/dsa/tag_sja1105.c:
		br_vlan_get_proto(br, &proto);

		return proto;

Would br_vlan_get_proto() be usable here as well, or should the driver
refuse 802.1ad bridges?

>  	xmit_vlan = skb->offload_fwd_mark ?
>  	    dsa_tag_8021q_bridge_vid(dsa_port_bridge_num_get(dp)) :
>  	    dsa_tag_8021q_standalone_vid(dp);

[Severity: High]
For a port under a VLAN-filtering bridge, only skb->offload_fwd_mark == 1
traffic takes the new branch above; everything else still falls through here
and gets a tag_8021q VID.

But mt7628_dsa_8021q_vlan_add() registers every tag_8021q VID as
MT7628_VLAN_TYPE_UNAWARE, and mt7628_vlan_sync() removes VLAN-filtering
ports from both the member and the untag mask of those blocks:

drivers/net/dsa/mt7628.c:
		if (vlan->type == MT7628_VLAN_TYPE_AWARE)
			member_mask = esw->vlan_filtering;
		else
			member_mask = ~esw->vlan_filtering;
		member_mask |= MT7628_ESW_PORTS_CPU;
		...
		mt7628_esw_set_vmsc(esw, i, vlan->members & member_mask);
		mt7628_esw_set_vub(esw, i, vlan->untag & member_mask);

There is real traffic to such a port with offload_fwd_mark == 0:

  - locally originated frames sent on the swpX netdev, including STP BPDUs
    via br_send_bpdu() -> dev_queue_xmit(p->dev);

  - bridge-forwarded IGMP/MLD, because tx_fwd_offload is refused for them:

    net/bridge/br_switchdev.c, nbp_switchdev_can_offload_tx_fwd():
	if (br_multicast_igmp_type(skb))
		return false;

In those cases the tagger targets dsa_tag_8021q_standalone_vid(dp) for a
port that is not a member of that VLAN and whose egress untag bit was
cleared.  Does that frame get dropped by the switch, or does it leave the
port still carrying a DSA-reserved VID from the 3072-4095 range?

> @@ -47,9 +66,7 @@ static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,

[ ... ]

> @@ -77,6 +111,10 @@ static struct sk_buff *mt7628_tag_rcv(struct sk_buff *skb,
>  	skb_pull_rcsum(skb, MT7628_TAG_LEN);
>  	dsa_strip_etype_header(skb, MT7628_TAG_LEN);
>  	dsa_default_offload_fwd_mark(skb);
> +
> +	if (!is_dsa_8021q)
> +		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), tci);
> +

[Severity: Medium]
This restores the tag for every non-tag_8021q VID, including tags the switch
inserted itself from the port PVID for frames that arrived untagged on the
wire, which is the case the new comment above describes ("packets where the
VLAN tag is inserted by the PVID mechanism").

The core already has a mechanism for exactly this hardware behaviour:

include/net/dsa.h:
	/* Pop the default_pvid of VLAN-aware bridge ports from tagged frames.
	 * Useful if the switch cannot preserve the VLAN tag as seen on the
	 * wire for user port ingress, and chooses to send all frames as
	 * VLAN-tagged to the CPU, including those which were originally
	 * untagged.
	 */
	u32			untag_vlan_aware_bridge_pvid:1;

and it is only honoured when the driver opts in:

net/dsa/tag.c, dsa_switch_rcv():
	if (unlikely(cpu_dp->ds->untag_bridge_pvid ||
		     cpu_dp->ds->untag_vlan_aware_bridge_pvid)) {
		/* dsa_software_vlan_untag() drops skb on failure */
		nskb = dsa_software_vlan_untag(skb);

mt7628_setup() registers tag_8021q and max_num_bridges but never sets
untag_vlan_aware_bridge_pvid, so dsa_software_untag_vlan_aware_bridge() is
never reached for this switch.  Should the driver set that flag, so that
packet sockets, tc ingress filters, ebtables/nft VLAN matches and software
protocols running on the plain port do not see a tag that was never on the
wire?

>  	return skb;
>  }
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906171625.533915-1-joey%40tinyisr.com

[PATCH net-next v1 6/6] MAINTAINERS: add myself as MT7628 embedded switch maintainer

[Joris Vaisvila]

List all the files of the MT7628 embedded switch driver and add myself
as the maintainer.

Signed-off-by: Joris Vaisvila <joey@tinyisr.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b23fb6f2f4ef..95e361f5d0ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16781,6 +16781,15 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/i2c/mediatek,mt7621-i2c.yaml
 F:	drivers/i2c/busses/i2c-mt7621.c
 
+MEDIATEK MT7628 SWITCH DRIVER
+M:	Joris Vaisvila <joey@tinyisr.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/dsa/mediatek,mt7628-esw.yaml
+F:	drivers/net/dsa/mt7628.c
+F:	drivers/net/phy/mediatek/mtk-fe-soc.c
+F:	net/dsa/tag_mt7628.c
+
 MEDIATEK MTMIPS CLOCK DRIVER
 M:	Sergio Paracuellos <sergio.paracuellos@gmail.com>
 S:	Maintained
-- 
2.55.0