Re: [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery - sashiko-bot

From: sashiko-bot@kernel.org
To: "Coia Prant" <coiaprant@gmail.com>
Cc: neil.armstrong@linaro.org, olteanv@gmail.com, vkoul@kernel.org,
	conor+dt@kernel.org, linux-phy@lists.infradead.org,
	wsa+renesas@sang-engineering.com, robh@kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH net-next v5 06/11] net: pcs: xpcs: add ANRESTART support for SGMII link recovery
Date: Mon, 14 Sep 2026 19:44:21 +0000	[thread overview]
Message-ID: <20260914194421.944211F00893@smtp.kernel.org> (raw)
In-Reply-To: <20260913185849.907479-7-coiaprant@gmail.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditionally applying the RK3568 SGMII AN restart quirk causes an infinite interrupt storm or workqueue loop on standard interrupt-driven DW XPCS configurations.
--

commit 78eb5488d0c0d3fe9f609e56e59710ced52856fa
Author: Coia Prant <coiaprant@gmail.com>

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

This patch adds BMCR_ANRESTART to DW XPCS when configuring auto-negotiation
and when the link goes down. This is intended to resolve an issue on some
platforms (like RK3568) where the PCS does not automatically restart
auto-negotiation when the link goes down and comes back up.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 0337e2bcc0125..8c3875b6985b9 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
[ ... ]
> @@ -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);

[Severity: Medium]
Could unconditionally applying this AN restart quirk cause an infinite
interrupt storm or MDIO polling loop on standard DW XPCS configurations?

If a cable is disconnected on an SGMII port, the PHY may continuously transmit
the 'Link Down' AN configuration word. When the MAC completes AN and generates
an interrupt, xpcs_get_state_c37_sgmii() is called. It clears the interrupt
and unconditionally sets BMCR_ANRESTART.

Will this cause the hardware to reset its AN state machine, immediately
complete AN again with the PHY's continuous 'Link Down' word, and fire another
interrupt, creating an endless loop?

Should this behavior be isolated to only the platforms that require the quirk
(like RK3568), rather than applied to all hardware using the DesignWare XPCS IP?

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

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy