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

["Rafał Miłecki" <zajec5@gmail.com>]

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;