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

[PATCH net-next v5 0/3] 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.

Patch 1 lets the PHY node say so with needs-host-firmware. Patch 2 makes
phylink poll for such a PHY instead of giving up, for a MAC that opts
in. Patch 3 opts in DSA user ports of switch drivers that set a flag,
and sets it in mt7530.

The poller can still lose a race against an unbind of the PHY driver,
between its readiness check and the attach. That window is phylib's:
any phy_attach_direct() caller racing an unbind has it. The attach
guard series [1] closes it.

Tested on that board with the series backported to its OpenWrt 6.18
kernel, together with a940003f44e7 and 07d995873960 (the mt7530
.get_stats64 atomic-context fix). The debug kernel used here wedges a
CPU without that fix, unrelated to this series. Two local debug
parameters, not part of the series, drove the error paths. One fails
the connect after a successful attach a given number of times, the
other ignores the opt-in.

 - boot: the switch set up its ports at 3.2 s, the PHY driver loaded
   its firmware at 8.7 s and lan4 attached at 9.5 s. Link up at 1 Gb/s.
 - opt-in ignored: the old behaviour. The generic driver took the PHY
   at switch setup and the port never polled.
 - two injected failures: "failed to connect late PHY: -EIO" twice, a
   second apart, and the third attempt attached. Link up at 1 Gb/s.
 - 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.
 - switch unbound while the poller waited: the poll stopped, nothing
   oopsed, and the port attached normally after a rebind.

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

Changes in v5 (since v4):
https://lore.kernel.org/r/20260925001209.2334139-1-f@lex.la/
 - Deferral is opt-in. A deferred connect returns 0 with no PHY
   attached, and some callers read 0 as a PHY being there. A MAC opts
   in with phylink_config.phy_may_probe_late. DSA sets it for user
   ports of drivers that set dsa_switch.phy_may_probe_late, and mt7530
   does (new patch 3).
 - Defer only with a known interface mode. The poller no longer fills
   in PHY_INTERFACE_MODE_NA, so the MAC is never started with NA.
 - kernel-doc names the opt-in and the state after the retries run out.
 - Rebased onto current net-next. Patch 1 is unchanged, Acked-by added.

[1] https://lore.kernel.org/r/20261001130120.104628-1-f@lex.la/

Aleksei Sviridkin (3):
  dt-bindings: net: ethernet-phy: add needs-host-firmware
  net: phylink: wait for PHYs that are known to probe late
  net: dsa: let user ports wait for a PHY that probes late

 .../devicetree/bindings/net/ethernet-phy.yaml |   6 +
 drivers/net/dsa/mt7530.c                      |   1 +
 drivers/net/phy/phylink.c                     | 221 +++++++++++++++++-
 include/linux/phylink.h                       |   4 +
 include/net/dsa.h                             |   5 +
 net/dsa/user.c                                |   1 +
 6 files changed, 230 insertions(+), 8 deletions(-)


base-commit: 47a1446725732cd3996edf607e8739334bbf4d78
-- 
2.53.0

[PATCH net-next v5 1/3] 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
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
Changes in v5: none, Acked-by added.

 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 df50c4c33d76..ab95b9a5d95a 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

[PATCH net-next v5 2/3] 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, for a MAC that opts in with
phy_may_probe_late, 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.

The deferral is opt-in because it returns 0 with no PHY attached, and
some callers read 0 as a PHY being there: ucc_geth dereferences
dev->phydev later in the same open, and enetc, stmmac, mvneta, sparx5
and lan743x do one-time PHY setup at that point that a late attach
would skip. Those connect from ndo_open, where the problem does not
last: a PHY taken by genphy at one open is released at close, and the
next open finds the real driver. A MAC that connects once at setup has
no such second chance.

The deferral also needs an interface mode known up front, as without
one the MAC would be configured for PHY_INTERFACE_MODE_NA when started
before the PHY supplies its own.

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>
---
Changes in v5:
 - defer only for a MAC that sets phylink_config.phy_may_probe_late
 - defer only with a known interface mode, and drop the
   PHY_INTERFACE_MODE_NA fill-in from the poller
 - kernel-doc names the opt-in and the state after the retries run out

 drivers/net/phy/phylink.c | 221 ++++++++++++++++++++++++++++++++++++--
 include/linux/phylink.h   |   4 +
 2 files changed, 217 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a7d086cdc9b2..a663390fdc9f 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,135 @@ 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;
+	}
+
+	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()
@@ -2398,9 +2549,11 @@ EXPORT_SYMBOL_GPL(phylink_connect_phy);
  *
  * Connect the phy specified in the device node @dn to the phylink instance
  * specified by @pl. Actions specified in phylink_connect_phy() will be
- * performed.
+ * performed, except for a deferred connect, where they happen once the
+ * PHY attaches.
  *
- * 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 +2571,17 @@ 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 MAC set &phylink_config.phy_may_probe_late and has a known
+ * interface mode, and 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.
+ * If the connect keeps failing with the driver bound, the poller gives
+ * up after a few attempts and the port stays that way until the PHY is
+ * disconnected and connected again.
+ *
+ * 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 +2591,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 +2605,25 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	}
 
 	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	if (pl->config->phy_may_probe_late &&
+	    pl->link_interface != PHY_INTERFACE_MODE_NA &&
+	    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 +2665,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 +3233,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 +3313,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 +3490,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 +3532,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 +4022,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;
 
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 3a88a69882a6..6249156b51f1 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -147,6 +147,9 @@ enum phylink_op_type {
  * @default_an_inband: if true, defaults to MLO_AN_INBAND rather than
  *		       MLO_AN_PHY. A fixed-link specification will override.
  * @eee_rx_clk_stop_enable: if true, PHY can stop the receive clock during LPI
+ * @phy_may_probe_late: if true, a connect to a PHY marked needs-host-firmware
+ *			whose driver has not bound yet is deferred until that
+ *			driver binds; see phylink_fwnode_phy_connect().
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
  * @supported_interfaces: bitmap describing which PHY_INTERFACE_MODE_xxx
@@ -169,6 +172,7 @@ struct phylink_config {
 	bool mac_requires_rxc;
 	bool default_an_inband;
 	bool eee_rx_clk_stop_enable;
+	bool phy_may_probe_late;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);
 	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
-- 
2.53.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 5

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] The deferral gate in phylink_fwnode_phy_connect() does not
  exclude MLO_AN_INBAND.
- [Medium] phylink_phy_is_usable() checks only that a driver is bound.
- [Medium] phylink_phy_is_usable() calls
  device_is_bound(&phy_dev->mdio.dev) and reads phy_dev->drv without…
- [Low] Concern: nothing in the tree sets
  phylink_config.phy_may_probe_late, so the deferral in…
- [Low] The commit message argues that MACs which connect from ndo_open
  do not need the deferral, because "a PHY taken by genphy at one open…
- [Low] If a PHY from another path (an SFP module PHY via
  phylink_sfp_connect_phy()) attaches while the deferral is armed,…
- [Low] In phylink_late_phy_poll(), the lost-race check (`if (!ret &&
  phy_driver_is_genphy(phy_dev))`) only runs when phylink_attach_phy()…
- [Low] Concern: the deferral gate does not exclude MACs with
  phylink_config.mac_requires_rxc, so such a MAC could go through…

--

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

This adds an opt-in deferral to phylink_fwnode_phy_connect() for PHYs
marked needs-host-firmware whose driver has not bound yet. A delayed work
item polls under rtnl_trylock() until the driver binds, then attaches and
brings up the PHY, with a bounded number of retries on connect failure.
Until then the ethtool link settings paths report no link modes or return
-EOPNOTSUPP.

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

[Severity: Low]
Is this accurate for this commit on its own? At this point nothing in the
tree sets phylink_config.phy_may_probe_late. The deferral in
phylink_fwnode_phy_connect() cannot be reached on the KN-1012 until the
following patch, "net: dsa: let user ports wait for a PHY that probes
late". That patch sets ds->phy_may_probe_late in mt7530_probe_common() and
copies it into dp->pl_config in dsa_user_phy_setup().

With that patch applied the MT7531 path does reach the deferral, so this
is only about the wording here. Could the message say that the fix takes
effect together with the next patch?

> and lan743x do one-time PHY setup at that point that a late attach
> would skip. Those connect from ndo_open, where the problem does not
> last: a PHY taken by genphy at one open is released at close, and the
> next open finds the real driver. A MAC that connects once at setup has
> no such second chance.

[Severity: Low]
Does the next open actually find the real driver? That seems to hold only
if the PHY driver module loads while the interface is down.

Suppose the module loads while genphy is bound, for example because IP
autoconfig or early userspace brought the interface up before the rootfs
module was loaded. Then driver_attach() for the real driver skips the
device:

drivers/base/dd.c:__driver_probe_device() {
    ...
	if (dev->driver)
		return -EBUSY;
    ...
}

At close, phy_detach_internal() only releases genphy:

	if (phydev->is_genphy_driven) {
		device_release_driver(&phydev->mdio.dev);
		phydev->is_genphy_driven = 0;
	}

After that nothing re-probes the device. On the next open,
phy_attach_direct() sees d->driver == NULL and binds genphy again.

Wouldn't those ndo_open MACs then get the same "no second chance" result?
Could the reason for leaving them out be reworded?

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

[ ... ]

> @@ -2390,6 +2412,135 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)

[ ... ]

> +/* 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;
> +}

[Severity: Medium]
The kernel-doc for device_is_bound() says "This function must be called
with the device lock held." The comment explains why the lock cannot be
held across the attach. Is there a reason it cannot be taken just around
this check and dropped before phylink_attach_phy()?

Without the lock, can the check pass for a PHY whose unbind has already
started? __device_release_driver() runs device_remove(), and so
phy_remove(), before it removes knode_driver:

drivers/net/phy/phy_device.c:phy_remove() {
    ...
	phydev->drv = NULL;

	return 0;
}

klist_remove(&dev->p->knode_driver) only runs after that, so
device_is_bound() can still return true while drv is being cleared.

The poller then calls phy_attach_direct() with d->driver still set, so
there is no genphy fallback. The commit message's handling ("the generic
one binds instead") does not cover this case. If phydev->drv becomes NULL
during the attach, could phy_attach_direct()
(phy_drv_supports_irq(phydev->drv)) or phylink_bringup_phy()
(phy->drv->name) dereference NULL?

The commit message notes that the wider attach/unbind race belongs to
phylib. The unlocked device_is_bound() call, though, is new in this patch.

[ ... ]

> +	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;
> +	}

[Severity: Low]
Suppose the real driver unbinds between phylink_phy_is_usable() and the
attach, and the genphy attach that phy_attach_direct() then does fails.
What happens here?

The genphy probe/bind failure path clears is_genphy_driven:

error_module_put:
	module_put(d->driver->owner);
	phydev->is_genphy_driven = 0;
	d->driver = NULL;

The later error path through phy_detach_internal() clears it as well.
In both cases ret is non-zero, so lost_race stays false. The poller then
logs "failed to connect late PHY" and uses up one of
PHYLINK_LATE_PHY_RETRIES. If none are left, it prints "giving up" and
stops, even though the real driver was never tried.

Should a failed attach with no real driver bound be kept out of the retry
count?

[ ... ]

> +	} 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;

[Severity: Medium]
Can this retry end up attaching a PHY that another net_device owns?

phylink_phy_is_usable() does not check phydev->attached_dev. The PHY may
already be attached elsewhere, for example when two ports have a DT
phy-handle pointing at the same PHY, or when the PHY was attached through
another path. Then phy_attach_direct() fails here:

	if (phydev->attached_dev) {
		dev_err(&dev->dev, "PHY already attached\n");
		err = -EBUSY;
		goto error;
	}

The error label runs phy_detach_internal() on the other owner's
attachment. That clears attached_dev, phylink and phy_link_change while
the other phylink still holds the PHY in pl->phydev. This tear-down is
existing phylib behaviour.

What is new is that the -EBUSY arrives here and is retried. On the next
poll attached_dev is NULL, so the attach and phylink_bringup_phy()
succeed, and two phylink instances hold the same phy_device. A disconnect
on either side would then detach the PHY from under the other.

Should -EBUSY, or a PHY with attached_dev set, be treated as final rather
than retried?

[ ... ]

> @@ -2440,6 +2605,25 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
>  	}
>  
>  	phy_dev = fwnode_phy_find_device(phy_fwnode);
> +	if (pl->config->phy_may_probe_late &&
> +	    pl->link_interface != PHY_INTERFACE_MODE_NA &&
> +	    fwnode_property_present(phy_fwnode, "needs-host-firmware") &&
> +	    !phylink_phy_is_usable(phy_dev)) {

[Severity: Medium]
Should this gate exclude MLO_AN_INBAND, or hold the link down while the
deferral is pending?

With the deferral armed and phylink started, phylink_resolve() takes the
in-band path with phy == NULL. The PHY link state is not combined in:

		if (phy)
			link_state.link &= pl->phy_state.link;

If the PCS reports link, phylink_link_up() calls mac_link_up() and turns
carrier on. The PHY is not attached, and once the retries run out it may
never be.

When the poller later attaches the PHY, phy_attach_direct() calls
netif_carrier_off(phydev->attached_dev) directly, not through
phylink_link_down(). The forced major config then reads this as false:

	cur_link_state = phylink_link_is_up(pl);

so it skips phylink_link_down()/mac_link_down(). Because
pl->phy_state.link is still 0, no down transition follows either. The next
PHY link-up would then call mac_link_up() a second time with no
mac_link_down() in between.

Does that break the mac_link_up()/mac_link_down() pairing that MAC drivers
expect?

[ ... ]

> @@ -2481,6 +2665,8 @@ void phylink_disconnect_phy(struct phylink *pl)
>  
>  	ASSERT_RTNL();
>  
> +	phylink_late_phy_cancel(pl);
> +

[Severity: Low]
Is the deferred connect lost if an SFP PHY comes and goes while it is
armed?

If phylink_sfp_connect_phy() attaches a module PHY while the deferral is
armed, phylink_late_phy_poll() stops for good:

	/* A PHY arrived by another path, an SFP for one, while queued. */
	if (!phylink_late_phy_pending(pl)) {
		rtnl_unlock();
		return;
	}

When the module is removed, phylink_sfp_disconnect_phy() calls
phylink_disconnect_phy(). That now calls phylink_late_phy_cancel() and
drops late_phy_fwnode, even though the phy-handle PHY was never attached.

Nothing re-arms the deferral, so the needs-host-firmware PHY is never
connected, even after its driver binds. For a MAC that connects once, the
port stays without that PHY until the MAC is reprobed.

Reaching this needs a MAC with both an sfp property and a phy-handle to a
needs-host-firmware PHY. That may be unusual, but the comment in the
poller suggests this path is expected.

[ ... ]

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 3a88a69882a61..6249156b51f15 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -147,6 +147,9 @@ enum phylink_op_type {

[ ... ]

> + * @phy_may_probe_late: if true, a connect to a PHY marked needs-host-firmware
> + *			whose driver has not bound yet is deferred until that
> + *			driver binds; see phylink_fwnode_phy_connect().

[Severity: Low]
This isn't a bug today, but should phy_may_probe_late be documented as
incompatible with mac_requires_rxc, or checked against it?

While the connect is deferred, PHY_F_RXC_ALWAYS_ON is not passed to any
PHY. A MAC that needs RX_CLK during its own init would go through that
init with no PHY keeping it running.

No current configuration reaches this. In-tree only stmmac sets
mac_requires_rxc, and it does not opt in (the commit message names it as
a MAC that must not). mt7530 does not set mac_requires_rxc.

Adding !pl->config->mac_requires_rxc to the gate in
phylink_fwnode_phy_connect(), or noting the conflict here, would cover
future users.

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

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

[Aleksei Sviridkin]

> should phy_may_probe_late be documented as
> incompatible with mac_requires_rxc, or checked against it?

No. The deferred attach still passes PHY_F_RXC_ALWAYS_ON, and before
it the PHY would only have genphy, which ignores the flag.

The other points are valid and will be fixed in v6.

Aleksei

pw-bot: cr

[PATCH net-next v5 3/3] net: dsa: let user ports wait for a PHY that probes late

[Aleksei Sviridkin]

A user port connects its PHY once, when the switch sets up its ports.
If that PHY needs firmware from the host and its driver is a module not
loaded yet, the port gets no working PHY for the rest of the uptime.

Let a switch driver opt its user ports in to phylink waiting for such a
PHY, and opt in mt7530. Until the PHY attaches, .port_enable sees a NULL
phy, so the opt-in is per driver: qca8k dereferences that argument, and
gswip programs the PHY address from it at open, so a late attach leaves
it wrong until the next open. mt7530 does not use it. Shared ports are
left out.

Found on a Keenetic KN-1012 (MT7981B with an MT7531 switch), where the
EN8811H behind lan4 has its driver on the root filesystem and lan4 was
lost for the uptime.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---
New in v5.

 drivers/net/dsa/mt7530.c | 1 +
 include/net/dsa.h        | 5 +++++
 net/dsa/user.c           | 1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7781a63b4e6f..e4c155605e27 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -3548,6 +3548,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	priv->ds->priv = priv;
 	priv->ds->ops = &mt7530_switch_ops;
 	priv->ds->phylink_mac_ops = &mt753x_phylink_mac_ops;
+	priv->ds->phy_may_probe_late = true;
 	mutex_init(&priv->reg_mutex);
 	spin_lock_init(&priv->stats_lock);
 	INIT_DELAYED_WORK(&priv->stats_work, mt7530_stats_poll);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 5d12191b6f6f..f7e78c3a123d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -455,6 +455,11 @@ struct dsa_switch {
 	 */
 	u32			dscp_prio_mapping_is_global:1;
 
+	/* Drivers whose .port_enable copes with a NULL phy may set this to
+	 * let user ports wait for a PHY that needs host firmware.
+	 */
+	u32			phy_may_probe_late:1;
+
 	/* Listener for switch fabric events */
 	struct notifier_block	nb;
 
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 041f9060c8ef..41075bb74193 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -2661,6 +2661,7 @@ static int dsa_user_phy_setup(struct net_device *user_dev)
 
 	dp->pl_config.dev = &user_dev->dev;
 	dp->pl_config.type = PHYLINK_NETDEV;
+	dp->pl_config.phy_may_probe_late = ds->phy_may_probe_late;
 
 	/* The get_fixed_state callback takes precedence over polling the
 	 * link GPIO in PHYLINK (see phylink_get_fixed_state).  Only set
-- 
2.53.0

Re: [PATCH net-next v5 3/3] net: dsa: let user ports wait for a PHY that probes late

[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] This patch arms phylink's self-requeueing late-PHY poller
  (pl->late_phy_poll on system_freezable_power_efficient_wq) for…
- [Low] The comment this patch adds on struct
  dsa_switch::phy_may_probe_late (include/net/dsa.h:458-460) says a…

--

net: dsa: let user ports wait for a PHY that probes late

This adds a phy_may_probe_late bit to struct dsa_switch. dsa_user_phy_setup()
passes it to phylink so that a user port can wait for a PHY whose driver
needs host firmware and probes late. mt7530 opts in.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 7781a63b4e6fd..e4c155605e27f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3548,6 +3548,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
>  	priv->ds->priv = priv;
>  	priv->ds->ops = &mt7530_switch_ops;
>  	priv->ds->phylink_mac_ops = &mt753x_phylink_mac_ops;
> +	priv->ds->phy_may_probe_late = true;

[Severity: Medium]

With this opt-in, phylink's late PHY poller (pl->late_phy_poll) can be
pending on mt7530 user ports. Does anything on the switch shutdown path
cancel it?

mt7530_shutdown() and the mmio shutdown handler both call
dsa_switch_shutdown(). That function closes the conduits, detaches and
unlinks the user netdevs, and clears conduit->dsa_ptr:

net/dsa/dsa.c:dsa_switch_shutdown() {
    ...
	dsa_switch_for_each_user_port(dp, ds) {
		conduit = dsa_port_to_conduit(dp);
		user_dev = dp->user;

		netif_device_detach(user_dev);
		netdev_upper_dev_unlink(conduit, user_dev);
	}
    ...
}

It never calls phylink_disconnect_phy() or phylink_destroy(). Those two
functions and phylink_fwnode_phy_connect() are the only callers of
phylink_late_phy_cancel(). phylink_stop() does not cancel the work either.

Suppose the PHY driver binds shortly before a reboot or kexec. A poll that
is still queued (up to 30 seconds out after backoff) can then run after
.shutdown has returned:

phylink_late_phy_poll()
    phylink_attach_phy()
        phy_attach_direct()
            phy_init_hw()
            phy_resume()
    phylink_bringup_phy()
        phy_request_interrupt()

Could this cause MDIO traffic, a PHY soft reset and config_init, and an
IRQ request on hardware that has already been shut down? Could it also
leave that interrupt enabled for a kexec'd kernel?

For comparison, sfp_shutdown() calls cancel_delayed_work_sync() on
sfp->poll and sfp->timeout.

>  	mutex_init(&priv->reg_mutex);
>  	spin_lock_init(&priv->stats_lock);
>  	INIT_DELAYED_WORK(&priv->stats_work, mt7530_stats_poll);
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 5d12191b6f6f5..f7e78c3a123d3 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -455,6 +455,11 @@ struct dsa_switch {
>  	 */
>  	u32			dscp_prio_mapping_is_global:1;
>  
> +	/* Drivers whose .port_enable copes with a NULL phy may set this to
> +	 * let user ports wait for a PHY that needs host firmware.
> +	 */
> +	u32			phy_may_probe_late:1;

[Severity: Low]

Is "copes with a NULL phy" the whole requirement for opting in?

The only caller of .port_enable is dsa_user_open() ->
dsa_port_enable_rt(dp, dev->phydev). When phylink_late_phy_poll() attaches
the PHY later, it forces a major config and calls phy_start(). It does not
call .port_enable again with the new phy.

gswip_port_enable() already handles a NULL phy:

	if (phydev)
		mdio_phy = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;

So gswip meets the rule as written. Yet the commit message says gswip must
not opt in, because the PHY address would stay wrong until the next open.

The same applies to qca8k. Adding an if (phy) check around
phy_support_asym_pause(phy) in qca8k_port_enable() would satisfy this
comment. A PHY that attaches late would then never advertise asym pause.

Should the comment say that the driver must not depend on the phy argument
of .port_enable at all? That would match the reasoning in the commit
message.

> +
>  	/* Listener for switch fabric events */
>  	struct notifier_block	nb;
>  

[ ... ]

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