Re: [PATCH net v2] 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 (4) · Low: 0

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 fix is incomplete: the PI being disabled is still enabled
  again, and the power budget still leaks, on the priority-eviction path.

Pre-existing issues:
- [Medium] The priority cut-off in pse_pw_d_retry_power_delivery()
  (drivers/net/pse-pd/pse_core.c) never takes effect.
- [Medium] pse_set_config_isr() (drivers/net/pse-pd/pse_core.c line 1211)
  tests `notifs & PSE_BUDGET_EVAL_STRAT_DYNAMIC`.
- [Medium] pse_pi_disable() (drivers/net/pse-pd/pse_core.c lines 701-719)
  always returns 0, even when _pse_pi_disable() fails (for example an…
- [Medium] In the disconnection branch of pse_set_config_isr()
  (drivers/net/pse-pd/pse_core.c lines 1233-1243), isr_pd_detected is…

--

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

This patch moves the power domain retry out of _pse_pi_disable() and into
its callers. pse_pi_disable() and the disconnection path of
pse_set_config_isr() now retry only after they clear admin_state_enabled or
isr_pd_detected. pse_disable_pi_pol() still retries right after the
disable.

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

[ ... ]

> @@ -493,6 +490,8 @@ static int pse_disable_pi_pol(struct pse_controller_dev *pcdev, int id)
>  	ret = _pse_pi_disable(pcdev, id);
>  	if (ret)
>  		notifs |= ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR;
> +	else 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: High]
Can this retry still enable the PI that is being disabled? pse_disable_pi_pol()
is only reached through pse_disable_pi_prio(), from inside
pse_pi_allocate_pw_budget_static_prio(), while a higher priority PI (H) is
waiting for budget.

The evicted PI (L) still has admin_state_enabled and isr_pd_detected set. Once
its hardware is off, pse_pi_is_admin_enable_pending() returns true for L, and
that is the condition the subject line says is removed.

H is pending here too. On the classification path, pse_set_config_isr() sets
isr_pd_detected = true before calling _pse_pi_delivery_power_sw_pw_ctrl(), and
H is admin enabled:

pse_set_config_isr()                      /* CLASSIFICATION for H */
  _pse_pi_delivery_power_sw_pw_ctrl(H)
    pse_pi_allocate_pw_budget_static_prio(H)
      pse_disable_pi_prio()
        pse_disable_pi_pol(L)
          _pse_pi_disable(L)
          pse_pw_d_retry_power_delivery()
            _pse_pi_delivery_power_sw_pw_ctrl(H or L)

The retry scans in index order. If H has a lower index than L, the nested
retry allocates H's budget and enables H. Back in
pse_pi_allocate_pw_budget_static_prio(), the loop then asks for H's budget a
second time:

	while (regulator_request_power_budget(pi->pw_d->supply, pw_req) == -ERANGE) {
		...
		ret = pse_disable_pi_prio(pcdev, pi->pw_d, _prio);
		...
		_prio--;
	}

	pi->pw_allocated_mW = pw_req;

regulator_request_power_budget() adds to rdev->pw_requested_mW, so H is
reserved twice but only one reservation is recorded. Take a 25W budget, with L
(prio 2) holding 20W and H (prio 0) asking for 10W. H ends up with 20W
reserved and only 10W recorded, so the other 10W can never be freed. Isn't
this the same growing budget the commit message says is fixed?

If L has a lower index than H, the retry gives L back the budget it just
released. The ETHTOOL_PSE_EVENT_OVER_BUDGET notification is queued after the
retry, so user space hears that L was turned off for over budget while L is
already powered again.

In that same ordering, the nested _pse_pi_delivery_power_sw_pw_ctrl(H) can
get -ERANGE and evict L again through pse_disable_pi_prio() and
pse_disable_pi_pol(). That retries, enables L again, and recurses into H.
What bounds this recursion when pi_get_admin_state() reflects the change
immediately? If the hardware status lags instead, L would stay powered and H
would be refused, which inverts the priorities.

The v2 changelog says the retry powering such a PI again right away is left
for a separate patch. It doesn't mention the double allocation for the
requesting PI or the recursion. Should the eviction path be handled as part
of this fix?

>  
>  	ntf.notifs = notifs;
>  	ntf.id = id;
> @@ -709,8 +708,11 @@ static int pse_pi_disable(struct regulator_dev *rdev)
>  	pi = &pcdev->pi[id];
>  	mutex_lock(&pcdev->lock);
>  	ret = _pse_pi_disable(pcdev, id);
> -	if (!ret)
> +	if (!ret) {
>  		pi->admin_state_enabled = 0;
> +		if (pse_pw_d_is_sw_pw_control(pcdev, pi->pw_d))
> +			pse_pw_d_retry_power_delivery(pcdev, pi->pw_d);

[Severity: Medium]
This isn't a bug introduced by this patch, but this patch now calls
pse_pw_d_retry_power_delivery() directly from three places. Does the
prio_max cut-off in that function ever take effect?

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

prio_max is reset at the top of every iteration, so the value written after
-ERANGE is never read. A lower priority PI can still take freed budget after a
higher priority PI has failed. That goes against the comment "Do not try to
enable PI with a lower prio (higher value) than one which already can't be
enabled."

Should prio_max be declared outside the loop?

> +	}
>  
>  	mutex_unlock(&pcdev->lock);
>  	return 0;

[Severity: Medium]
This is a pre-existing issue, but pse_pi_disable() returns 0 even when
_pse_pi_disable() fails, for example when ops->pi_disable() gets an I2C error.

The regulator core then treats the disable as done and drops the consumer
enable count to 0. The PI stays powered, admin_state_enabled stays 1 and its
budget stays allocated, and user space is told the disable succeeded.

A later pse_ethtool_c33_set_config() with DISABLED sees admin_state_enabled
set and calls regulator_disable() again:

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

With the enable count already at zero, that call fails, so the disable can't
be retried. Should this return ret instead of 0?

> @@ -1229,10 +1231,16 @@ static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id,
>  				ret = 0;
>  		}
>  	} else if (notifs & ETHTOOL_C33_PSE_EVENT_DISCONNECTION) {

[Severity: Medium]
This is a pre-existing issue, but can this disconnection branch be skipped
entirely? Earlier in pse_set_config_isr():

	if (notifs & PSE_BUDGET_EVAL_STRAT_DYNAMIC)
		return 0;

PSE_BUDGET_EVAL_STRAT_DYNAMIC is a budget strategy value (1 << 2), not an
event bit. It has the same value as ETHTOOL_C33_PSE_EVENT_DETECTION (4), so
any batch of events that includes DETECTION returns 0 early.

tps23881_irq_event_handler() reads TPS23881_REG_DET_EVENT once. It can report
both DETECTION and CLASSIFICATION into the same notifs[i] through
tps23881_set_notifs_helper(). When that happens, isr_pd_detected is not set
and the PD is not powered.

A DETECTION | DISCONNECTION batch also skips the -EINVAL consistency check and
this branch. The disable, budget free and retry changed by this patch then
never run.

Was this check meant to test the domain's budget_eval_strategy rather than
notifs?

> -		if (pcdev->pi[id].admin_state_enabled &&
> -		    pcdev->pi[id].isr_pd_detected)
> +		struct pse_pi *pi = &pcdev->pi[id];
> +		bool disable = pi->admin_state_enabled && pi->isr_pd_detected;
> +
> +		/* Cleared first, so that the retry doesn't enable it again */
> +		pi->isr_pd_detected = false;
> +		if (disable) {
>  			ret = _pse_pi_disable(pcdev, id);

[Severity: Medium]
This is a pre-existing issue, since the old code also cleared isr_pd_detected
whatever the result was. What happens to this PI's budget if
_pse_pi_disable() fails here?

When ops->pi_disable() fails, _pse_pi_disable() returns before it calls
pse_pi_deallocate_pw_budget(), so pw_allocated_mW keeps the old reservation.
isr_pd_detected is already cleared, so later disconnection events compute
disable as false and never retry.

When the PD reconnects, the classification path calls
_pse_pi_delivery_power_sw_pw_ctrl() again. regulator_request_power_budget()
adds a new reservation, and pse_pi_allocate_pw_budget_static_prio()
overwrites the recorded amount:

	pi->pw_allocated_mW = pw_req;

Does the earlier reservation for this PI leak at that point? Could
isr_pd_detected be restored when _pse_pi_disable() fails?

> -		pcdev->pi[id].isr_pd_detected = false;
> +			if (!ret)
> +				pse_pw_d_retry_power_delivery(pcdev, pi->pw_d);
> +		}
>  	}

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