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

[Paolo Abeni]

On 10/6/26 14:47, Aleksei Sviridkin wrote:
> 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.
> 
> 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. It is limited to PHY mode without an
> SFP cage: in-band, the PCS can bring the link up with no PHY to gate it,
> and an SFP PHY can take the port while the wait is armed and drop the
> wait on removal. The poll runs only while phylink is started, so nothing
> touches the PHY of a stopped port, as at switch shutdown; a port that is
> down attaches at its next start.
> 
> 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 or the attach fails; when the driver is gone after the
> attach, the poll keeps waiting without counting a failed connect. 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 and the DSA opt-in that follows, lan4 attaches once the
> module has loaded and the port is up. 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 at lex.la>
> ---
> Changes in v6:
>   - defer only in MLO_AN_PHY and without an SFP bus
>   - poll only while started: phylink_start() queues the poll,
>     phylink_stop() cancels it, the poller's STOPPED test is gone
>   - a failed attach with no usable driver is a lost race, not a retry
>   - commit message: ndo_open argument dropped, board sentence updated
> 
>   drivers/net/phy/phylink.c | 232 ++++++++++++++++++++++++++++++++++++--
>   include/linux/phylink.h   |   5 +
>   2 files changed, 229 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b2..ec76138f1065 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -98,6 +98,16 @@ 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;
> +	bool late_phy_gave_up;
>   };
>   
>   #define phylink_printk(level, pl, fmt, ...) \
> @@ -1831,6 +1841,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 +2011,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 +2091,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 +2362,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 +2401,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 +2413,134 @@ 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 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;
> +	} else if (ret && !phylink_phy_is_usable(phy_dev)) {
> +		/* The driver went away under the attach: wait for it again. */
> +		lost_race = true;
> +	}
> +	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.
> +			 */
> +			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) {

The above `if` chains feels unneedly complex. I think you can simplify
it consolidating the out/error paths using more labels.

/P