Message-Id: <20251227-regulators-defer-v1-8-3104b22d84cb@linaro.org>
Date: Sat, 27 Dec 2025 12:17:52 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
 Javier Martinez Canillas <javier@....samsung.com>, 
 Jon Hunter <jonathanh@...dia.com>, Dmitry Baryshkov <lumag@...nel.org>, 
 Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Peter Griffin <peter.griffin@...aro.org>, 
 Tudor Ambarus <tudor.ambarus@...aro.org>, 
 Will McVicker <willmcvicker@...gle.com>, Juan Yescas <jyescas@...gle.com>, 
 kernel-team@...roid.com, linux-kernel@...r.kernel.org, 
 André Draszik <andre.draszik@...aro.org>
Subject: [PATCH 8/8] regulator: core: don't fail regulator_register() with
 missing required supply

Since commit 98e48cd9283d ("regulator: core: resolve supply for
boot-on/always-on regulators"), the regulator core returns
-EPROBE_DEFER if a supply can not be resolved at regulator_register()
time due to set_machine_constraints() requiring that supply (e.g.
because of always-on or boot-on).

In some hardware designs, multiple PMICs are used where individual
rails of each act as supplies for rails of the other, and vice-versa.
In such a design no PMIC driver can probe when registering one top-
level regulator device (as is common practice for almost all regulator
drivers in Linux) since that commit. Supplies are only considered when
their driver has fully bound, but because in a design like the above
two drivers / devices depend on each other, neither will have fully
bound while the other probes. The Google Pixel 6 and 6 Pro (oriole and
raven) are examples of such a design.

One way to make this work would be to register each rail as an
individual device, rather than just one top-level regulator device.
Then, fw-devlink and Linux' driver core could do their usual handling
of deferred device probe as each rail would be probed individually.
This approach was dismissed in [1] as each regulator driver would have
to take care of this itself.

Alternatively, we can change the regulator core to not fail
regulator_register() if a rail's required supply can not be resolved
while keeping the intended change from above mentioned commit, and
instead retry whenever a new rail is registered. This commit implements
such an approach:

    If set_machine_constraints() requests probe deferral,
    regulator_register() still succeeds and we retry setting
    constraints as part of regulator_resolve_supply().
    We still do not enable the regulator or allow consumers to use it
    until constraints have been set (including resolution of the
    supply) to prevent enabling of a regulator before its supply.

With this change, we keep track of regulators with missing required
supplies and can therefore try to resolve them again and try to set
the constraints again once more regulators become available.

Care has to be taken to not allow consumers to use regulators that
haven't had their constraints set yet. regulator_get() ensures that
and now returns -EPROBE_DEFER in that case.

The implementation is straight-forward, thanks to our newly introduced
regulator-bus. Locking in regulator_resolve_supply() has to be done
carefully, as a combination of regulator_(un)lock_two() and
regulator_(un)lock_dependent() is needed. The reason is that
set_machine_constraints() might call regulator_enable() which needs
rdev and all its dependents locked, but everything else requires to
only have rdev and its supply locked.

Link: https://lore.kernel.org/all/aRn_-o-vie_QoDXD@sirena.co.uk/ [1]
Signed-off-by: André Draszik <andre.draszik@...aro.org>
---
 drivers/regulator/core.c         | 148 +++++++++++++++++++++++++++++++--------
 include/linux/regulator/driver.h |   1 +
 2 files changed, 119 insertions(+), 30 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 08e92b1ba2dc2ff9efdabaa16187a4a38cf66fb2..8c2fd20edd50591c962454a358459e52e97c8ac0 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -98,6 +98,7 @@ struct regulator_event_work {
 	unsigned long event;
 };
 
+static int _regulator_enable(struct regulator *regulator);
 static int _regulator_is_enabled(struct regulator_dev *rdev);
 static int _regulator_disable(struct regulator *regulator);
 static int _regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags);
