Re: [PATCH net-next 6/7] dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `mt753x_check_passthrough` implicitly bans valid standard DSA cascade and multi-CPU configurations for all MT753x switches.
- [Medium] The binding introduces incomplete and overly restrictive validation for the new en751221 switches, incorrectly using 'passthrough' terminology for a standard cascade link.
--

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

dt-bindings: net: dsa: mediatek,mt7530: add econet,en751221

The EcoNet EN751221 MIPS SoC integrates an MT7530 switch memory-mapped
in the SoC register space. It is similar to the econet,en7528-switch
except that ports 0..3 are 10/100 only, port 4 has a gigabit PHY, and
port 5 has no PHY, but is in some implementations connected to a
downstream MT7530 multi-chip module via a TRGMII link.

> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index e29fb86934981..106c4087528d7 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
[ ... ]
> @@ -223,6 +237,20 @@ $defs:
>                  phy-mode:
>                    const: internal
>  
> +  mt7530-passthrough-dsa-port:
> +    patternProperties:
> +      "^(ethernet-)?ports$":
> +        patternProperties:
> +          "^(ethernet-)?port@[0-6]$":
> +            if:
> +              required: [ link ]
> +            then:
> +              properties:
> +                reg:
> +                  const: 6
> +                phy-mode:
> +                  const: trgmii
> +

[Severity: Medium]
Does this block incorrectly enforce that any cascade link (port with 'link'
property) must be on port 6?

It appears this restricts any cascade link to port 6, breaking cascade links
on port 5, and misapplies the passthrough terminology.

>    mt7530-dsa-port:
>      patternProperties:
>        "^(ethernet-)?ports$":
> @@ -330,6 +358,7 @@ allOf:
>              - airoha,en7581-switch
>              - airoha,an7583-switch
>              - econet,en7528-switch
> +            - econet,en751221-switch
>      then:
>        $ref: "#/$defs/builtin-dsa-port"
>        properties:

[Severity: Medium]
Does this block omit validation for the cascade link on port 5?

The upstream switch uses this block, which only validates `ethernet` ports.
Because its actual cascade link on port 5 uses `link`, it falls through with
no `phy-mode` validation.

> @@ -337,6 +366,16 @@ allOf:
>          mediatek,mcm: false
>          reset-names: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - econet,en751221
> +    then:
> +      $ref: "#/$defs/mt7530-passthrough-dsa-port"
> +      required:
> +        - mediatek,mcm
> +

[Severity: Medium]
Does this apply an overly restrictive definition to the downstream switch?

This block fails to reference `#/$defs/mt7530-dsa-port`, meaning its standard
CPU/user ports bypass `phy-mode` validation.


Regarding a regression introduced in a prior patch within this same patchset
(commit 1d284e3e71056b81f501b69bacab596eedd52079, "net: dsa: mediatek: add
support for passthrough mode") that does not appear to be fixed by the end
of the series:

> 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
[ ... ]
> @@ -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 implicitly ban valid standard DSA cascade and multi-CPU
configurations for all MT753x switches?

This block increments `passthrough_ports` for any port with `ethernet` or
`link` phandles. It incorrectly restricts any switch with exactly two
CPU/cascade ports to strictly having zero user ports, breaking standard
cascade setups.

[ ... ]
> @@ -3529,8 +3648,12 @@ EXPORT_SYMBOL_GPL(mt753x_table);
>  int
>  mt7530_probe_common(struct mt7530_priv *priv)
>  {
> +	int passthrough = mt753x_check_passthrough(priv->dev);
>  	struct device *dev = priv->dev;
>  
> +	if (passthrough < 0)
> +		return passthrough;
> +

[Severity: High]
Does this unconditionally evaluate the faulty passthrough logic for all MT753x
switches during probe?

Any switch with exactly 2 CPU/cascade ports and at least 1 user port will fail
driver probe with `-EINVAL`, breaking networking functionality across the
driver.

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