Re: [PATCH net] net: pse-pd: don't enable a PI again while disabling it

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (5) · Low: 0 (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).

- [High] The patch removes the power-domain retry from
  pse_disable_pi_pol().

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  pse_pi_disable() drops the error from _pse_pi_disable() and always…
- [Medium] This problem wasn't introduced by this patch, but in the
  DISCONNECTION branch of pse_set_config_isr() isr_pd_detected is…
- [Medium] This problem wasn't introduced by this patch, but
  pse_pi_is_admin_enable_pending() returns false only when…
- [Medium] This problem wasn't introduced by this patch, but
  pse_set_config_isr() starts with 'if (notifs &…
- [Medium] This problem wasn't introduced by this patch, but
  pse_ethtool_set_pw_limit() reads admin_state_enabled and…
- [Low] This problem wasn't introduced by this patch, but in
  pse_pw_d_retry_power_delivery() the variable 'int prio_max =…

--

net: pse-pd: don't enable a PI again while disabling it

This patch splits _pse_pi_disable() in two. __pse_pi_disable() disables
the PI and frees its budget, and _pse_pi_disable() also retries power
delivery in the power domain. Admin state and PD detection are now cleared
before _pse_pi_disable() runs, and pse_disable_pi_pol() no longer retries
the power domain.

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

[ ... ]

> +static int _pse_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> +	int ret;
> +
> +	ret = __pse_pi_disable(pcdev, id);
> +	if (ret)
> +		return ret;
> +
>  	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d))
>  		pse_pw_d_retry_power_delivery(pcdev, pcdev->pi[id].pw_d);

[Severity: Medium]
This is a pre-existing issue, but pse_pi_is_admin_enable_pending() only
treats a PI as not pending when pse_pi_is_hw_enabled() returns exactly 1:

	ret = pse_pi_is_hw_enabled(pcdev, id);
	/* PSE PI is already enabled at hardware level */
	if (ret == 1)
		return false;

	return true;

If ops->pi_get_admin_state() fails, a negative error is treated as
pending. One example is a failed i2c_smbus_read_word_data() of
TPS23881_REG_PW_STATUS in tps23881_pi_get_admin_state().

Can the retry loop then call _pse_pi_delivery_power_sw_pw_ctrl() on a PI
that is already powered? That would request its budget a second time.
pse_pi_allocate_pw_budget_static_prio() then overwrites
pi->pw_allocated_mW, which leaks the earlier allocation.

[Severity: Low]
This isn't a bug introduced by this patch, but in
pse_pw_d_retry_power_delivery() prio_max is declared inside the loop body:

	for (i = 0; i < pcdev->nr_lines; i++) {
		int prio_max = pcdev->nr_lines;
		...
		if (pcdev->pi[i].prio > prio_max)
			continue;

		ret = _pse_pi_delivery_power_sw_pw_ctrl(pcdev, i, &extack);
		if (ret == -ERANGE)
			prio_max = pcdev->pi[i].prio;
	}

The value saved after -ERANGE is lost on the next iteration. For the
static strategy, pse_ethtool_set_prio() rejects prio >= nr_lines, so can
the prio > prio_max check ever be true?

The comment "Do not try to enable PI with a lower prio (higher value)
than one which already can't be enabled" describes a cut-off that never
seems to happen. Should prio_max be declared outside the loop?

[ ... ]

> @@ -490,7 +513,10 @@ static int pse_disable_pi_pol(struct pse_controller_dev *pcdev, int id)
>  
>  	dev_dbg(pcdev->dev, "Disabling PI %d to free power budget\n", id);
>  
> -	ret = _pse_pi_disable(pcdev, id);
> +	/* The power freed is for the PI being allocated, and this PI waits
> +	 * for power again: no retry of the power domain here.
> +	 */
> +	ret = __pse_pi_disable(pcdev, id);

[Severity: High]
With the retry removed here, what powers the evicted PI back on when the
allocation that evicted it doesn't use all of the freed budget?

The evicted PI is left with admin_state_enabled = 1 and
isr_pd_detected = true, but it is off in hardware with
pw_allocated_mW = 0. pse_pw_d_retry_power_delivery() is only called from
_pse_pi_disable() and pse_ethtool_set_prio(). Nothing retries the domain
after the outer allocation finishes.

There seem to be two ways to get there. First,
pse_pi_allocate_pw_budget_static_prio() can return -ERANGE after it has
already turned off lower-priority PIs:

	while (regulator_request_power_budget(pi->pw_d->supply, pw_req) == -ERANGE) {
		if (_prio <= pi->prio) {
			...
			return -ERANGE;
		}

		ret = pse_disable_pi_prio(pcdev, pi->pw_d, _prio);
		...
		_prio--;
	}

For example, take a 60 W budget with H0 (prio 0) holding 30 W and L1
(prio 1) holding 30 W. A new prio 0 PD H2 asks for 40 W. L1 is turned off
and 30 W is freed, but the request still fails. L1 then stays off while
30 W is unused.

Second, pse_disable_pi_prio() turns off every hardware-enabled PI at a
priority level, not just as many as the request needs. With two prio 1
PIs at 30 W each and a 30 W prio 0 request, both prio 1 PIs go off. One
30 W share stays free, and neither PI is powered again.

The callers don't retry afterwards either. The CLASSIFICATION path of
pse_set_config_isr() and pse_pi_enable() both turn -ERANGE into success.
_pse_pi_delivery_power_sw_pw_ctrl() returns without a retry on both
success and failure.

Given this, is the new comment accurate? The commit message also says:

  That PI stays waiting for power and is retried when budget is freed
  later.