@@ -1432,6 +1433,7 @@ static int handle_notify_limits(struct regulator_dev *rdev,
 /**
  * set_machine_constraints - sets regulator constraints
  * @rdev: regulator source
+ * @is_locked: whether or not this is called with locks held already
  *
  * Allows platform initialisation code to define and constrain
  * regulator circuits e.g. valid voltage/current ranges, etc.  NOTE:
@@ -1441,7 +1443,8 @@ static int handle_notify_limits(struct regulator_dev *rdev,
  *
  * Return: 0 on success or a negative error number on failure.
  */
-static int set_machine_constraints(struct regulator_dev *rdev)
+static int set_machine_constraints(struct regulator_dev *rdev,
+				   bool is_locked)
 {
 	int ret = 0;
 	const struct regulator_ops *ops = rdev->desc->ops;
@@ -1653,7 +1656,9 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 		if (rdev->supply &&
 		    (rdev->constraints->always_on ||
 		     !regulator_is_enabled(rdev->supply))) {
-			ret = regulator_enable(rdev->supply);
+			ret = (is_locked
+			       ? _regulator_enable(rdev->supply)
+			       : regulator_enable(rdev->supply));
 			if (ret < 0) {
 				_regulator_put(rdev->supply);
 				rdev->supply = NULL;
@@ -1781,6 +1786,15 @@ static int register_regulator_event_forwarding(struct regulator_dev *rdev)
 	return 0;
 }
 
+static void unregister_regulator_event_forwarding(struct regulator_dev *rdev)
+{
+	if (!rdev->supply_fwd_nb.notifier_call)
+		return;
+
+	regulator_unregister_notifier(rdev->supply, &rdev->supply_fwd_nb);
+	rdev->supply_fwd_nb.notifier_call = NULL;
+}
+
 /**
  * set_supply - set regulator supply regulator
  * @rdev: regulator (locked)
@@ -2169,6 +2183,8 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	struct regulator_dev *r;
 	struct device *dev = rdev->dev.parent;
 	struct ww_acquire_ctx ww_ctx;
+	struct regulator *supply;
+	bool do_final_setup;
 	int ret = 0;
 
 	/* No supply to resolve? */
@@ -2176,7 +2192,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return 0;
 
 	/* Supply already resolved? (fast-path without locking contention) */
-	if (rdev->supply)
+	if (rdev->supply && !rdev->constraints_pending)
 		return 0;
 
 	/* first do a dt based lookup on the node described in the virtual
@@ -2257,46 +2273,115 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 
 	/* Supply just resolved by a concurrent task? */
 	if (rdev->supply) {
+		/* Constraints might still be pending due to concurrency. */
+		bool done = !rdev->constraints_pending;
+
+		supply = rdev->supply;
+
 		regulator_unlock_two(rdev, r, &ww_ctx);
 		put_device(&r->dev);
-		goto out;
-	}
 
-	ret = set_supply(rdev, r);
-	if (ret < 0) {
+		/*
+		 * Supply resolved by concurrent task, and constraints set as
+		 * well (or not required): fast path.
+		 */
+		if (done)
+			goto out;
+
+		do_final_setup = false;
+	} else {
+		ret = set_supply(rdev, r);
+		if (ret < 0) {
+			regulator_unlock_two(rdev, r, &ww_ctx);
+			put_device(&r->dev);
+			goto out;
+		}
+
+		supply = rdev->supply;
+
+		/*
+		 * Automatically register for event forwarding from the new
+		 * supply. This creates the downstream propagation link for
+		 * events like under-voltage.
+		 */
+		ret = register_regulator_event_forwarding(rdev);
+		if (ret < 0) {
+			rdev_warn(rdev,
+				  "Failed to register event forwarding: %pe\n",
+				  ERR_PTR(ret));
+
+			goto unset_supply;
+		}
+
 		regulator_unlock_two(rdev, r, &ww_ctx);
-		put_device(&r->dev);
-		goto out;
+
+		do_final_setup = true;
 	}
 
 	/*
-	 * Automatically register for event forwarding from the new supply.
-	 * This creates the downstream propagation link for events like
-	 * under-voltage.
+	 * Now that we have the supply, we can retry setting the machine
+	 * constraints, if necessary.
 	 */
-	ret = register_regulator_event_forwarding(rdev);
-	if (ret < 0) {
-		struct regulator *supply;
-
-		rdev_warn(rdev, "Failed to register event forwarding: %pe\n",
-			  ERR_PTR(ret));
-
-		supply = rdev->supply;
-		rdev->supply = NULL;
+	regulator_lock_dependent(rdev, &ww_ctx);
+	if (rdev->constraints_pending) {
+		if (!rdev->supply) {
+			/*
+			 * Supply could have been released by another task that
+			 * failed to set the constraints or event forwarding.
+			 */
+			regulator_unlock_dependent(rdev, &ww_ctx);
+			ret = -EPROBE_DEFER;
+			goto out;
+		}
 
-		regulator_unlock_two(rdev, supply->rdev, &ww_ctx);
+		ret = set_machine_constraints(rdev, true);
+		if (ret < 0) {
+			regulator_unlock_dependent(rdev, &ww_ctx);
+
+			rdev_warn(rdev,
+				  "Failed to set machine constraints: %pe\n",
+				  ERR_PTR(ret));
+
+			regulator_lock_two(rdev, r, &ww_ctx);
+
+			if (supply != rdev->supply) {
+				/*
+				 * Supply could have been released by another
+				 * task that got here before us. If it did, it
+				 * will have released 'supply' (i.e. the
+				 * previous rdev->supply) and we shouldn't do
+				 * that again via unset_supply.
+				 */
+				regulator_unlock_two(rdev, r, &ww_ctx);
+				goto out;
+			}
 
-		regulator_put(supply);
-		goto out;
+			unregister_regulator_event_forwarding(rdev);
+			rdev->constraints_pending = true;
+			goto unset_supply;
+		}
+		rdev->constraints_pending = false;
 	}
+	regulator_unlock_dependent(rdev, &ww_ctx);
 
-	regulator_unlock_two(rdev, r, &ww_ctx);
+	if (!do_final_setup)
+		goto out;
 
 	/* rdev->supply was created in set_supply() */
-	link_and_create_debugfs(rdev->supply, r, &rdev->dev);
+	link_and_create_debugfs(rdev->supply, rdev->supply->rdev, &rdev->dev);
 
 out:
 	return ret;
+
+unset_supply:
+	lockdep_assert_held_once(&rdev->mutex.base);
+	lockdep_assert_held_once(&r->mutex.base);
+	rdev->supply = NULL;
+	regulator_unlock_two(rdev, supply->rdev, &ww_ctx);
+
+	regulator_put(supply);
+
+	return ret;
 }
 
 /* common pre-checks for regulator requests */
@@ -6067,7 +6152,7 @@ regulator_register(struct device *dev,
 		dangling_of_gpiod = false;
 	}
 
-	ret = set_machine_constraints(rdev);
+	ret = set_machine_constraints(rdev, false);
 	if (ret == -EPROBE_DEFER) {
 		/* Regulator might be in bypass mode or an always-on or boot-on
 		 * regulator and so needs its supply to set the constraints or
@@ -6081,14 +6166,17 @@ regulator_register(struct device *dev,
 			 rdev->supply_name);
 		ret = regulator_resolve_supply(rdev);
 		if (!ret)
-			ret = set_machine_constraints(rdev);
+			ret = set_machine_constraints(rdev, false);
 		else
 			rdev_dbg(rdev, "unable to resolve supply early: %pe\n",
 				 ERR_PTR(ret));
 		tried_supply_resolve = true;
 	}
-	if (ret < 0)
-		goto wash;
+	if (ret < 0) {
+		if (ret != -EPROBE_DEFER)
+			goto wash;
+		rdev->constraints_pending = true;
+	}
 
 	ret = regulator_init_coupling(rdev);
 	if (ret < 0)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d38353f2b56f8bbab865d903ad0ec97ca0b5c834..09f3b67638f9e63a32cfdbaf9c8654afbd02a547 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -650,6 +650,7 @@ struct regulator_dev {
 	struct regulator_enable_gpio *ena_pin;
 	unsigned int ena_gpio_state:1;
 
+	unsigned int constraints_pending:1;
 	unsigned int is_switch:1;
 
 	/* time when this regulator was disabled last time */

-- 
2.52.0.351.gbe84eed79e-goog

Powered by blists - more mailing lists