net: pse-pd: assorted lifetime and error-path fixes

pse_controller_register() allocates the notification kfifo before it turns nr_lines == 0 into 1, and kfifo_alloc() fails for a size below 2. So a controller with nr_lines 0 or 1 cannot register. pse_regulator (podl-pse-regulator) never sets nr_lines, so its probe has failed with -EINVAL ever since the kfifo was added.

Apply the default first, and allocate at least two entries.

Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/pse_core.c | 11 +++++++----
  1 file changed, 7 insertions(+), 4 deletions(-)

drivers/net/pse-pd/pse_core.c +7 -4

index a5e6d7b26b9f..c0573cbbba5b 100644
@@ -1044,16 +1044,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
  	mutex_init(&pcdev->lock);
  	INIT_LIST_HEAD(&pcdev->pse_control_head);
  	spin_lock_init(&pcdev->ntf_fifo_lock);
-	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
+
+	if (!pcdev->nr_lines)
+		pcdev->nr_lines = 1;
+
+	/* kfifo_alloc() rejects a size that rounds up to less than 2 */
+	ret = kfifo_alloc(&pcdev->ntf_fifo, max(pcdev->nr_lines, 2U),
+			  GFP_KERNEL);
  	if (ret) {
  		dev_err(pcdev->dev, "failed to allocate kfifo notifications\n");
  		return ret;
  	}
  	INIT_WORK(&pcdev->ntf_work, pse_send_ntf_worker);
 
