Re: [PATCH 3/5] net: phy: mxl-gpy: release MxL86211C Super Isolate

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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 patch clears Super Isolate only once, in mxl86211c_probe()
  (drivers/net/phy/mxl-gpy.c:459-463).

--

net: phy: mxl-gpy: release MxL86211C Super Isolate

This commit clears VSPEC1_PM_CTRL.SI in mxl86211c_probe() so that a
MxL86211C strapped into Super Isolate mode gets released. The clear
happens after the MDIO voltage regulator is registered, and gpy_probe()
now runs after it.

> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
> index 4cbbfae046317..790075632fc3f 100644
> --- a/drivers/net/phy/mxl-gpy.c
> +++ b/drivers/net/phy/mxl-gpy.c

[ ... ]

> @@ -459,7 +456,15 @@ static int mxl86211c_probe(struct phy_device *phydev)
>  		return PTR_ERR(rdev);
>  	}
>  
> -	return 0;
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND1, VSPEC1_PM_CTRL,
> +			     VSPEC1_PM_CTRL_SI,
> +			     0);

[Severity: High]
Can the PHY end up back in Super Isolate after a later hardware reset?

The commit message says the strap leaves "the PHY halted after reset until
software releases it". However, SI is only cleared here in probe, and
probe runs once per driver bind.

On boards that have a PHY reset-gpio or reset controller, phylib can
pulse the reset line after probe. phy_detach_internal() asserts it:

drivers/net/phy/phy_device.c:phy_detach_internal() {
    ...
	/* Assert the reset signal */
	phy_device_reset(phydev, 1);
    ...
}

On the next attach or resume, phy_init_hw() releases reset. After that it
only calls soft_reset (this PHY has none) and config_init:

drivers/net/phy/phy_device.c:phy_init_hw() {
	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);
    ...
	if (phydev->drv->config_init) {
		ret = phydev->drv->config_init(phydev);
    ...
}

The MxL86211C entry in genphy_driver[] uses gpy21x_config_init() as its
.config_init. That function only sets possible_interfaces and writes
PHY_ERRCNT, and nothing else in the driver clears VSPEC1_PM_CTRL_SI.

Take a MAC driver that connects the PHY in ndo_open and disconnects it in
ndo_stop. Wouldn't an "ip link set dev X down; ip link set dev X up"
cycle bring the PHY back isolated, with no link until the driver is
rebound?

The same could happen on a resume that power-cycles the PHY, because
mdio_bus_phy_resume() also goes through phy_init_hw().

Later in the series, "net: phy: mxl-gpy: support MxL86211C lane mapping"
adds mxl86211c_config_init(). It only calls
mxl86211c_configure_lane_asp_map() and gpy21x_config_init(), so the SI
clear is still only done in probe at the end of the series.

Would it be better to clear SI in a MxL86211C-specific config_init, so
that it runs after every phy_init_hw()?

> +	if (ret)
> +		return ret;
> +
> +	ret = gpy_probe(phydev);
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930122305.293686-1-robert.marko%40sartura.hr