Re: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AN complete status is cleared before reading MII_BMCR in the Wangxun-specific path, which can cause the link to stall permanently if a subsequent MDIO read fails.
- [Medium] The patch modifies the 1000BASE-X configuration path to assert BMCR_ANRESTART without any mention or justification in the commit message.
--

--- Patch [6]: [RFC PATCH 06/10] net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568 ---
commit 8c1976e4f52a5e7d8b4bab114f6cf214fe50a249
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: improve SGMII AN state handling for Rockchip RK3568

This patch modifies SGMII AN state handling for Rockchip RK3568 by checking
CL37_ANSGM_STS for link status. It clears the AN complete interrupt and
restarts AN for non-Wangxun platforms if complete, and clears CL37 AN
complete status before starting AN to ensure a clean initial state.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index e69fa2f0a0e8d..cf370ba247cac 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c

[ ... ]

> @@ -884,7 +889,7 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
>  
>  	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]
The commit message mentions improving SGMII AN state handling, but this
adds BMCR_ANRESTART to the 1000BASE-X configuration path in
xpcs_config_aneg_c37_1000basex(). Was this unintentional, or should the
commit message be updated to mention this behavior change for 1000BASE-X?

>  		if (ret < 0)
>  			return ret;
>  	}

[ ... ]

> @@ -1086,7 +1094,24 @@ static int xpcs_get_state_c37_sgmii(struct dw_xpcs *xpcs,
>  			state->duplex = DUPLEX_FULL;
>  		else
>  			state->duplex = DUPLEX_HALF;
> -	} else if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> +
> +		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);

[Severity: High]
Can clearing the interrupt here lead to a permanently stalled link on
Wangxun platforms if the subsequent xpcs_read() fails?

The original code cleared DW_VR_MII_AN_INTR_STS at the very end of the
DW_VR_MII_AN_STS_C37_ANCMPLT_INTR block, after successfully reading
MII_BMCR and MII_ADVERTISE.

By moving the interrupt clear here:

    if (state->an_complete)
        xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

If a transient MDIO error occurs during the subsequent MII_BMCR read:

    speed = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
    if (speed < 0)
        return speed;

The function will exit with an error and the link will remain down, but the
AN complete status has already been cleared. Will this prevent the Wangxun
initialization block from running on the next poll and leave the link
stuck?

> +
> +	if (xpcs->info.pma != WX_TXGBE_XPCS_PMA_10G_ID) {
> +		/* If the link down, restart Auto-Negotiation */
> +		if (state->an_complete)
> +			xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				    BMCR_ANRESTART);
> +
> +		return 0;
> +	}
> +
> +	if (ret == DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
>  		int speed, duplex;
>  
>  		state->link = true;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=6