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

Fixes for the PSE core and the pd692x0 driver, found while working on
the PSE controller teardown. Most of them are about what happens when a
controller unbinds or fails to register.

 1  The kfifo is too small: podl-pse-regulator and any controller with
    0 or 1 lines cannot register.
 2  Removes two checks that can never be true. Not a fix; it makes 3
    smaller.
 3  A module reference leaks when reading the port state fails.
 4  A failed IRQ request stays in pcdev->irq, so unregister disables a
    line another device may own.
 5  The notification worker can read a PHY that was already freed.
 6  Setting a power limit checks the PI state without the lock.
 7  A power domain shared by two controllers is freed when the one that
    created it unbinds, or when registration fails part way.
 8  Unregister frees the PI array before it stops the IRQ handler and
    the notification worker. The worker can also be queued again
    during unregister.
 9  Unregister frees the PI array while a lookup can still find the
    controller.
 10 The PI regulator ops can run after the PI array is freed.
 11 A failed registration leaks the kfifo.
 12 A shared power domain's supply handle goes stale when the controller
    that created it unbinds.
 13 pd692x0 leaks its manager power budget when probe fails.
 14 pd692x0 crashes on a manager supply that is not resolved yet.

Some patches need earlier ones: 7 needs the locked check from 6, 10 to
12 build on 7 to 9, and 14 relies on 13 to give back a budget that was
already reserved.

Testing: every patch builds on its own with W=1 and is checkpatch
clean. 1, 5, 7, 10 and 12 were tested in QEMU (arm64, KASAN, lockdep,
kmemleak) with test-only fake PSE controllers: without the patch, the
failure in its commit message shows up; with the whole series, the
tests are clean, except for the older lockdep report and the reserved
budget, both listed below. For v3, the whole series and 7 on its own
were tested again. The other patches were found by code inspection and
are build-tested only.

89 more lines

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.

Triggered in QEMU (arm64) with a test-only PSE controller with
nr_lines 0: its probe failed with "failed to allocate kfifo
notifications". With this patch it registers.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index a5e6d7b26b9f..c0573cbbba5b 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

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

-- 
2.43.0

pse_controller_register() already fails when a driver has no
pi_get_admin_state() callback. So the checks for it in
pse_pi_is_enabled() and pse_control_get_internal() are never true.
Remove them.

