Re: [PATCH v3 3/7] net: dsa: mediatek: add support for passthrough mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] net: dsa: mt7530: mt753x_check_passthrough() fails on dual-CPU-port switches
- [High] net: dsa: mt7530: mt7530_port_change_mtu() is a no-op in passthrough mode
- [High] net: dsa: mt7530: wrong upstream port and VLAN handling on downstream switch
- [High] net: dsa: mt7530: mt753x_trap_frames() drops egress link-local frames in passthrough mode
- [High] net: dsa: mt7530: NULL pointer dereference in mt7531_mac_config() on DSA port
- [Medium] net: dsa: mt7530: setup overwrites PVC_EG_TAG on passthrough upstream port
--

commit cb9a82b0090067bf9bb939c7fda22a9fd5e2d64d
Author: Caleb James DeLisle <cjd@cjdns.fr>

net: dsa: mediatek: add support for passthrough mode

This commit adds passthrough mode support to the mt7530 DSA driver so an
upstream MediaTek switch can forward traffic unmodified to and from a
downstream switch connected via a DSA link port. It also updates switch
setup and port configuration to support operating as the downstream switch
whose upstream port is a DSA link port rather than a CPU port.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index d10194488bdee..3eb2e11d69856 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> @@ -1286,13 +1292,41 @@ mt753x_trap_frames(struct mt7530_priv *priv)
>  				   TO_CPU_FW_CPU_ONLY);
>  }
>  
> +static int
> +mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu);
> +
> +/* If this switch is downstream of another switch that is in passthrough mode,
> + * the "CPU" port is actually a DSA port.
> + */
>  static void
>  mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  
> -	/* Enable Mediatek header mode on the cpu port */
> -	regmap_write(priv->regmap, MT7530_PVC_P(port), PORT_SPEC_TAG);
> +	if (priv->is_passthrough) {
> +		/* Disable parsing of the DSA tag, it will be forwarded blindly
> +		 * to the downstream switch.
> +		 */
> +		regmap_write(priv->regmap, MT7530_PVC_P(port),
> +			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
> +			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));

[Severity: High]
When priv->is_passthrough is true, mt753x_cpu_port_enable() leaves
PORT_SPEC_TAG unset on the upstream CPU port so frames from the CPU are
forwarded via the port matrix rather than by parsing the destination port in
the MediaTek special tag.

However, mt7530_setup() and mt7531_setup_common() still unconditionally call
mt753x_trap_frames(), which programs MT753X_BPC, MT753X_RGAC1, and
MT753X_RGAC2 with TO_CPU_FW_CPU_ONLY for 01:80:C2:00:00:{00,01,02,03,0E}
frames:

drivers/net/dsa/mt7530.c:mt753x_trap_frames() {
	regmap_update_bits(priv->regmap, MT753X_BPC,
			   PAE_BPDU_FR | PAE_EG_TAG_MASK | PAE_PORT_FW_MASK |
				   BPDU_EG_TAG_MASK | BPDU_PORT_FW_MASK,
			   PAE_BPDU_FR | PAE_EG_TAG(MT7530_VLAN_EG_DISABLED) |
				   PAE_PORT_FW(TO_CPU_FW_CPU_ONLY) |
				   BPDU_EG_TAG(MT7530_VLAN_EG_DISABLED) |
				   TO_CPU_FW_CPU_ONLY);
	...
}

Without PORT_SPEC_TAG enabled on the passthrough switch's ingress CPU port to
override the destination port from the special tag, won't link-local control
frames (STP BPDUs, 802.1X PAE, LACP, LLDP) transmitted by the CPU towards the
downstream switch get trapped back to the passthrough switch's own CPU port
and dropped instead of being forwarded out the downstream DSA link port?

> +
> +		/* The port is not configured to parse DSA tags, so they are
> +		 * liable to be confused for length fields, so length check is
> +		 * disabled.
> +		 */
> +		regmap_clear_bits(priv->regmap, MT753X_AGC, AGC_L2LEN_CHK);
> +
> +		/* In passthrough mode, MTU is only enforced downstream */
> +		mt7530_port_change_mtu(ds, port, MT7530_MAX_MTU);

[ ... ]

