Re: [PATCH v6 net-next] net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new "EcoNet EN751221 MCM PHY" driver entry can never bind,
  because its gate en751221_is_mcm_phy() (drivers/net/phy/mediatek/mtk-…
- [High] mt7530_is_gphy() (drivers/net/phy/mediatek/mtk-ge.c:150-154)
  masks the raw return value of phy_read(phydev, MII_ESTATUS) with…

--

net: phy: mediatek: support MT7530 PHYs on EN71221 MCM

This adds a second phy_driver entry for the MT7530 PHYs found on the
EcoNet EN751221 multi-chip module, which need extra register writes at
config_init time.  The MT7530 entry loses PHY_ID_MATCH_EXACT and both
entries now select hardware through custom match_phy_device callbacks.

> diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
> index 96d8ac5154e5e..62e3eff37b800 100644
> --- a/drivers/net/phy/mediatek/mtk-ge.c
> +++ b/drivers/net/phy/mediatek/mtk-ge.c

[ ... ]

> @@ -101,6 +102,69 @@ static int mt7530_phy_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +/*
> + * The EcoNet EN751221 "G" multi-chip module MT7530 requires additional PHY
> + * configuration.
> + */
> +static int en751221_mcm_phy_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_soft_reset(phydev);
> +	if (ret)
> +		return ret;
> +

[ ... ]

> +	return mt7530_phy_config_init(phydev);
> +}
> +
> +static bool en751221_is_mcm_phy(struct phy_device *phydev)
> +{
> +	struct device *parent = phydev->mdio.bus->parent;
> +
> +	return parent && parent->of_node &&
> +	       of_device_is_compatible(parent->of_node, "econet,en751221");
> +}

[Severity: High]
Can en751221_is_mcm_phy() ever return true?  The string
"econet,en751221" is documented as a root node compatible only:

Documentation/devicetree/bindings/mips/econet.yaml:
    properties:
      $nodename:
        const: '/'

      compatible:
        oneOf:
          - description: Boards with EcoNet EN751221 family SoC

and the only in-tree users are the root nodes of
arch/mips/boot/dts/econet/en751221.dtsi and
en751221_smartfiber_xp8421-b.dts.

The node inspected here is the MDIO bus parent, which is the switch or
MAC device, for example in mt7530_setup_mdio():

drivers/net/dsa/mt7530.c:
    bus->parent = dev;

so parent->of_node carries a switch compatible such as
"mediatek,mt7530" or "mediatek,mt7621", not the machine compatible.
Does that make en751221_phy_match() always return 0, leaving the new
"EcoNet EN751221 MCM PHY" entry unreachable and EN751221 MCM boards
silently bound to the plain "MediaTek MT7530 PHY" entry instead?

If the intent is machine level detection, would
of_machine_is_compatible() be the matching API here?  If node level
detection is intended, which compatible is expected on the switch or
MDIO node?  I could not find an EN751221 switch or MDIO node or driver
compatible upstream at all - grepping for "econet," under drivers/net/
only turns up "econet,en7528-switch" in mt7530-mmio.c plus this new
string - so is there any configuration in which this path runs today?

Also, the subject says "EN71221 MCM" while the changelog body and the
code use EN751221.

> +
> +/*
> + * MTK_GPHY_ID_MT7530 ID is also used for an EcoNet SoC FE phy, but that PHY
> + * does not advertise ESTATUS_1000_TFULL.
> + */
> +static bool mt7530_is_gphy(struct phy_device *phydev)
> +{
> +	return phydev->phy_id == MTK_GPHY_ID_MT7530 &&
> +	       (phy_read(phydev, MII_ESTATUS) & ESTATUS_1000_TFULL) != 0;
> +}

[Severity: High]
Should the phy_read() return value be checked for an error before it is
masked?  ESTATUS_1000_TFULL is BIT(13):

include/uapi/linux/mii.h:
    #define ESTATUS_1000_TFULL	0x2000	/* Can do 1000BT Full          */

phy_read() forwards mdiobus_read() errors as small negative values, and
every one of those has bit 13 set (-EIO is 0xfffffffb, likewise
-ETIMEDOUT, -EOPNOTSUPP).  Does an MDIO read failure therefore read as
"advertises 1000BASE-T full duplex" and turn the guard into an accept?

Second question on the same read: register 15 is only meaningful when
BMSR_ESTATEN is set in MII_BMSR, and a PHY that does not implement it
may return 0xffff, which again has bit 13 set.  genphy_read_abilities()
gates the access and checks the result:

drivers/net/phy/phy_device.c:genphy_read_abilities() {
	...
	if (val & BMSR_ESTATEN) {
		val = phy_read(phydev, MII_ESTATUS);
		if (val < 0)
			return val;
	...
}

Would the same gate plus error check be needed here, i.e. capture the
read in an int and return false when it is negative?

This matters because the FE PHY sharing MTK_GPHY_ID_MT7530 is exactly
the device this predicate is meant to reject.  On a false positive,
en751221_mcm_phy_config_init() runs genphy_soft_reset() followed by the
page 1 and vendor MMD writes against that PHY.

> +
> +static int mt7530_phy_match(struct phy_device *phydev,
> +			    const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && !en751221_is_mcm_phy(phydev);
> +}
> +
> +static int en751221_phy_match(struct phy_device *phydev,
> +			      const struct phy_driver *phydrv)
> +{
> +	return mt7530_is_gphy(phydev) && en751221_is_mcm_phy(phydev);
> +}
> +

[ ... ]

> @@ -126,7 +190,6 @@ static int mt7531_phy_config_init(struct phy_device *phydev)
>  
>  static struct phy_driver mtk_gephy_driver[] = {
>  	{
> -		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
>  		.name		= "MediaTek MT7530 PHY",

With PHY_ID_MATCH_EXACT gone, phy_bus_match() uses the callback result
verbatim:

drivers/net/phy/phy_device.c:phy_bus_match() {
	...
	if (phydrv->match_phy_device)
		return phydrv->match_phy_device(phydev, phydrv);
	...
}

so mt7530_is_gphy() is now the only binding gate for both entries.

[ ... ]

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