No functional change. It makes the next patch, which fixes an error
path in pse_control_get_internal(), smaller.

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index c0573cbbba5b..03da633c6382 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -412,13 +412,8 @@ static bool pse_pw_d_is_sw_pw_control(struct pse_controller_dev *pcdev,
 static int pse_pi_is_enabled(struct regulator_dev *rdev)
 {
 	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
-	const struct pse_controller_ops *ops;
 	int id, ret;
 
-	ops = pcdev->ops;
-	if (!ops->pi_get_admin_state)
-		return -EOPNOTSUPP;
-
 	id = rdev_get_id(rdev);
 	mutex_lock(&pcdev->lock);
 	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {

@@ -1425,11 +1420,6 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
 		goto free_psec;
 	}
 
-	if (!pcdev->ops->pi_get_admin_state) {
-		ret = -EOPNOTSUPP;
-		goto free_psec;
-	}
-
 	/* Initialize admin_state_enabled before the regulator_get. This
 	 * aims to have the right value reported in the first is_enabled
 	 * call in case of control managed by software.

-- 
2.43.0

pse_control_get_internal() takes a module reference, but when
pse_pi_is_hw_enabled() fails, it skips module_put(). This happens on
any I2C or UART error. Each time, one more reference leaks, and the
PSE driver can no longer be unloaded.

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

Found by code inspection. Not reproduced.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 03da633c6382..c6612232b820 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -1426,7 +1426,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,

-- 
2.43.0

pse_controller_unregister() flushes the power domains and frees the PI
array before it disables the IRQ, while pse_isr() uses both. No in-tree
driver hits this: tps23881, the only user of devm_pse_irq_helper(),
requests its IRQ after it registers the controller, so devres frees
the IRQ before unregister runs. A driver that keeps its IRQ across
unregister would read freed memory.

Disable the IRQ and drain the notification worker first, and only then
flush the power domains and release the PI array.

The worker can also be queued again after the drain. With the static
budget strategy, powering a PI can switch off a PI of lower priority
through pse_disable_pi_pol(), which queues the worker. A PI is powered
from pse_isr(), from a priority change, from the retry after a disable,
and when a consumer enables it. All of these go through pse_pi_enable()
or _pse_pi_delivery_power_sw_pw_ctrl().

So pse_controller_unregister() now first sets pcdev->unregistering
under pcdev->lock. After that, both functions return -ENODEV.
Disabling a PI still works.

The next patches in this series build on this order.

Found by code inspection and AI review.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 27 +++++++++++++++++++++++++--
 include/linux/pse-pd/pse.h    |  3 +++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 113a34b09386..36f8e4e7d191 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -612,6 +612,13 @@ static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev,
 	struct pse_pi *pi = &pcdev->pi[id];
 	int ret, pw_req;
 
+	/* pse_isr(), a priority change and the retry after a disable all
+	 * power a PI through here, so stop them here once unregistering
+	 * has started.
+	 */
+	if (pcdev->unregistering)
+		return -ENODEV;
+
 	if (!ops->pi_get_pw_req) {
 		/* No power allocation management */
 		ret = ops->pi_enable(pcdev, id);

@@ -668,6 +675,11 @@ static int pse_pi_enable(struct regulator_dev *rdev)
 
 	id = rdev_get_id(rdev);
 	mutex_lock(&pcdev->lock);
+	if (pcdev->unregistering) {
+		mutex_unlock(&pcdev->lock);
+		return -ENODEV;
+	}
+
 	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
 		/* Manage enabled status by software.
 		 * Real enable process will happen if a port is connected.

@@ -1160,11 +1172,22 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
-	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
+	/* From now on the PSE core must not power a PI, so nothing queues
+	 * a new notification once the worker is drained below.
+	 */
+	mutex_lock(&pcdev->lock);
+	pcdev->unregistering = true;
+	mutex_unlock(&pcdev->lock);
+
+	/* Stop the IRQ and the notification worker first: pse_isr() uses
+	 * pcdev->pi and the power domains that pse_flush_pw_ds() drops,
+	 * and the worker reads the kfifo that is freed below.
+	 */
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
 	cancel_work_sync(&pcdev->ntf_work);
+	pse_flush_pw_ds(pcdev);
+	pse_release_pis(pcdev);
 	kfifo_free(&pcdev->ntf_fifo);
 	mutex_lock(&pse_list_mutex);
 	list_del(&pcdev->list);

diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 4e5696cfade7..a917ac32dc0c 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h

@@ -298,6 +298,8 @@ struct pse_ntf {
  * @ntf_work: workqueue for PSE notification management
  * @ntf_fifo: PSE notifications FIFO
  * @ntf_fifo_lock: protect @ntf_fifo writer
+ * @unregistering: set under @lock when unregistering starts; the PSE
+ *		   core switches no PI on after that
  */
 struct pse_controller_dev {
 	const struct pse_controller_ops *ops;

@@ -317,6 +319,7 @@ struct pse_controller_dev {
 	struct work_struct ntf_work;
 	DECLARE_KFIFO_PTR(ntf_fifo, struct pse_ntf);
 	spinlock_t ntf_fifo_lock; /* Protect @ntf_fifo writer */
+	bool unregistering;
 };
 
 /**

-- 
2.43.0

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.

Found by code inspection. Not reproduced.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index c6612232b820..eab7e25f2039 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -1343,14 +1343,20 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
 	if (!h->notifs_mask)
 		return -ENOMEM;
 
+	/* pse_isr() can run as soon as the IRQ is enabled, and
+	 * pse_pw_d_is_sw_pw_control() reads pcdev->irq.
+	 */
+	pcdev->irq = irq;
 	ret = devm_request_threaded_irq(dev, irq, NULL, pse_isr,
 					IRQF_ONESHOT | irq_flags,
 					irq_name, h);
-	if (ret)
+	if (ret) {
+		pcdev->irq = 0;
 		dev_err(pcdev->dev, "Failed to request IRQ %d\n", irq);
+		return ret;
+	}
 
-	pcdev->irq = irq;
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
 

-- 
2.43.0

A power domain is shared by all controllers on the same supply and
counted with a kref, but it is devm memory of the controller that
created it. When that controller unbinds while another one still uses
the domain, devres frees it anyway, and the other controller and the
global map keep pointing at freed memory. The same happens when
pse_register_pw_ds() fails part way: the domains it already set up stay
in the map, and devres frees them.

Allocate the domain with kzalloc() and free it on its last kref_put().
If pse_register_pw_ds() fails, it now gives back every domain it took
while it still holds pse_pw_d_mutex, so no other controller can pick one
up first.

Because the domain can now be freed at the put, pse_flush_pw_ds() clears
pi[].pw_d under pcdev->lock before the put, and pse_ethtool_set_prio()
checks it under that lock. The PI regulator ops and pse_isr() use it
under the same lock.

Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers
on one supply: after the one that created the domain unbinds, binding
it again gives

  BUG: KASAN: slab-use-after-free in pse_controller_register+0xb98/0x1274

With this patch it is gone.

Fixes: 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 68 +++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 36f7dba8190a..113a34b09386 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -872,17 +872,41 @@ devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
 	return 0;
 }
 
-static void __pse_pw_d_release(struct kref *kref)
+/* Release for a put made with pse_pw_d_mutex already held. */
+static void pse_pw_d_release_locked(struct kref *kref)
 {
 	struct pse_power_domain *pw_d = container_of(kref,
 						     struct pse_power_domain,
 						     refcnt);
 
+	lockdep_assert_held(&pse_pw_d_mutex);
 	regulator_put(pw_d->supply);
 	xa_erase(&pse_pw_d_map, pw_d->id);
+	kfree(pw_d);
+}
+
+static void __pse_pw_d_release(struct kref *kref)
+{
+	pse_pw_d_release_locked(kref);
 	mutex_unlock(&pse_pw_d_mutex);
 }
 
+/* Clear pi[id].pw_d before the domain's reference is dropped. The PI
+ * regulator ops and pse_isr() hold pcdev->lock while they use it.
+ */
+static struct pse_power_domain *
+pse_pi_take_pw_d(struct pse_controller_dev *pcdev, int id)
+{
+	struct pse_power_domain *pw_d;
+
+	mutex_lock(&pcdev->lock);
+	pw_d = pcdev->pi[id].pw_d;
+	pcdev->pi[id].pw_d = NULL;
+	mutex_unlock(&pcdev->lock);
+
+	return pw_d;
+}
+
 /**
  * pse_flush_pw_ds - flush all PSE power domains of a PSE
  * @pcdev: a pointer to the initialized PSE controller device

@@ -893,10 +917,11 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
 	int i;
 
 	for (i = 0; i < pcdev->nr_lines; i++) {
-		if (!pcdev->pi[i].pw_d)
+		pw_d = pse_pi_take_pw_d(pcdev, i);
+		if (!pw_d)
 			continue;
 
-		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
+		pw_d = xa_load(&pse_pw_d_map, pw_d->id);
 		if (!pw_d)
 			continue;
 

@@ -906,24 +931,30 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
 }
 
 /**
- * devm_pse_alloc_pw_d - allocate a new PSE power domain for a device
- * @dev: device that is registering this PSE power domain
+ * pse_alloc_pw_d - allocate a new PSE power domain
+ *
+ * A power domain is shared by every controller whose PIs sit on the same
+ * supply, so it must not be tied to the lifetime of the controller that
+ * happened to create it. It is freed by pse_pw_d_release_locked() on the
+ * last kref_put().
  *
  * Return: Pointer to the newly allocated PSE power domain or error pointers
  */
-static struct pse_power_domain *devm_pse_alloc_pw_d(struct device *dev)
+static struct pse_power_domain *pse_alloc_pw_d(void)
 {
 	struct pse_power_domain *pw_d;
 	int index, ret;
 
-	pw_d = devm_kzalloc(dev, sizeof(*pw_d), GFP_KERNEL);
+	pw_d = kzalloc_obj(*pw_d);
 	if (!pw_d)
 		return ERR_PTR(-ENOMEM);
 
 	ret = xa_alloc(&pse_pw_d_map, &index, pw_d, XA_LIMIT(1, PSE_PW_D_LIMIT),
 		       GFP_KERNEL);
-	if (ret)
+	if (ret) {
+		kfree(pw_d);
 		return ERR_PTR(ret);
+	}
 
 	kref_init(&pw_d->refcnt);
 	pw_d->id = index;

@@ -969,7 +1000,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 			continue;
 		}
 
-		pw_d = devm_pse_alloc_pw_d(pcdev->dev);
+		pw_d = pse_alloc_pw_d();
 		if (IS_ERR(pw_d)) {
 			ret = PTR_ERR(pw_d);
 			goto out;

@@ -978,6 +1009,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 		supply = regulator_get(&rdev->dev, rdev->supply_name);
 		if (IS_ERR(supply)) {
 			xa_erase(&pse_pw_d_map, pw_d->id);
+			kfree(pw_d);
 			ret = PTR_ERR(supply);
 			goto out;
 		}

@@ -992,6 +1024,13 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 	}
 
 out:
+	/* Undo this call's domains before another controller can find them */
+	while (ret && i--) {
+		struct pse_power_domain *pw_d = pse_pi_take_pw_d(pcdev, i);
+
+		if (pw_d)
+			kref_put(&pw_d->refcnt, pse_pw_d_release_locked);
+	}
 	mutex_unlock(&pse_pw_d_mutex);
 	return ret;
 }

@@ -1949,15 +1988,16 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 	const struct pse_controller_ops *ops;
 	int ret = 0;
 
-	if (!pcdev->pi[psec->id].pw_d) {
-		NL_SET_ERR_MSG(extack, "no power domain attached");
-		return -EOPNOTSUPP;
-	}
-
 	/* We don't want priority change in the middle of an
 	 * enable/disable call or a priority mode change
 	 */
 	mutex_lock(&pcdev->lock);
+	if (!pcdev->pi[psec->id].pw_d) {
+		NL_SET_ERR_MSG(extack, "no power domain attached");
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
 	switch (pcdev->pi[psec->id].pw_d->budget_eval_strategy) {
 	case PSE_BUDGET_EVAL_STRAT_STATIC:
 		if (prio >= pcdev->nr_lines) {

-- 
2.43.0

The notification worker holds a reference on the pse_control, but not
on the PHY that psec->attached_phydev points to. If the PHY is removed
while the worker runs, the pse_control survives and the worker reads the
freed PHY. The same happens without a race when two PHYs share one PI:
attached_phydev keeps pointing at the first PHY after it is removed.

pse_control_put() unlinks and frees a pse_control under pse_list_mutex,
and phy_device_remove() calls it before the PHY is freed. So instead of
taking a reference, let the worker look the pse_control up and use
attached_phydev under pse_list_mutex. This nests pse_list_mutex inside
rtnl; nothing under pse_list_mutex takes rtnl.

For a PI shared by two PHYs there is no single netdev to report to, so
clear attached_phydev when a second PHY takes the pse_control, and warn.
Events for that PI are then no longer reported, even after one PHY goes
away. Failing the second PHY instead would fail its whole MDIO bus.

Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
removing a PHY while the worker waits for rtnl, and removing one of
two PHYs on a shared PI, both give

  BUG: KASAN: slab-use-after-free in pse_send_ntf_worker+0x2b0/0x2bc

With this patch both are gone.

Fixes: fc0e6db30941 ("net: pse-pd: Add support for reporting events")
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 44 ++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index eab7e25f2039..af9eb2979270 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -238,24 +238,22 @@ static int of_load_pse_pis(struct pse_controller_dev *pcdev)
  * @pcdev: a pointer to the PSE
  * @id: index of the PSE control
  *
- * Return: pse_control pointer or NULL. The device returned has had a
- *	   reference added and the pointer is safe until the user calls
- *	   pse_control_put() to indicate they have finished with it.
+ * Must be called with pse_list_mutex held. No reference is taken: the
+ * pointer is only valid until pse_list_mutex is released.
+ *
+ * Return: pse_control pointer or NULL.
  */
 static struct pse_control *
 pse_control_find_by_id(struct pse_controller_dev *pcdev, int id)
 {
 	struct pse_control *psec;
 
-	mutex_lock(&pse_list_mutex);
+	lockdep_assert_held(&pse_list_mutex);
+
 	list_for_each_entry(psec, &pcdev->pse_control_head, list) {
-		if (psec->id == id) {
-			kref_get(&psec->refcnt);
-			mutex_unlock(&pse_list_mutex);
+		if (psec->id == id)
 			return psec;
-		}
 	}
-	mutex_unlock(&pse_list_mutex);
 	return NULL;
 }
 

@@ -268,6 +266,7 @@ pse_control_find_by_id(struct pse_controller_dev *pcdev, int id)
 static struct net_device *pse_control_get_netdev(struct pse_control *psec)
 {
 	ASSERT_RTNL();
+	lockdep_assert_held(&pse_list_mutex);
 
 	if (!psec || !psec->attached_phydev)
 		return NULL;

@@ -1015,13 +1014,23 @@ static void pse_send_ntf_worker(struct work_struct *work)
 		struct net_device *netdev;
 		struct pse_control *psec;
 
-		psec = pse_control_find_by_id(pcdev, ntf.id);
+		/* If attached_phydev is set, it names the only PHY that
+		 * holds this pse_control: a second PHY on the same PI
+		 * clears it in pse_control_get_internal(). That PHY's
+		 * pse_control_put() unlinks and frees the pse_control under
+		 * pse_list_mutex before the PHY itself can be freed. Holding
+		 * the mutex across the lookup and the attached_phydev
+		 * dereference therefore keeps the PHY alive, which a
+		 * reference on the pse_control alone would not.
+		 */
 		rtnl_lock();
+		mutex_lock(&pse_list_mutex);
+		psec = pse_control_find_by_id(pcdev, ntf.id);
 		netdev = pse_control_get_netdev(psec);
 		if (netdev)
 			ethnl_pse_send_ntf(netdev, ntf.notifs);
+		mutex_unlock(&pse_list_mutex);
 		rtnl_unlock();
-		pse_control_put(psec);
 	}
 }
 

@@ -1412,6 +1421,19 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
 
 	list_for_each_entry(psec, &pcdev->pse_control_head, list) {
 		if (psec->id == index) {
+			/* attached_phydev can only name one PHY. If a second
+			 * one shares the PI, either may be removed first and
+			 * leave it pointing at a freed phy_device, so stop
+			 * reporting events for this PI instead.
+			 */
+			if (psec->attached_phydev &&
+			    psec->attached_phydev != phydev) {
+				dev_warn(pcdev->dev,
+					 "PI %u shared by several PHYs, not reporting its events\n",
+					 index);
+				psec->attached_phydev = NULL;
+			}
+
 			kref_get(&psec->refcnt);
 			return psec;
 		}

-- 
2.43.0

pse_ethtool_set_pw_limit() checks whether the PI is powered without
pcdev->lock. pse_isr() changes that state under the lock, so if the PD
disconnects in between, budget is reserved for a port that is no longer
powered.

Do the check in pse_pi_update_pw_budget() under the lock, and report
whether the budget was changed, so the rollback after a failed
regulator_set_current_limit() runs only then. This also rolls back a
budget that started from zero, which the old code skipped.

Found by code inspection. Not reproduced.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 39 ++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index af9eb2979270..36f7dba8190a 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -1821,22 +1821,35 @@ 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
+ * Return: Power allocated before the update, or a negative error code.
+ *	   0 is also returned when the budget was not changed, so callers
+ *	   check @updated before they roll back.
  */
 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)
 {
 	struct pse_pi *pi = &pcdev->pi[id];
 	int previous_pw_allocated;
 	int pw_diff, ret = 0;
 
+	*updated = false;
+
 	/* We don't want pw_allocated_mW value change in the middle of an
 	 * power budget update
 	 */
 	mutex_lock(&pcdev->lock);
+	/* Only a PI under software power control that is enabled and has a
+	 * PD detected gets a budget. Check this 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;
+
 	previous_pw_allocated = pi->pw_allocated_mW;
 	pw_diff = pw_req - previous_pw_allocated;
 	if (!pw_diff) {

@@ -1854,6 +1867,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,
 		regulator_free_power_budget(pi->pw_d->supply, -pw_diff);
 	}
 	pi->pw_allocated_mW = pw_req;
+	*updated = true;
 	ret = previous_pw_allocated;
 
 out:

@@ -1874,6 +1888,7 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,
 			     const unsigned int pw_limit)
 {
 	int uV, uA, ret, previous_pw_allocated = 0;
+	bool updated;
 	s64 tmp_64;
 
 	if (pw_limit > MAX_PI_PW)

@@ -1900,21 +1915,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);
-	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;

-- 
2.43.0

When pse_controller_register() fails after it allocated the
notification kfifo, it returns without freeing it.

Free the kfifo on every error path.

Found by code inspection and AI review.

Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index eccbdb3acbee..137570fe0da2 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -1121,24 +1121,25 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	    !pcdev->ops->pi_get_pw_status) {
 		dev_err(pcdev->dev,
 			"Mandatory status report callbacks are missing");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free_kfifo;
 	}
 
 	ret = of_load_pse_pis(pcdev);
 	if (ret)
-		return ret;
+		goto free_kfifo;
 
 	/* The PI regulator ops use pcdev->pi until devres unregisters the
 	 * regulators, so free it from devres too, after them.
 	 */
 	ret = devm_add_action_or_reset(pcdev->dev, devm_pse_release_pis, pcdev);
 	if (ret)
-		return ret;
+		goto free_kfifo;
 
 	if (pcdev->ops->setup_pi_matrix) {
 		ret = pcdev->ops->setup_pi_matrix(pcdev);
 		if (ret)
-			return ret;
+			goto free_kfifo;
 	}
 
 	/* Each regulator name len is pcdev dev name + 7 char +

@@ -1155,26 +1156,33 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 			continue;
 
 		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
-		if (!reg_name)
-			return -ENOMEM;
+		if (!reg_name) {
+			ret = -ENOMEM;
+			goto free_kfifo;
+		}
 
 		snprintf(reg_name, reg_name_len, "pse-%s_pi%d",
 			 dev_name(pcdev->dev), i);
 
 		ret = devm_pse_pi_regulator_register(pcdev, reg_name, i);
 		if (ret)
-			return ret;
+			goto free_kfifo;
 	}
 
 	ret = pse_register_pw_ds(pcdev);
 	if (ret)
-		return ret;
+		goto free_kfifo;
 
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);
 	mutex_unlock(&pse_list_mutex);
 
 	return 0;
+
+free_kfifo:
+	cancel_work_sync(&pcdev->ntf_work);
+	kfifo_free(&pcdev->ntf_fifo);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
 

-- 
2.43.0

pse_controller_unregister() frees the PI array, but the PI regulators
are only unregistered after it, by devres. Until then their ops still
run, for example on a read of the regulator's sysfs "state" or when a
consumer enables or disables the PI, and they read the freed array.

Free the PI array from a devm action that is added before the PI
regulators, so devres frees it after them. This also frees it when
registration fails, where it was leaked before.

Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
reading the PI regulators' sysfs "state" while the controller unbinds
gives

  BUG: KASAN: slab-use-after-free in pse_pi_is_enabled+0x294/0x2d4

With this patch it is gone.

Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 43f6e304fe5e..eccbdb3acbee 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -147,6 +147,11 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)
 	kfree(pcdev->pi);
 }
 
+static void devm_pse_release_pis(void *data)
+{
+	pse_release_pis(data);
+}
+
 /**
  * of_load_pse_pis - load all the PSE PIs
  * @pcdev: a pointer to the PSE controller device

@@ -1123,6 +1128,13 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	if (ret)
 		return ret;
 
+	/* The PI regulator ops use pcdev->pi until devres unregisters the
+	 * regulators, so free it from devres too, after them.
+	 */
+	ret = devm_add_action_or_reset(pcdev->dev, devm_pse_release_pis, pcdev);
+	if (ret)
+		return ret;
+
 	if (pcdev->ops->setup_pi_matrix) {
 		ret = pcdev->ops->setup_pi_matrix(pcdev);
 		if (ret)

@@ -1194,7 +1206,6 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	mutex_unlock(&pse_list_mutex);
 	cancel_work_sync(&pcdev->ntf_work);
 	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
 	kfifo_free(&pcdev->ntf_fifo);
 }
 EXPORT_SYMBOL_GPL(pse_controller_unregister);

-- 
2.43.0

A power domain shared by several controllers holds one supply handle.
Its consumer device is a PI regulator of the controller that created
the domain. When that controller unbinds, devres frees the regulator,
but the other controllers still use the domain and its handle, which
now points at a freed device. Reading the regulator summary in debugfs
then reads freed memory.

The domain does not need a handle of its own. The budget is counted in
the supply's regulator_dev, and each PI regulator already holds a
handle to that supply, which lives as long as the PI regulator. Use
that handle for the budget, and drop the domain's handle.
pse_flush_pw_ds() clears a PI's pw_d before devres unregisters the PI
regulator, so the budget is never used through a handle that is gone.

To find the domain of a new PI, compare its supply with the supply of a
PI already in the domain. For this, keep a list of the PIs of each
domain under pse_pw_d_mutex, which pse_flush_pw_ds() now holds for its
whole loop.

Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers on
one supply: after the one that created the domain unbinds, reading
/sys/kernel/debug/regulator/regulator_summary gives

  BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4

With this patch the report is gone. A second test, where one controller
changes its power limit in a loop while the other one unbinds, is clean
too.

Fixes: 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 70 +++++++++++++++++------------------
 include/linux/pse-pd/pse.h    |  2 +
 2 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 137570fe0da2..fac0814f60ae 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -45,16 +45,17 @@ struct pse_control {
 /**
  * struct pse_power_domain - a PSE power domain
  * @id: ID of the power domain
- * @supply: Power supply the Power Domain
  * @refcnt: Number of gets of this pse_power_domain
  * @budget_eval_strategy: Current power budget evaluation strategy of the
  *			  power domain
+ * @pis: PIs that use the power domain, under pse_pw_d_mutex. The supply
+ *	 handles of their regulators all point at the same supply.
  */
 struct pse_power_domain {
 	int id;
-	struct regulator *supply;
 	struct kref refcnt;
 	u32 budget_eval_strategy;
+	struct list_head pis;
 };
 
 static int of_load_single_pse_pi_pairset(struct device_node *node,

@@ -442,7 +443,7 @@ static void pse_pi_deallocate_pw_budget(struct pse_pi *pi)
 	if (!pi->pw_d || !pi->pw_allocated_mW)
 		return;
 
-	regulator_free_power_budget(pi->pw_d->supply, pi->pw_allocated_mW);
+	regulator_free_power_budget(pi->rdev->supply, pi->pw_allocated_mW);
 	pi->pw_allocated_mW = 0;
 }
 

@@ -555,7 +556,8 @@ pse_pi_allocate_pw_budget_static_prio(struct pse_controller_dev *pcdev, int id,
 	int ret, _prio;
 
 	_prio = pcdev->nr_lines;
-	while (regulator_request_power_budget(pi->pw_d->supply, pw_req) == -ERANGE) {
+	while (regulator_request_power_budget(pi->rdev->supply, pw_req) ==
+	       -ERANGE) {
 		if (_prio <= pi->prio) {
 			NL_SET_ERR_MSG_FMT(extack,
 					   "PI %d: not enough power budget available",

@@ -890,24 +892,17 @@ devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
 }
 
 /* Release for a put made with pse_pw_d_mutex already held. */
-static void pse_pw_d_release_locked(struct kref *kref)
+static void pse_pw_d_release(struct kref *kref)
 {
 	struct pse_power_domain *pw_d = container_of(kref,
 						     struct pse_power_domain,
 						     refcnt);
 
 	lockdep_assert_held(&pse_pw_d_mutex);
-	regulator_put(pw_d->supply);
 	xa_erase(&pse_pw_d_map, pw_d->id);
 	kfree(pw_d);
 }
 
-static void __pse_pw_d_release(struct kref *kref)
-{
-	pse_pw_d_release_locked(kref);
-	mutex_unlock(&pse_pw_d_mutex);
-}
-
 /* Clear pi[id].pw_d before the domain's reference is dropped. The PI
  * regulator ops and pse_isr() hold pcdev->lock while they use it.
  */

@@ -924,6 +919,14 @@ pse_pi_take_pw_d(struct pse_controller_dev *pcdev, int id)
 	return pw_d;
 }
 
+/* Drop @pi from @pw_d, with pse_pw_d_mutex held */
+static void pse_pw_d_leave(struct pse_power_domain *pw_d, struct pse_pi *pi)
+{
+	lockdep_assert_held(&pse_pw_d_mutex);
+	list_del(&pi->pw_d_node);
+	kref_put(&pw_d->refcnt, pse_pw_d_release);
+}
+
 /**
  * pse_flush_pw_ds - flush all PSE power domains of a PSE
  * @pcdev: a pointer to the initialized PSE controller device

@@ -933,18 +936,13 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
 	struct pse_power_domain *pw_d;
 	int i;
 
+	mutex_lock(&pse_pw_d_mutex);
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		pw_d = pse_pi_take_pw_d(pcdev, i);
-		if (!pw_d)
-			continue;
-
-		pw_d = xa_load(&pse_pw_d_map, pw_d->id);
-		if (!pw_d)
-			continue;
-
-		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
-			       &pse_pw_d_mutex);
+		if (pw_d)
+			pse_pw_d_leave(pw_d, &pcdev->pi[i]);
 	}
+	mutex_unlock(&pse_pw_d_mutex);
 }
 
 /**

@@ -952,8 +950,8 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
  *
  * A power domain is shared by every controller whose PIs sit on the same
  * supply, so it must not be tied to the lifetime of the controller that
- * happened to create it. It is freed by pse_pw_d_release_locked() on the
- * last kref_put().
+ * happened to create it. It is freed by pse_pw_d_release() on the last
+ * kref_put().
  *
  * Return: Pointer to the newly allocated PSE power domain or error pointers
  */

@@ -992,7 +990,6 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		struct regulator_dev *rdev = pcdev->pi[i].rdev;
 		struct pse_power_domain *pw_d;
-		struct regulator *supply;
 		bool present = false;
 		unsigned long index;
 

@@ -1003,10 +1000,15 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 			continue;
 
 		xa_for_each(&pse_pw_d_map, index, pw_d) {
+			struct pse_pi *pi = list_first_entry(&pw_d->pis,
+							     struct pse_pi,
+							     pw_d_node);
+
 			/* Power supply already registered as a PSE power
 			 * domain.
 			 */
-			if (regulator_is_equal(pw_d->supply, rdev->supply)) {
+			if (regulator_is_equal(pi->rdev->supply,
+					       rdev->supply)) {
 				present = true;
 				pcdev->pi[i].pw_d = pw_d;
 				break;

@@ -1014,6 +1016,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 		}
 		if (present) {
 			kref_get(&pw_d->refcnt);
+			list_add_tail(&pcdev->pi[i].pw_d_node, &pw_d->pis);
 			continue;
 		}
 

@@ -1023,20 +1026,13 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 			goto out;
 		}
 
-		supply = regulator_get(&rdev->dev, rdev->supply_name);
-		if (IS_ERR(supply)) {
-			xa_erase(&pse_pw_d_map, pw_d->id);
-			kfree(pw_d);
-			ret = PTR_ERR(supply);
-			goto out;
-		}
-
-		pw_d->supply = supply;
 		if (pcdev->supp_budget_eval_strategies)
 			pw_d->budget_eval_strategy = pcdev->supp_budget_eval_strategies;
 		else
 			pw_d->budget_eval_strategy = PSE_BUDGET_EVAL_STRAT_DISABLED;
 		kref_init(&pw_d->refcnt);
+		INIT_LIST_HEAD(&pw_d->pis);
+		list_add_tail(&pcdev->pi[i].pw_d_node, &pw_d->pis);
 		pcdev->pi[i].pw_d = pw_d;
 	}
 

@@ -1046,7 +1042,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
 		struct pse_power_domain *pw_d = pse_pi_take_pw_d(pcdev, i);
 
 		if (pw_d)
-			kref_put(&pw_d->refcnt, pse_pw_d_release_locked);
+			pse_pw_d_leave(pw_d, &pcdev->pi[i]);
 	}
 	mutex_unlock(&pse_pw_d_mutex);
 	return ret;

@@ -1940,7 +1936,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,
 	if (!pw_diff) {
 		goto out;
 	} else if (pw_diff > 0) {
-		ret = regulator_request_power_budget(pi->pw_d->supply, pw_diff);
+		ret = regulator_request_power_budget(pi->rdev->supply, pw_diff);
 		if (ret) {
 			NL_SET_ERR_MSG_FMT(extack,
 					   "PI %d: not enough power budget available",

@@ -1949,7 +1945,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,
 		}
 
 	} else {
-		regulator_free_power_budget(pi->pw_d->supply, -pw_diff);
+		regulator_free_power_budget(pi->rdev->supply, -pw_diff);
 	}
 	pi->pw_allocated_mW = pw_req;
 	*updated = true;

diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index a917ac32dc0c..17738ffcd224 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h

@@ -247,6 +247,7 @@ struct pse_pi_pairset {
  * @rdev: regulator represented by the PSE PI
  * @admin_state_enabled: PI enabled state
  * @pw_d: Power domain of the PSE PI
+ * @pw_d_node: entry in the list of PIs that use @pw_d
  * @prio: Priority of the PSE PI. Used in static budget evaluation strategy
  * @isr_pd_detected: PSE PI detection status managed by the interruption
  *		     handler. This variable is relevant when the power enabled

@@ -261,6 +262,7 @@ struct pse_pi {
 	struct regulator_dev *rdev;
 	bool admin_state_enabled;
 	struct pse_power_domain *pw_d;
+	struct list_head pw_d_node;
 	int prio;
 	bool isr_pd_detected;
 	int pw_allocated_mW;

-- 
2.43.0

pse_controller_unregister() frees the PI array while the controller is
still on pse_controller_list, and only removes it from the list
afterwards. of_pse_control_get() walks that list under pse_list_mutex
and reads pcdev->pi[i].np in of_pse_match_pi(). A consumer that probes
at that moment can read the freed PI array.

Remove the controller from the list first, before the worker is
drained, the power domains are flushed and the PI array is freed. Both
sides take pse_list_mutex, so once the controller is off the list, no
new lookup can find it, and a lookup that is already running finishes
before the PI array is freed.

Found by code inspection. Not reproduced.

Fixes: 9be9567a7c59 ("net: pse-pd: Add support for PSE PIs")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pse_core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 36f8e4e7d191..43f6e304fe5e 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c

@@ -1185,13 +1185,17 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	 */
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	/* Unlink before tearing down: of_pse_control_get() walks the list
+	 * and reads pcdev->pi[], and must not hand out a new control for a
+	 * controller that is going away.
+	 */
+	mutex_lock(&pse_list_mutex);
+	list_del(&pcdev->list);
+	mutex_unlock(&pse_list_mutex);
 	cancel_work_sync(&pcdev->ntf_work);
 	pse_flush_pw_ds(pcdev);
 	pse_release_pis(pcdev);
 	kfifo_free(&pcdev->ntf_fifo);
-	mutex_lock(&pse_list_mutex);
-	list_del(&pcdev->list);
-	mutex_unlock(&pse_list_mutex);
 }
 EXPORT_SYMBOL_GPL(pse_controller_unregister);
 

-- 
2.43.0

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.

Found by code inspection. Not reproduced.

Fixes: 359754013e6a ("net: pse-pd: pd692x0: Add support for PSE PI priority feature")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pd692x0.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 34f0dfbccd06..568789bd912f 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c

@@ -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 */

-- 
2.43.0

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.

Found by code inspection. Not reproduced.

Fixes: 1c67f9c54cdc ("net: pse-pd: pd692x0: Fix power budget leak in manager setup error path")
Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>
Signed-off-by: Carlo Szelinsky <redacted>
---
 drivers/net/pse-pd/pd692x0.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index 209de9cec849..34f0dfbccd06 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c

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

-- 
2.43.0

Fixes for the PSE core and the pd692x0 driver, found while working on
the PSE controller teardown. Most of them are about what happens when a
controller unbinds or fails to register.

 1  The kfifo is too small: podl-pse-regulator and any controller with
    0 or 1 lines cannot register.
 2  Removes two checks that can never be true. Not a fix; it makes 3
    smaller.
 3  A module reference leaks when reading the port state fails.
 4  A failed IRQ request stays in pcdev->irq, so unregister disables a
    line another device may own.
 5  The notification worker can read a PHY that was already freed.
 6  Setting a power limit checks the PI state without the lock.
 7  A power domain shared by two controllers is freed when the one that
    created it unbinds, or when registration fails part way.
 8  Unregister frees the PI array before it stops the IRQ handler and
    the notification worker. The worker can also be queued again
    during unregister.
 9  Unregister frees the PI array while a lookup can still find the
    controller.
 10 The PI regulator ops can run after the PI array is freed.
 11 A failed registration leaks the kfifo.
 12 A shared power domain's supply handle goes stale when the controller
    that created it unbinds.
 13 pd692x0 leaks its manager power budget when probe fails.
 14 pd692x0 crashes on a manager supply that is not resolved yet.

Some patches need earlier ones: 7 needs the locked check from 6, 10 to
12 build on 7 to 9, and 14 relies on 13 to give back a budget that was
already reserved.

Testing: every patch builds on its own with W=1 and is checkpatch
clean. 1, 5, 7, 10 and 12 were tested in QEMU (arm64, KASAN, lockdep,
kmemleak) with test-only fake PSE controllers: without the patch, the
failure in its commit message shows up; with the whole series, the
tests are clean, except for the older lockdep report and the reserved
budget, both listed below. For v3, the whole series and 7 on its own
were tested again. The other patches were found by code inspection and
are build-tested only.

It would be nice to test these fixes on real hardware, to validate that
it still works without issues.
I will try to do it when I find time but if someone does it beforehand I
would be thankful.

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

On Tue, 03 Nov 2026 23:02:05 +0100, Carlo Szelinsky [off-list ref] wrote:

pse_controller_register() already fails when a driver has no
pi_get_admin_state() callback. So the checks for it in
pse_pi_is_enabled() and pse_control_get_internal() are never true.
Remove them.

No functional change. It makes the next patch, which fixes an error
path in pse_control_get_internal(), smaller.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

47 more quoted lines

Can you summarize this comment. It's hard to read.
Also we have the commit message that explain the why.

Same here. The warn message explain already the issue.

With these changes you can add my Reviewed-by.

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

On Tue, 03 Nov 2026 23:02:10 +0100, Carlo Szelinsky [off-list ref] wrote:

A power domain is shared by all controllers on the same supply and
counted with a kref, but it is devm memory of the controller that
created it. When that controller unbinds while another one still uses
the domain, devres frees it anyway, and the other controller and the
global map keep pointing at freed memory. The same happens when
pse_register_pw_ds() fails part way: the domains it already set up stay
in the map, and devres frees them.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

On Tue, 03 Nov 2026 23:02:11 +0100, Carlo Szelinsky [off-list ref] wrote:

pse_controller_unregister() flushes the power domains and frees the PI
array before it disables the IRQ, while pse_isr() uses both. No in-tree
driver hits this: tps23881, the only user of devm_pse_irq_helper(),
requests its IRQ after it registers the controller, so devres frees
the IRQ before unregister runs. A driver that keeps its IRQ across
unregister would read freed memory.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

On Tue, 03 Nov 2026 23:02:13 +0100, Carlo Szelinsky [off-list ref] wrote:

pse_controller_unregister() frees the PI array, but the PI regulators
are only unregistered after it, by devres. Until then their ops still
run, for example on a read of the regulator's sysfs "state" or when a
consumer enables or disables the PI, and they read the freed array.

Free the PI array from a devm action that is added before the PI
regulators, so devres frees it after them. This also frees it when
registration fails, where it was leaked before.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

On Tue, 03 Nov 2026 23:02:14 +0100, Carlo Szelinsky [off-list ref] wrote:

When pse_controller_register() fails after it allocated the
notification kfifo, it returns without freeing it.

Free the kfifo on every error path.

Found by code inspection and AI review.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

On Tue, 03 Nov 2026 23:02:15 +0100, Carlo Szelinsky [off-list ref] wrote:

A power domain shared by several controllers holds one supply handle.
Its consumer device is a PI regulator of the controller that created
the domain. When that controller unbinds, devres frees the regulator,
but the other controllers still use the domain and its handle, which
now points at a freed device. Reading the regulator summary in debugfs
then reads freed memory.

[...]

Reviewed-by: Kory Maincent <kory.maincent@bootlin.com>

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

On Wed, 04 Nov 2026 16:19:34 +0100, Kory Maincent wrote:

It would be nice to test these fixes on real hardware, to validate that
it still works without issues.
I will try to do it when I find time but if someone does it beforehand I
would be thankful.

Hi Kory,

thanks for the reviews.

I will make the two comments in patch 5 shorter in v4.

Before I send v4, I will test the series on my hardware and report the
results here.

Oleksij, I would be happy to get your feedback on v3 as well, mainly on
7, 8, 10, 11 and 12, which changed since v2. I will also wait for the
sashiko review of v3 before I send v4.

Thanks,
Carlo

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch changes what `pse_control::attached_phydev` means and
  how it must be locked, but the kernel-doc was not updated to match.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  `pse_send_ntf_worker()` uses the net_device without holding a…

--

net: pse-pd: do not reach a freed PHY from the notification worker

With this patch, pse_send_ntf_worker() looks up the pse_control and reads
attached_phydev under pse_list_mutex. It no longer relies on a reference
to the pse_control, so a removed PHY can no longer be reached. It also
clears attached_phydev and warns when a second PHY takes the same PI.

[ ... ]

[Severity: Low]
This isn't a bug, but should the kernel-doc be updated to match the new
rules?

pse_control_find_by_id() got a "Must be called with pse_list_mutex held"
note. The kernel-doc of pse_control_get_netdev() still only says:

 * Return: netdev pointer or NULL

The struct pse_control doc in pse_core.c also still describes the field as:

 * @attached_phydev: PHY device pointer attached by the PSE control

After this patch, pse_control_get_internal() sets attached_phydev to NULL
as soon as a second PHY shares the PI. It stays NULL while PHYs still hold
the pse_control.

The field can also now only be read under pse_list_mutex. That rule is
written down only in the block comment inside pse_send_ntf_worker().

Could the struct doc and the pse_control_get_netdev() doc mention the
locking requirement and the shared-PI NULL case?

[ ... ]

[ ... ]

[Severity: High]
This is a pre-existing issue, but is the net_device itself still left
unprotected here?

The new locking keeps the phy_device alive. No reference is taken on
psec->attached_phydev->attached_dev, though.

ethnl_pse_send_ntf() dereferences that pointer in ethnl_fill_reply_header()
(dev->ifindex, dev->name) and in ethnl_multicast() (dev_net(dev)). Both
calls come after genlmsg_new(GFP_KERNEL), which can sleep.

Neither rtnl nor pse_list_mutex appears to keep the netdev alive. The
comment in phy_detach_internal() says:

		 * Drivers may detach without RTNL while dev is unregistered,

Also, free_netdev() asserts RTNL only while reg_state is
NETREG_UNREGISTERING. For a device that is already NETREG_UNREGISTERED it
goes straight to put_device().

As an in-tree example, hisi_femac_drv_remove() does:

	unregister_netdev(ndev);

	phy_disconnect(ndev->phydev);
	clk_disable_unprepare(priv->clk);
	free_netdev(ndev);

The last two calls there run without rtnl. Can this sequence happen?

worker                                 hisi_femac_drv_remove()
                                       unregister_netdev(ndev)
                                         (rtnl released)
rtnl_lock()
mutex_lock(&pse_list_mutex)
pse_control_get_netdev()
  returns attached_dev (still set)
ethnl_pse_send_ntf()
  genlmsg_new(GFP_KERNEL) sleeps

15 more lines

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

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 0

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

- [High] The check that moved under the lock does not test whether the PI
  is powered, so the bug the commit says it fixes ("budget is reserved…
- [Medium] The rollback in pse_ethtool_set_pw_limit() writes back
  previous_pw_allocated, which was read in an earlier critical section.

Pre-existing issues:
- [Medium] In pse_ethtool_set_pw_limit(), the return value of the
  rollback call to pse_pi_update_pw_budget() is ignored.
- [Medium] pse_ethtool_set_pw_limit() still runs in two phases.

--

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

This moves the check for software power control, admin_state_enabled and
isr_pd_detected out of pse_ethtool_set_pw_limit() and into
pse_pi_update_pw_budget(), where it runs under pcdev->lock. It also adds an
updated out-parameter. With it, the rollback after a failed
regulator_set_current_limit() runs only when the budget was actually
changed, including a budget that started from zero.

[ ... ]

[ ... ]

[Severity: High]
Does this check tell whether the PI is actually powered? Both flags can be
set while the port is off and pw_allocated_mW is 0, and no race is needed
to get there.

The first case is the admin-enable pending state. pse_set_config_isr() sets
isr_pd_detected and swallows -ERANGE:

	pcdev->pi[id].isr_pd_detected = true;
	if (pcdev->pi[id].admin_state_enabled) {
		ret = _pse_pi_delivery_power_sw_pw_ctrl(pcdev, id,
							&extack);
		if (ret == -ERANGE)
			ret = 0;
	}

pse_pi_enable() does the same for admin_state_enabled:

	if (!ret || ret == -ERANGE) {
		pcdev->pi[id].admin_state_enabled = 1;
		ret = 0;

In both cases pse_pi_allocate_pw_budget_static_prio() returned before it
set pw_allocated_mW and before ops->pi_enable() was called.

The second case is priority shedding from pse_isr(), which is the state
change the commit message refers to. pse_disable_pi_pol() calls
_pse_pi_disable(), which powers the PI off and zeroes its budget but leaves
both flags set:

	ret = ops->pi_disable(pcdev, id);
	if (ret)
		return ret;

	pse_pi_deallocate_pw_budget(&pcdev->pi[id]);

pse_pi_is_admin_enable_pending() already deals with this by also calling
pse_pi_is_hw_enabled().

24 more lines

[Severity: Medium]
This isn't a bug introduced by this patch, but the budget update and the
hardware limit write still happen in separate pcdev->lock sections. Can
pse_isr() run between them and allocate budget based on the stale hardware
limit?

Suppose a DISCONNECTION and then a CLASSIFICATION are handled in that
window. _pse_pi_delivery_power_sw_pw_ctrl() would then use the old limit:

	if (ops->pi_get_pw_limit) {
		ret = ops->pi_get_pw_limit(pcdev, id);
		if (ret < 0)
			return ret;

		if (ret < pw_req)
			pw_req = ret;
	}

For example, when lowering the limit from 30000 mW to 15000 mW, the ISR
could reserve 30000 mW before the 15000 mW limit is programmed. The port
would stay over-reserved until the next PD event.

On the failure path, the rollback would also replace the allocation the ISR
just made with the stale value from before the call.

Simply holding pcdev->lock across regulator_set_current_limit() would not
work, because pse_pi_set_current_limit() takes that lock itself.

[Severity: Medium]
Is previous_pw_allocated still valid when the rollback runs? It was read in
the first pse_pi_update_pw_budget() call. pcdev->lock is dropped after that
call, then taken and dropped again inside
regulator_set_current_limit()->pse_pi_set_current_limit(), and only then
taken again for the rollback.

Two things gate the rollback: updated, which describes the first call, and
the flag check. Neither notices if pw_allocated_mW changed in between.

Here is one possible interleaving on tps23881 with the static strategy:

CPU0
pse_ethtool_set_pw_limit()
  pse_pi_update_pw_budget()        /* P -> X, unlock */

CPU1
pse_isr()                          /* higher prio PI classified */
  pse_pi_allocate_pw_budget_static_prio()
    pse_disable_pi_prio()
      pse_disable_pi_pol()
        _pse_pi_disable()          /* our PI off, pw_allocated_mW = 0 */

CPU0
  regulator_set_current_limit()    /* fails */
  pse_pi_update_pw_budget()        /* flags still set, requests P */

The rollback then sets pw_allocated_mW to P for a port that is powered off.
The next retry overwrites it in pse_pi_allocate_pw_budget_static_prio()
without freeing it, so P would leak.

The new rollback-from-zero behavior adds another case:

1. A pending PI gets X reserved.
2. Retry delivery powers it inside the window with R, which leaks X.
3. The rollback to 0 frees R and sets pw_allocated_mW to 0 while the port
   is powered and drawing R.

Would it be safer to restore only when pw_allocated_mW still equals the

23 more lines

 
 	return ret;

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

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

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

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] net: pse-pd: spurious error event in pse_isr() on -ENODEV
  during unregister

--

[Severity: Medium]
pse_isr() can report a false power control error during unregister.

Say the threaded pse_isr() is already blocked on pcdev->lock when
pse_controller_unregister() sets pcdev->unregistering and drops the
lock. It then takes the lock while disable_irq() is still waiting for
it to finish.

For a classification event on a PI with admin_state_enabled set,
pse_set_config_isr() calls _pse_pi_delivery_power_sw_pw_ctrl(). That
now returns -ENODEV at this new check.

pse_set_config_isr() only clears -ERANGE, so -ENODEV goes back to
pse_isr(). pse_isr() treats any non-zero return as a failure and sets
ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR for that PI. It pushes the event
into pcdev->ntf_fifo and schedules ntf_work before cancel_work_sync()
runs.

If pse_send_ntf_worker() is already running at that point, it can
pick up the entry and send it through ethnl_pse_send_ntf(). Userspace
then sees a power control error for a PI that did not fail; the
controller is only going away.

This is not a memory safety problem, since disable_irq() orders the
queueing before the drain, but the event is misleading.

Should pse_set_config_isr() filter the -ENODEV from the unregistering
check the same way it filters -ERANGE? Or would you rather have
pse_isr() skip the PIs once pcdev->unregistering is set?

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

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] This patch frees pcdev->pi from a devm action.
- [Low] The commit message has only 'Fixes: d83e13761d5b' and does not
  mention that the fix depends on two earlier patches in the series:…

--

net: pse-pd: keep the PI data until the PI regulators are gone

This patch stops freeing the PSE PI array in pse_controller_unregister().
The array is now freed from a devm action that is registered before the
PI regulators, so devres frees it after the regulators are unregistered.
This also fixes a leak of the PI array when registration fails.

Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")

[Severity: Low]

Should the commit message say that this patch depends on two earlier
patches in the series?

  "net: pse-pd: do not tie a shared power domain to its creator's devres"
  "net: pse-pd: stop async event sources before freeing PI data in
  unregister"

On the baseline, pse_flush_pw_ds() does not clear pcdev->pi[i].pw_d. The
domain is allocated with devm_kzalloc() in pse_register_pw_ds(), and that
runs after the PI regulators are registered. So devres frees the domain
before it unregisters the regulators.

Suppose this patch is backported without the power domain patch. The
reproducer in the message would then still go through:

pse_pi_is_enabled()
  pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)

It would read pw_d->budget_eval_strategy from freed devm memory, so the
use-after-free would just move from pi[] to pi[].pw_d.

The unregistering gate has the same kind of dependency. Without it, a
consumer enable after unregister would get past the pi[] access and into
pse_disable_pi_pol(). That function uses the kfifo and the work item,
and unregister has already torn both down.

Both prerequisites have their own Fixes tags (50f8b341d268 and
ffef61d6d273), so this only matters for a partial stable backport.

[ ... ]

[Severity: High]

Does this devres ordering cover the case where a consumer enables or
disables the PI, which the commit message also mentions?

regulator_unregister() does not take the rdev lock and does not wait for
consumer calls that are already running. If consumers still hold handles,
it only does WARN_ON(rdev->open_count).

pse_control_get_internal() takes the PSE consumer handle with the
non-devm regulator_get_exclusive():

    psec->ps = regulator_get_exclusive(pcdev->dev,
                       rdev_get_name(pcdev->pi[index].rdev));

The PHY only drops that handle in phy_device_remove()->pse_control_put().
pse_controller_unregister() never walks pcdev->pse_control_head, so these
handles survive the controller unbind.

I can see two ways to hit this. The first is a consumer disable racing
with the unbind:

CPU0 (for example ethtool PSE admin-disable)
pse_ethtool_set_config()
  regulator_disable(psec->ps)
    pse_pi_disable()
      pi = &pcdev->pi[id];
      mutex_lock(&pcdev->lock);
      _pse_pi_disable()
        ops->pi_disable()   /* sleeps on I2C */

CPU1 (unbind)
devres release
  regulator_unregister()
  devm_pse_release_pis()
    kfree(pcdev->pi)        /* without pcdev->lock */
  frees the driver priv that embeds pcdev

When CPU0 returns from ops->pi_disable(), it runs:

23 more lines

On Thu, 05 Nov 2026 10:08:32 +0000, netdev-bot+sashiko@kernel.org wrote:

Could the struct doc and the pse_control_get_netdev() doc mention the
locking requirement and the shared-PI NULL case?

Yes. In v4 the rule moves into the kernel-doc, and the comment in the
worker gets shorter, as Kory asked.

This is a pre-existing issue, but is the net_device itself still left
unprotected here?

Yes, the worker holds no reference on the net_device. This was the
same before the patch, which only fixes the phy_device part. I keep it
for a separate fix.

Carlo

On Thu, 05 Nov 2026 10:08:34 +0000, netdev-bot+sashiko@kernel.org wrote:

Does this check tell whether the PI is actually powered? Both flags can be
set while the port is off and pw_allocated_mW is 0, and no race is needed
to get there.

No, and it did not before this patch either. The patch only moves the
same check under the lock. That fixes the race in the commit message:
a disconnect clears isr_pd_detected, and the check now sees it.

The pending and shed cases need their own fix. Kory and Oleksij do not
want set_pw_limit to skip a pending PI, so it is not a one-line change.
I keep it out of this series.

Is previous_pw_allocated still valid when the rollback runs?

This is a pre-existing issue, but the return value of the rollback
pse_pi_update_pw_budget() call is ignored.

This isn't a bug introduced by this patch, but the budget update and the
hardware limit write still happen in separate pcdev->lock sections.

All three come from the same thing: the budget update and the hardware
limit write run in two lock sections. That was already the case before
this patch. The real fix is to do both under one lock, as Oleksij
suggested, and that needs a different structure. I leave it for a
follow-up.

Carlo

On Thu, 05 Nov 2026 10:08:35 +0000, netdev-bot+sashiko@kernel.org wrote:

Should pse_set_config_isr() filter the -ENODEV from the unregistering
check the same way it filters -ERANGE? Or would you rather have
pse_isr() skip the PIs once pcdev->unregistering is set?

Right, thanks. In v4, pse_set_config_isr() does not try to power the
PI once pcdev->unregistering is set, so no false error event is sent.

pw-bot: cr

Carlo

On Thu, 05 Nov 2026 10:08:37 +0000, netdev-bot+sashiko@kernel.org wrote:

Should the commit message say that this patch depends on two earlier
patches in the series?

Yes. v4 says that it needs patches 7 and 8.

pcdev itself is freed in the same teardown. Can any ordering of the PI
array free close these paths? Would the existing pse_control objects have
to be revoked in pse_controller_unregister(), or pcdev refcounted, before
the consumer case is fixed?

No, this patch does not close them. A pse_control that outlives its
controller is an older and bigger problem, and this patch does not
change it. It needs its own fix in net-next, together with Kyle's rework
of the PSE regulators.

Carlo