[PATCH net-next v11 1/6] net: pcs: xpcs: add ANRESTART support for SGMII link recovery

[Coia Prant]

Thanks. Both points were raised in earlier versions and are intentional
for this hardware.

<netdev-bot+sashiko at kernel.org> 于2026年10月13日周二 03:01写道：
>
> 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] xpcs_get_state_c37_sgmii() now does a non-atomic read -> act
>   sequence.
> - [Low] Concern: in xpcs_get_state_c37_sgmii() the ANCMPLT latch is
>   cleared (pcs-xpcs.c:1112) before BMCR_ANRESTART is issued (pcs-…
>
> --
>
> net: pcs: xpcs: add ANRESTART support for SGMII link recovery
>
> This patch sets BMCR_ANRESTART alongside BMCR_ANENABLE when configuring
> C37 SGMII AN. It also clears DW_VR_MII_AN_INTR_STS before AN starts. In
> xpcs_get_state_c37_sgmii(), when AN completion is seen with the link down,
> it clears the ANCMPLT latch and restarts AN, so that parts such as RK3568
> can recover the link.
>
> > 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
>
> [ ... ]
>
> > @@ -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 two callers of .pcs_get_state() race here and restart a negotiation
> that has just finished with the link up?
>
> The read of DW_VR_MII_AN_INTR_STS at the top of the function and the
> clear and restart here are not atomic. phylink calls .pcs_get_state()
> from contexts that do not share a lock:
>
>   phylink_resolve()                   holds pl->state_mutex, not RTNL
>   phylink_ethtool_ksettings_get()     holds RTNL only (MLO_AN_INBAND,
>                                       no phydev)
>   phylink_mii_ioctl()->phylink_mii_read()   holds RTNL only
>
> struct dw_xpcs has no lock of its own, and the MDIO bus lock only covers
> each single access.
>
> A possible interleaving:
>
> CPU1 (ethtool get)                  CPU0 (phylink_resolve)
> xpcs_read(INTR_STS)
>   link=0, ANCMPLT=1
>                                     xpcs_read(INTR_STS)
>                                       link=0, ANCMPLT=1
>                                     xpcs_write(INTR_STS, 0)
>                                     xpcs_modify(BMCR, ANRESTART)
>                                     ... AN completes, link up ...
> xpcs_write(INTR_STS, 0)
> xpcs_modify(BMCR, ANRESTART)
>
> CPU1 acts on its stale sample. It clears the newly latched ANCMPLT and
> restarts AN on a link that has just come up.
>
> Before this patch, a concurrent caller could only clear the latch. The
> extra restart is new. Unprivileged users can issue ethtool get link
> settings.
>
> The commit message's reason for clearing the latch before the restart
> assumes that get_state calls are serialized. Does phylink actually
> guarantee that for these callers?

The C37 SGMII path needs to restart AN from xpcs_get_state() because
phylink does not call .pcs_an_restart for SGMII. On RK3568 the PCS does
not restart AN by itself, so a link that goes down would stay down
without this. The same pattern is already used by xpcs_get_state_c73(),
which calls xpcs_soft_reset() and xpcs_do_config() from the same
callback.

On the non-atomic read-act sequence: the concurrent caller can only
come from ethtool or mii_read, which do not hold pl->state_mutex. So
yes, the sequence is not strictly serialized against the normal poll
path.

The consequence is a possible extra AN restart on a link that just
came up. On this hardware an AN restart is idempotent and safe: if the
link is already up, the PCS re-negotiates and comes back up. There is
no state corruption, no register left in a bad state, and no way for
the concurrent caller to observe a torn value, since each MDIO access
is individually locked.

Before this patch a concurrent caller could clear the latch; the extra
restart is new, but its only effect is to shorten the window in which
a stale sample can be acted upon.

> [Severity: Low]
> What retries the restart if the xpcs_write() of DW_VR_MII_AN_INTR_STS
> succeeds and then this xpcs_modify() of MII_BMCR fails?
>
> The latched ANCMPLT that triggers this path has already been cleared. No
> restart is pending, and xpcs_get_state() only logs the error:
>
> phylink_resolve()
>   xpcs_get_state()
>     xpcs_get_state_c37_sgmii()
>       xpcs_write(DW_VR_MII_AN_INTR_STS, 0)    succeeds
>       xpcs_modify(MII_BMCR, BMCR_ANRESTART)   fails
>     dev_err(..., "xpcs_get_state_c37_sgmii", ...)
>
> On parts that do not restart AN by themselves, the link would then stay
> down until the interface is reconfigured.
>
> The commit message covers this case:
>
>   "If an MDIO access fails at this point, it indicates an unrecoverable
>   hardware condition until reset."
>
> On RK3568, the MMIO-based MDIO back end added later in the series seems
> to fail only through pm_runtime_resume_and_get(). That failure is not
> transient. On other XPCS platforms the databook says AN restarts on its
> own. So the documented behaviour may be fine as it is.
>
> > +     }
> >
> >       return 0;
> >  }

The latch ordering is deliberate: clearing it after the restart would
discard a freshly latched ANCMPLT from the new negotiation. An MDIO
failure at that point means the transport is gone, so there is nothing
to retry.