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

[netdev-bot+sashiko@kernel.org]

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