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

[netdev-bot+sashiko@kernel.org]

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