Re: [PATCH net-next v7 5/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 3 (1) · Low: 3

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] phy_try_attach_pse() drops -EPROBE_DEFER silently on the
  assumption that a later PSE_REGISTERED event will retry.
- [Medium] PHY registration now succeeds before the PSE controller
  exists, but fw_devlink still treats `pses` as a supplier (parse_pses…
- [Medium] PHY attach now runs synchronously from PSE_REGISTERED inside
  pse_controller_register(), which the PSE driver calls in the middle of…
- [Low] The commit message says the patch fixes user-visible problems: a
  deferred-probe retry storm, and PHYs or DSA switches that never…
- [Low] The comment above phy_try_attach_pse() says 'Any other error
  means a broken binding'.
- [Low] pd692x0_i2c_probe() registers the PSE controller even when
  fw_state is PD692X0_FW_BROKEN or PD692X0_FW_NEED_UPDATE.

Pre-existing issues:
- [High] struct pse_control stores a raw `attached_phydev` pointer and
  holds no device reference on it.
- [Medium] pse_control_get_internal() leaks a module reference.

--

net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

This patch moves ownership of phydev->psec from
fwnode_mdiobus_register_phy() into phylib. The handle is attached from
phy_device_register() and from the PSE_REGISTERED notifier walk. It is
released from phy_device_remove() and from the PSE_UNREGISTERED walk.

A new pse_phy_mutex in pse_core serialises phydev->psec access in place of
rtnl, and the ethtool PSE paths take it as well.

Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/

> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index ba70915182654..7bd979b59f496 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c

[ ... ]

> @@ -159,14 +136,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  			goto clean_phy;
>  	}
>  
> -	psec = fwnode_find_pse_control(child, phy);
> -	if (IS_ERR(psec)) {
> -		rc = PTR_ERR(psec);
> -		goto unregister_phy;
> -	}
> -
> -	phy->psec = psec;
> -

[Severity: Low]
The commit message describes a user-visible deferred probe retry storm, and
there are Reported-by and Closes tags, but no Fixes: tag. The -EPROBE_DEFER
pass-through removed here came from commit 5e82147de1cb ("net: mdiobus:
search for PSE nodes by parsing PHY nodes.").

Should a Fixes: tag be added? Leaving it out may be intentional. The earlier
patches in the series suggest it targets net-next. A Fixes: tag could also
pull this patch into stable without the patches it depends on.

[ ... ]

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 5b13a74e2fa92..971d9326d2265 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1012,9 +1013,111 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>  }
>  EXPORT_SYMBOL(get_phy_device);
>  
> +/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
> + * Caller must hold pse_phy_lock(). A missing phandle (-ENOENT) or a
> + * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
> + * retries the latter at PSE_REGISTERED time. Any other error means a broken
> + * binding and is warned about, but left non-fatal so the phy still registers.

[Severity: Low]
This isn't a bug, but is "Any other error means a broken binding" accurate?

of_pse_control_get() -> pse_control_get_internal() can also fail with:

  - -ENOMEM from the allocation
  - errors from pse_pi_is_hw_enabled() -> ops->pi_get_admin_state(), such
    as i2c failures or a firmware-unavailable code
  - regulator_get_exclusive() errors such as -EBUSY or -EPERM

The commit message itself calls these transient failures.

