[RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Vladimir Oltean]

This patch set completes the picture described by
'[RFC,devicetree] of: property: mark "interrupts" as optional for fw_devlink'
https://patchwork.kernel.org/project/netdevbpf/patch/20220513201243.2381133-1-vladimir.oltean@nxp.com/

I've CCed non-networking maintainers just in case they want to gain a
better understanding. If not, apologies and please ignore the rest.



My use case is to migrate a PHY driver from poll mode to interrupt mode
without breaking compatibility between new device trees and old kernels
which did not have a driver for that IRQ parent, and therefore (for
things to work) did not even have that interrupt listed in the "vintage
correct" DT blobs. Note that current kernels as of today are also
"old kernels" in this description.

Creating some degree of compatibility has multiple components.

1. A PHY driver must eventually give up waiting for an IRQ provider,
   since the dependency is optional and it can fall back to poll mode.
   This is currently supported thanks to commit 74befa447e68 ("net:
   mdio: don't defer probe forever if PHY IRQ provider is missing").

2. Before it finally gives up, the PHY driver has a transient phase of
   returning -EPROBE_DEFER. That transient phase causes some breakage
   which is handled by this patch set, details below.

3. PHY device probing and Ethernet controller finding it and connecting
   to it are async events. When both happen during probing, the problem
   is that finding the PHY fails if the PHY defers probe, which results
   in a missing PHY rather than waiting for it. Unfortunately there is
   no universal way to address this problem, because the majority of
   Ethernet drivers do not connect to the PHY during probe. So the
   problem is fixed only for the driver that is of interest to me in
   this context, DSA, and with special API exported by phylink
   specifically for this purpose, to limit the impact on other drivers.

Note that drivers that connect to the PHY at ndo_open are superficially
"fixed" by the patch at step 1 alone, and therefore don't need the
mechanism introduced in phylink here. This is because of the larger span
of time between PHY probe and opening the network interface (typically
initiated by user space). But this is the catch, nfsroot and other
in-kernel networking users can also open the net device, and this will
still expose the EPROBE_DEFER as a hard error for this second kind of
drivers. I don't know how to fix that. From this POV, it's better to do
what DSA does (connect to the PHY on probe).

Vladimir Oltean (2):
  net: phylink: allow PHY driver to defer probe when connecting via OF
    node
  net: dsa: wait for PHY to defer probe

 drivers/net/phy/phylink.c | 73 ++++++++++++++++++++++++++++++---------
 include/linux/phylink.h   |  2 ++
 net/dsa/dsa2.c            |  2 ++
 net/dsa/port.c            |  6 ++--
 net/dsa/slave.c           | 10 +++---
 5 files changed, 70 insertions(+), 23 deletions(-)

-- 
2.25.1

[RFC PATCH net 1/2] net: phylink: allow PHY driver to defer probe when connecting via OF node

[Vladimir Oltean]

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@nxp.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

[RFC PATCH net 2/2] net: dsa: wait for PHY to defer probe

[Vladimir Oltean]

DSA is among the 3 drivers which call phylink.*phy_connect() during
probe time (vs 11 doing so during ndo_open). So there is no guarantee
that the PHY driver will have finished probing by the time we connect to
it.

Use the newly introduced phylink_of_phy_connect_probe() to wait for this
to happen, and propagate the error code all the way to dsa_register_switch(),
which switch drivers call from their own probe function.

Notably, in dsa_tree_setup_ports() we treat errors on slave interface
registration as "soft" and continue probing the ports that didn't fail.
This is useful on systems which have riser cards with PHYs, and some of
these cards can be missing. But this logic needs to be adapted, since
-EPROBE_DEFER is an error we want to propagate regardless.

Fixes: 25396f680dd6 ("net: phylink: introduce phylink_fwnode_phy_connect()")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c  |  2 ++
 net/dsa/port.c  |  6 ++++--
 net/dsa/slave.c | 10 +++++-----
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cf933225df32..3a2983a1a7dd 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1011,6 +1011,8 @@ static int dsa_tree_setup_ports(struct dsa_switch_tree *dst)
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_user(dp) || dsa_port_is_unused(dp)) {
 			err = dsa_port_setup(dp);
+			if (err == -EPROBE_DEFER)
+				goto teardown;
 			if (err) {
 				err = dsa_port_reinit_as_unused(dp);
 				if (err)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 075a8db536c6..8a2fc99ca0ad 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1628,9 +1628,11 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
 	if (err)
 		return err;
 
-	err = phylink_of_phy_connect(dp->pl, port_dn, 0);
+	err = phylink_of_phy_connect_probe(dp->pl, port_dn, 0);
 	if (err && err != -ENODEV) {
-		pr_err("could not attach to PHY: %d\n", err);
+		dev_err_probe(ds->dev, err,
+			      "DSA/CPU port %d could not attach to PHY: %pe\n",
+			      dp->index, ERR_PTR(err));
 		goto err_phy_connect;
 	}
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 5ee0aced9410..a5407e717c68 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2252,18 +2252,18 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)
 	if (ds->ops->get_phy_flags)
 		phy_flags = ds->ops->get_phy_flags(ds, dp->index);
 
-	ret = phylink_of_phy_connect(dp->pl, port_dn, phy_flags);
+	ret = phylink_of_phy_connect_probe(dp->pl, port_dn, phy_flags);
 	if (ret == -ENODEV && ds->slave_mii_bus) {
 		/* We could not connect to a designated PHY or SFP, so try to
 		 * use the switch internal MDIO bus instead
 		 */
 		ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags);
 	}
