Re: [PATCH net 05/10] net: pse-pd: check the PI state under the lock when setting a power limit

On 10/20/26 07:40, Oleksij Rempel wrote:

CCing regulator related folks.

On Mon, Oct 19, 2026 at 02:30:24PM +0200, Kory Maincent wrote:

quoted

On 10/19/26 10:51, netdev-bot+sashiko@kernel.org wrote:

quoted

quoted

   	previous_pw_allocated = pi->pw_allocated_mW;
   	pw_diff = pw_req - previous_pw_allocated;
   	if (!pw_diff) {

[ ... ]

quoted

@@ -1908,21 +1920,17 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,
   	/* Update power budget only in software power control case and
   	 * if a Power Device is powered.
   	 */
-	if (pse_pw_d_is_sw_pw_control(psec->pcdev,
-				      psec->pcdev->pi[psec->id].pw_d) &&
-	    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);
-		if (ret < 0)
-			return ret;
-		previous_pw_allocated = ret;
-	}
+	ret = pse_pi_update_pw_budget(psec->pcdev, psec->id, pw_limit,
+				      &updated, extack);
+	if (ret < 0)
+		return ret;
+	previous_pw_allocated = ret;
   	ret = regulator_set_current_limit(psec->ps, 0, uA);

[Severity: Medium]
The scenario in the commit message, where budget is reserved for a port
that is no longer powered, looks closed now. But can the budget and the
hardware limit still drift apart in the other direction?

The state check and the budget change now share one pcdev->lock critical
section. The hardware limit is still written in a separate one:

pse_ethtool_set_pw_limit()
    pse_pi_update_pw_budget()        <- takes and drops pcdev->lock
    regulator_set_current_limit()
      pse_pi_set_current_limit()     <- takes pcdev->lock again
        ops->pi_set_pw_limit()

The threaded pse_isr() only needs pcdev->lock, so it can run in between:

CPU0 (ethtool set_pw_limit)
pse_pi_update_pw_budget()
    sees isr_pd_detected == false
    *updated = false, nothing reserved
    mutex_unlock(&pcdev->lock)

CPU1 (pse_isr, classification event)
pse_set_config_isr()
    isr_pd_detected = true
    _pse_pi_delivery_power_sw_pw_ctrl()
      pw_req = min(pi_get_pw_req(), pi_get_pw_limit())  <- old hw limit
      pse_pi_allocate_pw_budget(pcdev, id, pw_req, extack)
      ops->pi_enable()

CPU0
regulator_set_current_limit()
    pse_pi_set_current_limit()
      ops->pi_set_pw_limit()         <- new, higher limit

@Oleksij, Here is another example of an issue related to the PSE PI being a
regulator provider.

@Carlo, Kyle is working on a series that remove the support for a PSE PI
being a regulator provider. We will see where it goes.

Removing the regulator provider support is a bad idea:

- We will end up implementing a poor man's regulator framework inside the
network subsystem. Networking folks will have to deal with physical constraints
already solved by the power management community, requiring a different mindset
and domain experience.

That's what we thought at the beginning of the implementation of PoE but in fact that's not the case. I don't see currently any cases of using already solved regulator constraint in the PoE core.
The drawback are:
- locks issues: solved one by using worker but there are still others
  appearing like this one
- code complexity due to all the regulator callback wrapper
- converting mW to mA for regulator framework then to mW again for the
  PSE driver callbacks. There are even devices that are using current
  limit therefore there will be 3 conversions.

The more painful issues are the lock issues.

- We lose built-in regulator infrastructure: DT safety constraints
(regulator-max-microamp), over-current/under-voltage notifications, and
sysfs/debugfs (regulator_summary) visibility.

I disagree here.
regulator-max-microamp: has not interest from the PI side but this is related to its power supply. The PI should still stay as a regulator consumer so the power supply could describe this binding.

over-current/under-voltage notifications: we are already reporting these PSE PIs notification through ethtool. We will loose regulator_notifier_call_chain() call but well there is no consumer to the PI regulator provider so it's mainly useless.

Yes, we will loose sysfs/debugfs visibility, but is someone really using it as all PSE communication goes through ethtool?

- The race above is just a lock scope issue across pcdev->lock and the
regulator call, not a flaw in the abstraction.

That's not something easy to solve as PSE and regulator provider are nested together.
Either we put all of the lock issues inside workers, it will not be synchronous and will smell hackish but well...
Or we try to combine the PSE lock with the regulator nested lock in one way or another.
Do you have a better idea?

- Writing a custom power state machine in net/ will introduce more bugs than it
fixes - especially if we overestimate our ability to upstream complex custom
logic under LLM review supervision.

Removing regulator provider will simplify a lot the implementation so I think it will rather remove bugs than introduce new one.
I think we should let Kyle works on it with a RFC and we will see where it goes.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com