> + *
> + * A phy with psec_detached set is skipped: it is either not registered yet
> + * or on its way out, and nothing would release a handle attached now.
> + */
> +static void phy_try_attach_pse(struct phy_device *phydev)
> +{

[ ... ]

> +	psec = of_pse_control_get(np, phydev);
> +	if (IS_ERR(psec)) {
> +		if (PTR_ERR(psec) != -EPROBE_DEFER && PTR_ERR(psec) != -ENOENT)

[Severity: Medium]
Can a controller that is already registered still return -EPROBE_DEFER
here? The commit message says:

  -EPROBE_DEFER stays silent because the notifier retries it at
  PSE_REGISTERED time, and the preceding patch makes sure an already
  registered controller cannot keep returning it.

pse_control_get_internal() can still defer through the PI's vpwr supply:

pse_control_get_internal()
  regulator_get_exclusive()
    _regulator_get_common()
      regulator_resolve_supply()

drivers/regulator/core.c:regulator_resolve_supply() {
    ...
	if (r->dev.parent && r->dev.parent != rdev->dev.parent) {
		if (!device_is_bound(r->dev.parent)) {
			put_device(&r->dev);
			ret = -EPROBE_DEFER;
			goto out;
		}
	}
    ...
}

The preceding patch adds the pse_pi_check_supply() pre-check. That check
only calls of_regulator_get_optional(), which does not look at
device_is_bound(). That patch's message also says "a PSE probe
interleaving with the provider's own probe can still pass the check and
resolve late".

In that case PSE_REGISTERED fires, the lookup defers, and the phy is
skipped with no message. When the supply resolves later, nothing triggers
another PSE_REGISTERED or phy registration.

Would the port then stay at "No PSE is attached" for the rest of the boot?
Before this patch, deferred probe of the MDIO bus would have retried it.

> +			phydev_warn(phydev, "failed to get PSE control: %pe\n",
> +				    psec);
> +		return;
> +	}

[Severity: Low]
This isn't a regression, because the commit message documents it and the
failure is logged. There is one case where no later retry ever comes,
though.

pd692x0_i2c_probe() registers the controller while fw_state is still
PD692X0_FW_BROKEN or PD692X0_FW_NEED_UPDATE, and firmware_upload_register()
runs after that. The PSE_REGISTERED walk gets the pd692x0_fw_unavailable()
error from pi_get_admin_state() and gives up here.

After a successful firmware upload, pd692x0_fw_poll_complete() only does:

	priv->fw_state = PD692X0_FW_OK;
	return FW_UPLOAD_ERR_NONE;

Nothing re-runs the attach, so the phys stay without PSE until the PSE or
MAC driver is rebound. Is that the intended behaviour?

> +
> +	phydev->psec = psec;
> +}

[ ... ]

> +static int phy_pse_detach_one(struct device *dev, void *data)
> +{

[ ... ]

> +	phydev->psec = NULL;
> +	pse_control_put(psec);
> +	return 0;
> +}

[Severity: Medium]
Can this power down ports when a PSE driver fails later in its own probe?

The PSE_REGISTERED walk now runs synchronously inside
pse_controller_register(), partway through the PSE driver's probe. For a
PI that the hardware already has powered, pse_control_get_internal() sets
admin_state_enabled from pse_pi_is_hw_enabled(), and
regulator_get_exclusive() sets use_count to 1.

If a later probe step fails, devm unwinding calls
pse_controller_unregister(), and this walk drops the last reference:

phy_pse_detach_one()
  pse_control_put()
    __pse_control_release()
      if (psec->pcdev->pi[psec->id].admin_state_enabled)
          regulator_disable(psec->ps);
            -> pse_pi_disable() -> ops->pi_disable()

For example, tps23881_i2c_probe() calls tps23881_setup_irq() after
devm_pse_controller_register(), and that returns -EINVAL when client->irq
is 0:

	if (!irq) {
		dev_err(&client->dev, "interrupt is missing");
		return -EINVAL;
	}

pd692x0 can fail the same way in firmware_upload_register().

On a tps23881 board with no interrupt in DT, would every boot switch off
ports that were delivering power before the probe began? Before this
patch, a failed PSE probe left those ports alone. A normal unbind now also
powers down every PI attached to a phy.