-	if (!pcdev->nr_lines)
-		pcdev->nr_lines = 1;
-
  	if (!pcdev->ops->pi_get_admin_state ||
  	    !pcdev->ops->pi_get_pw_status) {
  		dev_err(pcdev->dev,

Could the commit message say that the first branch is a defensive check and cannot be reached today?

pse_control_get_internal() takes a module reference and then has two error paths that skip module_put(): a controller without pi_get_admin_state(), and a failing pse_pi_is_hw_enabled(). The second one happens on any I2C or UART error. Each time, one more reference leaks, and the PSE driver can no longer be unloaded.

Send both to put_module, like the regulator_get_exclusive() error path.

Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/pse_core.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/pse-pd/pse_core.c +2 -2

index c0573cbbba5b..1710f9bb7d27 100644
@@ -1427,7 +1427,7 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
 
  	if (!pcdev->ops->pi_get_admin_state) {
  		ret = -EOPNOTSUPP;
-		goto free_psec;
+		goto put_module;
  	}
 
  	/* Initialize admin_state_enabled before the regulator_get. This

@@ -1436,7 +1436,7 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
  	 */
  	ret = pse_pi_is_hw_enabled(pcdev, index);
  	if (ret < 0)
-		goto free_psec;
+		goto put_module;
 
  	pcdev->pi[index].admin_state_enabled = ret;
  	psec->ps = regulator_get_exclusive(pcdev->dev,

As spotted by sashiko you can remove the two checks on pi_get_admin_state callback as it is already done in the pse_controller_register() function.
Maybe you can do it in a patch before this one even if it is not a fix.

devm_pse_irq_helper() keeps the IRQ number in pcdev->irq even when the IRQ request failed. The failed probe then runs pse_controller_unregister(), which calls disable_irq() on that line. If another device owns it, that device stops getting interrupts.

Clear pcdev->irq when the request fails. It is now set before the request, because pse_isr() can run as soon as the IRQ is enabled and pse_pw_d_is_sw_pw_control() reads it.

[Severity: Low]
Could the comment say that it relies on attached_phydev being cleared in the shared-PI case?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. This patch relies on the pse_control_put() in phy_device_remove(). Does that call leave phydev->psec dangling?

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 18 lines

drivers/net/pse-pd/pse_core.c +2 -1

index 336f00d82b5f8..d5f863834179c 100644
@@ -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)
  {

[ ... ]

drivers/net/pse-pd/pse_core.c EXPORT_SYMBOL_GPL(pse_ethtool_set_config); · line 1857

Quoted 8 lines

  	mutex_lock(&pcdev->lock);

drivers/net/pse-pd/pse_core.c +7 -0

+	/* 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 3 lines

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

[ ... ]

Quoted 11 lines

drivers/net/pse-pd/pse_core.c +0 -7

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

drivers/net/pse-pd/pse_core.c int pse_ethtool_set_pw_limit(struct pse_control *psec, · line 1929

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +5 -3

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

Quoted 8 lines

drivers/net/pse-pd/pse_core.c +3 -2

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

drivers/net/pse-pd/pse_core.c int pse_ethtool_set_pw_limit(struct pse_control *psec, · line 1936

Quoted 1 line

drivers/net/pse-pd/pse_core.c +0 -0

  	return ret;

I think you can ignore this sashiko point as for now.

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

Quoted 53 lines · On 10/19/26 10:51, [email protected] wrote:

Quoted 3 lines

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

[ ... ]

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +0 -6

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

drivers/net/pse-pd/pse_core.c int pse_ethtool_set_pw_limit(struct pse_control *psec, · line 1928

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +5 -4

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

Quoted 8 lines

     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.

CCing regulator related folks.

Quoted 64 lines · Kory Maincent

Quoted 57 lines · On 10/19/26 10:51, [email protected] wrote:

Quoted 3 lines

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

[ ... ]

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +0 -6

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

drivers/net/pse-pd/pse_core.c int pse_ethtool_set_pw_limit(struct pse_control *psec, · line 1928

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +5 -4

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

Quoted 4 lines

CPU0
regulator_set_current_limit()

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

Quoted 5 lines

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

Quoted 8 lines · Oleksij Rempel

CCing regulator related folks.

Quoted 5 lines · Kory Maincent

Quoted 3 lines · On 10/19/26 10:51, [email protected] wrote:

Quoted 3 lines

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

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 11 lines · Kory Maincent

Quoted 8 lines · Oleksij Rempel

CCing regulator related folks.

Quoted 5 lines · Kory Maincent

Quoted 3 lines · On 10/19/26 10:51, [email protected] wrote:

Quoted 3 lines

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

[...]

Quoted 58 lines

Quoted 10 lines

Quoted 9 lines

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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

For what it's worth, and for others that might be interested but I missed on cc, the RFC is here:

https://lore.kernel.org/netdev/[email protected]/

[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?

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

[Severity: High]
Is the shared domain still tied to its creator through this supply handle?

[Severity: Medium]
What happens to the PIs handled before the failing one?

[Severity: Medium]
Can the PI regulator ops still run here and see pw_d == NULL?

pd692x0 reserves a power budget on each manager's supply in setup_pi_matrix(), but gives it back only on that function's own errors and in remove(). If probe fails later, or the request fails for one manager after others succeeded, the budget stays reserved on vmain. Each probe retry, e.g. after -EPROBE_DEFER, reserves it again until nothing is left.

Release the budget from a devm action instead, and drop the explicit release calls.

Fixes 1c67f9c54cdc ("net: pse-pd: pd692x0: Fix power budget leak in manager setup error path")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/pd692x0.c | 22 +++++++++++++++-------
  1 file changed, 15 insertions(+), 7 deletions(-)

drivers/net/pse-pd/pd692x0.c +15 -7

index 209de9cec849..34f0dfbccd06 100644
@@ -1224,8 +1224,9 @@ static void pd692x0_of_put_managers(struct pd692x0_priv *priv,
  	}
  }
 
-static void pd692x0_managers_free_pw_budget(struct pd692x0_priv *priv)
+static void pd692x0_managers_free_pw_budget(void *data)
  {
+	struct pd692x0_priv *priv = data;
  	int i;
 
  	for (i = 0; i < PD692X0_MAX_MANAGERS; i++) {

@@ -1279,31 +1280,39 @@ static int pd692x0_setup_pi_matrix(struct pse_controller_dev *pcdev)
  	if (ret)
  		goto err_of_managers;
 
+	/* Frees whatever pd692x0_req_managers_pw_budget() manages to
+	 * request, on any later probe failure and on unbind. Added after
+	 * the manager regulators so that it runs before devres unregisters
+	 * them and their supply goes away.
+	 */
+	ret = devm_add_action_or_reset(&priv->client->dev,
+				       pd692x0_managers_free_pw_budget, priv);
+	if (ret)
+		goto err_of_managers;
+
  	ret = pd692x0_req_managers_pw_budget(priv);
  	if (ret)
  		goto err_of_managers;
 
  	ret = pd692x0_set_ports_matrix(priv, manager);
  	if (ret)
-		goto err_managers_req_pw;
+		goto err_of_managers;
 
  	/* Do not init the conf if it is already saved */
  	if (!priv->cfg_saved) {
  		ret = pd692x0_hw_conf_init(priv);
  		if (ret)
-			goto err_managers_req_pw;
+			goto err_of_managers;
 
  		ret = pd692x0_save_user_byte(priv);
  		if (ret)
-			goto err_managers_req_pw;
+			goto err_of_managers;
  	}
 
  	pd692x0_of_put_managers(priv, manager);
  	kfree(manager);
  	return 0;
 
-err_managers_req_pw:
-	pd692x0_managers_free_pw_budget(priv);
  err_of_managers:
  	pd692x0_of_put_managers(priv, manager);
  err_free_manager:

@@ -1855,7 +1864,6 @@ static void pd692x0_i2c_remove(struct i2c_client *client)
  {
  	struct pd692x0_priv *priv = i2c_get_clientdata(client);
 
-	pd692x0_managers_free_pw_budget(priv);
  	firmware_upload_unregister(priv->fwl);
  }
 

pd692x0_req_managers_pw_budget() uses each manager regulator's supply without a NULL check. The supply is NULL while the vmain regulator is described but not registered yet, because regulator_register() does not wait for its supply. Probe then crashes.

Return -EPROBE_DEFER in that case. On net, each retry also leaks the notification kfifo and the PI array until vmain appears; the fix for that is in the PSE core fixes for net-next.

Fixes 359754013e6a ("net: pse-pd: pd692x0: Add support for PSE PI priority feature")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/pd692x0.c | 9 +++++++++
  1 file changed, 9 insertions(+)

drivers/net/pse-pd/pd692x0.c +9 -0

index 34f0dfbccd06..568789bd912f 100644
@@ -1056,6 +1056,15 @@ pd692x0_req_managers_pw_budget(struct pd692x0_priv *priv)
  		struct regulator *supply = priv->manager_reg[i]->supply;
  		int pw_budget;
 
+		/* The regulator core registers the manager regulator even
+		 * when its vmain supply cannot be resolved yet and retries
+		 * later, leaving ->supply NULL until then.
+		 */
+		if (!supply)
+			return dev_err_probe(&priv->client->dev, -EPROBE_DEFER,
+					     "manager %d: vmain supply not resolved\n",
+					     i);
+
  		pw_budget = regulator_get_unclaimed_power_budget(supply);
  		if (!pw_budget)
  			/* Do nothing if no power budget */

If devm_add_action_or_reset() fails, it has already unregistered the ancillary I2C client, and the error message then reads that client's device. It can only fail with -ENOMEM, which is already reported, so drop the message.

Fixes a2317231df4b ("net: pse-pd: Add Si3474 PSE controller driver")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/si3474.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

drivers/net/pse-pd/si3474.c +1 -3

index 1845b9c51cf7..2f4f241a48af 100644
@@ -516,10 +516,8 @@ static int si3474_i2c_probe(struct i2c_client *client)
  		return PTR_ERR(priv->client[1]);
 
  	ret = devm_add_action_or_reset(dev, si3474_ancillary_i2c_remove, priv->client[1]);
-	if (ret < 0) {
-		dev_err(&priv->client[1]->dev, "Cannot register remove callback\n");
+	if (ret)
  		return ret;
-	}
 
  	ret = i2c_smbus_read_byte_data(priv->client[1], VENDOR_IC_ID_REG);
  	if (ret < 0) {

That's not really a fix but well:

Reviewed-by Kory Maincent <[email protected]>

pse_ethtool_set_pw_limit() turns the power limit into a current as mW * 10^9 / uV and stores it in an int. For the largest allowed limit, 99900 mW, that overflows below about 46.5 mV. Only 0 V is rejected, and si3474 reports the measured voltage of each port. A negative result then hits the BUG_ON() in regulator_check_current_limit().

pse_pi_get_current_limit() does the same conversion and can return a wrapped value that looks like an error code.

Do the conversion in one helper that returns -ERANGE when the result does not fit an int.

Fixes 4a83abcef5f4 ("net: pse-pd: Add new power limit get and set c33 features")

Signed-off-by Carlo Szelinsky <[email protected]>

---
  drivers/net/pse-pd/pse_core.c | 25 +++++++++++++++----------
  1 file changed, 15 insertions(+), 10 deletions(-)

drivers/net/pse-pd/pse_core.c +15 -10

index 0d1e5d5039f8..3e333bef4e48 100644
@@ -741,12 +741,19 @@ static int pse_pi_get_voltage(struct regulator_dev *rdev)
  	return ret;
  }
 
+/* uA = mW * 1000000000 / uV, which does not fit an int for a low uV */
+static int pse_mw_to_ua(unsigned int mW, int uV)
+{
+	u64 uA = DIV_ROUND_CLOSEST_ULL((u64)mW * 1000000000ull, uV);
+
+	return uA > INT_MAX ? -ERANGE : uA;
+}
+
  static int pse_pi_get_current_limit(struct regulator_dev *rdev)
  {
  	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
  	const struct pse_controller_ops *ops;
  	int id, uV, mW, ret;
-	s64 tmp_64;
 
  	ops = pcdev->ops;
  	id = rdev_get_id(rdev);

@@ -769,10 +776,7 @@ static int pse_pi_get_current_limit(struct regulator_dev *rdev)
  		goto out;
  	uV = ret;
 
-	tmp_64 = mW;
-	tmp_64 *= 1000000000ull;
-	/* uA = mW * 1000000000 / uV */
-	ret = DIV_ROUND_CLOSEST_ULL(tmp_64, uV);
+	ret = pse_mw_to_ua(mW, uV);
 
  out:
  	mutex_unlock(&pcdev->lock);

@@ -1910,7 +1914,6 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,
  {
  	int uV, uA, ret, previous_pw_allocated = 0;
  	bool updated;
-	s64 tmp_64;
 
  	if (pw_limit > MAX_PI_PW)
  		return -ERANGE;

@@ -1928,10 +1931,12 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,
  	}
  	uV = ret;
 
-	tmp_64 = pw_limit;
-	tmp_64 *= 1000000000ull;
-	/* uA = mW * 1000000000 / uV */
-	uA = DIV_ROUND_CLOSEST_ULL(tmp_64, uV);
+	uA = pse_mw_to_ua(pw_limit, uV);
+	if (uA < 0) {
+		NL_SET_ERR_MSG(extack,
+			       "PSE voltage read is too low to set the limit");
+		return uA;
+	}
 
  	/* Update power budget only in software power control case and
  	 * if a Power Device is powered.

Same as patch 9. This is not a fix.
Can you send both to net-next instead.
You can add my R-by tag to it.

It changes _pse_pi_disable(), pse_disable_pi_pol(), pse_pi_disable() and pse_set_config_isr(), none of which your series touches, so I don't expect the two to get in each other's way.

Is a v6 of that planned once these fixes are in?