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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] The supply pre-check can still pass when the regulator core
  will defer the same PI.
- [Medium] This patch adds an -EPROBE_DEFER exit in
  pse_controller_register() (pse_core.c:1226-1227 and 1238-1240) that…
- [Low] The commit message leaves out a present-day behaviour change.
- [Low] The concern says the commit message relies on an unreachable
  premise: no in-tree consumer retries only on PSE_REGISTERED, and…

--

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

pse_controller_register() now checks each PI's vpwr supply after
setup_pi_matrix() and before any PI regulator is registered. A missing
provider therefore makes the PSE driver's own probe defer. Like
regulator_resolve_supply(), the check looks at the PI node first and then
at the controller device.

> That leaves pse_controller_register() completing for a controller whose
> PIs cannot be handed out: regulator_get_exclusive() in
> pse_control_get_internal() resolves the supply itself and returns
> -EPROBE_DEFER until the provider appears, so of_pse_control_get() keeps
> failing for this PI even though the controller is registered. A
> consumer that only retries when a controller registers - which is what
> phylib becomes once it attaches from the PSE notifier - then never gets
> its PI.

[Severity: Low]
This isn't a bug, but the commit message only describes the future
phylib notifier case. It leaves out a change that applies today.

Before this patch, a PI whose vpwr provider was not registered yet when
pse_controller_register() ran got a PI regulator with
rdev->supply == NULL. pse_register_pw_ds() is called only once, and it
skips that PI:

	if (!rdev || !rdev->supply)
		continue;

So pcdev->pi[i].pw_d stayed NULL for good. That disabled budget
evaluation, power allocation and priority for that PI:
pse_pw_d_is_sw_pw_control() returns false, and pse_ethtool_set_prio()
returns -EOPNOTSUPP with "no power domain attached". This happened even
with the current fwnode_mdio consumer, which does get the PI once the
provider appears.

With the new check the controller defers until the supply is there, so
these PIs now get a power domain.

Could the commit message mention this? It matters when deciding on a
Fixes: tag or a stable backport.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index eeefbf25e6716..e560833ad0340 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -860,6 +861,98 @@ static const struct regulator_ops pse_pi_ops = {

[ ... ]

> +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 while the PI regulator registration still defers its
supply?

of_regulator_get_optional() here, and regulator_get_optional() in
pse_controller_check_supply(), both go through _regulator_get_common().
That path never checks whether the provider's parent device is bound.
The resolution done when the PI regulator is registered does check it:

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

Consider a PSE probe that overlaps the probe of an external vpwr
provider. The provider has called regulator_register() but
driver_bound() has not run yet, or the provider later fails its probe
and unregisters. This check then returns 0, and the PI regulators
register with rdev->supply == NULL. PSE_REGISTERED then fires, and
of_pse_control_get()->regulator_get_exclusive() returns -EPROBE_DEFER
for that PI.

Later in the series, "net: phy: own phydev->psec via PSE notifier and
remove fwnode_mdio hook" makes phylib retry only on PSE_REGISTERED.
Wouldn't phylib then miss this PI until the next PSE_REGISTERED or a PHY
re-registration? Later patches in the series do not change these check
functions.

The commit message mentions this window, but not a second effect that
does not go away on its own. pse_register_pw_ds() is called only once,
from pse_controller_register(), and skips a PI whose supply is not
resolved:

	if (!rdev || !rdev->supply)
		continue;

Does pcdev->pi[i].pw_d then stay NULL for that PI even after the
regulator core resolves the supply later? Budget, power allocation and
priority handling would stay off until the controller is re-bound.

The tps23881 and si3474 bindings put vpwr-supply in the pse-pi nodes.
As the commit message notes, that only creates a SYNC_STATE_ONLY
fw_devlink link, which does not hold back the controller's probe. Async
probing or module loads on different CPUs could reach this.

[ ... ]

> @@ -1116,6 +1210,38 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  			goto release_pis;
>  	}
>  
> +	/* Check every PI supply before any regulator of ours is registered:

[ ... ]

> +	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 -EPROBE_DEFER exit leak the pd692x0 power budget claimed on
the vmain regulator?

This exit, and the one after pse_controller_check_supply(), run after
pcdev->ops->setup_pi_matrix(). On pd692x0, setup_pi_matrix() has already
done two things that release_pis does not undo:

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

Also, when !priv->cfg_saved, it programs the MCU power bank and port
matrix and saves the user byte.

The budget is released only from setup_pi_matrix()'s own error labels or
from pd692x0_i2c_remove(). pd692x0_i2c_probe() just returns the error:

	ret = devm_pse_controller_register(dev, &priv->pcdev);
	if (ret)
		return dev_err_probe(dev, ret,

Regulator put and unregister do not lower rdev->pw_requested_mW; only
regulator_free_power_budget() does. So each deferred retry adds another
claim on vmain.

Suppose vmain has regulator-power-budget-milliwatt set. On retry,
regulator_get_unclaimed_power_budget() returns 0, so manager_pw_budget[i]
stays 0. cfg_saved is now true, so the MCU is not reprogrammed and keeps
the first attempt's budget, and the driver and hardware disagree. With
PSE_BUDGET_EVAL_STRAT_DYNAMIC, could enabling any PI whose supply chains
through a manager to vmain then fail with -ERANGE in
regulator_request_power_budget()?

The documented binding points each pse-pi vpwr-supply at the driver's
own manager regulators, so it does not hit this. The schema does not
restrict the phandle target, though. A pd692x0 board with one PI on an
external provider that has not probed yet would hit this on every retry.

The commit message acknowledges this and leaves the cleanup to the
driver. This patch adds a deferral at this point that is retried by
design. Should the pd692x0 budget cleanup come with it in this series?

[ ... ]

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