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

[netdev-bot+sashiko]

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

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

- [High] The new MCM detection in en751221_is_mcm_phy()
  (drivers/net/phy/mediatek/mtk-ge.c:138-144) tests the MDIO bus *parent…
- [Medium] mt7530_is_gphy() (drivers/net/phy/mediatek/mtk-ge.c:150-154)
  masks the raw return value of phy_read(phydev, MII_ESTATUS) with…
- [Low] The commit subject reads "net: phy: mediatek: support MT7530 PHYs
  on EN71221 MCM" while the commit body, the new code…

--

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

This adds a second phy_driver entry for MTK_GPHY_ID_MT7530 with an extra
register sequence for the EcoNet EN751221 multi-chip module, selects
between the two entries with new match_phy_device callbacks, and skips
MT7530-ID PHYs that report no gigabit support.

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

[Severity: Low]
This isn't a bug, but the subject says EN71221 while the body, the code
(en751221_is_mcm_phy(), en751221_mcm_phy_config_init(), the driver name
"EcoNet EN751221 MCM PHY"), the compatible "econet,en751221" and every
other in-tree reference say EN751221.  Grepping the tree finds no
EN71221 identifier at all.

> diff --git a/drivers/net/phy/mediatek/mtk-ge.c b/drivers/net/phy/mediatek/mtk-ge.c
> index 96d8ac5154e5e..eddb66218d7c0 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;

[ ... ]

> +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 this predicate ever return true?

"econet,en751221" is documented only as a machine compatible on the DT
root node:

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 the only in-tree users put it on '/':

arch/mips/boot/dts/econet/en751221.dtsi
  / {
        compatible = "econet,en751221";

arch/mips/boot/dts/econet/en751221_smartfiber_xp8421-b.dts
  / {
        model = "SmartFiber XP8421-B";
        compatible = "smartfiber,xp8421-b", "econet,en751221";

The node handed to of_device_is_compatible() here is the MDIO bus parent
device, which for MT7530-internal PHYs is the switch device:

drivers/net/dsa/mt7530.c:mt7530_setup_mdio() {
    ...
	bus->parent = dev;
    ...
}

of_device_is_compatible() only inspects the compatible list of the node it
is given, so a switch node that is a child of '/' will not match a root
node compatible.  Does that make en751221_phy_match() always return zero,
leaving the new "EcoNet EN751221 MCM PHY" entry and
en751221_mcm_phy_config_init() unreachable, with mt7530_phy_match()
binding the plain MT7530 entry on the MCM hardware instead?

Would matching the switch's own compatible work better here?  The EcoNet
MCM switch already has one in-tree:

drivers/net/dsa/mt7530-mmio.c
  { .compatible = "econet,en7528-switch", .data = &mt753x_table[ID_EN7528], },

Alternatively, of_machine_is_compatible("econet,en751221") would test the
root node, or a new documented compatible for the MCM switch/MDIO node
could be added.

> +
> +/*
> + * 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: Medium]
Should the phy_read() return value be checked for an error before it is
masked?

ESTATUS_1000_TFULL is 0x2000, i.e. bit 13, and every small negative errno
has bit 13 set in two's complement (-EIO is 0xfffffffb, -ETIMEDOUT is
0xffffff92).  So a failed MDIO read makes mt7530_is_gphy() report
"gigabit capable", which is the opposite of the intended exclusion of the
FE PHY that shares MTK_GPHY_ID_MT7530.

phy_bus_match() returns 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 on a transient bus error the FE PHY would be claimed and programmed
with the GbE sequence in en751221_mcm_phy_config_init() (paged write of
0x3a04, MMD VEND2 0x0417, VEND1 0xa6 and 0x12) or mt7530_phy_config_init().

Should MII_ESTATUS also be gated on BMSR_ESTATEN, the way phylib does it
elsewhere?

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

Without that check, the value read from register 15 on a PHY that does not
implement it is not defined by Clause 22.

> +
> +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);
> +}
> +
>  static int mt7531_phy_config_init(struct phy_device *phydev)
>  {
>  	mtk_gephy_config_init(phydev);
> @@ -135,6 +199,23 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		 */
>  		.config_intr	= genphy_no_config_intr,
>  		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.match_phy_device = mt7530_phy_match,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_phy_read_page,
> +		.write_page	= mtk_phy_write_page,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
> +		.name		= "EcoNet EN751221 MCM PHY",
> +		.probe		= mt7530_phy_probe,
> +		.config_init	= en751221_mcm_phy_config_init,
> +		/* Interrupts are handled by the switch, not the PHY
> +		 * itself.
> +		 */
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.match_phy_device = en751221_phy_match,
>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.read_page	= mtk_phy_read_page,

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