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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern: the patch goes against the phylink_pcs_ops contract.
- [Low] Race between concurrent pcs_get_state() callers.

--

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

This patch sets BMCR_ANRESTART together with BMCR_ANENABLE in
xpcs_config_aneg_c37_sgmii(), and clears DW_VR_MII_AN_INTR_STS there as
well. It also restarts AN from xpcs_get_state_c37_sgmii() when the link is
down and AN completion is latched. The goal is to recover the link on
RK3568 XPCS hardware, which does not restart SGMII AN by itself after a
disconnect.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index b415b93d77c15..6466e0ff2a98b 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);

[Severity: Low]
This is a small question about the phylink_pcs_ops contract, not a
functional regression. The pcs_config() kernel-doc in
include/linux/phylink.h says:

 * pcs_config() will be called when configuration of the PCS is required
 * or when the advertisement is possibly updated. It must not unnecessarily
 * disrupt an established link.

As far as I can tell, the existing code earlier in
xpcs_config_aneg_c37_sgmii() already clears BMCR_ANENABLE and sets it again
on every in-band call, and that alone restarts CL37 AN:

	if (mdio_ctrl & BMCR_ANENABLE) {
		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
				 mdio_ctrl & ~BMCR_ANENABLE);

Returning a positive value from pcs_config() to request a restart is also
not an option for SGMII. phylink_pcs_an_restart() only calls
pcs_an_restart() when phy_interface_mode_is_8023z() is true.

So the added BMCR_ANRESTART should not disrupt an established link any more
than before. Is the extra bit expected to have an effect beyond the
existing ANENABLE toggle on RK3568? If so, would it help to say so in the
comment above?

>
>  	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;
> +
> +		/* 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: Low]
Can two pcs_get_state() callers running at the same time race on this
sequence?

xpcs_get_state_c37_sgmii() reads DW_VR_MII_AN_INTR_STS to compute
an_complete. It then clears the register and sets BMCR_ANRESTART, each in
its own MDIO transaction. The MDIO bus lock only covers one access at a
time.

Nothing serialises the callers against each other:

  phylink_resolve() -> phylink_mac_pcs_get_state()
    holds pl->state_mutex but not RTNL

  phylink_ethtool_ksettings_get() -> phylink_mac_pcs_get_state()
    holds only RTNL (MLO_AN_INBAND with no pl->phydev)

  phylink_mii_ioctl() -> phylink_mii_read() -> phylink_mac_pcs_get_state()
    holds only RTNL

One possible interleaving:

ethtool path                            phylink_resolve()
xpcs_get_state_c37_sgmii()
  read AN_INTR_STS: ANCMPLT, no link
  <delayed>
                                        xpcs_get_state_c37_sgmii()
                                          read AN_INTR_STS: ANCMPLT
                                          write AN_INTR_STS = 0
                                          set BMCR_ANRESTART
                                        new AN completes, link up
  write AN_INTR_STS = 0
  set BMCR_ANRESTART

Here, would the late write from the ethtool path discard the newly latched
ANCMPLT? Would its ANRESTART then abort the negotiation that just
succeeded?

The same interleaving looks possible against xpcs_config_aneg_c37_sgmii().
That function now also clears DW_VR_MII_AN_INTR_STS and sets
BMCR_ANENABLE | BMCR_ANRESTART while holding only state_mutex.

The commit message says:

    The latch is cleared before issuing the restart, not after: clearing it
    afterwards would discard a freshly latched ANCMPLT from the new
    negotiation.

Doesn't that reasoning assume a single caller? Before this patch, the clear
in get_state was not followed by a restart, so this race had no visible
effect.

With the restart, the result would be a spurious AN restart. That can delay
link-up, or briefly flap a link that was just reported up, until the next
negotiation completes.

>
>  	return 0;
>  }

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