[PATCH net-next v4 0/2] net: phylink: wait for a PHY that probes after the MAC

[PATCH net-next v4 0/2] net: phylink: wait for a PHY that probes after the MAC

[Aleksei Sviridkin]

On the Keenetic KN-1012 (MT7981B with an MT7531 switch), the Airoha
EN8811H behind lan4 has its PHY driver built as a module on the root
filesystem. The switch sets up its ports before that filesystem is
mounted, so the port is validated against the generic driver, fails its
phy-mode and stays dead for the uptime. DSA does not retry it.

Let the PHY node say so with needs-host-firmware and have phylink poll
for the PHY instead of giving up. Patch 1 adds the property, patch 2
does the waiting; the reasoning is in the patches.

A late bringup failure has to leave pl->phydev clear, because the
poller reads a set pl->phydev as a PHY that arrived by another path.
v3 depended on a pending phylink fix for that; it went in as commit
a940003f44e7 ("net: phylink: record the PHY only once bringup cannot
fail"), so this series applies to net-next on its own.

Tested on that board with both patches and a940003f44e7 backported to
its OpenWrt 6.18 kernel. The retry runs used a local debug parameter,
not part of this series, that fails the connect after a successful
attach a given number of times; the switch was unbound and rebound
with the PHY driver unbound, so the poller was waiting when the driver
was bound by hand:

 - at boot the switch set up its ports at 2.0 s, the PHY driver loaded
   its firmware at 6.3 s and the port attached at 7.2 s
 - two injected failures: "failed to connect late PHY: -EIO" twice, a
   second apart, and the third attempt attached
 - failures that do not stop: four attempts, then one "giving up on
   /soc/ethernet@15100000/mdio-bus/ethernet-phy@d after 4 attempts",
   and no further poll in the 25 s that followed
 - unbinding the switch while the poller waited: the poll stopped,
   nothing oopsed, and the port attached normally after a rebind

Unbinding the switch after the poller had attached lan4's PHY prints a
kernfs WARN: dsa_user_destroy() unregisters the netdev before
disconnecting the PHY, and only a PHY attached after registration has a
"phydev" link to remove. That ordering predates this series.

In one rebind the EN8811H's own probe failed (MCU not ready); the
poller kept polling at 1 Hz, as patch 2 describes for a driver whose
probe has failed (the run was too short to reach the backoff).

Conor, on 2026-09-10 [3] I said the property would go and the wait would
key off firmware-name. I went back on that. firmware-name names the file
to load, and the EN8811H driver keeps its two blob names in code, so it
would only be read as a presence flag. If firmware-name is still
preferred, I will redo it that way in the next version.

No in-tree device tree sets needs-host-firmware yet. The board is
supported out of tree, in OpenWrt.

Alternatives considered. The poll follows the plan in [1]; a
BUS_NOTIFY_BOUND_DRIVER notifier would give the exact edge, at the cost
of registering a bus notifier for each phylink instance that waits. The
poller repeats what phylink_fwnode_phy_connect() does - choose the
interface, attach, bring up, detach on failure - but drops its reference
at a different point, so the two are not merged into one helper.

Changes since v3:
 - Rebased onto net-next, which now carries a940003f44e7; the code is
   unchanged, and the prerequisite line is gone.
 - No longer RFC. The two open questions are settled above.
 - The board is named, and each patch says how the problem was found.
 - The binding description keeps only what the hardware does, and the
   comments in patch 2 are shorter.
 - Tested again on the rebased code, including the teardown cancel that
   v3 could not reach before the mt7530 unbind fixes were merged.
 - The NULL phydev->drv window mentioned in v3 is left to phylib; a
   separate series for net addresses it [2].

v3: https://lore.kernel.org/r/20260914211137.2760618-1-f@lex.la/

[1] https://lore.kernel.org/r/a230d199-5d4d-4637-aff3-e725a37e1da1@lunn.ch/
[2] https://lore.kernel.org/r/20260924215951.2127682-1-f@lex.la/
[3] https://lore.kernel.org/r/20260910153811.778387-1-f@lex.la/

Aleksei Sviridkin (2):
  dt-bindings: net: ethernet-phy: add needs-host-firmware
  net: phylink: wait for PHYs that are known to probe late

 .../devicetree/bindings/net/ethernet-phy.yaml |   6 +
 drivers/net/phy/phylink.c                     | 220 +++++++++++++++++-
 2 files changed, 219 insertions(+), 7 deletions(-)


base-commit: 42a9fb3382fc2573e92f41d203b095d9a372cfc9
-- 
2.53.0

[PATCH net-next v4 1/2] dt-bindings: net: ethernet-phy: add needs-host-firmware

[Aleksei Sviridkin]

A PHY can be one that the host has to load firmware into before it can
be driven at all. A controller that connects to such a PHY at setup,
before its driver has loaded, gets the generic driver or no PHY at all,
and one that connects only once gets no working PHY on that port for the
rest of the uptime, even though the PHY works seconds later.

The flag declares that. A consumer that sees it keeps the port and
connects the PHY once its driver binds. It describes the PHY, so it
sits on the PHY node and needs no prefix naming one.

firmware-name is not used for this: it names the file to load, and the
EN8811H driver keeps its two blob names in code, so it would only be
read as a presence flag.

The need is not derived from the compatible because the knowledge that
an ID needs host firmware lives in the PHY driver, and that driver is a
module not yet loaded when the MAC connects, so it has to come from the
device tree.

Found on a Keenetic KN-1012, where the EN8811H behind lan4 has its
driver on the root filesystem and the switch sets its ports up before
that is mounted, so lan4 stayed dead for the uptime.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index c3ebb3af8b52..73827662ddce 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -215,6 +215,12 @@ properties:
       used. The absence of this property indicates the muxers
       should be configured so that the external PHY is used.
 
+  needs-host-firmware:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      This PHY runs firmware that the host must load before it can be
+      driven, and is not usable until then.
+
   resets:
     maxItems: 1
 
-- 
2.53.0

Re: [PATCH net-next v4 1/2] dt-bindings: net: ethernet-phy: add needs-host-firmware

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 53 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v4 1/2] dt-bindings: net: ethernet-phy: add needs-host-firmware

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] For the only PHY the patch names, the new generic `needs-host-
  firmware` flag can be deduced from the compatible.
