Re: [PATCH net v2] net: phy: intel-xway: workaround 100BASE-TX Link-Up issue

[netdev-bot+sashiko]

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

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

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).

- [Medium] phy: intel-xway: workaround only applied to one PHY instead of
  unconditionally
- [Low] Unhandled error path leaves the PHY latched in cable-diagnostic
  test mode and skips the remaining init.

--

net: phy: intel-xway: workaround 100BASE-TX Link-Up issue

This adds the MaxLinear GSW12x/GSW14x errata workaround to the Intel XWAY
driver by toggling Cable Diagnostic Mode on and off in a new config_init
wrapper.  The wrapper is wired up for the PHY11G (xRX v1.2 integrated)
entry and then calls the existing xway_gphy_config_init().

> diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c
> index afbcec7117443..3cee31bb931f2 100644
> --- a/drivers/net/phy/intel-xway.c
> +++ b/drivers/net/phy/intel-xway.c

[ ... ]

> @@ -326,6 +331,28 @@ static int xway_gphy_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int xway_11g_int_config_init(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	/* An issue has been sporadically observed after device power-on on the
> +	 * first link-up attempt in 100BASE-TX mode resulting in either the
> +	 * link-up taking a long time, or failing to link-up altogether.
> +	 *
> +	 * Workaround:
> +	 * After power-on, enable Cable Diagnostic Mode for all ports and
> +	 * disable it.
> +	 */
> +	err = phy_modify(phydev, MII_CTRL1000, XWAY_MDIO_GCTRL_TM_MASK, XWAY_MDIO_GCTRL_TM_CDIAG);
> +	if (err)
> +		return err;
> +	err = phy_modify(phydev, MII_CTRL1000, XWAY_MDIO_GCTRL_TM_MASK, XWAY_MDIO_GCTRL_TM_NOP);
> +	if (err)
> +		return err;
> +
> +	return xway_gphy_config_init(phydev);
> +}

[Severity: Medium]
The commit message says the workaround is implemented "unconditionally
in the Intel XWAY driver", and argues that the diagnostic bits have the
same meaning in older PHYs such as GPY111/PEF7071/PHY11G so it should
not hurt them.  But the only entry in xway_gphy[] that is switched over
to xway_11g_int_config_init() is PHY_ID_PHY11G_VR9_1_2 ("Intel XWAY
PHY11G (xRX v1.2 integrated)").

Every other entry still points .config_init at xway_gphy_config_init():
the PEF 7071/7072 PHY11G v1.3, v1.4 and v1.5/v1.6 entries, the PEF 7061
PHY22F entries, the xRX v1.1 integrated PHY11G/PHY22F entries, and even
PHY_ID_PHY22F_VR9_1_2, the xRX v1.2 integrated sibling of the entry that
was changed.  On all of those PHYs the cable diagnostic toggle never
runs.  If any of them is affected by the same errata, the sporadic
100BASE-TX link-up failure described in the commit message remains.

Which behaviour is intended?  The name xway_11g_int_config_init()
suggests the restriction to one PHY ID is deliberate, which contradicts
the changelog.  If the workaround is only meant for the PHY ID that the
GSW12x/GSW14x integrated ports report, could the commit message say so
instead of "unconditionally", and name that PHY ID?  If it is meant to
cover the whole driver, the remaining entries need to be wired up as
well.  For the PHY22F variants it would also be worth confirming that
bits 15:13 of MII_CTRL1000 carry the same test mode meaning on a Fast
Ethernet only PHY before writing them.

[Severity: Low]

What happens to the PHY state if the second phy_modify() in
xway_11g_int_config_init() fails?  The first write has already latched
MII_CTRL1000[15:13] to XWAY_MDIO_GCTRL_TM_CDIAG, and the only write that
leaves cable-diagnostic mode is this separate read-modify-write.

phy_modify() only reports negative errno for real MDIO read/write
failures, so the "if (err)" here fires exactly on bus errors:

drivers/net/phy/phy-core.c:__phy_modify() {
	ret = __phy_modify_changed(phydev, regnum, mask, set);

	return ret < 0 ? ret : 0;
}

On that path the function returns with the PHY still in diagnostic mode
and with no attempt to restore XWAY_MDIO_GCTRL_TM_NOP.  It also skips
xway_gphy_config_init() entirely, so XWAY_MDIO_IMASK is never masked,
the pending interrupt read of XWAY_MDIO_ISTAT never happens, and the
RGMII skew, LED and error-counter setup are all left unprogrammed.

Do the callers recover from this?  phy_init_hw() just propagates the
error to phy_attach_direct(), and on resume:

drivers/net/phy/phy_device.c:mdio_bus_phy_resume() {
	ret = phy_init_hw(phydev);
	if (ret < 0)
		return ret;

	ret = phy_resume(phydev);
	...
}

returns before phy_resume(), so config_init is not re-run in that cycle
and the diagnostic mode persists until the next attach or init.

Granted, this only triggers after the MDIO bus has already failed, where
a rollback write would likely fail too, and the next successful
phy_init_hw() re-runs the toggle.  Still, would it be worth noting that
before this change a config_init failure never left the PHY in a mode
that by itself prevents link-up?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922075251.23386-1-alexander.sverdlin%40siemens.com