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

[Kory Maincent]

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:
> > On 10/19/26 10:51, netdev-bot+sashiko@xxxxxxxxxx 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.

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