Re: [PATCH net-next v12 1/6] net: pcs: xpcs: add ANRESTART support for SGMII link recovery
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-18 06:45:46
Also in:
linux-arm-kernel, linux-devicetree, linux-rockchip, lkml
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] In xpcs_get_state_c37_sgmii() (drivers/net/pcs/pcs-xpcs.c lines 1110-1121), the ANCMPLT latch is cleared with… -- net: pcs: xpcs: add ANRESTART support for SGMII link recovery This commit sets BMCR_ANRESTART along with BMCR_ANENABLE in xpcs_config_aneg_c37_sgmii() and clears DW_VR_MII_AN_INTR_STS before starting AN. It also makes xpcs_get_state_c37_sgmii() clear the ANCMPLT latch and trigger an AN restart when the link is down and AN complete is latched. This works around RK3568 XPCS, which does not restart AN on its own.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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]
What happens if the DW_VR_MII_AN_INTR_STS write succeeds but the
xpcs_modify() of MII_BMCR fails?
At that point the ANCMPLT latch is already cleared. If the read fails,
__mdiobus_c45_modify_changed() returns without writing anything:
drivers/net/phy/mdio_bus.c:__mdiobus_c45_modify_changed() {
...
ret = __mdiobus_c45_read(bus, addr, devad, regnum);
if (ret < 0)
return ret;
...
}
On hardware that needs this workaround, the latched ANCMPLT seems to be
the only thing that triggers the restart. Later calls to
xpcs_get_state_c37_sgmii() would see the link down with no ANCMPLT, so
the restart would never be retried.
xpcs_get_state() only logs the error:
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;
phylink_resolve() only reconfigures the PCS when the interface changes
or force_major_config is set:
if ((mac_config && link_state.interface != pl->link_config.interface) ||
pl->force_major_config) {
Would the link then stay down until pcs_config runs again, for example
after an interface down/up?
The commit message says:
If an MDIO access fails at this point, it indicates an
unrecoverable hardware condition until reset.
Does that hold for RK3568? In the later patch "net: pcs: xpcs: add
Rockchip RK3568 platform glue driver", xpcs_rk_read_reg() and
xpcs_rk_write_reg() appear to fail only through
pm_runtime_resume_and_get(). A failed runtime resume callback does not
set the sticky power.runtime_error, so the failure could be transient
rather than permanent.
quoted hunk ↗ jump to hunk
+ } return 0; }
-- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261017054406.1828061-1-coiaprant%40gmail.com