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

[sashiko-bot@kernel.org]

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

New issues:
- [High] mt753x_check_passthrough() incorrectly rejects valid device tree topologies, breaking probe for cascaded switches.

Pre-existing issues:
- [High] mt753x_conduit_state_change() misconfigures trap rules on downstream switches by using a remote switch's port index.
--

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

net: dsa: mediatek: add support for passthrough mode

This commit adds passthrough mode support for MediaTek MT7530 switches. It
allows the main switch to pass all traffic to and from a downstream switch
without altering the DSA tag, which is useful when the downstream switch is
more capable but the upstream switch is an irremovable part of the SoC.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 9a50a492e6f08..58190e13813c1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -3302,7 +3351,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 &&

[Severity: High]
This isn't a bug introduced by this patch, but while noting that the CPU
port can mean an upstream DSA link, is it possible that the trap rules
are misconfigured on downstream switches by using a remote switch's index?

When a downstream switch is probed, mt753x_conduit_state_change() retrieves
the upstream conduit's port:

    struct dsa_port *cpu_dp = conduit->dsa_ptr;

It then programs the local switch's MT753X_MFC register using the remote
switch's port index:

    mask = BIT(cpu_dp->index);
    ...
    val = MT7530_CPU_EN | MT7530_CPU_PORT(__ffs(priv->active_cpu_ports));
    regmap_update_bits(priv->regmap, MT753X_MFC, ... val);

If the remote root switch uses port 6 as its CPU port, but the local
downstream switch's upstream link is port 5, wouldn't this route trapped
control frames (like BPDUs or IGMP) to a non-existent or incorrect port
on the downstream switch? Should this logic perhaps be using
dsa_switch_upstream_port(ds) to reference the local port?

[ ... ]
> @@ -3397,6 +3446,76 @@ 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)
> +{
[ ... ]
> +	/*
> +	 * 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]
Does this logic inadvertently reject valid device tree topologies for
cascaded switches?

If a standard upstream switch has exactly one CPU port and one DSA link
(totaling 2 passthrough_ports), but also has multiple user ports
configured (making enabled_ports > 2), this check evaluates to true and
returns -EINVAL.

Could this cause the probe function to completely fail for standard
cascade topologies or single switches configured with two CPU ports?

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