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

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

[Ross Cawston]

With software power control, _pse_pi_disable() frees the power budget
of the PI and then retries the power delivery of the PIs waiting for
power in its power domain: those that are admin enabled, have a PD
detected, and are not enabled in hardware. On an admin disable or a
disconnection, the PI being disabled is one of them, as its admin state
or PD detection is only cleared after _pse_pi_disable() returns, so it
is enabled again right away:

- an admin disable of a port with a PD keeps the port powered, and the
  next admin enable allocates its budget a second time
- a disconnection enables the now empty port again, with a new budget
  allocation that is never freed: when the port reports the
  disconnection again, its PD detection is already cleared

Move the retry out of _pse_pi_disable() into its callers:
pse_pi_disable() and the disconnection handling of pse_set_config_isr()
retry after they clear the admin state or the PD detection, and
pse_disable_pi_pol() retries as before.

The problem was found while testing the budget on a switch with two
Realtek RTL8239 PSE controllers, whose driver (not in mainline yet) uses
the static budget strategy like the TPS23881. Before this change,
"ethtool --set-pse <port> c33-pse-admin-control disable" left a port
with a PD powered, and the budget allocated in the power domain grew
with every disable and every unplug. With it, the port stays off, and
the allocated budget is back to 0 after a disable and after an unplug.
Tested on that switch with OpenWrt's 6.18 kernel; the change for
mainline is the same apart from context and is build-tested.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Signed-off-by: Ross Cawston <ross@r-sc.ca>
---
Changes in v2:
- Move the retry of the power domain out of _pse_pi_disable() into its
  callers, instead of splitting out a __pse_pi_disable() without it
  (Kory)
- Keep the retry after pse_disable_pi_pol(): without it, PIs turned off
  for a PI of higher priority stay off even when their budget wasn't
  needed (Sashiko). That the retry powers such a PI again right away is
  left for a separate patch.

v1: https://lore.kernel.org/netdev/20261018182005.1193-1-ross@r-sc.ca/

 drivers/net/pse-pd/pse_core.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index a5e6d7b26b9..c71acfd5337 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -469,9 +469,6 @@ static int _pse_pi_disable(struct pse_controller_dev *pcdev, int id)
 
 	pse_pi_deallocate_pw_budget(&pcdev->pi[id]);
 
-	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);
-
 	return 0;
 }
 
@@ -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);
 
 	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);
+	}
 
 	mutex_unlock(&pcdev->lock);
 	return 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) {
-		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);
-		pcdev->pi[id].isr_pd_detected = false;
+			if (!ret)
+				pse_pw_d_retry_power_delivery(pcdev, pi->pw_d);
+		}
 	}
 
 	return ret;
-- 
2.55.0.windows.5

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

[netdev-bot+sashiko]

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

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

[Kory Maincent]

On 10/22/26 12:08, netdev-bot+sashiko@kernel.org wrote:
> 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.

Can you take a look at the above issues?
The other ones are currently in discussion with Kyle due to similar 
report he faced on his series.

> - [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…
> -- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com