[PATCH net-next 0/4] net: pse-pd: core fixes ahead of the controller notifier

[PATCH net-next 0/4] net: pse-pd: core fixes ahead of the controller notifier

[Carlo Szelinsky]

These four PSE core fixes were part of the "decouple controller lookup
from MDIO probe" series [1]; Oleksij asked to split them out. They do
not need phylib, and each one fixes a problem on its own:

 1. Unregister frees the PI data while the controller can still be
    found. Take it off the list and stop the IRQ and the worker first.
 2. A failed registration leaks memory, and can leave freed power
    domains in the global map. Clean up on the error paths.
 3. si3474: use dev_err_probe(), so the deferral from 4 is not logged
    as an error.
 4. Defer the PSE driver's probe until every PI's vpwr supply is there.

Patch 1 conflicts with the pending net series [2]; keep the order from
patch 1. Patch 2 conflicts with two of the net fixes posted alongside
[3]; the note below its --- explains how to merge them.

Tested in QEMU on arm64 with KASAN, lockdep and kmemleak: controllers
whose supply never appears defer, and an injected pse_register_pw_ds()
failure is cleaned up and retried. The only KASAN reports are the known
stale-handle bug at MDIO bus unbind, the same without this series; the
decouple series fixes it. This series, the decouple series and the net
fixes together also run on a Hasivo S600WP (hs104), with 200 controller
unbind/bind cycles and no warnings or leaks. Every patch builds with
W=1, and PHYLIB=m, PHYLIB=n and PSE_CONTROLLER=n link.

[1] https://lore.kernel.org/netdev/20261004164219.1161294-1-github@szelinsky.de/
[2] https://lore.kernel.org/netdev/20260813200653.980170-1-github@szelinsky.de/
[3] "net: pse-pd: assorted lifetime and error-path fixes", for net

Carlo Szelinsky (4):
  net: pse-pd: unlink the controller before freeing its PI data
  net: pse-pd: unwind allocations when controller registration fails
  net: pse-pd: si3474: use dev_err_probe() for controller registration
  net: pse-pd: check the PI vpwr supply before registering the
    controller

 drivers/net/pse-pd/pse_core.c | 175 ++++++++++++++++++++++++++++++----
 drivers/net/pse-pd/si3474.c   |   7 +-
 2 files changed, 158 insertions(+), 24 deletions(-)


base-commit: 98c2ad4f5d805dbc1ea436a334ee59311806a48c
-- 
2.43.0

[PATCH net-next 1/4] net: pse-pd: unlink the controller before freeing its PI data

[Carlo Szelinsky]

pse_controller_unregister() frees the PI array before it takes the
controller off pse_controller_list. In that window of_pse_control_get()
can still find the controller and read the freed array, and so can
pse_isr() and the notification worker.

Disable the IRQ, take the controller off the list and stop the worker
first. Only then free the PI data.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index a5e6d7b26b9f..364acc025097 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -1114,15 +1114,19 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
-	pse_flush_pw_ds(pcdev);
-	pse_release_pis(pcdev);
+	/* Stop the IRQ, new lookups and the worker before freeing pcdev->pi. */
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
-	cancel_work_sync(&pcdev->ntf_work);
-	kfifo_free(&pcdev->ntf_fifo);
+
 	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);
 }
 EXPORT_SYMBOL_GPL(pse_controller_unregister);
 
-- 
2.43.0

[PATCH net-next 2/4] net: pse-pd: unwind allocations when controller registration fails

[Carlo Szelinsky]

When pse_controller_register() fails, it leaks the notification kfifo,
the PI array and the OF node references. Worse, if pse_register_pw_ds()
fails part way, the power domains it created stay in the global map
while devres frees them, so the next controller that registers reads
freed memory. A later patch makes -EPROBE_DEFER a normal result here,
so this will happen more often.

Free what was allocated on each error path. The PI array is still
leaked once the PI regulators are registered, because their ops use it
until devres unregisters them.

pse_register_pw_ds() now undoes its own power domains while it still
holds pse_pw_d_mutex, so no other controller can pick one up just
before it is freed. pi[].pw_d is cleared under pcdev->lock before its
reference is dropped, and pse_ethtool_set_prio() and
pse_pi_update_pw_budget() now check it under that lock.

