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

Jakub Kicinski [off-list ref] 于2026年9月13日周日 04:13写道：

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pcs: xpcs: add ANRESTART support for SGMII link recovery

The patch sets BMCR_ANRESTART together with BMCR_ANENABLE when configuring
C37 SGMII in-band negotiation, and clears the latched CL37 AN complete
status before doing so.  It also restarts negotiation from
xpcs_get_state_c37_sgmii() when AN completion is latched but the link is
reported down.

quoted

    On some hardware using the DesignWare XPCS IP (e.g., RK3568 MAC side
    SGMII), the PCS does not automatically restart auto-negotiation when the
    link goes down and comes back up. Without an explicit ANRESTART, the link
    stays down forever.

[Severity: Low]
The changelog describes a user visible failure ("the link stays down
forever") in long standing shared code, but there is no Fixes: tag and no
statement about which histories are affected.

The only hardware named here, RK3568 XPCS, does not exist in the baseline
tree: searching for xpcs_rk, rk3568-xpcs or pcs-xpcs-rk under drivers/net at
879e280b8486 returns nothing, and the glue driver arrives later in this
series ("net: pcs: xpcs: add Rockchip RK3568 platform glue driver").

Could the changelog either name the pre-existing commit being fixed, or say
explicitly that the affected platform is introduced later in the same
series, so that stable tooling does not have to guess?

The affected platform (RK3568 XPCS) is introduced later in the same
series. I'll make that explicit in the changelog so stable tooling does
not have to guess.

quoted

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 0337e2bcc0125..50b9cdf94fdaa 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c

@@ -816,9 +816,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: Medium]
Should the comment at the top of xpcs_config_aneg_c37_sgmii() be updated
along with this change?  It still says the opposite of what the code now
does:

drivers/net/pcs/pcs-xpcs.c:xpcs_config_aneg_c37_sgmii() {
        /* For AN for C37 SGMII mode, the settings are :-
         ...
         * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
         *
         * Note that VR_MII_MMD_CTRL is MII_BMCR.
         *
         * Note: Since it is MAC side SGMII, there is no need to set
         *       SR_MII_AN_ADV. ... There is also no need to
         *       trigger AN restart for MAC-side SGMII.
         */

On the default path this same function selects MAC side SGMII:

        } else {
                tx_conf = DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII;
        }

so the restart is now issued for exactly the case the comment says does not
need it.  The new DW_VR_MII_AN_INTR_STS write is also a sixth step that the
enumerated sequence does not mention.

If the databook statement no longer holds for these parts, could the
comment be corrected, so that a later reader does not remove the ANRESTART
bit as a mistake?

You're right. The comment still states that AN restart is not needed for
MAC side SGMII, which now contradicts the code. I'll update it to note
that on some hardware (e.g. RK3568) the PCS does not restart AN
automatically when the link comes back, so an explicit ANRESTART is
required.

quoted

@@ -1093,10 +1098,15 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
              return 0;
      }

-     /* Clear AN complete status or interrupt */
-     if (state->an_complete)
+     if (state->an_complete) {
+             /* Clear AN complete status or interrupt */
              xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

+             /* Initiate the next round of AN */
+             xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
+                         BMCR_ANRESTART);
+     }
+
      return 0;
 }

[Severity: Medium]
Can this sequence get stuck if the xpcs_modify() fails?

The latched status is cleared first, and DW_VR_MII_AN_STS_C37_ANCMPLT_INTR
is the only thing that brings execution back into this branch:

        state->an_complete = ret & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;

So if the clear succeeds but the restart does not, every later poll reads
ANCMPLT as 0, state->an_complete stays false, and the restart is never
retried, leaving the port down with AN idle until something calls
pcs_config again.

mdiodev_c45_modify() does a read followed by a write, so there are two
places it can return an error, for example a bus -ETIMEDOUT, or with the
Rockchip glue added later in this series the pm_runtime_resume_and_get()
error path in xpcs_rk_read_reg() / xpcs_rk_write_reg().

Both return values are also discarded and the function still ends with
return 0, so the diagnostic in the caller can never fire for them:

        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));

Would it be better to assert BMCR_ANRESTART before clearing the latched
status, and to propagate both return codes to xpcs_get_state()?

The current order (clear the latched status first, then restart AN) is
intentional. If the restart is issued first, the newly completed AN status
could be cleared by the subsequent write, losing the completion event.

I'll keep the original order and propagate the return value of
xpcs_modify() so the caller can observe errors on the restart path.

Best,
Coia