[PATCH net v1 1/1] net: phy: microchip: Reset LAN88xx PHY to ensure clean link state on LAN7800/7850

[PATCH net v1 1/1] net: phy: microchip: Reset LAN88xx PHY to ensure clean link state on LAN7800/7850

[Oleksij Rempel]

Fix outdated MII_LPA data in the LAN88xx PHY, which is used in LAN7800
and LAN7850 USB Ethernet controllers. Due to a hardware limitation, the
PHY cannot reliably update link status after parallel detection when the
link partner does not support auto-negotiation. To mitigate this, add a
PHY reset in `lan88xx_link_change_notify()` when `phydev->state` is
`PHY_NOLINK`, ensuring the PHY starts in a clean state and reports
accurate fixed link parallel detection results.

Fixes: 792aec47d59d9 ("add microchip LAN88xx phy driver")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/microchip.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
index d3273bc0da4a..3c8bc87da70e 100644
--- a/drivers/net/phy/microchip.c
+++ b/drivers/net/phy/microchip.c
@@ -351,6 +351,21 @@ static int lan88xx_config_aneg(struct phy_device *phydev)
 static void lan88xx_link_change_notify(struct phy_device *phydev)
 {
 	int temp;
+	int ret;
+
+	/* Reset PHY, otherwise MII_LPA will provide outdated information.
+	 * This issue is reproducible only with after parallel detection
+	 * where link partner do not supports auto-negotiation.
+	 */
+	if (phydev->state == PHY_NOLINK) {
+		ret = phy_init_hw(phydev);
+		if (ret < 0)
+			goto link_change_notify_failed;
+
+		ret = _phy_start_aneg(phydev);
+		if (ret < 0)
+			goto link_change_notify_failed;
+	}

 	/* At forced 100 F/H mode, chip may fail to set mode correctly
 	 * when cable is switched between long(~50+m) and short one.
@@ -377,6 +392,11 @@ static void lan88xx_link_change_notify(struct phy_device *phydev)
 		temp |= LAN88XX_INT_MASK_MDINTPIN_EN_;
 		phy_write(phydev, LAN88XX_INT_MASK, temp);
 	}
+
+	return;
+
+link_change_notify_failed:
+	phydev_err(phydev, "Link change process failed %pe\n", ERR_PTR(ret));
 }

 /**
--
2.39.5

Re: [PATCH net v1 1/1] net: phy: microchip: Reset LAN88xx PHY to ensure clean link state on LAN7800/7850

[Maxime Chevallier]

Hi Oleksij,

On Sun, 17 Nov 2024 11:21:47 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Fix outdated MII_LPA data in the LAN88xx PHY, which is used in LAN7800
> and LAN7850 USB Ethernet controllers. Due to a hardware limitation, the
> PHY cannot reliably update link status after parallel detection when the
> link partner does not support auto-negotiation. To mitigate this, add a
> PHY reset in `lan88xx_link_change_notify()` when `phydev->state` is
> `PHY_NOLINK`, ensuring the PHY starts in a clean state and reports
> accurate fixed link parallel detection results.
> 
> Fixes: 792aec47d59d9 ("add microchip LAN88xx phy driver")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

This looks like the issue in the Asix AX88772A, but your patch has
better error handling :)

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net v1 1/1] net: phy: microchip: Reset LAN88xx PHY to ensure clean link state on LAN7800/7850

[Oleksij Rempel]

On Mon, Nov 18, 2024 at 05:48:49PM +0100, Maxime Chevallier wrote:
> Hi Oleksij,
> 
> On Sun, 17 Nov 2024 11:21:47 +0100
> Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > Fix outdated MII_LPA data in the LAN88xx PHY, which is used in LAN7800
> > and LAN7850 USB Ethernet controllers. Due to a hardware limitation, the
> > PHY cannot reliably update link status after parallel detection when the
> > link partner does not support auto-negotiation. To mitigate this, add a
> > PHY reset in `lan88xx_link_change_notify()` when `phydev->state` is
> > `PHY_NOLINK`, ensuring the PHY starts in a clean state and reports
> > accurate fixed link parallel detection results.
> > 
> > Fixes: 792aec47d59d9 ("add microchip LAN88xx phy driver")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> This looks like the issue in the Asix AX88772A, but your patch has
> better error handling :)

It was my code in Asix. Need to add proper error handling too. It is not
nice if device is detached but kernel is continuing executing things and
printing a lot of errors.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net v1 1/1] net: phy: microchip: Reset LAN88xx PHY to ensure clean link state on LAN7800/7850

[Jakub Kicinski]

On Sun, 17 Nov 2024 11:21:47 +0100 Oleksij Rempel wrote:
> +	/* Reset PHY, otherwise MII_LPA will provide outdated information.
> +	 * This issue is reproducible only with after parallel detection
> +	 * where link partner do not supports auto-negotiation.

sorry for late nit, the comment needs a bit better grammar

	 * This issue is reproducible only with after parallel detection
	                                   ^^^^^^^^^^ ?
	 * where link partner do not supports auto-negotiation.
                              ^^^^^^^^^^^^^^^
	                      does not support ?
-- 
pw-bot: cr