[PATCH net v2] amd-xgbe: do not double read link status

[PATCH net v2] amd-xgbe: do not double read link status

[Raju Rangoju]

The link status is latched low so that momentary link drops
can be detected. Avoid double-reading the status to identify
short link interruptions, unless the link was already down.

This prevents unnecessary duplicate readings of the link status.

Fixes: 4f3b20bfbb75 ("amd-xgbe: add support for rx-adaptation")
Signed-off-by: Raju Rangoju <Raju.Rangoju@amd.com>
---
- skip double-read to detect short link drops
- refine the subject line for clarity

 drivers/net/ethernet/amd/xgbe/xgbe-mdio.c   |  4 +++
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c | 27 +++++++++++++--------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
index 71449edbb76d..f8a75532b11c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-mdio.c
@@ -1295,6 +1295,10 @@ static void xgbe_phy_status(struct xgbe_prv_data *pdata)
 
 	pdata->phy.link = pdata->phy_if.phy_impl.link_status(pdata,
 							     &an_restart);
+	/* bail out if the link status register read fails */
+	if (pdata->phy.link < 0)
+		return;
+
 	if (an_restart) {
 		xgbe_phy_config_aneg(pdata);
 		goto adjust_link;
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 7a4dfa4e19c7..94125197738c 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -2746,8 +2746,7 @@ static bool xgbe_phy_valid_speed(struct xgbe_prv_data *pdata, int speed)
 static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
 {
 	struct xgbe_phy_data *phy_data = pdata->phy_data;
-	unsigned int reg;
-	int ret;
+	int reg, ret;
 
 	*an_restart = 0;
 
@@ -2780,13 +2779,23 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
 		if (!phy_data->phydev->link)
 			return 0;
 	}
-
-	/* Link status is latched low, so read once to clear
-	 * and then read again to get current state
+	/* Link status is latched low so that momentary link drops
+	 * can be detected. Do not double-read the status to detect
+	 * short link drops except the link was already down.
 	 */
-	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
-	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
+	if (!pdata->phy.link) {
+		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
+		if (reg < 0)
+			return reg;
+
+		if (reg & MDIO_STAT1_LSTATUS)
+			goto skip_read;
+	}
 
+	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
+	if (reg < 0)
+		return reg;
+skip_read:
 	if (pdata->en_rx_adap) {
 		/* if the link is available and adaptation is done,
 		 * declare link up
@@ -2804,9 +2813,7 @@ static int xgbe_phy_link_status(struct xgbe_prv_data *pdata, int *an_restart)
 			xgbe_phy_set_mode(pdata, phy_data->cur_mode);
 		}
 
-		/* check again for the link and adaptation status */
-		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
-		if ((reg & MDIO_STAT1_LSTATUS) && pdata->rx_adapt_done)
+		if (pdata->rx_adapt_done)
 			return 1;
 	} else if (reg & MDIO_STAT1_LSTATUS)
 		return 1;
-- 
2.34.1

Re: [PATCH net v2] amd-xgbe: do not double read link status

[Jakub Kicinski]

On Wed, 25 Jun 2025 15:23:15 +0530 Raju Rangoju wrote:
> The link status is latched low so that momentary link drops
> can be detected. Avoid double-reading the status to identify
> short link interruptions, unless the link was already down.

Took me a minute to understand this. How about:

The link status is latched low so that momentary link drops
can be detected. Always double-reading the status defeats this
design feature. Only double read if link was already down.

> -	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> -	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> +	if (!pdata->phy.link) {
> +		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> +		if (reg < 0)
> +			return reg;
> +
> +		if (reg & MDIO_STAT1_LSTATUS)
> +			goto skip_read;
> +	}
>  
> +	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> +	if (reg < 0)
> +		return reg;
> +skip_read:
>  	if (pdata->en_rx_adap) {
>  		/* if the link is available and adaptation is done,
>  		 * declare link up

Don't use gotos for normal function control flow :/

	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
	if (reg < 0)
		return reg;
	/* Link status is latched low so that momentary link drops
	 * can be detected. If link was already down read again
	 * to get the latest state.
 	 */
	if (!pdata->phy.link && !(reg & MDIO_STAT1_LSTATUS)) {
 		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
		if (reg < 0)
			return reg;
	}

	if (pdata->en_rx_adap) {
 		/* if the link is available and adaptation is done,
-- 
pw-bot: cr

Re: [PATCH net v2] amd-xgbe: do not double read link status

[Rangoju, Raju]

On 7/1/2025 6:42 AM, Jakub Kicinski wrote:
> On Wed, 25 Jun 2025 15:23:15 +0530 Raju Rangoju wrote:
>> The link status is latched low so that momentary link drops
>> can be detected. Avoid double-reading the status to identify
>> short link interruptions, unless the link was already down.
> 
> Took me a minute to understand this. How about:
> 
> The link status is latched low so that momentary link drops
> can be detected. Always double-reading the status defeats this
> design feature. Only double read if link was already down.

Sounds good, I'll update commit message accordingly.

> 
>> -	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
>> -	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
>> +	if (!pdata->phy.link) {
>> +		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
>> +		if (reg < 0)
>> +			return reg;
>> +
>> +		if (reg & MDIO_STAT1_LSTATUS)
>> +			goto skip_read;
>> +	}
>>   
>> +	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
>> +	if (reg < 0)
>> +		return reg;
>> +skip_read:
>>   	if (pdata->en_rx_adap) {
>>   		/* if the link is available and adaptation is done,
>>   		 * declare link up
> 
> Don't use gotos for normal function control flow :/

Sure. I'll re-spin v3 with the suggested changes.

> 
> 	reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> 	if (reg < 0)
> 		return reg;
> 	/* Link status is latched low so that momentary link drops
> 	 * can be detected. If link was already down read again
> 	 * to get the latest state.
>   	 */
> 	if (!pdata->phy.link && !(reg & MDIO_STAT1_LSTATUS)) {
>   		reg = XMDIO_READ(pdata, MDIO_MMD_PCS, MDIO_STAT1);
> 		if (reg < 0)
> 			return reg;
> 	}
> 
> 	if (pdata->en_rx_adap) {
>   		/* if the link is available and adaptation is done,