Re: [PATCH net-next v11 2/6] dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: pcs: unconditional AN restart quirk may break standard DW SGMII PHYs
- [Medium] net: pcs: xpcs: interrupt storm in SGMII mode due to uncleared AN status
--

commit 9bbe537f2904ae4cf66c0b099833bc944c6e3043
Author: Coia Prant <coiaprant@gmail.com>

dt-bindings: net: pcs: add rockchip,rk3568-xpcs support

Add device tree binding documentation for the Synopsys DesignWare
XPCS integrated on the Rockchip RK3568 SoC.

> 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
[ ... ]
> @@ -816,9 +825,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]
Will this unconditional AN restart quirk break standard DW SGMII PHYs?

According to the DW databook, an AN restart is not needed for MAC-side SGMII.
While explicitly documented as a workaround for a timing quirk in
Rockchip RK3568, applying it unconditionally to all compliant SGMII deployments
(e.g., NXP SJA1105) could trigger link flapping or state machine violations by
continually interrupting a link that is transitioning to an 'up' state.

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

[Severity: Medium]
Can this cause an interrupt storm in SGMII mode due to uncleared AN status?

In xpcs_get_state_c37_sgmii(), when the link successfully comes up,
DW_VR_MII_C37_ANSGM_SP_LNKSTS is true and the function returns 0 early:

drivers/net/pcs/pcs-xpcs.c:xpcs_get_state_c37_sgmii() {
    ...
	if (ret & DW_VR_MII_C37_ANSGM_SP_LNKSTS) {
		...
		state->link = true;
		...
		return 0;
	}
    ...
}

By returning early, the function bypasses this block that explicitly clears
DW_VR_MII_AN_INTR_STS. Since the register is not read-to-clear, the
ANCMPLT_INTR bit remains set.

If a driver operates with xpcs->pcs.poll = false (like Wangxun TXGBE) and the
interrupt is enabled, will the un-cleared status cause the MAC interrupt
handler to loop indefinitely?

> +		/* Initiate the next round of AN */
> +		ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR, BMCR_ANRESTART,
> +				  BMCR_ANRESTART);

[Severity: Medium]
Does this unconditional AN restart quirk break standard DW SGMII PHYs?

Similar to xpcs_config_aneg_c37_sgmii(), this applies an explicit
Auto-Negotiation (AN) restart via BMCR_ANRESTART unconditionally when AN
completes.

Since this workaround is for Rockchip RK3568, executing it on any non-Rockchip
DW SGMII interface when AN finishes could interrupt a link that is
transitioning to an 'up' state.

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

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