- [Low] Concern: the new `needs-host-firmware` property in
  Documentation/devicetree/bindings/net/ethernet-phy.yaml has no…

--

dt-bindings: net: ethernet-phy: add needs-host-firmware

This adds a generic needs-host-firmware flag to the ethernet-phy binding.
The flag says the host has to load firmware into the PHY before it can be
driven. A consumer that sees the flag keeps the port and connects the PHY
once its real driver binds, which fixes a dead lan4 port on a Keenetic
KN-1012 with an EN8811H PHY.

> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index c3ebb3af8b528..73827662ddce2 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -215,6 +215,12 @@ properties:
>        used. The absence of this property indicates the muxers
>        should be configured so that the external PHY is used.
>  
> +  needs-host-firmware:
> +    $ref: /schemas/types.yaml#/definitions/flag

[Severity: Low]
In this patch alone, nothing reads needs-host-firmware. Is anything in
this commit meant to consume it?

A later patch in the series, "net: phylink: wait for PHYs that are known
to probe late", appears to resolve this. There, phylink_fwnode_phy_connect()
checks:

    if (fwnode_property_present(phy_fwnode, "needs-host-firmware") &&
        !phylink_phy_is_usable(phy_dev)) {

When the check matches, it arms the phylink_late_phy_poll() delayed work
instead of attaching genphy. This is noted only for completeness, since
the binding lands before its user in the same series.

> +    description:
> +      This PHY runs firmware that the host must load before it can be
> +      driven, and is not usable until then.

[Severity: Medium]
For the EN8811H, doesn't the compatible already imply this flag?
Documentation/devicetree/bindings/writing-bindings.rst says:

  - DON'T add properties to avoid a specific compatible. DON'T add
    properties if they are implied by (deducible from) the compatible.

airoha,en8811h.yaml fixes the compatible to ethernet-phy-id03a2.a411. The
driver loads host firmware for every instance without any condition:

drivers/net/phy/air_en8811h.c:en8811h_probe() {
    ...
	ret = en8811h_load_firmware(phydev);
	if (ret < 0)
		return ret;
    ...
}

en8811h_load_firmware() always calls request_firmware_direct() for
EN8811H_MD32_DM and EN8811H_MD32_DSP. The AN8811HB probe calls
an8811hb_load_firmware() the same way.

The commit message says:

    The need is not derived from the compatible because the knowledge that
    an ID needs host firmware lives in the PHY driver, and that driver is a
    module not yet loaded when the MAC connects, so it has to come from the
    device tree.

Isn't that a question of Linux module loading and rootfs timing, rather
than something about the hardware? The ethernet-phy-id compatible is
already in the DT and can be seen before any PHY module loads.

The EN8811H binding is also not updated for the flag.
airoha,en8811h.yaml neither requires it nor shows it in its example, and
its required list only has:

    required:
      - reg

The existing in-tree EN8811H node in mt7981b-openwrt-one.dts does not get
the flag either:

    phy15: ethernet-phy@f {
        compatible = "ethernet-phy-id03a2.a411";

Would an EN8811H node without the flag still pass dt_binding_check? If so,
it would keep the dead-port behavior whenever the driver is a module that
loads late.

Could the late-connect behavior come from the compatible instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925001209.2334139-1-f%40lex.la

[PATCH net-next v4 2/2] net: phylink: wait for PHYs that are known to probe late

[Aleksei Sviridkin]

A PHY that needs firmware from the host and whose driver has not bound
when the MAC sets up its port is either taken by the generic driver,
which cannot drive it, or not found at all; a MAC that connects once at
setup, as DSA does, gets no working PHY on that port for the rest of the
uptime. The case this reaches is a driver built as a module on a
filesystem that is mounted after the MAC probes. Let the PHY declare it
with needs-host-firmware and poll until the driver binds instead of
failing. A driver that has bound is not covered, whatever it does about
firmware afterwards. Neither is one whose probe has already failed: the
driver core does not retry it, and the poller cannot tell that apart
from a driver that has yet to load, so it keeps polling.

Deferring the MAC's own probe is not an option: it keeps every port of
that MAC down until the module loads, and forever if it never does, and
those ports can include the one needed to mount the filesystem that
holds the module. Return 0 rather than -ENODEV, because DSA reads
-ENODEV as permission to look for the PHY on the switch's internal MDIO
bus, which is the wrong device.

Wait for a driver that has bound, not for a device that exists, because
the generic driver would otherwise bind and cannot drive such a PHY. If
the real driver goes away between that test and the attach, the generic
one binds instead; the poll detaches it and keeps waiting. The
attach-versus-unbind window itself is phylib's to close and is not
closed here.

A connect that fails with the real driver bound is retried a few times
and then given up on with one line, because silence from a poller reads
like success. Each retry re-runs the PHY's init and, on boards whose DT
gives it a reset line, pulses that reset, at a cost that depends on the
board and the PHY, so the retries are bounded. Stopping after the first
failure would leave a DSA port, which connects once, dead until the
switch driver is rebound.

Until a PHY attaches, report no link modes and refuse the ethtool
settings that would configure the MAC alone for a link that cannot come
up.

Found on a Keenetic KN-1012 (MT7981B with an MT7531 switch): the EN8811H
behind lan4 has its driver on the root filesystem, the switch sets its
ports up before that is mounted, and lan4 was lost for the uptime. With
this change lan4 attaches once the module loads. The retry path was
driven there by a local debug parameter that fails the connect after a
successful attach: two injected failures were retried a second apart and
the third attempt attached, and with failures that never stop, four
attempts ended in one "giving up" line and no further polls.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
 drivers/net/phy/phylink.c | 220 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 213 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a7d086cdc9b2..4a5b98b55889 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -98,6 +98,15 @@ struct phylink {
 
 	u32 wolopts_mac;
 	u8 wol_sopass[SOPASS_MAX];
+
+	/* The poller writes these while it runs; arming cancels it first. */
+	struct fwnode_handle *late_phy_fwnode;
+	u32 late_phy_flags;
+	struct delayed_work late_phy_poll;
+	unsigned int late_phy_poll_ms;
+	unsigned int late_phy_waited_ms;
+	u8 late_phy_retries;
+	bool late_phy_warned;
 };
 
 #define phylink_printk(level, pl, fmt, ...) \
@@ -1831,6 +1840,18 @@ int phylink_set_fixed_link(struct phylink *pl,
 }
 EXPORT_SYMBOL_GPL(phylink_set_fixed_link);
 
+static void phylink_late_phy_poll(struct work_struct *work);
+
+/* Synchronous: the poller reads the node put here. It only trylocks
+ * rtnl, so a caller holding rtnl cannot deadlock on it.
+ */
+static void phylink_late_phy_cancel(struct phylink *pl)
+{
+	cancel_delayed_work_sync(&pl->late_phy_poll);
+	fwnode_handle_put(pl->late_phy_fwnode);
+	pl->late_phy_fwnode = NULL;
+}
+
 /**
  * phylink_update_pause_state() - Update the phylink pause frame configuration
  * @pl: a pointer to a &struct phylink instance
@@ -1989,6 +2010,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 	mutex_init(&pl->phydev_mutex);
 	mutex_init(&pl->state_mutex);
 	INIT_WORK(&pl->resolve, phylink_resolve);
+	INIT_DELAYED_WORK(&pl->late_phy_poll, phylink_late_phy_poll);
 
 	pl->config = config;
 	if (config->type == PHYLINK_NETDEV) {
@@ -2068,6 +2090,8 @@ EXPORT_SYMBOL_GPL(phylink_create);
  */
 void phylink_destroy(struct phylink *pl)
 {
+	phylink_late_phy_cancel(pl);
+
 	sfp_bus_del_upstream(pl->sfp_bus);
 	if (pl->link_gpio)
 		gpiod_put(pl->link_gpio);
@@ -2337,10 +2361,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 }
 
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
-			      phy_interface_t interface)
+			      phy_interface_t interface, u32 flags)
 {
-	u32 flags = 0;
-
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED))
 		return -EINVAL;
 
@@ -2378,7 +2400,7 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 		pl->link_config.interface = pl->link_interface;
 	}
 
-	ret = phylink_attach_phy(pl, phy, pl->link_interface);
+	ret = phylink_attach_phy(pl, phy, pl->link_interface, 0);
 	if (ret < 0)
 		return ret;
 
@@ -2390,6 +2412,143 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 }
 EXPORT_SYMBOL_GPL(phylink_connect_phy);
 
+#define PHYLINK_LATE_PHY_POLL_MS	1000
+#define PHYLINK_LATE_PHY_WARN_MS	60000
+#define PHYLINK_LATE_PHY_POLL_MAX_MS	30000
+#define PHYLINK_LATE_PHY_RETRIES	3
+
+static bool phylink_late_phy_pending(struct phylink *pl)
+{
+	return pl->late_phy_fwnode && !pl->phydev;
+}
+
+/* Stale the moment it returns: the device lock this wants cannot be held
+ * across the attach, whose own failure path takes it again.
+ */
+static bool phylink_phy_is_usable(struct phy_device *phy_dev)
+{
+	return phy_dev && device_is_bound(&phy_dev->mdio.dev) && phy_dev->drv;
+}
+
+static void phylink_late_phy_backoff(struct phylink *pl)
+{
+	pl->late_phy_poll_ms = min_t(unsigned int, pl->late_phy_poll_ms * 2,
+				     PHYLINK_LATE_PHY_POLL_MAX_MS);
+}
+
+static void phylink_late_phy_poll(struct work_struct *work)
+{
+	struct phylink *pl = container_of(to_delayed_work(work), struct phylink,
+					  late_phy_poll);
+	struct phy_device *phy_dev;
+	bool again = false, lost_race = false;
+	int ret;
+
+	if (!rtnl_trylock()) {
+		pl->late_phy_waited_ms += pl->late_phy_poll_ms;
+		goto requeue;
+	}
+
+	/* A PHY arrived by another path, an SFP for one, while queued. */
+	if (!phylink_late_phy_pending(pl)) {
+		rtnl_unlock();
+		return;
+	}
+
+	/* Stable here: whoever clears it waits for this work first. */
+	phy_dev = fwnode_phy_find_device(pl->late_phy_fwnode);
+	if (!phylink_phy_is_usable(phy_dev)) {
+		if (phy_dev)
+			phy_device_free(phy_dev);
+
+		if (!pl->late_phy_warned &&
+		    pl->late_phy_waited_ms >= PHYLINK_LATE_PHY_WARN_MS) {
+			pl->late_phy_warned = true;
+			phylink_warn(pl,
+				     "still waiting for %pfw (needs-host-firmware)\n",
+				     pl->late_phy_fwnode);
+		}
+		/* Past the warn it may never come: stop paying 1 Hz for it. */
+		if (pl->late_phy_waited_ms >= PHYLINK_LATE_PHY_WARN_MS)
+			phylink_late_phy_backoff(pl);
+		/* The first run is immediate, so count the sleep ahead. */
+		pl->late_phy_waited_ms += pl->late_phy_poll_ms;
+		rtnl_unlock();
+		goto requeue;
+	}
+
+	/* Under the mutex, unlike at connect: this port may be live. */
+	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
+		mutex_lock(&pl->state_mutex);
+		pl->link_interface = phy_dev->interface;
+		pl->link_config.interface = pl->link_interface;
+		mutex_unlock(&pl->state_mutex);
+	}
+
+	ret = phylink_attach_phy(pl, phy_dev, pl->link_interface,
+				 pl->late_phy_flags);
+	if (!ret && phy_driver_is_genphy(phy_dev)) {
+		/* Lost the race: the attach bound the generic driver, which
+		 * is the outcome this poller exists to avoid.
+		 */
+		phy_detach(phy_dev);
+		lost_race = true;
+		ret = -EAGAIN;
+	}
+	if (!ret) {
+		ret = phylink_bringup_phy(pl, phy_dev,
+					  pl->link_config.interface);
+		if (ret) {
+			phy_detach(phy_dev);
+		} else {
+			/* Only a major config programs the masks bringup
+			 * narrowed.
+			 */
+			if (!test_bit(PHYLINK_DISABLE_STOPPED,
+				      &pl->phylink_disable_state)) {
+				mutex_lock(&pl->state_mutex);
+				pl->force_major_config = true;
+				mutex_unlock(&pl->state_mutex);
+				/* MAC before the PHY, the order a start
+				 * uses.
+				 */
+				phylink_run_resolve(pl);
+				flush_work(&pl->resolve);
+				phy_start(phy_dev);
+			}
+		}
+	}
+	if (lost_race) {
+		/* Not a failed connect: the next poll waits for the real
+		 * driver.
+		 */
+		again = true;
+	} else if (ret) {
+		phylink_err(pl, "failed to connect late PHY: %pe\n",
+			    ERR_PTR(ret));
+		/* Bounded: each retry re-runs the PHY's init, maybe its reset. */
+		if (pl->late_phy_retries) {
+			pl->late_phy_retries--;
+			again = true;
+		} else {
+			/* Silence from here reads as success otherwise. */
+			phylink_err(pl, "giving up on %pfw after %u attempts\n",
+				    pl->late_phy_fwnode,
+				    PHYLINK_LATE_PHY_RETRIES + 1);
+		}
+	}
+	phy_device_free(phy_dev);
+	rtnl_unlock();
+
+	if (!again)
+		return;
+
+requeue:
+	queue_delayed_work(system_freezable_power_efficient_wq,
+			   &pl->late_phy_poll,
+			   msecs_to_jiffies(pl->late_phy_poll_ms));
+}
+
 /**
  * phylink_of_phy_connect() - connect the PHY specified in the DT mode.
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -2400,7 +2559,8 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);
  * specified by @pl. Actions specified in phylink_connect_phy() will be
  * performed.
  *
- * Returns 0 on success or a negative errno.
+ * Returns what phylink_fwnode_phy_connect() returns, including 0 for a
+ * deferred connect with no PHY attached yet.
  */
 int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
 			   u32 flags)
@@ -2418,7 +2578,13 @@ EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
  * Connect the phy specified @fwnode to the phylink instance specified
  * by @pl.
  *
- * Returns 0 on success or a negative errno.
+ * If the PHY node carries the needs-host-firmware property and the
+ * PHY is not usable yet, 0 is returned with no PHY connected: a poller
+ * connects it once its driver has probed. Until then the MAC runs
+ * without a PHY and ethtool reports no link modes.
+ *
+ * Returns 0 on success - the PHY connected, or the deferred connect
+ * armed - or a negative errno.
  */
 int phylink_fwnode_phy_connect(struct phylink *pl,
 			       const struct fwnode_handle *fwnode,
@@ -2428,6 +2594,8 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	struct phy_device *phy_dev;
 	int ret;
 
+	phylink_late_phy_cancel(pl);
+
 	if (!phylink_expects_phy(pl))
 		return 0;
 
@@ -2440,6 +2608,23 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	}
 
 	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	if (fwnode_property_present(phy_fwnode, "needs-host-firmware") &&
+	    !phylink_phy_is_usable(phy_dev)) {
+		/* -ENODEV here would also send DSA to the switch's own bus. */
+		if (phy_dev)
+			phy_device_free(phy_dev);
+
+		pl->late_phy_fwnode = phy_fwnode;
+		pl->late_phy_flags = flags;
+		pl->late_phy_poll_ms = PHYLINK_LATE_PHY_POLL_MS;
+		pl->late_phy_waited_ms = 0;
+		pl->late_phy_retries = PHYLINK_LATE_PHY_RETRIES;
+		pl->late_phy_warned = false;
+		queue_delayed_work(system_freezable_power_efficient_wq,
+				   &pl->late_phy_poll, 0);
+		return 0;
+	}
+
 	/* We're done with the phy_node handle */
 	fwnode_handle_put(phy_fwnode);
 	if (!phy_dev)
