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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] In xpcs_get_state_c37_sgmii() (drivers/net/pcs/pcs-
  xpcs.c:1110-1121) the AN-complete latch is destroyed before the…
- [Medium] The patch turns the pcs_get_state callback into a writer of
  MII_BMCR without any serialization.

--

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

This adds an explicit BMCR_ANRESTART when starting C37 SGMII
auto-negotiation, and also clears DW_VR_MII_AN_INTR_STS plus triggers an
AN restart from the link status path when the link is down but AN
completion is latched.  The stated motivation is RK3568 MAC side SGMII,
which does not re-arm AN by itself after a link drop.

> 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

[ ... ]

> @@ -816,9 +825,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Clear CL37 AN complete status */
> +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>  		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> -				 mdio_ctrl | BMCR_ANENABLE);
> +				 mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);
>  
>  	return ret;
>  }

> @@ -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;
> +

[Severity: Medium]
Can the link stay down permanently here if the restart below fails?

The only condition that lets xpcs_get_state_c37_sgmii() enter this branch is
the latched AN complete bit:

	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
	...
	state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;

The latch is destroyed by the write above before the restart is attempted.

The restart itself can fail before touching the hardware, because
xpcs_modify() -> mdiodev_c45_modify() -> __mdiobus_c45_modify_changed() does
a read first and bails out on a read error:

	ret = __mdiobus_c45_read(bus, addr, devad, regnum);
	if (ret < 0)
		return ret;

So on a transient bus or CSR access error the sequence is: latch cleared,
restart never issued.  On the next poll DW_VR_MII_AN_STS_C37_ANCMPLT_INTR
reads as 0, state->an_complete is false, and this branch is not re-entered.

xpcs_get_state() can only log the error, since the phylink pcs_get_state op
returns void:

	if (ret)
		dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
			"xpcs_get_state_c37_sgmii", ERR_PTR(ret));

No pending-restart state is recorded, and phylink_resolve() will not re-run
pcs_config() while the interface is unchanged.  Given the premise of this
patch, that this hardware does not restart AN on its own, would it be better
to issue the ANRESTART before clearing the latch, or to leave the latch
intact (or set a pending-restart flag) when the restart fails so later polls
retry?

> +		/* 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]
Is the pcs_get_state callback allowed to write MII_BMCR without any
serialization against pcs_config?

phylink reaches pcs_get_state from paths with disjoint locks.
phylink_resolve() calls phylink_mac_pcs_get_state() while holding
pl->state_mutex and not RTNL, and in the same section can call
phylink_major_config() -> phylink_pcs_config() -> xpcs_config_aneg_c37_sgmii():

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

While phylink_ethtool_ksettings_get() and phylink_mii_read() reach it with
RTNL only and no state_mutex:

drivers/net/phy/phylink.c:phylink_ethtool_ksettings_get() {
	case MLO_AN_INBAND:
		if (pl->phydev)
			break;

		phylink_mac_pcs_get_state(pl, &link_state);
}

struct dw_xpcs has no mutex or spinlock, and mdiodev_c45_modify() only makes
a single register read-modify-write atomic on the bus, so can one CPU be
part-way through the multi-register sequence in xpcs_config_aneg_c37_sgmii():

	mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
	...
		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
				 mdio_ctrl & ~BMCR_ANENABLE);
	/* reprogram DW_VR_MII_AN_CTRL, DW_VR_MII_DIG_CTRL1, clear AN_INTR_STS */
	...
		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
				 mdio_ctrl | BMCR_ANENABLE | BMCR_ANRESTART);

while the other CPU, in the ethtool or ioctl query path, clears
DW_VR_MII_AN_INTR_STS and sets BMCR_ANRESTART?  That would kick the AN state
machine while PCS_MODE/TX_CONFIG are half reprogrammed and AN is
deliberately disabled.

A second variant: both paths read link-down plus AN-complete, and the one
that was descheduled then issues ANRESTART from a stale snapshot, restarting
negotiation on a link that has meanwhile come back up.

The driver already exposes .pcs_an_restart = xpcs_an_restart(), which phylink
calls under state_mutex:

	xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
		    BMCR_ANRESTART);

Could the restart be driven through that callback instead of open-coding the
same register write in the status path?

>  
>  	return 0;
>  }

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