A power domain shared by two controllers is still devm memory of the
controller that created it; that is fixed separately on net.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---

Notes:
    Merging with the net fixes:
    - "net: pse-pd: do not tie a shared power domain to its creator's
      devres": move its kfree(pw_d) to the end of
      pse_pw_d_release_locked(), and name that function in its
      pse_alloc_pw_d() comment. In pse_flush_pw_ds(), keep this patch's
      pse_pi_take_pw_d() call.
    - "net: pse-pd: check the PI state under the lock when setting a power
      limit": in pse_pi_update_pw_budget(), keep the net fix's check.

 drivers/net/pse-pd/pse_core.c | 82 ++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 16 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 364acc025097..05b02d736931 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -878,7 +878,8 @@ 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,
@@ -886,9 +887,30 @@ static void __pse_pw_d_release(struct kref *kref)
 
 	regulator_put(pw_d->supply);
 	xa_erase(&pse_pw_d_map, pw_d->id);
+}
+
+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 its reference is dropped. The PI regulator
+ * ops 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
@@ -899,10 +921,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;
 
@@ -998,6 +1021,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;
 }
@@ -1058,17 +1088,18 @@ 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;
 
 	if (pcdev->ops->setup_pi_matrix) {
 		ret = pcdev->ops->setup_pi_matrix(pcdev);
 		if (ret)
-			return ret;
+			goto release_pis;
 	}
 
 	/* Each regulator name len is pcdev dev name + 7 char +
@@ -1076,7 +1107,9 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	 */
 	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
 
-	/* Register PI regulators */
+	/* Register PI regulators. Their ops index pcdev->pi[] until devres
+	 * unregisters them, so failures from here on must not free it.
+	 */
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		char *reg_name;
 
@@ -1085,26 +1118,38 @@ 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;
+
+release_pis:
+	pse_release_pis(pcdev);
+	pcdev->pi = NULL;
+
+free_kfifo:
+	cancel_work_sync(&pcdev->ntf_work);
+	kfifo_free(&pcdev->ntf_fifo);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
 
@@ -1820,6 +1865,10 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,
 	 * power budget update
 	 */
 	mutex_lock(&pcdev->lock);
