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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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

- [Medium] The patch puts the PD-state check and the budget change in one
  pcdev->lock critical section inside pse_pi_update_pw_budget().
- [Low] The kernel-doc for pse_pi_update_pw_budget() (drivers/net/pse-
  pd/pse_core.c:1835) still says "Return: Previous power allocated on…

--

net: pse-pd: check the PI state under the lock when setting a power limit

This moves the software power control, admin state and PD detected checks
from pse_ethtool_set_pw_limit() into pse_pi_update_pw_budget(), so they now
run under pcdev->lock. A new updated output parameter reports whether the
budget changed. The rollback after a failed regulator_set_current_limit()
now runs only when updated is true.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 336f00d82b5f8..d5f863834179c 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1829,22 +1829,32 @@ EXPORT_SYMBOL_GPL(pse_ethtool_set_config);
>   * @pcdev: a pointer to the PSE controller device
>   * @id: index of the PSE PI
>   * @pw_req: power requested
> + * @updated: set to true if the budget was changed
>   * @extack: extack for reporting useful error messages
>   *
>   * Return: Previous power allocated on success and failure value on error
>   */
>  static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,
> -				   const unsigned int pw_req,
> +				   const unsigned int pw_req, bool *updated,
>  				   struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	mutex_lock(&pcdev->lock);
> +	/* Only a powered PI under software power control holds a budget.
> +	 * Check it here, as pse_isr() changes it under the same lock.
> +	 */
> +	if (!pse_pw_d_is_sw_pw_control(pcdev, pi->pw_d) ||
> +	    !pi->admin_state_enabled || !pi->isr_pd_detected)
> +		goto out;
> +

[Severity: Low]
This isn't a bug, but the kernel-doc for pse_pi_update_pw_budget() still
says:

 * Return: Previous power allocated on success and failure value on error

This new early exit returns 0 with *updated = false, whatever
pi->pw_allocated_mW holds. The existing !pw_diff exit also returns 0.

pw_allocated_mW can be non-zero when this check fails. For example,
_pse_pi_disable() returns before pse_pi_deallocate_pw_budget() if
ops->pi_disable() fails, but pse_set_config_isr() still clears
isr_pd_detected on disconnection.

Could the Return description say that 0 can also mean no change was made?
It could also say that callers should check *updated, not the return
value, before rolling back.

>  	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

The port would then be powered with a hardware limit above its reserved
budget, and nothing later corrects pi->pw_allocated_mW. Could that
over-subscribe the power domain?

A disconnect followed by a reconnect between the two sections seems to end
the same way. If regulator_set_current_limit() then fails, this rollback
writes back the previous_pw_allocated value that was read before the
reconnect:

	if (ret < 0 && updated) {
		pse_pi_update_pw_budget(psec->pcdev, psec->id,
					previous_pw_allocated, &updated,
					extack);

Would a full fix need the state check, the budget update and
ops->pi_set_pw_limit() in a single pcdev->lock critical section?
pse_pi_set_current_limit() takes pcdev->lock itself, so that looks like it
needs a different structure from calling regulator_set_current_limit()
after pse_pi_update_pw_budget().

> -	if (ret < 0 && previous_pw_allocated) {
> +	if (ret < 0 && updated) {
>  		pse_pi_update_pw_budget(psec->pcdev, psec->id,
> -					previous_pw_allocated, extack);
> +					previous_pw_allocated, &updated,
> +					extack);
>  	}
>  
>  	return ret;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018082314.48950-1-github%40szelinsky.de