netdev - Re: Race in PHY subsystem? Attaching to PHY devices before they get probed

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 for Android: free password hash cracker in your pocket

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

Message-ID: <05f333aa-5457-409a-8f53-148b9f4d0da9@gmail.com>
Date: Mon, 22 Jan 2024 10:48:40 +0100
From: Rafał Miłecki <zajec5@...il.com>
To: Network Development <netdev@...r.kernel.org>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
 Russell King <linux@...linux.org.uk>, Robert Marko <robimarko@...il.com>,
 Ansuel Smith <ansuelsmth@...il.com>, Daniel Golle <daniel@...rotopia.org>
Subject: Re: Race in PHY subsystem? Attaching to PHY devices before they get
 probed

On 22.01.2024 08:09, Rafał Miłecki wrote:
> I have MT7988 SoC board with following problem:
> [   26.887979] Aquantia AQR113C mdio-bus:08: aqr107_wait_reset_complete failed: -110
> 
> This issue is known to occur when PHY's firmware is not running. After
> some debugging I discovered that .config_init() CB gets called while
> .probe() CB is still being executed.
> 
> It turns out mtk_soc_eth.c calls phylink_of_phy_connect() before my PHY
> gets fully probed and it seems there is nothing in PHY subsystem
> verifying that. Please note this PHY takes quite some time to probe as
> it involves sending firmware to hardware.
> 
> Is that a possible race in PHY subsystem?
> Should we have phy_attach_direct() wait for PHY to be fully probed?

I don't expect this to be an acceptable solution but it works as a quick
workaround & proof of issue.

[   24.763875] mtk_soc_eth 15100000.ethernet eth2: Waiting for PHY mdio-bus:08 to become ready
[   38.645874] mtk_soc_eth 15100000.ethernet eth2: PHY mdio-bus:08 is ready now

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..cdb766b0ea22 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1435,8 +1435,21 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
  	struct device *d = &phydev->mdio.dev;
  	struct module *ndev_owner = NULL;
  	bool using_genphy = false;
+	unsigned long time_left;
  	int err;

+	if (!try_wait_for_completion(&phydev->probed)) {
+		netdev_info(dev, "Waiting for PHY %s to become ready\n", phydev_name(phydev));
+
+		time_left = wait_for_completion_timeout(&phydev->probed, msecs_to_jiffies(20000));
+		if (!time_left) {
+			netdev_warn(dev, "PHY %s is still not ready!\n", phydev_name(phydev));
+			return -EPROBE_DEFER;
+		}
+
+		netdev_info(dev, "PHY %s is ready now\n", phydev_name(phydev));
+	}
+
  	/* For Ethernet device drivers that register their own MDIO bus, we
  	 * will have bus->owner match ndev_mod, so we do not want to increment
  	 * our own module->refcnt here, otherwise we would not be able to
@@ -1562,6 +1575,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
  		phydev->devlink = device_link_add(dev->dev.parent, &phydev->mdio.dev,
  						  DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);

+	complete(&phydev->probed);
+
  	return err;

  error:
@@ -3382,6 +3397,9 @@ static int phy_probe(struct device *dev)
  	if (err)
  		phy_device_reset(phydev, 1);

+	if (!err)
+		complete(&phydev->probed);
+
  	return err;
  }

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 684efaeca07c..d95b68dfad59 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -541,6 +541,7 @@ struct macsec_ops;
   * struct phy_device - An instance of a PHY
   *
   * @mdio: MDIO bus this PHY is on
+ * @probed: Completion of PHY probing
   * @drv: Pointer to the driver for this PHY instance
   * @devlink: Create a link between phy dev and mac dev, if the external phy
   *           used by current mac interface is managed by another mac interface.
@@ -636,6 +637,8 @@ struct macsec_ops;
  struct phy_device {
  	struct mdio_device mdio;

+	struct completion probed;
+
  	/* Information about the PHY type */
  	/* And management functions */
  	struct phy_driver *drv;