+	/* pi->pw_d may have been cleared since the caller looked. */
+	if (!pi->pw_d)
+		goto out;
+
 	previous_pw_allocated = pi->pw_allocated_mW;
 	pw_diff = pw_req - previous_pw_allocated;
 	if (!pw_diff) {
@@ -1921,15 +1970,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

[PATCH net-next 3/4] net: pse-pd: si3474: use dev_err_probe() for controller registration

[Carlo Szelinsky]

The next patch makes devm_pse_controller_register() return
-EPROBE_DEFER until a PI's vpwr supply is there. Use dev_err_probe(), as
pd692x0 and tps23881 already do, so a deferral is not logged as an error
on every retry.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/si3474.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pse-pd/si3474.c b/drivers/net/pse-pd/si3474.c
index 1845b9c51cf7..91800f39971f 100644
--- a/drivers/net/pse-pd/si3474.c
+++ b/drivers/net/pse-pd/si3474.c
@@ -541,10 +541,9 @@ static int si3474_i2c_probe(struct i2c_client *client)
 	priv->pcdev.nr_lines = SI3474_MAX_CHANS;
 
 	ret = devm_pse_controller_register(dev, &priv->pcdev);
-	if (ret) {
-		dev_err(dev, "Failed to register PSE controller: 0x%x\n", ret);
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register PSE controller\n");
 
 	return 0;
 }
-- 
2.43.0

[PATCH net-next 4/4] net: pse-pd: check the PI vpwr supply before registering the controller

[Carlo Szelinsky]

A regulator registers even when its supply is not there yet. So a PSE
controller registers while its vpwr supply is missing, and every PI
lookup returns -EPROBE_DEFER until the supply appears. Today the MDIO
bus owner keeps deferring until then, but a consumer that only looks a
PI up when a controller registers, as phylib will with the PSE
notifier, would never get it.

Check every PI's vpwr supply before the PI regulators are registered,
and defer the PSE driver's probe if one is missing. The check runs
after setup_pi_matrix(), because pd692x0 registers the supply
regulators there.

The check is simpler than the regulator core's: a supply that is
registered, but whose device is not bound yet, still passes, and that
PI then gets no power domain. On the other hand, a PI whose supply
appears late now gets a power domain; before, pse_register_pw_ds()
skipped it.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 81 +++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 05b02d736931..44986749600e 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>
+#include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/rtnetlink.h>
@@ -826,6 +827,65 @@ static const struct regulator_ops pse_pi_ops = {
 	.set_current_limit = pse_pi_set_current_limit,
 };
 
+/* Look up PI @id's vpwr supply on its own node, as the regulator core
+ * will. Only if the PI node has a vpwr-supply phandle: otherwise the
+ * lookup falls back to of_get_child_regulator() on the controller node,
+ * which can return a sibling PI's supply.
+ *
+ * Return: 0 if this PI needs nothing more, 1 if the controller node has
+ *	   to answer for it, or a negative error such as -EPROBE_DEFER.
+ */
+static int pse_pi_check_supply(struct pse_controller_dev *pcdev, int id)
+{
+	struct device_node *np;
+	struct regulator *supply;
+	int ret;
+
+	/* The PIs the registration loop skips. */
+	if (!pcdev->no_of_pse_pi && !pcdev->pi[id].np)
+		return 0;
+
+	np = pcdev->pi[id].np ?
+	     of_parse_phandle(pcdev->pi[id].np, "vpwr-supply", 0) : NULL;
+	if (!np)
+		return 1;
+
+	of_node_put(np);
+	supply = of_regulator_get_optional(pcdev->dev, pcdev->pi[id].np,
+					   "vpwr");
+	if (!IS_ERR(supply)) {
+		regulator_put(supply);
+		return 0;
+	}
+
+	ret = PTR_ERR(supply);
+	if (ret == -ENODEV)
+		return 1;
+
+	return dev_err_probe(pcdev->dev, ret,
+			     "PI %d: failed to get vpwr supply\n", id);
+}
+
+/* The core's fallback: a vpwr-supply on the controller node. */
+static int pse_controller_check_supply(struct pse_controller_dev *pcdev)
+{
+	struct regulator *supply;
+	int ret;
+
+	supply = regulator_get_optional(pcdev->dev, "vpwr");
+	if (!IS_ERR(supply)) {
+		regulator_put(supply);
+		return 0;
+	}
+
+	ret = PTR_ERR(supply);
+	if (ret == -ENODEV)
+		return 0;
+
+	return dev_err_probe(pcdev->dev, ret,
+			     "failed to get vpwr supply\n");
+}
+
 static int
 devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,
 			       char *name, int id)
@@ -1068,6 +1128,7 @@ static void pse_send_ntf_worker(struct work_struct *work)
  */
 int pse_controller_register(struct pse_controller_dev *pcdev)
 {
+	bool ctrl_supply_ok = false;
 	size_t reg_name_len;
 	int ret, i;
 
@@ -1102,6 +1163,26 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 			goto release_pis;
 	}
 
+	/* Check vpwr supplies while pcdev->pi can still be freed. Not before
+	 * setup_pi_matrix(): pd692x0 registers the regulators its PIs name
+	 * there.
+	 */
+	for (i = 0; i < pcdev->nr_lines; i++) {
+		ret = pse_pi_check_supply(pcdev, i);
+		if (ret < 0)
+			goto release_pis;
+
+		/* Same answer for every PI: ask once. */
+		if (!ret || ctrl_supply_ok)
+			continue;
+
+		ret = pse_controller_check_supply(pcdev);
+		if (ret)
+			goto release_pis;
+
+		ctrl_supply_ok = true;
+	}
+
 	/* Each regulator name len is pcdev dev name + 7 char +
 	 * int max digit number (10) + 1
 	 */
-- 
2.43.0

Re: [PATCH net-next 0/4] net: pse-pd: core fixes ahead of the controller notifier

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.