Re: [PATCH net-next v10 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] In xpcs_get_state_c37_sgmii() (drivers/net/pcs/pcs-
  xpcs.c:1110-1121) the ANCMPLT latch is cleared first…
- [Medium] xpcs_get_state_c37_sgmii() now mutates PCS hardware state
  (write-0-to-clear of DW_VR_MII_AN_INTR_STS and the BMCR_ANRESTART…

--

net: pcs: xpcs: add ANRESTART support for SGMII link recovery

This adds BMCR_ANRESTART to the C37 SGMII paths: xpcs_config_aneg_c37_sgmii()
now clears DW_VR_MII_AN_INTR_STS and sets ANRESTART together with ANENABLE,
and xpcs_get_state_c37_sgmii() clears the AN latch and issues an AN restart
when the link is down but AN completion was latched.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..8c3875b6985b9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c

[ ... ]

> @@ -1093,9 +1107,18 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  		return 0;
>  	}
>  
> -	/* Clear AN complete status or interrupt */
> -	if (state->an_complete)
> -		xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	if (state->an_complete) {
> +		/* Clear AN complete status or interrupt */
> +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Initiate the next round of AN */
> +		ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				  BMCR_ANRESTART);
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: Medium]
Can this ordering leave the restart permanently disarmed if the xpcs_modify()
fails?

The latched ANCMPLT bit is the only piece of hardware state this code uses to
decide that a restart is needed, and it is destroyed before the restart is
actually requested.  xpcs_modify() is a real read-modify-write:

xpcs_modify() -> mdiodev_c45_modify() -> mdiobus_c45_modify() ->
__mdiobus_c45_modify_changed()

and that last function returns the read error before issuing any write:

drivers/net/phy/mdio_bus.c:__mdiobus_c45_modify_changed() {
	...
	ret = __mdiobus_c45_read(bus, addr, devad, regnum);
	if (ret < 0)
		return ret;
	...
}

So on a transient register read error the PCS stays AN-complete and link-down
with no negotiation started, while the latch that would have re-triggered this
branch has already been cleared.  Every later poll then sees an_complete == 0
and never re-enters the branch, which is the "link stays down forever" case the
commit message describes.

The commit message says:

	"If an MDIO access fails at this point, it indicates an
	 unrecoverable hardware condition until reset."

Where does that reset happen?  .pcs_get_state is void, and the caller only
logs the value:

drivers/net/pcs/pcs-xpcs.c:xpcs_get_state() {
	...
	case DW_AN_C37_SGMII:
		ret = xpcs_get_state_c37_sgmii(xpcs, state);
		if (ret)
			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
				"xpcs_get_state_c37_sgmii", ERR_PTR(ret));
		break;
	...
}

Nothing sets xpcs->need_reset, resets the PCS or re-arms the restart, so
recovery seems to require an administrative down/up to re-run
xpcs_config_aneg_c37_sgmii().  Would it be safer to issue the ANRESTART first
and only clear the latch once the restart write succeeded, or to set
xpcs->need_reset on failure?

[Severity: Medium]
What serializes this new register write against a concurrent
.pcs_get_state call?

phylink reaches this callback from two paths that share no lock:

drivers/net/phy/phylink.c:phylink_resolve() {
	mutex_lock(&pl->state_mutex);
	...
		phylink_mac_pcs_get_state(pl, &link_state);
	...
}

drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
	ASSERT_RTNL();
	...
		phylink_mac_pcs_get_state(pl, &link_state);
	...
}

phylink_mii_read() via phylink_mii_ioctl() is in the same ASSERT_RTNL()-only
category.  struct dw_xpcs in drivers/net/pcs/pcs-xpcs.h has no lock of its own,
and the MDIO bus mutex only makes each individual transaction atomic, not the
read of DW_VR_MII_AN_INTR_STS followed by the clear and the ANRESTART.

So can an unprivileged ETHTOOL_GLINKSETTINGS query (or SIOCGMIIREG) racing with
phylink_resolve() clear a latch belonging to a negotiation the other caller just
started, and then issue a second BMCR_ANRESTART that aborts that in-flight
negotiation?

Even without a race, the read of DW_VR_MII_AN_INTR_STS and the subsequent blind
write of 0 are not atomic with respect to the hardware latch, so is an AN
completion latched between those two accesses silently dropped?

Before this patch the callback only cleared the latch, so the state-changing
write is new here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922200336.2201212-1-coiaprant%40gmail.com