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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] State getter `xpcs_get_state_c37_sgmii` mutates hardware state (restarting autonegotiation) and applies a hardware-specific workaround universally to all platforms.
--

commit 71d0b44c6ef52210f70310761abecf6962cd887a
Author: Coia Prant <coiaprant@gmail.com>

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

This commit introduces support for restarting autonegotiation on the DesignWare
XPCS IP to handle link recovery on platforms like the RK3568 MAC side SGMII.

> 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
[ ... ]
> @@ -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);
> +	}
> +

[Severity: High]
Does this mutation inside xpcs_get_state_c37_sgmii violate the phylink API
semantics, which typically require state getters to be side-effect free?

Because the phylink_resolve workqueue polls the state periodically when the
link is down, won't this cause the getter to constantly restart the
autonegotiation state machine and generate continuous MDIO traffic?

For instance, when the SGMII PHY drops the copper link, it sends an SGMII
config word with link=0. The MAC completes autonegotiation based on this word,
meaning state->an_complete is true but state->link is false, which triggers
this exact path.

Additionally, does this apply an RK3568-specific workaround unconditionally to
all Generic DW XPCS controllers? Standard MAC-side SGMII automatically handles
link state updates via the SGMII config word without needing manual AN
restarts. Could this override that conformant behavior on other platforms?

>  	return 0;
>  }
>

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