> @@ -1386,11 +1446,14 @@ mt7530_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	int length;
>  	u32 val;
>  
> +	if (priv->is_passthrough)
> +		return 0;

[Severity: High]
In mt7530_probe_common(), priv->is_passthrough is set before the switch is
registered. When mt753x_cpu_port_enable() later calls
mt7530_port_change_mtu(ds, port, MT7530_MAX_MTU) under if
(priv->is_passthrough), won't this early return cause mt7530_port_change_mtu()
to return 0 immediately without writing MT7530_GMACCR, leaving the hardware at
its 1522-byte default RX length limit?

[ ... ]

> @@ -2511,7 +2609,7 @@ mt7530_setup(struct dsa_switch *ds)
>  		/* Disable learning by default on all ports */
>  		regmap_set_bits(priv->regmap, MT7530_PSC_P(i), SA_DIS);
>  
> -		if (dsa_is_cpu_port(ds, i)) {
> +		if (dsa_is_upstream_port(ds, i)) {
>  			mt753x_cpu_port_enable(ds, i);
>  		} else {
>  			mt7530_port_disable(ds, i);

[ ... ]

> @@ -2635,7 +2733,7 @@ mt7531_setup_common(struct dsa_switch *ds)
>  		regmap_set_bits(priv->regmap, MT7531_DBG_CNT(i),
>  				MT7531_DIS_CLR);
>  
> -		if (dsa_is_cpu_port(ds, i)) {
> +		if (dsa_is_upstream_port(ds, i)) {
>  			mt753x_cpu_port_enable(ds, i);
>  		} else {
>  			mt7530_port_disable(ds, i);

[Severity: Medium]
In both mt7530_setup() and mt7531_setup_common(), right after the
if (dsa_is_upstream_port(ds, i)) block calls mt753x_cpu_port_enable(ds, i),
the per-port loop runs:

		/* Enable consistent egress tag */
		regmap_update_bits(priv->regmap, MT7530_PVC_P(i),
				   PVC_EG_TAG_MASK,
				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));

When priv->is_passthrough is true, mt753x_cpu_port_enable() configures
MT7530_PVC_P(port) with PVC_EG_TAG(MT7530_VLAN_EG_DISABLED), and unlike the
downstream DSA link port, the upstream port is skipped in
mt7530_port_enable(). Doesn't this regmap_update_bits() call overwrite
PVC_EG_TAG on the passthrough switch's upstream port back to
MT7530_VLAN_EG_CONSISTENT?

[Severity: High]
Also in mt7531_setup_common() and mt7530_port_enable(), enabling support for
DSA_PORT_TYPE_DSA link ports allows port 5 on an MT7531 switch to be
configured as an upstream or downstream DSA link port. When phylink configures
an RGMII mode on MT7531 port 5 via mt753x_phylink_mac_config() ->
mt7531_mac_config():

drivers/net/dsa/mt7530.c:mt7531_mac_config() {
	if (phy_interface_mode_is_rgmii(interface)) {
		dp = dsa_to_port(ds, port);
		phydev = dp->user->phydev;
		mt7531_rgmii_setup(priv, interface, phydev);
	}
}

Since dp->user is only populated for DSA_PORT_TYPE_USER ports and is NULL for
DSA_PORT_TYPE_DSA ports, can dereferencing dp->user->phydev cause a NULL
pointer dereference when port 5 is used as an RGMII DSA link port?

[ ... ]

> @@ -3304,7 +3405,7 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
>  
>  	/* Set the CPU port to trap frames to for MT7530. Trapped frames will be
>  	 * forwarded to the numerically smallest CPU port whose conduit
> -	 * interface is up.
> +	 * interface is up. NOTE: "CPU port" can also mean an upstream DSA link.
>  	 */
>  	if (priv->id != ID_MT7530 && priv->id != ID_MT7621 &&
>  	    priv->id != ID_EN7528)

[Severity: High]
When a switch is downstream of a passthrough switch, its local upstream port
has type DSA_PORT_TYPE_DSA (so dsa_port_is_cpu(dp) is false and
dsa_is_upstream_port(ds, port) is true), while dp->cpu_dp and
conduit->dsa_ptr point to the DSA_PORT_TYPE_CPU port on the upstream switch.

In mt753x_conduit_state_change(), mt7530_update_port_member(), and
mt7530_port_vlan_filtering(), the code uses cpu_dp->index:

drivers/net/dsa/mt7530.c:mt753x_conduit_state_change() {
	struct dsa_port *cpu_dp = conduit->dsa_ptr;
	...
	mask = BIT(cpu_dp->index);
}

drivers/net/dsa/mt7530.c:mt7530_update_port_member() {
	struct dsa_port *cpu_dp = dp->cpu_dp;
	u32 port_bitmap = BIT(cpu_dp->index);
}

drivers/net/dsa/mt7530.c:mt7530_port_vlan_filtering() {
	struct dsa_port *dp = dsa_to_port(ds, port);
	struct dsa_port *cpu_dp = dp->cpu_dp;
	...
	if (vlan_filtering) {
		mt7530_port_set_vlan_aware(ds, port);
		mt7530_port_set_vlan_aware(ds, cpu_dp->index);
	}
}

And when a VLAN is added on the downstream switch, dsa_switch_vlan_add() also
targets the downstream switch's upstream DSA_PORT_TYPE_DSA port, where
mt7530_hw_vlan_add() checks dsa_port_is_cpu(dp):

drivers/net/dsa/mt7530.c:mt7530_hw_vlan_add() {
	if (dsa_port_is_cpu(dp))
		val = MT7530_VLAN_EGRESS_STACK;
	else if (entry->untagged)
		val = MT7530_VLAN_EGRESS_UNTAG;
	else
		val = MT7530_VLAN_EGRESS_TAG;
}

On a downstream switch, won't cpu_dp->index refer to the port number on the
upstream switch rather than the local upstream DSA port on ds (when the two
switches use different port indices), and won't mt7530_hw_vlan_add() program
MT7530_VLAN_EGRESS_UNTAG or MT7530_VLAN_EGRESS_TAG instead of
MT7530_VLAN_EGRESS_STACK on the local upstream DSA port?

[ ... ]

> @@ -3399,6 +3505,55 @@ static int mt7988_setup(struct dsa_switch *ds)
>  	return mt7531_setup_common(ds);
>  }
>  
> +/* 1 if passthrough, negative if error. */
> +static int mt753x_check_passthrough(struct device *dev)
> +{
> +	struct device_node *ports, *port;
> +	int passthrough_ports = 0;
> +	int enabled_ports = 0;
> +
> +	ports = of_get_child_by_name(dev->of_node, "ports");
> +	if (!ports)
> +		ports = of_get_child_by_name(dev->of_node, "ethernet-ports");
> +
> +	if (!ports) {
> +		dev_err(dev, "no ports child node found\n");
> +		return -EINVAL;
> +	}
> +
> +	for_each_available_child_of_node(ports, port) {
> +		struct device_node *link;
> +
> +		enabled_ports++;
> +
> +		link = of_parse_phandle(port, "ethernet", 0);
> +		if (!link)
> +			link = of_parse_phandle(port, "link", 0);
> +
> +		if (!link)
> +			continue;
> +
> +		of_node_put(link);
> +
> +		passthrough_ports++;
> +	}
> +
> +	of_node_put(ports);
> +
> +	/*
> +	 * A switch is considered passthrough if exactly two available
> +	 * ports have an "ethernet" or "link" phandle.
> +	 */
> +	if (passthrough_ports > 2 ||
> +	    (passthrough_ports == 2 && enabled_ports != 2)
> +	) {
> +		dev_err(dev, "Only two ports allowed in passthrough mode\n");
> +		return -EINVAL;
> +	}
> +
> +	return passthrough_ports == 2;
> +}

[Severity: High]
In mt753x_check_passthrough(), passthrough_ports is incremented for any
available port that has either an "ethernet" phandle (CPU port) or a "link"
phandle (DSA cascade port), without requiring a "link" phandle to be present.

On boards with a single switch that uses two CPU ports (both port@5 and
port@6 having "ethernet" phandles) along with enabled user ports,
passthrough_ports will be 2 while enabled_ports is greater than 2. Won't
mt753x_check_passthrough() return -EINVAL in that case and cause
mt7530_probe_common() to fail probing the switch?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028011317.1557238-1-cjd@cjdns.fr?part=3