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

[Sverdlin, Alexander]

Hi all,

On Wed, 2026-09-23 at 22:55 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> 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.

The patch amends the init procedure for the narrowest subset of known
affected PHY revisions. However, from this subset, PHY_ID_PHY11G_VR9_1_2
("Intel XWAY PHY11G (xRX v1.2 integrated)") there were original
integrations of this PHY IP block (into Intel/Lantiq xRX SoCs), which
were not affected by the bug and the new integrations into Maxlinear
GSW1xx standalone Ethernet switches, which are affected by the bug.
That is at least the latest information from Maxlinear. I doubt that
Intel will ever analyse their xRX SoC in this regard.

So the current patch is the simplest approach, but a bit overreacting
for the older SoCs. But at least from the PHY ID registers' PoV, those
PHYs are absolutely idential and officially I cannot differentiate them.
That's where this "unconditional" comes from in the commit message. My
understanding is that the workaround is harmless for older PHY
integrations, I at least checked that the touched registers exist and
have the same meaning in the corresponding documentation for older
integrations.

I indeed mentioned that GPY111/PEF7071 also have the same diagnostics
registers/bits and this is true, but the patch indeed doesn't touch
these PHYs and this is deliberate, because these are not the revisions
integrated in GSW1xx.

Hope this clarifies the questions, but I'm happy to add this verbose
info into the commit message and re-spin if anyone sees this as a good
idea.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com