In these cases the budget may already be free. The evicted PIs only come
back after an unrelated event, such as an admin disable or unplug of
another PI in the domain, or pse_ethtool_set_prio().

TPS23881 may recover by chance, because tps23881_pi_disable() re-arms
detection and classification. The core doesn't require pi_disable to do
that, so other controllers might not recover. The RTL8239 driver named
in the commit message is one example.

Could the domain retry be deferred until the outer allocation finishes,
or could the evictions be undone when the allocation fails?

[ ... ]

> @@ -703,14 +729,18 @@ static int pse_pi_disable(struct regulator_dev *rdev)
>  {
>  	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
>  	struct pse_pi *pi;
> +	bool enabled;
>  	int id, ret;
>  
>  	id = rdev_get_id(rdev);
>  	pi = &pcdev->pi[id];
>  	mutex_lock(&pcdev->lock);
> +	/* Disabled before _pse_pi_disable() retries the power domain */
> +	enabled = pi->admin_state_enabled;
> +	pi->admin_state_enabled = 0;
>  	ret = _pse_pi_disable(pcdev, id);
> -	if (!ret)
> -		pi->admin_state_enabled = 0;
> +	if (ret)
> +		pi->admin_state_enabled = enabled;
>  
>  	mutex_unlock(&pcdev->lock);
>  	return 0;

[Severity: Medium]
This is a pre-existing issue, but pse_pi_disable() still returns 0 when
_pse_pi_disable() fails. admin_state_enabled is restored on error, yet
the regulator core sees success. _regulator_disable() then drops
rdev->use_count and the consumer's enable_count.

If ops->pi_disable() fails, can the PSE state and the regulator state
get out of sync? An I2C write error in tps23881_pi_disable() would do it.

A later admin enable in pse_ethtool_c33_set_config() does nothing,
because admin_state_enabled is still 1. A later admin disable goes
through:

	case ETHTOOL_C33_PSE_ADMIN_STATE_DISABLED:
		if (psec->pcdev->pi[psec->id].admin_state_enabled)
			err = regulator_disable(psec->ps);

and hits this in _regulator_disable():

	if (WARN(regulator->enable_count == 0,
		 "unbalanced disables for %s\n", rdev_get_name(rdev)))
		return -EIO;

Should pse_pi_disable() return ret here?

> @@ -1229,10 +1259,12 @@ static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id,
>  				ret = 0;
>  		}

[Severity: Medium]
This isn't a bug introduced by this patch, but pse_set_config_isr()
starts with:

	if (notifs & PSE_BUDGET_EVAL_STRAT_DYNAMIC)
		return 0;

PSE_BUDGET_EVAL_STRAT_DYNAMIC is a budget strategy value (1 << 2), not
an event mask. It has the same value as ETHTOOL_C33_PSE_EVENT_DETECTION
(4). tps23881_set_notifs_helper() ORs events together, so detection and
classification can arrive in the same notification.

Does a DETECTION | CLASSIFICATION notification return early here? That
would skip setting isr_pd_detected and skip power delivery for that PD
in the classification branch.

>  	} else if (notifs & ETHTOOL_C33_PSE_EVENT_DISCONNECTION) {
> -		if (pcdev->pi[id].admin_state_enabled &&
> -		    pcdev->pi[id].isr_pd_detected)
> -			ret = _pse_pi_disable(pcdev, id);
> +		bool pd_detected = pcdev->pi[id].isr_pd_detected;
> +
> +		/* Gone before _pse_pi_disable() retries the power domain */
>  		pcdev->pi[id].isr_pd_detected = false;
> +		if (pcdev->pi[id].admin_state_enabled && pd_detected)
> +			ret = _pse_pi_disable(pcdev, id);

[Severity: Medium]
This is a pre-existing issue, but isr_pd_detected is cleared here
whatever _pse_pi_disable() returns. If ops->pi_disable() fails,
__pse_pi_disable() returns before it calls pse_pi_deallocate_pw_budget().
pi->pw_allocated_mW then stays non-zero while isr_pd_detected is false.

A later DISCONNECTION skips the disable. The next CLASSIFICATION goes
through
_pse_pi_delivery_power_sw_pw_ctrl()->pse_pi_allocate_pw_budget_static_prio(),
which does:

	pi->pw_allocated_mW = pw_req;

without freeing the old allocation. Is that budget lost from the supply
for good?

Simply restoring isr_pd_detected on error would let the retry re-enable
the empty port. The fix may belong in how pw_allocated_mW is tracked
instead.

[Severity: Medium]
This is a pre-existing issue, but pse_ethtool_set_pw_limit() checks
admin_state_enabled and isr_pd_detected without holding pcdev->lock:

	    psec->pcdev->pi[psec->id].admin_state_enabled &&
	    psec->pcdev->pi[psec->id].isr_pd_detected) {
		ret = pse_pi_update_pw_budget(psec->pcdev, psec->id,
					      pw_limit, extack);

pse_pi_update_pw_budget() takes the lock but doesn't check them again.
Can an ethtool power limit change race with this disconnection path?

CPU1
pse_ethtool_set_pw_limit()
    sees admin_state_enabled && isr_pd_detected

CPU2
pse_isr()->pse_set_config_isr()
    isr_pd_detected = false
    _pse_pi_disable()
        pw_allocated_mW = 0

CPU1
pse_pi_update_pw_budget()
    pw_diff = pw_req - 0
    regulator_request_power_budget() for the empty port

Later disconnections do nothing, because isr_pd_detected is already
false. The next classification overwrites pw_allocated_mW in
pse_pi_allocate_pw_budget_static_prio(). Would that requested budget
leak?

>  	}
>  
>  	return ret;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018182005.1193-1-ross%40r-sc.ca