Date:   Sat, 14 May 2022 02:36:39 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vladimir Oltean <olteanv@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Saravana Kannan <saravanak@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        John Stultz <jstultz@...gle.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: [RFC PATCH net 1/2] net: phylink: allow PHY driver to defer probe when connecting via OF node

phylink_fwnode_phy_connect() relies on fwnode_phy_find_device(), which
does not have insight into why a PHY is missing, it just returns NULL,
case in which the connect returns -ENODEV. So there seems to be no
handling of probe deferral for the MDIO bus driver, PHY driver, etc,
there is an implicit assumption that these have all been bound before
the phy_connect() call. This is probably why so many drivers do the PHY
connect operation from ndo_open() rather than at boot time, to maximize
the chances of the PHY driver having been probed.

Normally fw_devlink enforces automatic device links based on OF
phandles, and this simply blocks the consumer (Ethernet controller, user
of phylink) driver from probing, rather than letting it probe just to
get an -EPROBE_DEFER when the supplier (PHY driver) is unavailable).

However support for "phy-handle" is sporadic, most recently having been
removed in commit 3782326577d4 ("Revert "of: property: fw_devlink: Add
support for "phy-handle" property"").

So practically, rather than relying on fw_devlink, it is desirable for
callers of phylink_fwnode_phy_connect() to consider that PHY drivers
might defer probing, and apply backpressure on their own probe if they
can.

The approach we take is to use driver_deferred_probe_check_state(),
which basically decides whether to defer probe based on whether it is
still possible for a driver to become available. If not, we return the
same old -ENODEV as before (a return value some callers have come to
expect).

As a consequence of this change, during boot (up to the late_initcall
stage), we will wait slightly longer for a PHY that is truly missing
before declaring it absent.

Before, a phylink user that called phy_connect() from probe was worse
off than one who did so from ndo_open(). But with the change, it is
actually better, because PHY probe deferral during the nfsroot case is
better treated with connect-at-probe than with connect-at-open.

Russell King points out that -EPROBE_DEFER is not exported to user
space, and drivers who connect at probe may propagate the phylink
phy_connect() call to user space. So we need to create a new entry point
into phylink, used only by drivers which guarantee they do not propagate
this error to user space, only use it for backpressure purposes.

It's hard to put the blame on a commit since this functionality never
worked, but there is also a sense of urgency to backport this change to
stable kernels. The reason being new DT compatibility with old kernels.
A PHY may have a missing supplier (IRQ parent) described in DT which has
no driver on an old kernel. fwnode_mdiobus_phy_device_register() handles
this to the extent that the PHY defers probe until the late_initcall
stage, then it gives up and falls back to poll mode. We need to wait for
that PHY driver to fall back, and not return -ENODEV early while it is
still waiting for its IRQ to become available (which never will).
So the blamed commit chosen is the oldest one where this change will
apply.

Fixes: 25396f680dd6 ("net: phylink: introduce phylink_fwnode_phy_connect()")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/phylink.c | 73 ++++++++++++++++++++++++++++++---------
 include/linux/phylink.h   |  2 ++
 2 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 066684b80919..889c0efded51 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1495,20 +1495,9 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 }
 EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
 
-/**
- * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
- * @pl: a pointer to a &struct phylink returned from phylink_create()
- * @fwnode: a pointer to a &struct fwnode_handle.
- * @flags: PHY-specific flags to communicate to the PHY device driver
- *
- * Connect the phy specified @fwnode to the phylink instance specified
- * by @pl.
- *
- * Returns 0 on success or a negative errno.
- */
-int phylink_fwnode_phy_connect(struct phylink *pl,
-			       struct fwnode_handle *fwnode,
-			       u32 flags)
+static int __phylink_fwnode_phy_connect(struct phylink *pl,
+					struct fwnode_handle *fwnode,
+					u32 flags, bool probe)
 {
 	struct fwnode_handle *phy_fwnode;
 	struct phy_device *phy_dev;
@@ -1530,8 +1519,21 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	phy_dev = fwnode_phy_find_device(phy_fwnode);
 	/* We're done with the phy_node handle */
 	fwnode_handle_put(phy_fwnode);
-	if (!phy_dev)
-		return -ENODEV;
+	if (!phy_dev) {
+		/* Drivers that connect to the PHY from ndo_open do not support
+		 * waiting for the PHY to defer probe now, because doing so
+		 * would propagate -EPROBE_DEFER to user space, which is an
+		 * error code the kernel does not export.
+		 */
+		if (!probe)
+			return -ENODEV;
+
+		/* Allow the PHY driver to defer probing, and return -ENODEV if
+		 * it times out or if we know it will never become available.
+		 */
+		ret = driver_deferred_probe_check_state(pl->dev);
+		return ret == -EPROBE_DEFER ? ret : -ENODEV;
+	}
 
 	/* Use PHY device/driver interface */
 	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
@@ -1552,6 +1554,45 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 
 	return ret;
 }
+
+/**
+ * phylink_of_phy_connect_probe() - connect the PHY in the DT node during probe.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @dn: a pointer to a &struct device_node.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Performs the same action as %phylink_of_phy_connect(), but allows the PHY
+ * driver to defer probing, and propagates -EPROBE_DEFER to the caller.
+ * This function cannot be called from contexts that propagate the return code
+ * to user space.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_of_phy_connect_probe(struct phylink *pl, struct device_node *dn,
+				 u32 flags)
+{
+	return __phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags,
+					    true);
+}
+EXPORT_SYMBOL_GPL(phylink_of_phy_connect_probe);
+
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags)
+{
+	return __phylink_fwnode_phy_connect(pl, fwnode, flags, false);
+}
 EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
 
 /**
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 6d06896fc20d..f38f8100c41d 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -531,6 +531,8 @@ void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
+int phylink_of_phy_connect_probe(struct phylink *pl, struct device_node *dn,
+				 u32 flags);
 int phylink_fwnode_phy_connect(struct phylink *pl,
 			       struct fwnode_handle *fwnode,
 			       u32 flags);
-- 
2.25.1

Powered by blists - more mailing lists