Re: [PATCH net-next v8 6/7] of: property: do not let "pses" block a consumer's probe

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] At this commit only, the full PHY driver probe and remove now
  run on every deferral of the MDIO bus owner while the PSE controller…
- [Medium] At this commit only, deferred probing can loop without end
  while the PSE controller is absent.
- [Low] At this commit the commit message describes phylib behaviour that
  does not exist yet.

--

of: property: do not let "pses" block a consumer's probe

This marks the "pses" supplier binding in of_supplier_bindings[] with
FWLINK_FLAG_IGNORE, so fw_devlink no longer creates a device link from a
PHY to its PSE controller. The goal is to stop a MAC or DSA switch from
binding genphy while the PHY's own driver is held back by that link,
once phylib gets PSE handles from a notifier.

> The link is not needed for correctness. PSE is not a resource the
> consumer must have before it probes: phylib looks a PI up when its
> controller becomes available and releases it when the controller goes
> away, and of_pse_control_get() is the only reader of the property.

[Severity: Low]
Is this description accurate at this point in the series?

At this commit, pse_register_notifier() in drivers/net/pse-pd/pse_core.c
has no callers. The only PSE consumer is still
fwnode_mdiobus_register_phy()->fwnode_find_pse_control(). It takes the
handle once, when the PHY is registered, and the handle is only released
in phy_device_remove():

drivers/net/phy/phy_device.c:phy_device_remove() {
    ...
	pse_control_put(phydev->psec);
    ...
}

The same applies to this later paragraph:

> binds. Both are given up on purpose: phylib's notifier takes the PI
> away on unbind and hands it back on bind without tearing the PHY driver
> down, and with no deferral left there is nothing for an autoprobe to
> wait for.

With only this patch applied, the managed link that passed a PSE
controller unbind on to the PHY driver is gone, and nothing replaces it.

The replacement comes in the final patch of the series, "net: phy: own
phydev->psec via PSE notifier and remove fwnode_mdio hook". That patch
adds phy_pse_notifier and removes fwnode_find_pse_control().

Could the message say that a later patch adds the notifier behaviour, so
it reads correctly for anyone who bisects or backports to this commit?

> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 72cf12907de03..5ec0f05b87ac3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1564,7 +1564,10 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_backlight, },
>  	{ .parse_prop = parse_panel, },
>  	{ .parse_prop = parse_msi_parent, },
> -	{ .parse_prop = parse_pses, },
> +	{
> +		.parse_prop = parse_pses,
> +		.fwlink_flags = FWLINK_FLAG_IGNORE,
> +	},

[Severity: Medium]
fwnode_mdio still does the PSE lookup at this point. Does that mean every
deferral of the MDIO bus owner now runs a full PHY driver probe and
remove, including the hardware setup done in probe?

With the link ignored, fw_devlink_create_devlink() returns early:

drivers/base/core.c:fw_devlink_create_devlink() {
    ...
	if (link->flags & FWLINK_FLAG_IGNORE)
		return 0;
    ...
}

So device_links_check_suppliers() no longer holds the PHY's probe back.

fwnode_mdiobus_register_phy() still registers the PHY before the lookup,
and PHY drivers are PROBE_FORCE_SYNCHRONOUS. While the PSE controller is
unbound, each retry therefore goes through:

fwnode_mdiobus_register_phy()
  fwnode_mdiobus_phy_device_register()
    phy_device_register()
      device_add()
        phy_probe()            <- deasserts reset, calls drv->probe
  fwnode_find_pse_control()
    of_pse_control_get()       <- returns -EPROBE_DEFER
  phy_device_remove()
    phy_remove()
      phy_device_reset(phydev, 1)

Some probe routines do real work here. For example, aqr107_probe() loads
firmware through aqr_firmware_load(), and that now happens again on every
retry until the PSE controller binds.

The commit message mentions the probe/remove cycle, but not the repeated
reset toggling or firmware loading.

The cycle goes away later in the series with "net: phy: own phydev->psec
via PSE notifier and remove fwnode_mdio hook". That patch removes
fwnode_find_pse_control() from fwnode_mdiobus_register_phy(). A bisect or
partial backport that stops at this commit would still hit the cycle.

[Severity: Medium]
Can this also make deferred probing loop forever while the PSE controller
is absent?

The PHY driver now binds inside device_add() during the MAC or DSA probe,
so driver_bound() fires a deferred probe trigger:

drivers/base/dd.c:driver_bound() {
    ...
	driver_deferred_probe_del(dev);
	driver_deferred_probe_trigger();
    ...
}

Next, the PSE lookup returns -EPROBE_DEFER, the PHY is removed, and the
MAC/DSA probe returns -EPROBE_DEFER. driver_probe_device() then sees that
a trigger happened while it was probing:

drivers/base/dd.c:driver_probe_device() {
    ...
		if (trigger_count != atomic_read(&deferred_trigger_count) &&
		    !defer_all_probes)
			driver_deferred_probe_trigger();
    ...
}

That puts the MAC straight back on the active list, and
deferred_probe_work_func() picks it up again.

The PSE controller may never register, for example if its driver is not
built, its probe fails, or one of its own suppliers is missing. In that
case of_pse_control_get() keeps returning -EPROBE_DEFER and the loop never
ends.

Before this patch, device_links_check_suppliers() deferred the PHY's
probe. driver_bound() therefore did not run, and the trigger count did
not change on these retries. That holds at least on buses where every
PHY with a driver has a pses link.

The final patch of the series, "net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook", removes the PSE lookup from
fwnode_mdiobus_register_phy(), and the loop goes with it.

Would it be better to apply this change after that patch, so that no
intermediate commit behaves like this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004164219.1161294-1-github%40szelinsky.de