Re: [PATCH net-next 4/4] net: pse-pd: check the PI vpwr supply before registering the controller

[netdev-bot+sashiko]

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] The new check in pse_pi_check_supply() and
  pse_controller_check_supply() is weaker than what the regulator core…
- [Medium] The new check loop in pse_controller_register() can fail
  (typically with -EPROBE_DEFER) after pcdev->ops->setup_pi_matrix() has…
- [Low] The comment above pse_controller_check_supply() says the core's
  fallback is 'a vpwr-supply on the controller node'.

--

net: pse-pd: check the PI vpwr supply before registering the controller

This commit adds a check to pse_controller_register(). After
setup_pi_matrix() and before the PI regulators are registered, it looks
up each PI's vpwr supply, or the controller's fallback vpwr supply. If a
supply is missing, the PSE driver's probe is deferred, so that a consumer
that looks a PI up when the controller registers does not get
-EPROBE_DEFER.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 05b02d736931c..44986749600e3 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -826,6 +827,65 @@ static const struct regulator_ops pse_pi_ops = {
>  	.set_current_limit = pse_pi_set_current_limit,
>  };
>  

[ ... ]

> +static int pse_pi_check_supply(struct pse_controller_dev *pcdev, int id)
> +{

[ ... ]

> +	of_node_put(np);
> +	supply = of_regulator_get_optional(pcdev->dev, pcdev->pi[id].np,
> +					   "vpwr");
> +	if (!IS_ERR(supply)) {
> +		regulator_put(supply);
> +		return 0;
> +	}

[Severity: Medium]

Can this check pass even though resolving the PI's supply later still
defers?

of_regulator_get_optional() and regulator_get_optional() both go through
_regulator_get_common(). That calls regulator_resolve_supply() on the
supply rdev itself, which only resolves the supply's own upstream supply.
So the check never asks whether the supply's parent device is bound.
pse_controller_check_supply() accepts the supply under the same
condition.

When devm_pse_pi_regulator_register() registers the PI regulator,
regulator_resolve_supply() on the PI rdev does run that bound check:

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

regulator_register() ignores that error, which leaves the PI rdev with
rdev->supply == NULL. pse_register_pw_ds() then skips the PI, and the
controller is still added to pse_controller_list.

A PI lookup in that window still returns -EPROBE_DEFER:

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

The window opens when the supplier driver has called regulator_register()
but its probe has not returned yet. Examples are async probing,
fw_devlink=permissive or off, or a supplier probe that registers the
regulator and then defers. regulator_put() runs right after the check, so
nothing holds the supplier in place between the check and PI
registration.

The commit message says:

  a supply that is registered, but whose device is not bound yet, still
  passes, and that PI then gets no power domain.

In this case, wouldn't PI lookups also keep returning -EPROBE_DEFER after
the controller has registered? That looks like the failure the commit
message gives as its motivation, for a consumer such as the planned
phylib PSE notifier that only looks a PI up when a controller registers.

Also, pse_register_pw_ds() only runs at registration. Would that PI then
never get a power domain?

> +
> +	ret = PTR_ERR(supply);
> +	if (ret == -ENODEV)
> +		return 1;
> +
> +	return dev_err_probe(pcdev->dev, ret,
> +			     "PI %d: failed to get vpwr supply\n", id);
> +}
> +
> +/* The core's fallback: a vpwr-supply on the controller node. */

[Severity: Low]

This isn't a bug, but is this comment accurate? The lookup goes
regulator_get_optional(pcdev->dev, "vpwr")->regulator_dev_lookup()->
regulator_dt_lookup()->of_get_regulator(). If the controller node has no
vpwr-supply property, of_get_regulator() does this:

	regnode = of_get_child_regulator(dev->of_node, prop_name);

That walks every descendant node, including sibling pse-pi nodes, before
it falls back to the supply map and the lookup by name.

This matches what the regulator core does. However, the comment on
pse_pi_check_supply() names the sibling PI fallback as the hazard it
avoids.

Could this comment say that the fallback also walks the controller's
child nodes?

> +static int pse_controller_check_supply(struct pse_controller_dev *pcdev)
> +{

[ ... ]

> @@ -1102,6 +1163,26 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  			goto release_pis;
>  	}
>  
> +	/* Check vpwr supplies while pcdev->pi can still be freed. Not before
> +	 * setup_pi_matrix(): pd692x0 registers the regulators its PIs name
> +	 * there.
> +	 */
> +	for (i = 0; i < pcdev->nr_lines; i++) {
> +		ret = pse_pi_check_supply(pcdev, i);
> +		if (ret < 0)
> +			goto release_pis;

[Severity: Medium]

Does this new exit, and the pse_controller_check_supply() exit just
below it, leak the vmain power budget that pd692x0 reserved in
setup_pi_matrix()?

By this point pd692x0_setup_pi_matrix() has already returned success,
after this call chain:

pd692x0_setup_pi_matrix()
  pd692x0_req_managers_pw_budget()
    regulator_request_power_budget(supply, pw_budget);

That raises rdev->pw_requested_mW on vmain. The reservation is released
only by pd692x0_managers_free_pw_budget(), and that is called only from:

  - the error labels inside pd692x0_setup_pi_matrix()
  - pd692x0_i2c_remove(), which does not run when probe fails

release_pis only calls pse_release_pis(), and no driver hook undoes
setup_pi_matrix(). When devm unregisters the manager regulators, it puts
the vmain consumer handle, but that does not lower pw_requested_mW
either.

On the next probe attempt, regulator_get_unclaimed_power_budget() returns
the reduced value, possibly 0, so manager_pw_budget[i] comes out smaller.
cfg_saved is already true, so the hardware power bank is not
reprogrammed. The first reservation can never be freed, because remove
only frees manager_pw_budget[i].

This can happen when a pd692x0 PI's vpwr-supply points at a regulator
that is late or missing, so pse_pi_check_supply() returns -EPROBE_DEFER.

Would it make sense to give drivers a way to undo setup_pi_matrix() on
this error path?

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