@@ -2481,6 +2666,8 @@ void phylink_disconnect_phy(struct phylink *pl)
 
 	ASSERT_RTNL();
 
+	phylink_late_phy_cancel(pl);
+
 	mutex_lock(&pl->phydev_mutex);
 	phy = pl->phydev;
 	if (phy) {
@@ -3047,6 +3234,14 @@ int phylink_ethtool_ksettings_get(struct phylink *pl,
 
 	ASSERT_RTNL();
 
+	/* No PHY yet: the port supports nothing, not what the MAC alone can. */
+	if (phylink_late_phy_pending(pl)) {
+		kset->base.port = pl->link_port;
+		kset->base.speed = SPEED_UNKNOWN;
+		kset->base.duplex = DUPLEX_UNKNOWN;
+		return 0;
+	}
+
 	if (pl->phydev)
 		phy_ethtool_ksettings_get(pl->phydev, kset);
 	else
@@ -3119,6 +3314,10 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 
 	ASSERT_RTNL();
 
+	/* Would configure the MAC alone, for a link that cannot come up. */
+	if (phylink_late_phy_pending(pl))
+		return -EOPNOTSUPP;
+
 	if (pl->phydev) {
 		struct ethtool_link_ksettings phy_kset = *kset;
 
@@ -3292,6 +3491,9 @@ int phylink_ethtool_nway_reset(struct phylink *pl)
 
 	ASSERT_RTNL();
 
+	if (phylink_late_phy_pending(pl))
+		return -EOPNOTSUPP;
+
 	if (pl->phydev)
 		ret = phy_restart_aneg(pl->phydev);
 	phylink_pcs_an_restart(pl);
@@ -3331,6 +3533,10 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	if (pl->req_link_an_mode == MLO_AN_FIXED)
 		return -EOPNOTSUPP;
 
+	/* pl->supported still describes the MAC, so the test below passes. */
+	if (phylink_late_phy_pending(pl))
+		return -EOPNOTSUPP;
+
 	if (!phylink_test(pl->supported, Pause) &&
 	    !phylink_test(pl->supported, Asym_Pause))
 		return -EOPNOTSUPP;
@@ -3817,7 +4023,7 @@ static int phylink_sfp_config_phy(struct phylink *pl, struct phy_device *phy)
 	/* Attach the PHY so that the PHY is present when we do the major
 	 * configuration step.
 	 */
-	ret = phylink_attach_phy(pl, phy, config.interface);
+	ret = phylink_attach_phy(pl, phy, config.interface, 0);
 	if (ret < 0)
 		return ret;
 
-- 
2.53.0

Re: [PATCH net-next v4 2/2] net: phylink: wait for PHYs that are known to probe late

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Returning success before the PHY is attached causes a NULL
  pointer dereference in ucc_geth_open() on CONFIG_PPC_85xx.
- [Medium] The patch changes what a 0 return from
  phylink_fwnode_phy_connect()/phylink_of_phy_connect() means: it can…
- [Medium] The poller attaches the PHY based on a lockless readiness
  check, phylink_phy_is_usable() (`device_is_bound()` &&…
- [Low] Before this patch, a successful phylink_fwnode_phy_connect() in
  MLO_AN_PHY mode always replaced a PHY_INTERFACE_MODE_NA link_interface…
- [Low] The new kernel-doc of phylink_fwnode_phy_connect() says that for
  a needs-host-firmware PHY that is not usable yet, 'a poller connects…

--

net: phylink: wait for PHYs that are known to probe late

This commit changes phylink_fwnode_phy_connect() for PHYs whose DT node
has needs-host-firmware and whose driver has not bound yet. It now
returns 0 and arms a delayed work poller, which attaches the PHY once
the driver binds. Until then, the ethtool paths report no link modes and
refuse configuration changes.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b25..4a5b98b558891 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> +/* Stale the moment it returns: the device lock this wants cannot be held
> + * across the attach, whose own failure path takes it again.
> + */
> +static bool phylink_phy_is_usable(struct phy_device *phy_dev)
> +{
> +	return phy_dev && device_is_bound(&phy_dev->mdio.dev) && phy_dev->drv;
> +}

[ ... ]

> +	ret = phylink_attach_phy(pl, phy_dev, pl->link_interface,
> +				 pl->late_phy_flags);
> +	if (!ret && phy_driver_is_genphy(phy_dev)) {
> +		/* Lost the race: the attach bound the generic driver, which
> +		 * is the outcome this poller exists to avoid.
> +		 */
> +		phy_detach(phy_dev);
> +		lost_race = true;
> +		ret = -EAGAIN;
> +	}
> +	if (!ret) {
> +		ret = phylink_bringup_phy(pl, phy_dev,
> +					  pl->link_config.interface);

[Severity: Medium]
What happens if an unbind of the PHY driver overlaps this attach? Sysfs
unbind, rmmod and MDIO bus teardown take only the device lock, not rtnl.
__device_release_driver() runs its steps in this order:

__device_release_driver()
    device_remove(dev)
        phy_remove()
            phydev->drv = NULL;
    ...
    device_unbind_cleanup(dev)          <- clears dev->driver
    klist_remove(&dev->p->knode_driver);

In that window phy_attach_direct() still sees d->driver non-NULL. It
does not select genphy and leaves is_genphy_driven at 0. Then it calls:

	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))

That dereferences the NULL drv.

If the attach does get through, phy_driver_is_genphy() is false here, so
the lost_race recovery does not run. phylink_bringup_phy() then does:

	phylink_info(pl,
		     "PHY [%s] driver [%s] (irq=%s)\n",
		     dev_name(&phy->mdio.dev), phy->drv->name, irq_str);

Under rmmod, try_module_get(d->driver->owner) fails with -EIO instead.
That uses up one of the bounded retries as if the connect had really
failed.

The commit message says the attach-versus-unbind window is phylib's to
close. But the poller decides when to attach, based on a check it
documents as stale. Could this oops the work item while it holds rtnl?

[ ... ]

> @@ -2418,7 +2578,13 @@ EXPORT_SYMBOL_GPL(phylink_of_phy_connect);
>   * Connect the phy specified @fwnode to the phylink instance specified
>   * by @pl.
>   *
> - * Returns 0 on success or a negative errno.
> + * If the PHY node carries the needs-host-firmware property and the
> + * PHY is not usable yet, 0 is returned with no PHY connected: a poller
> + * connects it once its driver has probed. Until then the MAC runs
> + * without a PHY and ethtool reports no link modes.

[Severity: Low]
Is this description complete? phylink_late_phy_poll() can stop for good
after PHYLINK_LATE_PHY_RETRIES + 1 failed connects with the real driver
bound:

		} else {
			/* Silence from here reads as success otherwise. */
			phylink_err(pl, "giving up on %pfw after %u attempts\n",
				    pl->late_phy_fwnode,
				    PHYLINK_LATE_PHY_RETRIES + 1);
		}

That branch leaves pl->late_phy_fwnode set, so
phylink_late_phy_pending() stays true. The ethtool paths then keep
reporting no link modes and returning -EOPNOTSUPP until the MAC calls
disconnect and connect again. Rebinding the PHY driver does not restart
the poller.

Could the kernel-doc mention this terminal state?

[ ... ]

> @@ -2440,6 +2608,23 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
>  	}
>  
>  	phy_dev = fwnode_phy_find_device(phy_fwnode);
> +	if (fwnode_property_present(phy_fwnode, "needs-host-firmware") &&
> +	    !phylink_phy_is_usable(phy_dev)) {
> +		/* -ENODEV here would also send DSA to the switch's own bus. */
> +		if (phy_dev)
> +			phy_device_free(phy_dev);
> +
> +		pl->late_phy_fwnode = phy_fwnode;
> +		pl->late_phy_flags = flags;
> +		pl->late_phy_poll_ms = PHYLINK_LATE_PHY_POLL_MS;
> +		pl->late_phy_waited_ms = 0;
> +		pl->late_phy_retries = PHYLINK_LATE_PHY_RETRIES;
> +		pl->late_phy_warned = false;
> +		queue_delayed_work(system_freezable_power_efficient_wq,
> +				   &pl->late_phy_poll, 0);
> +		return 0;
> +	}

[Severity: High]
Can this leave dev->phydev NULL for callers that take 0 to mean the PHY
is attached? ucc_geth_open() is one of them:

	err = phylink_of_phy_connect(ugeth->phylink, ugeth->dev->of_node, 0);
	if (err) {
		dev_err(&dev->dev, "Could not attach to PHY\n");
		return -ENODEV;
	}
	...
	device_set_wakeup_capable(&dev->dev,
			qe_alive_during_sleep() || dev->phydev->irq);

With CONFIG_PPC_85xx, qe_alive_during_sleep() always returns 0, so
dev->phydev->irq is evaluated. The deferral branch never calls
phy_attach_direct(), so dev->phydev is still NULL at that point.

Would this oops in ndo_open with rtnl held on an 85xx QE board? The
conditions are a PHY node with needs-host-firmware and a PHY driver that
is not loaded yet.

Before this patch, the same situation either bound genphy or returned
-ENODEV, and ucc_geth_open() handles both.

[Severity: Medium]
A related question: several callers do one-time PHY setup right after a
successful connect. The late attach in phylink_late_phy_poll() only does
phylink_attach_phy(), phylink_bringup_phy() and phy_start(), so that
setup is never repeated.

enetc_phylink_connect():

	/* disable EEE autoneg, until ENETC driver supports it */
	memset(&edata, 0, sizeof(struct ethtool_keee));
	phylink_ethtool_set_eee(priv->phylink, &edata);

With pl->phydev NULL, phylink_ethtool_set_eee() returns -EOPNOTSUPP and
changes nothing. ENETC has no LPI mac ops, so phylink_bringup_phy() does
not touch EEE either. Does the late PHY end up advertising EEE on a MAC
that explicitly tries to turn it off?

stmmac_init_phy() passes priv->tx_lpi_timer to phylib through
phylink_ethtool_get_eee() and phylink_ethtool_set_eee(). Both fail when
there is no PHY, so a timer set before the first open looks to be lost.

mvneta_mdio_probe() calls device_set_wakeup_capable() once, from
phylink_ethtool_get_wol(), right after the connect. Nothing updates it
after the late attach, so PHY WoL would never be reported.

sparx5_port_open() takes a NULL phydev after a successful connect to
mean the port has no PHY, and powers up the SerDes:

	if (!ndev->phydev) {
		/* power up serdes */
		port->conf.power_down = false;

The phylink_of_phy_connect() kernel-doc also still says "Actions
specified in phylink_connect_phy() will be performed". That is not true
on the deferred path.

Is there a way for these callers to learn that the connect was deferred,
or for the poller to re-run their post-connect setup?

[Severity: Low]
Does returning here skip the PHY_INTERFACE_MODE_NA fix-up that the
non-deferred path does further down?

	/* Use PHY device/driver interface */
	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
		pl->link_interface = phy_dev->interface;
		pl->link_config.interface = pl->link_interface;
	}

A DSA user port with no phy-mode in DT gets PHY_INTERFACE_MODE_NA from
dsa_port_phylink_create():

	err = of_get_phy_mode(dp->dn, &mode);
	if (err)
		mode = PHY_INTERFACE_MODE_NA;

Before this patch, a successful connect in MLO_AN_PHY mode meant the
interface was set before phylink_start() ran. Now
phylink_start()->phylink_mac_initial_config() copies pl->phy_state with
the interface still NA. phylink_major_config() then passes it to
mac_select_pcs(), mac_prepare() and mac_config().

The poller's forced major config fixes this later. In the meantime, can
the MAC/PCS be programmed with PHY_INTERFACE_MODE_NA, or fail on it and
set major_config_failed?

>  	/* We're done with the phy_node handle */
>  	fwnode_handle_put(phy_fwnode);
>  	if (!phy_dev)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925001209.2334139-1-f%40lex.la