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

[netdev-bot+sashiko@kernel.org]

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

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

- [High] net: dsa: lockless child_bus access causes MDIO race condition
- [Medium] The new switch-device compatible `econet,en751221`
  (mediatek,mt7530.yaml line 126) is the identical string already…
- [Medium] The new `econet,en751221` conditional (mediatek,mt7530.yaml
  lines 369-377) applies only `$ref: "#/$defs/mt7530-passthrough-dsa-…
- [Medium] The series patch "net: dsa: mediatek: support MDIO switch
  downstream of MMIO switch" moved `mt7530_setup_mdio_irq()` out of…
- [Medium] In passthrough mode `mt753x_cpu_port_enable()` (added by the
  series patch "net: dsa: mediatek: add support for passthrough mode")…
- [Low] The paragraph added to the binding description
  (mediatek,mt7530.yaml lines 32-36) states that "EcoNet EN751221 SoCs…
- [Low] Concern that neither `econet,en751221-switch` nor
  `econet,en751221` has any consumer: at this commit…

--

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

This adds two compatible strings to the MT7530 binding: econet,en751221-switch
for the memory mapped switch built into the EcoNet EN751221 SoC, and
econet,en751221 for the MT7530 multi-chip module that some EN751221 packages
attach to port 5 of the on-die switch via TRGMII. A new $defs block constrains
the DSA link port of the multi-chip module.

A few questions on the schema below, plus two observations about other
patches in the same series.

> 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
> @@ -29,6 +29,12 @@ description: |
>    and the switch registers are directly mapped into SoC's memory map rather than
>    using MDIO. The DSA driver currently doesn't support MT7620 variants.
>  
> +  EcoNet EN751221 SoCs have a memory mapped MT7530 with 3x 10/100 PHYs and 1x
                                                          ^^^^

[Severity: Low]
Should this be 4x 10/100 PHYs?  The commit message of this patch says
"ports 0..3 are 10/100 only, port 4 has a gigabit PHY", which is four fast
ethernet PHYs, and en751221_mac_port_get_caps() in the later driver patch of
this series agrees:

	/* EN751221 ports 0..3 are connected to the integrated FE PHYs. */
	case 0 ... 3:
		__set_bit(PHY_INTERFACE_MODE_INTERNAL, config->supported_interfaces);
		config->mac_capabilities |= MAC_10 | MAC_100;

Since there is no in-tree EN751221 switch DTS yet, this paragraph is the only
port reference a DT author has.

> +  gigabit PHY. Some EN751221 chips such as EN7513G and EN7526G have a multi-chip
> +  module implementation of the MT7530 as well as the on-die switch. In these
> +  packages, the multi-chip module is connected to port 5 of the on-die switch
> +  via a TRGMII link.
> +
>    There is only the standalone version of MT7531.
>  
>    Port 5 on MT7530 supports various configurations:
> @@ -111,6 +117,14 @@ properties:
>            Built-in switch of the EcoNet EN7528 SoC
>          const: econet,en7528-switch
>  
> +      - description:
> +          Built-in switch of EcoNet EN751221 SoC
> +        const: econet,en751221-switch
> +
> +      - description:
> +          Multi-chip module MT7530 in EN751221 SoC
> +        const: econet,en751221

[Severity: Medium]
Can this string collide with the existing SoC compatible?  econet,en751221 is
already documented as the EN751221 root node compatible in
Documentation/devicetree/bindings/mips/econet.yaml:

  properties:
    $nodename:
      const: '/'

    compatible:
      oneOf:
        - description: Boards with EcoNet EN751221 family SoC
          items:
            - enum:
                - smartfiber,xp8421-b
            - const: econet,en751221

and arch/mips/boot/dts/econet/en751221.dtsi uses it on the root node:

  / {
	compatible = "econet,en751221";

dtschema derives the select clause of a binding from the strings enumerated
under properties:compatible, so after this patch mediatek,mt7530.yaml is also
selected for that root node.  The switch schema has required: [compatible,
reg], unevaluatedProperties: false, and via the new conditional below also
required: [mediatek,mcm] (which in turn pulls in resets and reset-names).  Does
make dtbs_check on arch/mips/boot/dts/econet/en751221_smartfiber_xp8421-b.dtb
now report the root node as an invalid MT7530 switch node?

The reverse direction looks affected too: a switch@... node carrying
compatible = "econet,en751221" is selected by mips/econet.yaml and fails its
$nodename: const: '/' and its two-entry board/SoC compatible list.

The analogous MT7621 case does not collide because the SoC root uses
mediatek,mt7621-soc.  Since the SoC compatible is already released, would a
distinct string for the switch, e.g. econet,en751221-mcm-switch, work here?

> +
>    reg:
>      maxItems: 1
>  

[ ... ]

> @@ -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
> +
>    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:
> @@ -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: High]
One more observation about another patch in this series, so there is
no hunk to quote for it either.

In "net: dsa: mediatek: support PLL setup on MMIO MT7530", is the MDIO
bus lock lost for the new child_bus path?  core_write() and core_rmw()
now do:

	if (!bus)
		bus = priv->child_bus;

	if (WARN_ON_ONCE(!bus))
		return;

	mt7530_mutex_lock(priv);

	/* Write the desired MMD Devad */
	ret = bus->write(bus, ctl_phy, MII_MMD_CTRL, MDIO_MMD_VEND2);

but mt7530_mutex_lock() only knows about priv->bus:

	if (priv->bus)
		mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);

On an MMIO switch priv->bus is NULL, so the guard makes
mt7530_mutex_lock()/mt7530_mutex_unlock() a no-op while the following
bus->write()/bus->read() calls are raw driver callbacks issued on
child_bus without child_bus->mdio_lock held.  The indirect MMD sequence
(MII_MMD_CTRL devad, MII_MMD_DATA address, then the data phase) is not
atomic on the wire, so a concurrent mdiobus_read()/mdiobus_write() on
the same child bus - phylib polling the downstream MT7530 PHYs, or a
regmap access from the mt7530 MDIO path - can interleave and leave the
MMD devad/address latch pointing somewhere else, giving a read or write
to the wrong core register.  This is reachable from mt7530_setup_port6()
and the PLL setup path whenever a port changes state, i.e. during normal
operation, and the missing lock is invisible to lockdep because nothing
is taken at all.

Should mt7530_mutex_lock()/mt7530_mutex_unlock() fall back to
priv->child_bus->mdio_lock the same way core_write()/core_rmw() do, or
would it be cleaner for the child_bus path to call mdiobus_write()/
mdiobus_read() (which take the lock themselves) instead of the raw
bus->write()/bus->read() callbacks?  Either way the two places that
pick the bus should agree on which lock protects it.

[Severity: Medium]
Are the MT7530 per-port interface-mode constraints intentionally dropped for
this compatible?  The commit message says the multi-chip module "is similar to
the MT7621 multi-chip module", and the mediatek,mt7621 conditional in the same
allOf applies:

      $ref: "#/$defs/mt7530-dsa-port"
      required:
        - mediatek,mcm

A JSON-Schema object can only carry one $ref, so referencing
mt7530-passthrough-dsa-port here means $defs/mt7530-dsa-port (port 5:
gmii/mii/rgmii, other ports: rgmii/trgmii) is never evaluated.  A DT giving
this multi-chip module an ethernet (CPU) port with phy-mode = "sgmii",
"2500base-x" or "internal" would then pass dtbs_check and only fail later in
mt7530_mac_config()/phylink.

Related to that, the new $defs/mt7530-passthrough-dsa-port rule is gated on:

            if:
              required: [ link ]

so a node with no DSA link port gets no port validation at all, and ports
carrying ethernet are unconstrained.

On the on-die side, econet,en751221-switch is only added to the builtin enum,
whose $defs/builtin-dsa-port rule keys off required: [ ethernet ].  Nothing
pins the cascade port to port 5 with trgmii as the added description states, so
link can be declared on port@0..port@4, which en751221_mac_port_get_caps()
wires to the internal FE/GE PHYs and which offers TRGMII on port 5 only.

Should an example be added for the new on-die plus multi-chip module topology?
Without one, the new $def, the required mediatek,mcm and the implied
resets/reset-names combination are never exercised by dt_binding_check.

> +
>  unevaluatedProperties: false
>  
>  examples:

Two further observations concern other patches in the same series, so there are
no hunks to quote here.

[Severity: Medium]
In "net: dsa: mediatek: support MDIO switch downstream of MMIO switch", is the
new call site of mt7530_setup_mdio_irq() too late?  It used to run inside
mt7530_setup_mdio() before devm_of_mdiobus_register(), and now runs in
mt753x_setup() after mt7530_setup_mdio() has returned:

	ret = mt7530_setup_mdio(priv);
	if (ret)
		return ret;

	if (priv->irq_domain && ds->user_mii_bus)
		mt7530_setup_mdio_irq(priv);

Bus registration creates the phy_devices, and phy_device_create() copies the
IRQ once at creation time:

	dev->irq = bus->irq[addr];

mdiobus_alloc_size() pre-fills bus->irq[i] = PHY_POLL, so the internal PHYs are
created with phydev->irq == PHY_POLL and the later
ds->user_mii_bus->irq[p] = irq_create_mapping(...) writes an array that is not
read again.  Does this leave every MT7530/MT7531 board that declares
interrupt-controller on the switch node and has no mdio child node (binding
examples 3 to 7, MT7621 boards) on polled link detection instead of the
switch PHY link-change interrupt?  The ordering is unchanged at the end of the
series.

[Severity: Medium]
In "net: dsa: mediatek: add support for passthrough mode", is the egress tag
setting for the upstream port immediately overwritten?
mt753x_cpu_port_enable() programs:

	if (priv->is_passthrough) {
		/* Disable parsing of the DSA tag ... */
		regmap_write(priv->regmap, MT7530_PVC_P(port),
			     VLAN_ATTR(MT7530_VLAN_TRANSPARENT) |
			     PVC_EG_TAG(MT7530_VLAN_EG_DISABLED));

but both callers, mt7530_setup() and mt7531_setup_common(), do this in the same
loop iteration right after the call:

		if (dsa_is_upstream_port(ds, i)) {
			mt753x_cpu_port_enable(ds, i);
		} else {
			...
		}
		/* Enable consistent egress tag */
		regmap_update_bits(priv->regmap, MT7530_PVC_P(i),
				   PVC_EG_TAG_MASK,
				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));

MT7530_VLAN_EG_DISABLED is 0 and MT7530_VLAN_EG_CONSISTENT is 1 in
drivers/net/dsa/mt7530.h, so the field really changes.  The downstream DSA link
port is re-programmed to PVC_EG_TAG(MT7530_VLAN_EG_DISABLED) later in
mt7530_port_enable(), but with priv->is_passthrough set the upstream port takes
neither the user port branch nor the dsa_port_is_dsa(dp) && dp->index !=
upstream_pt branch there, so there is no later write for it.  Is the write in
mt753x_cpu_port_enable() then dead, or should the EG_CONSISTENT update be
skipped for the upstream port in passthrough mode?  The commit message of that
patch states both ports end up at MT7530_VLAN_EG_DISABLED.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915174643.253660-1-cjd%40cjdns.fr