> +static int phy_pse_notifier_event(struct notifier_block *nb,
> +				  unsigned long event, void *data)
> +{
> +	switch (event) {
> +	case PSE_REGISTERED:
> +		pse_phy_lock();
> +		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
> +				 phy_pse_attach_one);
> +		pse_phy_unlock();
> +		return NOTIFY_OK;

[Severity: Medium]
This is a pre-existing issue, but does this retry make a module reference
leak in pse_control_get_internal() easier to hit?

After try_module_get(pcdev->owner) succeeds, two error paths jump past
the module_put(): the !pcdev->ops->pi_get_admin_state branch and the
pse_pi_is_hw_enabled() failure:

	ret = pse_pi_is_hw_enabled(pcdev, index);
	if (ret < 0)
		goto free_psec;

The put_module label sits above free_psec, so it is skipped.

The lookup is now best effort. It is repeated for every unattached phy on
each PSE_REGISTERED walk, and again in phy_device_register(). Each attach
that hits a failing pi_get_admin_state() (an i2c error, or a pd692x0
firmware-unavailable code) would leak one reference. The PSE driver
module could then no longer be unloaded.

Should those paths use goto put_module instead?

[ ... ]

> @@ -1034,12 +1137,25 @@ int phy_device_register(struct phy_device *phydev)

[ ... ]

> +	pse_phy_lock();
> +	phydev->psec_detached = false;
> +	phy_try_attach_pse(phydev);
> +	pse_phy_unlock();
> +
>  	return 0;

[Severity: Medium]
Could this leave the phy bound to the generic driver? Registration now
succeeds before the PSE controller exists, but fw_devlink still treats
pses as a supplier:

drivers/of/property.c:of_supplier_bindings[] {
	...
	{ .parse_prop = parse_pses, },
	...
}

With fw_devlink enforcing, device_links_check_suppliers() defers the phy's
own driver probe until the PSE device binds. If the MAC or DSA switch
connects to the phy in that window, phy_attach_direct() falls back to the
generic driver:

	if (!d->driver) {
		if (phydev->is_c45)
			d->driver = &genphy_c45_driver.mdiodrv.driver;
		else
			d->driver = &genphy_driver.mdiodrv.driver;
		phydev->is_genphy_driven = 1;
	}

device_bind_driver() -> device_links_force_bind() then binds it despite
the pending supplier link.

When the PSE module loads later, the notifier only attaches phydev->psec.
Would the phy then stay on genphy until a reboot or rebind?

Before this patch, phy registration itself deferred until the PSE
controller registered, so this window did not exist. The trigger is the
modular PSE case this patch is aimed at.

> @@ -1061,8 +1177,22 @@ EXPORT_SYMBOL(phy_device_register);

[ ... ]

> +	pse_phy_lock();
> +	psec = phydev->psec;
> +	phydev->psec = NULL;
> +	phydev->psec_detached = true;
> +	pse_control_put(psec);
> +	pse_phy_unlock();
> +
>  	device_del(&phydev->mdio.dev);

[Severity: High]
This is a pre-existing issue, but psec->attached_phydev is a raw pointer
that holds no device reference. Neither this path nor phy_pse_detach_one()
clears it.

Can pse_send_ntf_worker() still use it after the phy has been freed?

pse_send_ntf_worker()
  psec = pse_control_find_by_id(pcdev, ntf.id);
      (takes a kref, then drops pse_list_mutex)
  rtnl_lock();
      (may block)
  netdev = pse_control_get_netdev(psec);
      return psec->attached_phydev->attached_dev;

Suppose the netdev detaches the phy and the MDIO bus is torn down while
the worker waits on rtnl. This put drops the count from 2 to 1, then
device_del() and phy_device_free() free the phy_device. The worker then
reads freed memory.

There is a second route when two phys resolve the same PI id.
pse_control_get_internal() returns the existing psec with a kref_get(),
but it only sets attached_phydev when it creates a new psec. After the
first phy is removed, the surviving phy's psec still points at the freed
phy.

The worker never takes pse_phy_lock(), so the new lock does not cover
this.

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