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

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

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.

quoted

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

[ ... ]

quoted

  	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.

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.
I think you can ignore this sashiko point as for now.

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

quoted

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

With the change ask by sashiko on the kdoc you can add my Reviewed-by tag.

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