-	if (ret) {
+	if (ret && ret != -EPROBE_DEFER)
 		netdev_err(slave_dev, "failed to connect to PHY: %pe\n",
 			   ERR_PTR(ret));
+	if (ret)
 		phylink_destroy(dp->pl);
-	}
 
 	return ret;
 }
@@ -2386,12 +2386,12 @@ int dsa_slave_create(struct dsa_port *port)
 	netif_carrier_off(slave_dev);
 
 	ret = dsa_slave_phy_setup(slave_dev);
-	if (ret) {
+	if (ret && ret != -EPROBE_DEFER)
 		netdev_err(slave_dev,
 			   "error %d setting up PHY for tree %d, switch %d, port %d\n",
 			   ret, ds->dst->index, ds->index, port->index);
+	if (ret)
 		goto out_gcells;
-	}
 
 	rtnl_lock();
 
-- 
2.25.1

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Andrew Lunn]

On Sat, May 14, 2022 at 02:36:38AM +0300, Vladimir Oltean wrote:
> This patch set completes the picture described by
> '[RFC,devicetree] of: property: mark "interrupts" as optional for fw_devlink'
> https://patchwork.kernel.org/project/netdevbpf/patch/20220513201243.2381133-1-vladimir.oltean@nxp.com/
> 
> I've CCed non-networking maintainers just in case they want to gain a
> better understanding. If not, apologies and please ignore the rest.
> 
> 
> 
> My use case is to migrate a PHY driver from poll mode to interrupt mode
> without breaking compatibility between new device trees and old kernels
> which did not have a driver for that IRQ parent, and therefore (for
> things to work) did not even have that interrupt listed in the "vintage
> correct" DT blobs. Note that current kernels as of today are also
> "old kernels" in this description.
> 
> Creating some degree of compatibility has multiple components.
> 
> 1. A PHY driver must eventually give up waiting for an IRQ provider,
>    since the dependency is optional and it can fall back to poll mode.
>    This is currently supported thanks to commit 74befa447e68 ("net:
>    mdio: don't defer probe forever if PHY IRQ provider is missing").
> 
> 2. Before it finally gives up, the PHY driver has a transient phase of
>    returning -EPROBE_DEFER. That transient phase causes some breakage
>    which is handled by this patch set, details below.
> 
> 3. PHY device probing and Ethernet controller finding it and connecting
>    to it are async events. When both happen during probing, the problem
>    is that finding the PHY fails if the PHY defers probe, which results
>    in a missing PHY rather than waiting for it. Unfortunately there is
>    no universal way to address this problem, because the majority of
>    Ethernet drivers do not connect to the PHY during probe. So the
>    problem is fixed only for the driver that is of interest to me in
>    this context, DSA, and with special API exported by phylink
>    specifically for this purpose, to limit the impact on other drivers.

There is a very different approach, which might be simpler.

We know polling will always work. And it should be possible to
transition between polling and interrupt at any point, so long as the
phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
state in phydev that there should be an irq, but it is not around yet.
When the phy is started, and phylib starts polling, look for the state
and try getting the IRQ again. If successful, swap to interrupts, if
not, keep polling. Maybe after 60 seconds of polling and trying, give
up trying to find the irq and stick with polling.

     Andrew

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Saravana Kannan]

On Fri, May 13, 2022 at 4:37 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> This patch set completes the picture described by
> '[RFC,devicetree] of: property: mark "interrupts" as optional for fw_devlink'
> https://patchwork.kernel.org/project/netdevbpf/patch/20220513201243.2381133-1-vladimir.oltean@nxp.com/

I replied to that patch. I don't think we can pull that in.

> I've CCed non-networking maintainers just in case they want to gain a
> better understanding. If not, apologies and please ignore the rest.
>
>
>
> My use case is to migrate a PHY driver from poll mode to interrupt mode
> without breaking compatibility between new device trees and old kernels
> which did not have a driver for that IRQ parent, and therefore (for
> things to work) did not even have that interrupt listed in the "vintage
> correct" DT blobs. Note that current kernels as of today are also
> "old kernels" in this description.
>
> Creating some degree of compatibility has multiple components.
>
> 1. A PHY driver must eventually give up waiting for an IRQ provider,
>    since the dependency is optional and it can fall back to poll mode.
>    This is currently supported thanks to commit 74befa447e68 ("net:
>    mdio: don't defer probe forever if PHY IRQ provider is missing").
>
> 2. Before it finally gives up, the PHY driver has a transient phase of
>    returning -EPROBE_DEFER. That transient phase causes some breakage
>    which is handled by this patch set, details below.
>
> 3. PHY device probing and Ethernet controller finding it and connecting
>    to it are async events. When both happen during probing, the problem
>    is that finding the PHY fails if the PHY defers probe, which results
>    in a missing PHY rather than waiting for it. Unfortunately there is
>    no universal way to address this problem, because the majority of
>    Ethernet drivers do not connect to the PHY during probe. So the
>    problem is fixed only for the driver that is of interest to me in
>    this context, DSA, and with special API exported by phylink
>    specifically for this purpose, to limit the impact on other drivers.

I'll take a closer look at this later this week, but once we add
phy-handle support to fw_devlink (the device_bind_driver() is making
it hard to add support), I think we can address most/all of these
problems automatically. So hopefully we can work towards that?
Actually this patch might already fix this for you:
https://lore.kernel.org/lkml/20220429220933.1350374-1-saravanak@google.com/

Before fw_devlink, we'd give up on waiting on all suppliers, whether
they had a driver (but hadn't yet probed for a multitude of reasons)
or not. fw_devlink is smart about allowing consumers to probe without
their suppliers only if the supplier has no driver or the driver fails
(I'll send a patch for this). The deferred_probe_timeout is what's
used to decide when to give up waiting for drivers.

-Saravana

>
> Note that drivers that connect to the PHY at ndo_open are superficially
> "fixed" by the patch at step 1 alone, and therefore don't need the
> mechanism introduced in phylink here. This is because of the larger span
> of time between PHY probe and opening the network interface (typically
> initiated by user space). But this is the catch, nfsroot and other
> in-kernel networking users can also open the net device, and this will
> still expose the EPROBE_DEFER as a hard error for this second kind of
> drivers. I don't know how to fix that. From this POV, it's better to do
> what DSA does (connect to the PHY on probe).
>
> Vladimir Oltean (2):
>   net: phylink: allow PHY driver to defer probe when connecting via OF
>     node
>   net: dsa: wait for PHY to defer probe
>
>  drivers/net/phy/phylink.c | 73 ++++++++++++++++++++++++++++++---------
>  include/linux/phylink.h   |  2 ++
>  net/dsa/dsa2.c            |  2 ++
>  net/dsa/port.c            |  6 ++--
>  net/dsa/slave.c           | 10 +++---
>  5 files changed, 70 insertions(+), 23 deletions(-)
>
> --
> 2.25.1
>

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Vladimir Oltean]

Hi Andrew,

On Sat, May 14, 2022 at 02:23:51AM +0200, Andrew Lunn wrote:
> On Sat, May 14, 2022 at 02:36:38AM +0300, Vladimir Oltean wrote:
> > This patch set completes the picture described by
> > '[RFC,devicetree] of: property: mark "interrupts" as optional for fw_devlink'
> > https://patchwork.kernel.org/project/netdevbpf/patch/20220513201243.2381133-1-vladimir.oltean@nxp.com/
> > 
> > I've CCed non-networking maintainers just in case they want to gain a
> > better understanding. If not, apologies and please ignore the rest.
> > 
> > My use case is to migrate a PHY driver from poll mode to interrupt mode
> > without breaking compatibility between new device trees and old kernels
> > which did not have a driver for that IRQ parent, and therefore (for
> > things to work) did not even have that interrupt listed in the "vintage
> > correct" DT blobs. Note that current kernels as of today are also
> > "old kernels" in this description.
> > 
> > Creating some degree of compatibility has multiple components.
> > 
> > 1. A PHY driver must eventually give up waiting for an IRQ provider,
> >    since the dependency is optional and it can fall back to poll mode.
> >    This is currently supported thanks to commit 74befa447e68 ("net:
> >    mdio: don't defer probe forever if PHY IRQ provider is missing").
> > 
> > 2. Before it finally gives up, the PHY driver has a transient phase of
> >    returning -EPROBE_DEFER. That transient phase causes some breakage
> >    which is handled by this patch set, details below.
> > 
> > 3. PHY device probing and Ethernet controller finding it and connecting
> >    to it are async events. When both happen during probing, the problem
> >    is that finding the PHY fails if the PHY defers probe, which results
> >    in a missing PHY rather than waiting for it. Unfortunately there is
> >    no universal way to address this problem, because the majority of
> >    Ethernet drivers do not connect to the PHY during probe. So the
> >    problem is fixed only for the driver that is of interest to me in
> >    this context, DSA, and with special API exported by phylink
> >    specifically for this purpose, to limit the impact on other drivers.
> 
> There is a very different approach, which might be simpler.
> 
> We know polling will always work. And it should be possible to
> transition between polling and interrupt at any point, so long as the
> phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> state in phydev that there should be an irq, but it is not around yet.
> When the phy is started, and phylib starts polling, look for the state
> and try getting the IRQ again. If successful, swap to interrupts, if
> not, keep polling. Maybe after 60 seconds of polling and trying, give
> up trying to find the irq and stick with polling.

That doesn't sound like something that I'd backport to stable kernels.
Letting the PHY driver dynamically switch from poll to IRQ mode risks
racing with phylink's workqueue, and generally speaking, phylink doesn't
seem to be built around the idea that "bool poll" can change after
phylink_start().

What motivates me to make these changes in the first place is the idea
that current kernels should work with updated device trees. If I won't
be able to achieve that, I see no point in adding logic to transition
from poll to IRQ mode even in net-next, since I'd have to update the
kernel when I update the DT, and by then, I'd have a proper driver for
the IRQ parent anyway. Sorry.

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Russell King (Oracle)]

On Thu, May 19, 2022 at 02:59:36PM +0000, Vladimir Oltean wrote:
> Hi Andrew,
> 
> On Sat, May 14, 2022 at 02:23:51AM +0200, Andrew Lunn wrote:
> > There is a very different approach, which might be simpler.
> > 
> > We know polling will always work. And it should be possible to
> > transition between polling and interrupt at any point, so long as the
> > phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> > state in phydev that there should be an irq, but it is not around yet.
> > When the phy is started, and phylib starts polling, look for the state
> > and try getting the IRQ again. If successful, swap to interrupts, if
> > not, keep polling. Maybe after 60 seconds of polling and trying, give
> > up trying to find the irq and stick with polling.
> 
> That doesn't sound like something that I'd backport to stable kernels.
> Letting the PHY driver dynamically switch from poll to IRQ mode risks
> racing with phylink's workqueue, and generally speaking, phylink doesn't
> seem to be built around the idea that "bool poll" can change after
> phylink_start().

I think you're confused. Andrew is merely talking about phylib's
polling, not phylink's.

Phylink's polling is only ever used in two circumstances:

1. In fixed-link mode where we have an interruptless GPIO.
2. In in-band mode when the PCS specifies it needs to be polled.

This is not used to poll ethernet PHYs - ethernet PHY polling is
handled entirely by phylib itself.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Vladimir Oltean]

On Thu, May 19, 2022 at 04:15:32PM +0100, Russell King (Oracle) wrote:
> On Thu, May 19, 2022 at 02:59:36PM +0000, Vladimir Oltean wrote:
> > Hi Andrew,
> > 
> > On Sat, May 14, 2022 at 02:23:51AM +0200, Andrew Lunn wrote:
> > > There is a very different approach, which might be simpler.
> > > 
> > > We know polling will always work. And it should be possible to
> > > transition between polling and interrupt at any point, so long as the
> > > phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> > > state in phydev that there should be an irq, but it is not around yet.
> > > When the phy is started, and phylib starts polling, look for the state
> > > and try getting the IRQ again. If successful, swap to interrupts, if
> > > not, keep polling. Maybe after 60 seconds of polling and trying, give
> > > up trying to find the irq and stick with polling.
> > 
> > That doesn't sound like something that I'd backport to stable kernels.
> > Letting the PHY driver dynamically switch from poll to IRQ mode risks
> > racing with phylink's workqueue, and generally speaking, phylink doesn't
> > seem to be built around the idea that "bool poll" can change after
> > phylink_start().
> 
> I think you're confused. Andrew is merely talking about phylib's
> polling, not phylink's.
> 
> Phylink's polling is only ever used in two circumstances:
> 
> 1. In fixed-link mode where we have an interruptless GPIO.
> 2. In in-band mode when the PCS specifies it needs to be polled.
> 
> This is not used to poll ethernet PHYs - ethernet PHY polling is
> handled entirely by phylib itself.

You're right, I would have probably figured that out if I actually tried
to implement what Andrew is proposing and not just superficially looking
at the code. I guess I'll try that now and see where that leads me to.
The only thing that remains in that case is fw_devlink blocking PHY
probing for lack of a supplier.

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Andrew Lunn]

> > There is a very different approach, which might be simpler.
> > 
> > We know polling will always work. And it should be possible to
> > transition between polling and interrupt at any point, so long as the
> > phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> > state in phydev that there should be an irq, but it is not around yet.
> > When the phy is started, and phylib starts polling, look for the state
> > and try getting the IRQ again. If successful, swap to interrupts, if
> > not, keep polling. Maybe after 60 seconds of polling and trying, give
> > up trying to find the irq and stick with polling.
> 
> That doesn't sound like something that I'd backport to stable kernels.

> What motivates me to make these changes in the first place is the idea
> that current kernels should work with updated device trees.

By current, you mean old kernels, LTS etc. You want an LTS kernel to
work with a new DT blob? You want forward compatibility with a DT
blob. Do the stable rules say anything about that?

      Andrew

Re: [RFC PATCH net 0/2] Make phylink and DSA wait for PHY driver that defers probe

[Vladimir Oltean]

On Thu, May 19, 2022 at 05:32:35PM +0200, Andrew Lunn wrote:
> > > There is a very different approach, which might be simpler.
> > > 
> > > We know polling will always work. And it should be possible to
> > > transition between polling and interrupt at any point, so long as the
> > > phylock is held. So if you get -EPROBE_DEFFER during probe, mark some
> > > state in phydev that there should be an irq, but it is not around yet.
> > > When the phy is started, and phylib starts polling, look for the state
> > > and try getting the IRQ again. If successful, swap to interrupts, if
> > > not, keep polling. Maybe after 60 seconds of polling and trying, give
> > > up trying to find the irq and stick with polling.
> > 
> > That doesn't sound like something that I'd backport to stable kernels.
> 
> > What motivates me to make these changes in the first place is the idea
> > that current kernels should work with updated device trees.
> 
> By current, you mean old kernels, LTS etc. You want an LTS kernel to
> work with a new DT blob? You want forward compatibility with a DT
> blob. Do the stable rules say anything about that?
> 
>       Andrew

Hmm, not sure about stable rules, but at least Marc Zyngier has
suggested in the past that this is something which should work:
https://lore.kernel.org/linux-arm-kernel/87czlzjxmz.wl-maz@kernel.org/

To quote:

| > As for compatibility between old kernel and new DT: I guess you'll hear
| > various opinions on this one.
| > https://www.spinics.net/lists/linux-mips/msg07778.html
| > 
| > | > Are we okay with the new device tree blobs breaking the old kernel?
| > |
| > | From my point of view, newer device trees are not required to work on
| > | older kernel, this would impose an unreasonable limitation and the use
| > | case is very limited.
| 
| My views are on the opposite side. DT is an ABI, full stop. If you
| change something, you *must* guarantee forward *and* backward
| compatibility. That's because:
| 
| - you don't control how updatable the firmware is
| 
| - people may need to revert to other versions of the kernel because
|   the new one is broken
| 
| - there are plenty of DT users beyond Linux, and we are not creating
|   bindings for Linux only.
| 
| You may disagree with this, but for the subsystems I maintain, this is
| the rule I intent to stick to.