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

[Oleksij Rempel <o.rempel@pengutronix.de>]

CCing regulator related folks.

On Mon, Oct 19, 2026 at 02:30:24PM +0200, Kory Maincent wrote:
> 
> On 10/19/26 10:51, netdev-bot+sashiko@kernel.org wrote:
> > >   	previous_pw_allocated = pi->pw_allocated_mW;
> > >   	pw_diff = pw_req - previous_pw_allocated;
> > >   	if (!pw_diff) {
> > 
> > [ ... ]
> > 
> > > @@ -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.

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

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

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

Let's fix the lock serialization instead of dropping the framework.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |