netdev - [RFC PATCH net] net: phy: allow MDIO bus PM ops to start/stop state machine for phylink-controlled PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20250225153156.3589072-1-vladimir.oltean@nxp.com>
Date: Tue, 25 Feb 2025 17:31:56 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Wei Fang <wei.fang@....com>
Subject: [RFC PATCH net] net: phy: allow MDIO bus PM ops to start/stop state machine for phylink-controlled PHY

DSA has 2 kinds of drivers:

1. Those who call dsa_switch_suspend() and dsa_switch_resume() from
   their device PM ops: qca8k-8xxx, bcm_sf2, microchip ksz
2. Those who don't: all others. The above methods should be optional.

For type 1, dsa_switch_suspend() calls dsa_user_suspend() -> phylink_stop(),
and dsa_switch_resume() calls dsa_user_resume() -> phylink_start().
These seem good candidates for setting mac_managed_pm = true because
that is essentially its definition, but that does not seem to be the
biggest problem for now, and is not what this change focuses on.

Talking strictly about the 2nd category of drivers here, I have noticed
that these also trigger the

	WARN_ON(phydev->state != PHY_HALTED && phydev->state != PHY_READY &&
		phydev->state != PHY_UP);

from mdio_bus_phy_resume(), because the PHY state machine is running.

It's running as a result of a previous dsa_user_open() -> ... ->
phylink_start() -> phy_start(), and AFAICS, mdio_bus_phy_suspend() was
supposed to have called phy_stop_machine(), but it didn't. So this is
why the PHY is in state PHY_NOLINK by the time mdio_bus_phy_resume()
runs.

mdio_bus_phy_suspend() did not call phy_stop_machine() because for
phylink, the phydev->adjust_link function pointer is NULL. This seems a
technicality introduced by commit fddd91016d16 ("phylib: fix PAL state
machine restart on resume"). That commit was written before phylink
existed, and was intended to avoid crashing with consumer drivers which
don't use the PHY state machine - phylink does.

Make the conditions dependent on the PHY device having a
phydev->phy_link_change() implementation equal to the default
phy_link_change() provided by phylib. Otherwise, just check that the
custom phydev->phy_link_change() has been provided and is non-NULL.
Phylink provides phylink_phy_change().

Thus, we will stop the state machine even for phylink-controlled PHYs
when using the MDIO bus PM ops.

Reported-by: Wei Fang <wei.fang@....com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
I've only spent a few hours debugging this, and I'm unsure which patch
to even blame. I haven't noticed other issues apart from the WARN_ON()
originally added by commit 744d23c71af3 ("net: phy: Warn about incorrect
mdio_bus_phy_resume() state").

 drivers/net/phy/phy_device.c | 38 ++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7c4e1ad1864c..e2996fe8c498 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -241,6 +241,27 @@ static bool phy_drv_wol_enabled(struct phy_device *phydev)
 	return wol.wolopts != 0;
 }

+static void phy_link_change(struct phy_device *phydev, bool up)
+{
+	struct net_device *netdev = phydev->attached_dev;
+
+	if (up)
+		netif_carrier_on(netdev);
+	else
+		netif_carrier_off(netdev);
+	phydev->adjust_link(netdev);
+	if (phydev->mii_ts && phydev->mii_ts->link_state)
+		phydev->mii_ts->link_state(phydev->mii_ts, phydev);
+}
+
+static bool phy_has_attached_dev(struct phy_device *phydev)
+{
+	if (phydev->phy_link_change == phy_link_change)
+		return phydev->attached_dev && phydev->adjust_link;
+
+	return phydev->phy_link_change;
+}
+
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 {
 	struct device_driver *drv = phydev->mdio.dev.driver;
@@ -307,7 +328,7 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	 * may call phy routines that try to grab the same lock, and that may
 	 * lead to a deadlock.
 	 */
-	if (phydev->attached_dev && phydev->adjust_link)
+	if (phy_has_attached_dev(phydev))
 		phy_stop_machine(phydev);

 	if (!mdio_bus_phy_may_suspend(phydev))
@@ -361,7 +382,7 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
 		}
 	}

-	if (phydev->attached_dev && phydev->adjust_link)
+	if (phy_has_attached_dev(phydev))
 		phy_start_machine(phydev);

 	return 0;
@@ -1052,19 +1073,6 @@ struct phy_device *phy_find_first(struct mii_bus *bus)
 }
 EXPORT_SYMBOL(phy_find_first);

-static void phy_link_change(struct phy_device *phydev, bool up)
-{
-	struct net_device *netdev = phydev->attached_dev;
-
-	if (up)
-		netif_carrier_on(netdev);
-	else
-		netif_carrier_off(netdev);
-	phydev->adjust_link(netdev);
-	if (phydev->mii_ts && phydev->mii_ts->link_state)
-		phydev->mii_ts->link_state(phydev->mii_ts, phydev);
-}
-
 /**
  * phy_prepare_link - prepares the PHY layer to monitor link status
  * @phydev: target phy_device struct
-- 
2.43.0