[PATCH v3 0/4] regulator_sync_state() support

[PATCH v3 0/4] regulator_sync_state() support

[Saravana Kannan]

Consider the following example:
- regulator-X is provided by device-X.
- regulator-X is a supplier to device-A, device-B and device-C.
- device-A is off/inactive from boot.
- device-B and device-C are left on/active by the bootloader
- regulator-X is left on boot by the bootloader at 2000 mV to supply
  device-B and device-C.

Example boot sequence 1:
1. device-X is probed successfully.
2. device-A is probed by driver-A
   a. driver-A gets regulator-X
   b. driver-A votes on regulator-X
   c. driver-A initializes device-A
   d. driver-A votes off regulator-X
   e. regulator-X is turned off.
3. System crashes or device-B and device-C become unreliable because
   regulator-X was turned off without following the proper quiescing
   steps for device-B and device-C.

Example boot sequence 2:
1. device-X is probed successfully.
2. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B lowers device-B performance point.
   d. driver-B lowers voltage vote to 1000 mV.
   e. regulator-X voltage is lowered to 1000 mV.
3. System crashes or device-C becomes unreliable because regulator-X
   voltage was lowered to 1000 mV when device-C still needed it at 2000 mV

This patch series makes sure these examples are handled correctly and
system crash or device instability is avoided and the system remains
usable.

More details provided in the commit texts.

v2->v3:
Patch 2/4 - No functional change. Simple refactor.
Patch 3/4
- Was Patch 2/2 in v2.
- Rewrote commit text to hopefully address all previous points.
- Renamed variable/functions. Hope it's clearer.
- Added more comments.
- Added logging
- Fixed timeout functionality.
- Handle exclusive consumers properly
- Handle coupled regulators properly
Patch 4/4 - Prevents voltage from going too low during boot.

v1->v2:
Patch 1/2
- New patch
Patch 2/2
- This was the only patch in v1
- Made the late_initcall_sync timeout a commandline param
- If timeout is set, we also give up waiting for all consumers after
  the timeout expires.
- Made every regulator driver add sync_state() support

Saravana Kannan (4):
  driver core: Add dev_set_drv_sync_state()
  regulator: core: Add destroy_regulator()
  regulator: core: Add basic enable/disable support for sync_state()
    callbacks
  regulator: core: Add voltage support for sync_state() callbacks

 drivers/regulator/core.c         | 200 ++++++++++++++++++++++++++++---
 include/linux/device.h           |  12 ++
 include/linux/regulator/driver.h |   2 +
 3 files changed, 198 insertions(+), 16 deletions(-)

-- 
2.28.0.rc0.105.gf9edc3c819-goog

[PATCH v3 1/4] driver core: Add dev_set_drv_sync_state()

[Saravana Kannan]

This can be used by frameworks to set the sync_state() helper functions
for drivers that don't already have them set.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 include/linux/device.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..2f56afdd9107 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -806,6 +806,18 @@ static inline bool dev_has_sync_state(struct device *dev)
 	return false;
 }
 
+static inline int dev_set_drv_sync_state(struct device *dev,
+					 void (*fn)(struct device *dev))
+{
+	if (!dev || !dev->driver)
+		return 0;
+	if (dev->driver->sync_state && dev->driver->sync_state != fn)
+		return -EBUSY;
+	if (!dev->driver->sync_state)
+		dev->driver->sync_state = fn;
+	return 0;
+}
+
 /*
  * High level routines for use by the bus drivers
  */
-- 
2.28.0.rc0.105.gf9edc3c819-goog

[PATCH v3 2/4] regulator: core: Add destroy_regulator()

[Saravana Kannan]

Part of the regulator_get() code is already factored out into
create_regulator(). This patch factors out some of the regulator_put()
code into destroy_regulator() so that create_regulator() has a
corresponding unwind function. Subsequent patches will use this
function.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 03154f5b939f..c9615d3530c7 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -105,6 +105,7 @@ static int regulator_balance_voltage(struct regulator_dev *rdev,
 static struct regulator *create_regulator(struct regulator_dev *rdev,
 					  struct device *dev,
 					  const char *supply_name);
+static void destroy_regulator(struct regulator *regulator);
 static void _regulator_put(struct regulator *regulator);
 
 const char *rdev_get_name(struct regulator_dev *rdev)
@@ -2034,20 +2035,9 @@ struct regulator *regulator_get_optional(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL_GPL(regulator_get_optional);
 
-/* regulator_list_mutex lock held by regulator_put() */
-static void _regulator_put(struct regulator *regulator)
+static void destroy_regulator(struct regulator *regulator)
 {
-	struct regulator_dev *rdev;
-
-	if (IS_ERR_OR_NULL(regulator))
-		return;
-
-	lockdep_assert_held_once(&regulator_list_mutex);
-
-	/* Docs say you must disable before calling regulator_put() */
-	WARN_ON(regulator->enable_count);
-
-	rdev = regulator->rdev;
+	struct regulator_dev *rdev = regulator->rdev;
 
 	debugfs_remove_recursive(regulator->debugfs);
 
@@ -2068,6 +2058,24 @@ static void _regulator_put(struct regulator *regulator)
 
 	kfree_const(regulator->supply_name);
 	kfree(regulator);
+}
+
+/* regulator_list_mutex lock held by regulator_put() */
+static void _regulator_put(struct regulator *regulator)
+{
+	struct regulator_dev *rdev;
+
+	if (IS_ERR_OR_NULL(regulator))
+		return;
+
+	lockdep_assert_held_once(&regulator_list_mutex);
+
+	/* Docs say you must disable before calling regulator_put() */
+	WARN_ON(regulator->enable_count);
+
+	rdev = regulator->rdev;
+
+	destroy_regulator(regulator);
 
 	module_put(rdev->owner);
 	put_device(&rdev->dev);
-- 
2.28.0.rc0.105.gf9edc3c819-goog

[PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Saravana Kannan]

Consider the following example:
- regulator-X is provided by device-X.
- regulator-X is a supplier to device-A, device-B and device-C.
- device-A is off/inactive from boot.
- device-B and device-C are left on/active by the bootloader
- regulator-X is left on boot by the bootloader at 2000 mV to supply
  device-B and device-C.

Example boot sequence:
1. device-X is probed successfully.
2. device-A is probed by driver-A
   a. driver-A gets regulator-X
   b. driver-A votes on regulator-X
   c. driver-A initializes device-A
   d. driver-A votes off regulator-X
   e. regulator-X is turned off.
3. System crashes or device-B and device-C become unreliable because
   regulator-X was turned off without following the proper quiescing
   steps for device-B and device-C.

There are Android devices that exhibit the issue in the example where
regulator-X is an LDO, device-A is a camera device and device-B and
device-C are UFS and USB. To avoid this, they have their own downstream
changes to the regulator framework.

This patch addresses the problem in the example by:

1. When a regulator is registered,
   a. The sync_state() callback for the regulator's device is set to
      regulator_sync_state(). The sync_state() callback is called when
      all the consumers of the regulator's device have probed
      successfully.
   b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
      for the regulator and an enable vote is made.

2. When the regulator_sync_state() callback comes, all the boot-on
   regulators registered by that device will have the BOOT-LIMITS enable
   vote removed and the BOOT-LIMITS consumer freed.

If an exclusive get is ever attempted on a boot-on regulator with an
active BOOT-LIMITS vote, the regulator is handed off to the new consumer
(and the BOOT-LIMITS freed) without affecting the regulator state. This
ensures, consumers doing exclusive gets continue to work after this
commit.

To maintain backward compatibility with systems where some consumers of
the device might never probe, a new regulator_cleanup_timeout kernel
commandline argument is added and defaulted to 30 seconds. When the
timeout is a non-zero value and it expires, all BOOT-LIMITS consumer
votes are removed even if the sync_state() callbacks haven't been
called.

In systems where all the consumers are expected to probe, the
regulator_cleanup_timeout can be set to 0. When that's done, the
BOOT-LIMITS consumer votes for a regulator are removed only when all the
consumers of the regulator's device have probed.

So with this patch and regulator_cleanup_timeout=0, the example will work
as follows:

1. device-X is probed successfully.
   a. regulator-X is registered.
   b. Since regulator-X is on, an enable vote is made by the BOOT-LIMITS
      consumer.
2. device-A is probed by driver-A
   a. driver-A gets regulator-X
   b. driver-A votes on regulator-X
   c. driver-A initializes device-A
   d. driver-A votes off regulator-X
   e. regulator-X is NOT turned off due to BOOT-LIMITS consumer.
3. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B initializes device-B.
   d. driver-B votes off regulator-X because device-B enters runtime
      idle.
   e. regulator-X is NOT turned off due to BOOT-LIMITS consumer.
4. device-C is probed by driver-C
   a. Does stuff similar to device-B and votes off regulator-X.
5. Since all consumers of device-X have probed, device-X gets
   sync_state() callback which is a call to regulator_sync_state():
   a. BOOT-LIMITS removes enable vote for regulator-X
   b. regulator-X is turned off.
   c. BOOT-LIMITS consumer is freed.
6. The system is stable because regulator-X is only turned off after
   device-B and device-C get to initialize and quiesce properly.

OR

Same steps for 1 - 3 as above.
4. device-C is never probed because driver-C isn't available.
5. Since all consumers of device-X have NOT probed, device-X doesn't get
   sync_state() callback.
   a. BOOT-LIMITS votes continue to be enforced for regulator-X
6. device-C continues to work in the mode the boot loader left it in and
   the system remains usable. For example, device C is
   - A display backlight that doesn't have a driver
   - An interconnect that doesn't have an interconnect provider driver

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c         | 155 ++++++++++++++++++++++++++++++-
 include/linux/regulator/driver.h |   2 +
 2 files changed, 154 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c9615d3530c7..f10ef6ec1af1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1769,6 +1769,115 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+/**
+ * regulator_add_boot_limits - Set up boot limits for a regulator device.
+ * @rdev: regulator device to set up boot limits for.
+ *
+ * Makes requests on the regulator device @rdev so that its operating state
+ * doesn't drop below the state in which the bootloader left it at before
+ * starting the kernel.
+ *
+ * If boot limits has already been set up, this function doesn't do anything.
+ *
+ * The boot limits can be dropped later by calling regulator_del_boot_limits().
+ */
+static void regulator_add_boot_limits(struct regulator_dev *rdev)
+{
+	/* We already set up boot limits. */
+	if (rdev->boot_limits)
+		return;
+
+	/*
+	 * If the regulator is disabled or has an error getting the status,
+	 * assume it's not on. We won't explicitly turn it off now, that can
+	 * happen later.
+	 */
+	if (_regulator_is_enabled(rdev) <= 0)
+		return;
+
+	/* Wait for supply to get resolved */
+	if (rdev->supply_name && !rdev->supply)
+		return;
+
+	/* Wait for coupled regulators to be resolved */
+	if (rdev->coupling_desc.n_resolved != rdev->coupling_desc.n_coupled)
+		return;
+
+	/*
+	 * Get a reference to a regulator to set up boot limits to prevent it
+	 * from going below the state in which the boot loader left it in.
+	 */
+	rdev->boot_limits = create_regulator(rdev, NULL, "BOOT-LIMITS");
+	if (rdev->boot_limits == NULL) {
+		dev_err(&rdev->dev, "Unable to get boot limits handle\n");
+		rdev->boot_limits = ERR_PTR(-EPERM);
+		return;
+	}
+	rdev->open_count++;
+
+	if (regulator_enable(rdev->boot_limits)) {
+		dev_err(&rdev->dev, "Unable to set boot limits\n");
+		destroy_regulator(rdev->boot_limits);
+		rdev->boot_limits = ERR_PTR(-EPERM);
+	}
+}
+
+/**
+ * regulator_del_boot_limits - Remove boot limits for a regulator device.
+ * @rdev: regulator device to remove boot limits for.
+ * @handoff: Flag to control if the regulator should be handed off without a
+ *	     state change to the hardware.
+ *
+ * If boot limits has never been set up for this regulator, this function
+ * doesn't do anything and returns immediately.
+ *
+ * If boot limits have already been set up for the regulator device @rdev, this
+ * functions removes the boot limits so that operating state of @rdev can drop below
+ * the state in which the bootloader left it at before starting the kernel.
+ *
+ * In addition, if @handoff is NOT set, the function immediately updates the
+ * state of the regulator (potentially disabling it too).  Otherwise, this
+ * function removes the boot limits and leaves the regulator state update to
+ * subsequent requests by other consumers. This is mainly useful for removing
+ * the boot limits when an exclusive consumer takes control of the regulator.
+ */
+static int regulator_del_boot_limits(struct regulator_dev *rdev, bool handoff)
+{
+	if (IS_ERR_OR_NULL(rdev->boot_limits))
+		return 0;
+
+	rdev_info(rdev, "removing boot limits\n");
+	if (!handoff)
+		regulator_disable(rdev->boot_limits);
+	else
+		rdev->use_count--;
+	destroy_regulator(rdev->boot_limits);
+	/*
+	 * Set it to an error value so that boot limits can't be set again once
+	 * it has been removed.
+	 */
+	rdev->boot_limits = ERR_PTR(-EINVAL);
+	return 0;
+}
+
+static int regulator_del_boot_limits_by_dev(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	if (dev->parent != data)
+		return 0;
+
+	regulator_del_boot_limits(rdev, false);
+	return 0;
+}
+
+void regulator_sync_state(struct device *dev)
+{
+	class_for_each_device(&regulator_class, NULL, dev,
+			      regulator_del_boot_limits_by_dev);
+}
+EXPORT_SYMBOL_GPL(regulator_sync_state);
+
 static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator_dev *r;
@@ -1827,6 +1936,10 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return ret;
 	}
 
+	mutex_lock(&regulator_list_mutex);
+	regulator_add_boot_limits(rdev);
+	mutex_unlock(&regulator_list_mutex);
+
 	/*
 	 * In set_machine_constraints() we may have turned this regulator on
 	 * but we couldn't propagate to the supply if it hadn't been resolved
@@ -1851,7 +1964,7 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 	struct regulator_dev *rdev;
 	struct regulator *regulator;
 	struct device_link *link;
-	int ret;
+	int ret, open_count;
 
 	if (get_type >= MAX_GET_TYPE) {
 		dev_err(dev, "invalid type %d in %s\n", get_type, __func__);
@@ -1908,7 +2021,15 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 		return regulator;
 	}
 
-	if (get_type == EXCLUSIVE_GET && rdev->open_count) {
+	/*
+	 * Adjust for boot limits that'll be dropped if EXCLUSIVE_GET can
+	 * succeed.
+	 */
+	open_count = rdev->open_count;
+	if (!IS_ERR_OR_NULL(rdev->boot_limits))
+		open_count--;
+
+	if (get_type == EXCLUSIVE_GET && open_count) {
 		regulator = ERR_PTR(-EBUSY);
 		put_device(&rdev->dev);
 		return regulator;
@@ -1947,6 +2068,9 @@ struct regulator *_regulator_get(struct device *dev, const char *id,
 
 	rdev->open_count++;
 	if (get_type == EXCLUSIVE_GET) {
+		/* Drop boot limits before exclusive consumer */
+		regulator_del_boot_limits(rdev, true);
+
 		rdev->exclusive = 1;
 
 		ret = _regulator_is_enabled(rdev);
@@ -4896,7 +5020,9 @@ static void regulator_resolve_coupling(struct regulator_dev *rdev)
 		regulator_unlock(c_rdev);
 
 		regulator_resolve_coupling(c_rdev);
+		regulator_add_boot_limits(c_rdev);
 	}
+	regulator_add_boot_limits(rdev);
 }
 
 static void regulator_remove_coupling(struct regulator_dev *rdev)
@@ -5223,6 +5349,8 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	rdev_init_debugfs(rdev);
 
+	dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state);
+
 	/* try to resolve regulators coupling since a new one was registered */
 	mutex_lock(&regulator_list_mutex);
 	regulator_resolve_coupling(rdev);
@@ -5755,6 +5883,17 @@ static int regulator_late_cleanup(struct device *dev, void *data)
 	return 0;
 }
 
+static int regulator_boot_limits_timeout(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	regulator_del_boot_limits(rdev, false);
+	return 0;
+}
+
+static unsigned int regulator_cleanup_timeout = 30000;
+core_param(regulator_cleanup_timeout, regulator_cleanup_timeout, uint, 0);
+
 static void regulator_init_complete_work_function(struct work_struct *work)
 {
 	/*
@@ -5767,6 +5906,16 @@ static void regulator_init_complete_work_function(struct work_struct *work)
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_register_resolve_supply);
 
+	/*
+	 * If regulator_cleanup_timeout is set to a non-zero value, it probably
+	 * means some of the consumers will never probe or the regulators have
+	 * some restrictions on how long they can stay ON. So, don't wait
+	 * forever for consumer devices to probe.
+	 */
+	if (regulator_cleanup_timeout)
+		class_for_each_device(&regulator_class, NULL, NULL,
+				      regulator_boot_limits_timeout);
+
 	/* If we have a full configuration then disable any regulators
 	 * we have permission to change the status for and which are
 	 * not in use or always_on.  This is effectively the default
@@ -5802,7 +5951,7 @@ static int __init regulator_init_complete(void)
 	 * command line option might be useful.
 	 */
 	schedule_delayed_work(&regulator_init_complete_work,
-			      msecs_to_jiffies(30000));
+			      msecs_to_jiffies(regulator_cleanup_timeout));
 
 	return 0;
 }
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 7eb9fea8e482..4761157a1528 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -451,6 +451,7 @@ struct regulator_dev {
 	struct regulator *supply;	/* for tree */
 	const char *supply_name;
 	struct regmap *regmap;
+	struct regulator *boot_limits;
 
 	struct delayed_work disable_work;
 
@@ -475,6 +476,7 @@ devm_regulator_register(struct device *dev,
 			const struct regulator_desc *regulator_desc,
 			const struct regulator_config *config);
 void regulator_unregister(struct regulator_dev *rdev);
+void regulator_sync_state(struct device *dev);
 void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev);
 
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
-- 
2.28.0.rc0.105.gf9edc3c819-goog

[PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks

[Saravana Kannan]

Consider the following example:
- regulator-X is provided by device-X.
- regulator-X is a supplier to device-A, device-B and device-C.
- device-A is off/inactive from boot.
- device-B and device-C are left on/active by the bootloader
- regulator-X is left on boot by the bootloader at 2000 mV to supply
  device-B and device-C.

Example boot sequence:
1. device-X is probed successfully.
2. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B lowers device-B performance point.
   d. driver-B lowers voltage vote to 1000 mV.
   e. regulator-X voltage is lowered to 1000 mV.
3. System crashes or device-C becomes unreliable because regulator-X
   voltage was lowered to 1000 mV when device-C still needed it at 2000 mV

The issue reported by Marek Szyprowski [1] between vdd_int and vdd_arm
is very similar to example 2, except driver-B lowers the voltage of
device-C due to a regulator coupling instead of a direct request from
driver-B.

This patch addresses the problem in the example by:
1. When a boot-on regulator is registered, a minimum voltage limit that
   matches the boot time voltage is placed on the regulator.

2. When the regulator_sync_state() callback comes, the minimum voltage
   limit is removed along with the rest of the boot limits.

So with this patch and regulator_cleanup_timeout=0, the example will
work as follows:

1. device-X is probed successfully.
   a. regulator-X is registered.
   b. Since regulator-X is on, a minimum voltage of 2000 mV is made by
      the BOOT-LIMITS consumer.
   c. Since regulator-X is on, an enable vote is made by the BOOT-LIMITS
      consumer.
2. device-B is probed by driver-B
   a. driver-B gets regulator-X
   b. driver-B votes on regulator-X
   c. driver-B lowers device-B performance point.
   d. driver-B lowers voltage vote to 1000 mV.
   e. regulator-X voltage is NOT lowered to 1000 mV.
3. device-C is probed by driver-C
   a. Does stuff similar to device-B.
4. Since all consumers of device-X have probed, device-X gets
   sync_state() callback which is a call to regulator_sync_state():
   a. BOOT-LIMITS removes enable vote for regulator-X
   b. regulator-X remains on becaue device-B and device-C have their
      enable votes in.
   c. BOOT-LIMITS consumer is freed.
   d. regulator-X voltage drops is lowered to 1000 mV.
5. The system is stable because regulator-X voltage is NOT lowered to
   1000 mV when device-C still needed it at 2000 mV.

[1] - https://lore.kernel.org/lkml/20200605063724.9030-1-m.szyprowski@samsung.com/#t
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f10ef6ec1af1..9b70295820f3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1783,6 +1783,8 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
  */
 static void regulator_add_boot_limits(struct regulator_dev *rdev)
 {
+	int boot_uV;
+
 	/* We already set up boot limits. */
 	if (rdev->boot_limits)
 		return;
@@ -1815,6 +1817,13 @@ static void regulator_add_boot_limits(struct regulator_dev *rdev)
 	}
 	rdev->open_count++;
 
+	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_VOLTAGE)) {
+		boot_uV = regulator_get_voltage_rdev(rdev);
+		if (boot_uV > 0)
+			regulator_set_voltage(rdev->boot_limits, boot_uV,
+								 INT_MAX);
+	}
+
 	if (regulator_enable(rdev->boot_limits)) {
 		dev_err(&rdev->dev, "Unable to set boot limits\n");
 		destroy_regulator(rdev->boot_limits);
@@ -1847,10 +1856,12 @@ static int regulator_del_boot_limits(struct regulator_dev *rdev, bool handoff)
 		return 0;
 
 	rdev_info(rdev, "removing boot limits\n");
-	if (!handoff)
+	if (!handoff) {
 		regulator_disable(rdev->boot_limits);
-	else
+		regulator_set_voltage(rdev->boot_limits, 0, INT_MAX);
+	} else {
 		rdev->use_count--;
+	}
 	destroy_regulator(rdev->boot_limits);
 	/*
 	 * Set it to an error value so that boot limits can't be set again once
-- 
2.28.0.rc0.105.gf9edc3c819-goog

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:

> There are Android devices that exhibit the issue in the example where
> regulator-X is an LDO, device-A is a camera device and device-B and
> device-C are UFS and USB. To avoid this, they have their own downstream
> changes to the regulator framework.

Can you provide any references to these bodges?

> 1. When a regulator is registered,
>    a. The sync_state() callback for the regulator's device is set to
>       regulator_sync_state(). The sync_state() callback is called when
>       all the consumers of the regulator's device have probed
>       successfully.

As I indicated on my previous review this doesn't seem OK, given that a
huge proportion of the regulators on most systems are part of a single
PMIC this means that devices won't be able to fully control regulators
for the majority of the boot process, possibly quite a long time after
userspace has started in systems where not all devices have drivers.
That seems unreasonably restrictive, it seems like this is shifting the
problem around so we have this new very late init phase where we
actually implement changes that devices asked for.  Devices can work
around it by using _get_exclusive() but that makes things more complex
if they can also support non-exclusive use.

I don't understand the motivation for doing things this way.  Like I
said last time it really feels like this turns the whole mechanism into
a very complicated way of implementing a new initcall.

>    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
>       for the regulator and an enable vote is made.

If something was left partially set up by the bootloader this means that
drivers are no longer able to remove power from the device as part of
getting it into a known good state even if they are the only consumer.
Having to tune things in the bootloader isn't great for some development
flows.

> +	/*
> +	 * If regulator_cleanup_timeout is set to a non-zero value, it probably
> +	 * means some of the consumers will never probe or the regulators have
> +	 * some restrictions on how long they can stay ON. So, don't wait
> +	 * forever for consumer devices to probe.
> +	 */
> +	if (regulator_cleanup_timeout)
> +		class_for_each_device(&regulator_class, NULL, NULL,
> +				      regulator_boot_limits_timeout);

It feels like this should be a negative value rather than zero, if the
timeout is 0 someone might reasonably expect things to happen
immediately rather than never.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On Wed, Jul 15, 2020 at 09:20:53PM -0700, Saravana Kannan wrote:

> -	if (!handoff)
> +	if (!handoff) {
>  		regulator_disable(rdev->boot_limits);
> -	else
> +		regulator_set_voltage(rdev->boot_limits, 0, INT_MAX);
> +	} else {
>  		rdev->use_count--;
> +	}
>  	destroy_regulator(rdev->boot_limits);

These sets should be completely redundant since they will go away when
the regulator is destroyed, if there's an issue with that we should fix
it rather than bodging around it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Saravana Kannan]

On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:
>
> > There are Android devices that exhibit the issue in the example where
> > regulator-X is an LDO, device-A is a camera device and device-B and
> > device-C are UFS and USB. To avoid this, they have their own downstream
> > changes to the regulator framework.
>
> Can you provide any references to these bodges?

This is the best I could dig up. The site is slow as molasses. I don't
want to focus or critique any specific vendor's downstream code
though. Providing these links just to prove that this is a real issue.

Search for "proxy" here. You'll notice how that will also need changes
to regulator header files, etc. The 4.9 kernel is the latest publicly
available version AFAIK.
https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9

As for the specific example of those devices, I'm not sure how much of
that stuff is publicly available and I don't want to deal with finding
that out.

> > 1. When a regulator is registered,
> >    a. The sync_state() callback for the regulator's device is set to
> >       regulator_sync_state(). The sync_state() callback is called when
> >       all the consumers of the regulator's device have probed
> >       successfully.
>
> As I indicated on my previous review this doesn't seem OK, given that a
> huge proportion of the regulators on most systems are part of a single
> PMIC this means that devices won't be able to fully control regulators
> for the majority of the boot process, possibly quite a long time after
> userspace has started in systems where not all devices have drivers.

By default regulator_cleanup_timeout is set to 30s. So by default,
it's not going to be "possibly quite a long time". So, at worst, for
the first 30 seconds voltages can't be lowered below boot level and
regulators can't be turned off if they were left on by the boot
loader. If a consumer is not an exclusive consumer, then none of this
should break functional correctness for them and just be 30 seconds of
suboptimal power. And exclusive consumers won't have this issue.

I think the default behavior should be for functional correctness
(example in the commit text) and then go for optimization (being able
to power off regulators before 30s into boot). Even with the timeout
set, this series makes it much easier for driver developers to ensure
functional correctness instead of playing initcall chicken between the
supplier and the N consumers.

Actually on systems without all the drivers, I'd argue the correct
behavior is this patch series + regulator_cleanup_timeout=-1. This
patch series will prevent system instability/unusability (Eg: missing
display backlight driver) at the cost of power optimization. However,
to allow turning off boot on regulators in systems without all the
drivers where it happens to not cause functional correctness issues,
we have the timeout default to 30s.

> That seems unreasonably restrictive, it seems like this is shifting the
> problem around so we have this new very late init phase where we
> actually implement changes that devices asked for. Devices can work
> around it by using _get_exclusive() but that makes things more complex
> if they can also support non-exclusive use.

I agree, hacking around any issues using _get_exclusive() when they
don't really need exclusive access is not a solution.

> I don't understand the motivation for doing things this way.  Like I
> said last time it really feels like this turns the whole mechanism into
> a very complicated way of implementing a new initcall.

Treating this as a "LATER_initcall()" has several issues.
1. initcall levels set a max limit on the depth of device dependency.
Since DT devices are added at arch initcall level, that gives us about
5 levels if you ignore the _sync ones. Adding more isn't going to
scale or solve the problem because in reality, the dependencies are
much deeper.
2. Also, initcall call ordering also depends on Makefile/link
ordering. Which is fragile and error prone.
3. I know Greg hates playing initcall chicken.
4. "initcall levels" don't work for modules. The kernel initcall
levels are done by the time modules are loaded.
5. "initcall levels" don't work with deferred probing.
6. If the answer is, "have userspace tell us when all modules are
loaded" -- then we are depending on userspace for functional
correctness AND for turning off regulators. Which IMHO is worse than
this patch series.
7. If we somehow manage to add a "LATER_initcall" that doesn't have
the issues above, it has to work for all frameworks. So, it has to
come after ALL the devices in the system have probed. Not just "all
devices of a supplier". So, again, it's worse than this patch series,
at least for systems where all the drivers are present.

> >    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
> >       for the regulator and an enable vote is made.
>
> If something was left partially set up by the bootloader this means that
> drivers are no longer able to remove power from the device as part of
> getting it into a known good state even if they are the only consumer.

If they really NEED to turn power off to get to a known good state,
then they really need to be exclusive consumers.

> Having to tune things in the bootloader isn't great for some development
> flows.

I'm not sure I fully understand what you'd need to tune in the
bootloader. But independent of that, by default we still have the 30s
timeout, so there's no boot loader tuning necessary. Just a 30s delay
before being able to "optimize power" during development flow.

I think this patch series actually helps a lot during the development
phase (especially with regulator_cleanup_timeout=-1):
1. It will allow adding/enabling new drivers without worrying about
the system crashing due to the example scenario mentioned in the
commit text.
2. In my development flow, I had to do some stuff manually and then
load some modules. Without this series, when some of the regulator
drivers were built in, the regulators would get turned off after the
30s timeout before I could do my manual stuff. That would kill the
system. Or if I start off the boot and walk away to get coffee, I'd
come back to a dead device. It was super annoying to deal with this.
3. When the regulator drivers are loaded as modules (after 30s
timeout), some of the boot on regulators are never turned off until
one of their consumers starts making requests. For example, if a
regulator that supplies some camera component is left on by the
bootloader, it would never get turned off unless you open the camera
app. With this series, the regulator would get turned off after the
camera driver probes.

To be clear, I understand the cases you are mentioning and I'm not
discounting them. But compared to the 30 seconds of additional "on
time", the functional correctness issues are more important. I'm not
saying this series is the perfect solution, but it's certainly better
than what we have now and we have the default behavior to be as least
disruptive as possible to systems that work fine without this series.
And if I find incremental improvements in the future, I'll send
patches for that. But I'd hate to see perfect be the enemy of the
good.

> > +     /*
> > +      * If regulator_cleanup_timeout is set to a non-zero value, it probably
> > +      * means some of the consumers will never probe or the regulators have
> > +      * some restrictions on how long they can stay ON. So, don't wait
> > +      * forever for consumer devices to probe.
> > +      */
> > +     if (regulator_cleanup_timeout)
> > +             class_for_each_device(&regulator_class, NULL, NULL,
> > +                                   regulator_boot_limits_timeout);
>
> It feels like this should be a negative value rather than zero, if the
> timeout is 0 someone might reasonably expect things to happen
> immediately rather than never.

Makes sense. I'll fix this.

-Saravana

Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks

[Saravana Kannan]

On Mon, Jul 20, 2020 at 7:35 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 15, 2020 at 09:20:53PM -0700, Saravana Kannan wrote:
>
> > -     if (!handoff)
> > +     if (!handoff) {
> >               regulator_disable(rdev->boot_limits);
> > -     else
> > +             regulator_set_voltage(rdev->boot_limits, 0, INT_MAX);
> > +     } else {
> >               rdev->use_count--;
> > +     }
> >       destroy_regulator(rdev->boot_limits);
>
> These sets should be completely redundant since they will go away when
> the regulator is destroyed, if there's an issue with that we should fix
> it rather than bodging around it.

Yeah, I was aware of this, but I thought it was clearer to have an
explicit unwinding. Since you prefer the other way around, I can drop
the set voltage.

Btw, going a tangent, why is regulator_set_voltage() not dependent on
a consumer's regulator enable request? If they don't care if the
regulator goes off, do they really care if the voltage goes lower?
What's the reason behind not tying voltage request with the enable
request?

-Saravana

Re: [PATCH v3 0/4] regulator_sync_state() support

[Saravana Kannan]

On Wed, Jul 15, 2020 at 9:20 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Consider the following example:
> - regulator-X is provided by device-X.
> - regulator-X is a supplier to device-A, device-B and device-C.
> - device-A is off/inactive from boot.
> - device-B and device-C are left on/active by the bootloader
> - regulator-X is left on boot by the bootloader at 2000 mV to supply
>   device-B and device-C.
>
> Example boot sequence 1:
> 1. device-X is probed successfully.
> 2. device-A is probed by driver-A
>    a. driver-A gets regulator-X
>    b. driver-A votes on regulator-X
>    c. driver-A initializes device-A
>    d. driver-A votes off regulator-X
>    e. regulator-X is turned off.
> 3. System crashes or device-B and device-C become unreliable because
>    regulator-X was turned off without following the proper quiescing
>    steps for device-B and device-C.
>
> Example boot sequence 2:
> 1. device-X is probed successfully.
> 2. device-B is probed by driver-B
>    a. driver-B gets regulator-X
>    b. driver-B votes on regulator-X
>    c. driver-B lowers device-B performance point.
>    d. driver-B lowers voltage vote to 1000 mV.
>    e. regulator-X voltage is lowered to 1000 mV.
> 3. System crashes or device-C becomes unreliable because regulator-X
>    voltage was lowered to 1000 mV when device-C still needed it at 2000 mV

Marek,

I'd appreciate it if you could test this series to see if it fixes
your coupled regulator issue that you reported earlier. I'm expecting
it to fix it without needing any special case handling in your
regulator drivers. But it'll be good to get confirmation from you.

Thanks,
Saravana

Re: [PATCH v3 4/4] regulator: core: Add voltage support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

On Mon, Jul 20, 2020 at 08:24:59PM -0700, Saravana Kannan wrote:

> Btw, going a tangent, why is regulator_set_voltage() not dependent on
> a consumer's regulator enable request? If they don't care if the
> regulator goes off, do they really care if the voltage goes lower?
> What's the reason behind not tying voltage request with the enable
> request?

The most important thing is being able to control the conditions at
power on and power off, the period while the power is off is not so
important.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 10145 bytes --]

On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote:
> On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:

> > > There are Android devices that exhibit the issue in the example where
> > > regulator-X is an LDO, device-A is a camera device and device-B and
> > > device-C are UFS and USB. To avoid this, they have their own downstream
> > > changes to the regulator framework.

> > Can you provide any references to these bodges?

> This is the best I could dig up. The site is slow as molasses. I don't
> want to focus or critique any specific vendor's downstream code
> though. Providing these links just to prove that this is a real issue.

The issue is not that I can't see that there might be a problem.  The
issue I have is that you seem to be starting from the point of a
specific solution that happens to work for your systems and then working
back from that rather than reasoning forwards from the problem to come
to a solution with the result that I can't explain the design you're
proposing.  Even with this version I have difficulty telling from the
changelog that the change doesn't restrict things based on the consumers
of a given regulator but rather based on the PMIC and I don't see why
this is being done.

> Search for "proxy" here. You'll notice how that will also need changes
> to regulator header files, etc. The 4.9 kernel is the latest publicly
> available version AFAIK.
> https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9

Oh, the Qualcomm stuff - which looks a lot like what you've got here.

> > As I indicated on my previous review this doesn't seem OK, given that a
> > huge proportion of the regulators on most systems are part of a single
> > PMIC this means that devices won't be able to fully control regulators
> > for the majority of the boot process, possibly quite a long time after
> > userspace has started in systems where not all devices have drivers.

...

> I think the default behavior should be for functional correctness
> (example in the commit text) and then go for optimization (being able
> to power off regulators before 30s into boot). Even with the timeout
> set, this series makes it much easier for driver developers to ensure
> functional correctness instead of playing initcall chicken between the
> supplier and the N consumers.

I don't see how any of this stuff about initcall chicken or whatever is
relevant here.  You're replying to my concern that your change isn't
just waiting for all the consumers of a given regulator, it's waiting
for every other consumer of any other regulator or random other feature
that happens to be supplied by the same PMIC but only that PMIC.  That
is not init ordering, it just seems like an arbitrary delay even once
all the consumers are ready and I can't see any particular logic behind
it.  It's not just waiting for all the users of the individual resource
to be active but it's also not waiting for the system as a whole,
instead it's waiting for some effectively random grouping of devices
spread over the whole system to appear.  I can't articulate a reason why
we'd do that, it seems to be combining the worst aspects of the two
approaches.

Please engage with this issue, it is crucial but you keep on sending the
same thing without explaining why so I'm left guessing and I really
can't come up with anything.

> Actually on systems without all the drivers, I'd argue the correct
> behavior is this patch series + regulator_cleanup_timeout=-1. This
> patch series will prevent system instability/unusability (Eg: missing
> display backlight driver) at the cost of power optimization. However,
> to allow turning off boot on regulators in systems without all the
> drivers where it happens to not cause functional correctness issues,
> we have the timeout default to 30s.

It's also preventing turning off regulators that don't have any
consumers at all, not even potential ones.  Those are the main target
for the cleanup of idle regulators.

> > I don't understand the motivation for doing things this way.  Like I
> > said last time it really feels like this turns the whole mechanism into
> > a very complicated way of implementing a new initcall.

> Treating this as a "LATER_initcall()" has several issues.
> 1. initcall levels set a max limit on the depth of device dependency.
> Since DT devices are added at arch initcall level, that gives us about
> 5 levels if you ignore the _sync ones. Adding more isn't going to
> scale or solve the problem because in reality, the dependencies are
> much deeper.

I don't follow what you're saying here at all, sorry.  What does the
depth of the dependency graph have to do with how late we action
anything?  A lot of what you're saying here seems to be based on jumping
to abandoning deferred probe which is a bit of a leap here.

> 6. If the answer is, "have userspace tell us when all modules are
> loaded" -- then we are depending on userspace for functional
> correctness AND for turning off regulators. Which IMHO is worse than
> this patch series.

> 7. If we somehow manage to add a "LATER_initcall" that doesn't have
> the issues above, it has to work for all frameworks. So, it has to
> come after ALL the devices in the system have probed. Not just "all
> devices of a supplier". So, again, it's worse than this patch series,
> at least for systems where all the drivers are present.

Run the initcall when all devices in the system have a driver bound
which as far as I can tell seems to be what the end result of this
series is intended to be anyway, just implemented with much more
complexity.  That doesn't need any involvement from userspace, though I
can't see why we'd want to do that rather than just work per regulator.

> > >    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
> > >       for the regulator and an enable vote is made.

> > If something was left partially set up by the bootloader this means that
> > drivers are no longer able to remove power from the device as part of
> > getting it into a known good state even if they are the only consumer.

> If they really NEED to turn power off to get to a known good state,
> then they really need to be exclusive consumers.

There's *need* to and there's can do on systems that can support the
thing, the same driver will often be able to handle both situations.
This also goes to a risk of regressions, it's a fairly major
transformation to be doing.  These issues would be massively mitigated
if this were per regulator as I have repeatedly suggested.

> 1. It will allow adding/enabling new drivers without worrying about
> the system crashing due to the example scenario mentioned in the
> commit text.

Or just don't set constraints which alow things to be changed until
you're ready...

> 2. In my development flow, I had to do some stuff manually and then
> load some modules. Without this series, when some of the regulator
> drivers were built in, the regulators would get turned off after the
> 30s timeout before I could do my manual stuff. That would kill the
> system. Or if I start off the boot and walk away to get coffee, I'd
> come back to a dead device. It was super annoying to deal with this.

That's a new one, the normal thing would've been to let the driver load
and then remove it to load whatever test stuff.  I'm not sure that's
super practical to be honest, taking advantage of it really does seem to
require a vertically integrated system where the DT exactly matches what
you want at runtime.

> 3. When the regulator drivers are loaded as modules (after 30s
> timeout), some of the boot on regulators are never turned off until
> one of their consumers starts making requests. For example, if a
> regulator that supplies some camera component is left on by the
> bootloader, it would never get turned off unless you open the camera
> app. With this series, the regulator would get turned off after the
> camera driver probes.

Right, that's an issue but there seem to be less invasive solutions.

> To be clear, I understand the cases you are mentioning and I'm not
> discounting them. But compared to the 30 seconds of additional "on
> time", the functional correctness issues are more important. I'm not
> saying this series is the perfect solution, but it's certainly better
> than what we have now and we have the default behavior to be as least
> disruptive as possible to systems that work fine without this series.
> And if I find incremental improvements in the future, I'll send
> patches for that. But I'd hate to see perfect be the enemy of the
> good.

Having the requests from drivers be acted on promptly is also a
potential correctness and robustness issue.  You seem very focused on
the disabling of unused regulators for power reasons case but that's
really not the only thing going on here, there's also just the need to
control things as part of what the device is supposed to be doing.
Systems aren't always built to let us do all the things we're really
supposed to do with hardware so it's often advantageous to have drivers
that will do these things but cope gracefully, this means that it
doesn't always make sense to use exclusive gets even where you really
actually want things to happen as requested if they can.  Often the
devices are in practice robust enough most of the time things are fine
if the requests get ignored which is why systems can be built that way
but strictly we're not supposed to do that so where we can we should
avoid it.  Forcing consumers to be exclusive means that we need to add a
call that lets these consumers figure out which kind of get to use and
then have the consumer have multiple code paths for that (or more
realistically factor that out into the core, but then we could even more
easily just not have the system wide restrictions).

There's also issues around being able to reason about what the system is
supposed to do and why, hence my concerns about waiting for all devices
on the PMIC to appear.  Like I said above this is absolutely crucial.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 0/4] regulator_sync_state() support

[Mark Brown]

On Wed, 15 Jul 2020 21:20:49 -0700, Saravana Kannan wrote:
> Consider the following example:
> - regulator-X is provided by device-X.
> - regulator-X is a supplier to device-A, device-B and device-C.
> - device-A is off/inactive from boot.
> - device-B and device-C are left on/active by the bootloader
> - regulator-X is left on boot by the bootloader at 2000 mV to supply
>   device-B and device-C.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/1] regulator: core: Add destroy_regulator()
      commit: e1794aa43f17bf2512c10370c6be6ea24a6f29d0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Saravana Kannan]

On Tue, Jul 21, 2020 at 1:18 PM Mark Brown <broonie@kernel.org> wrote:
>
Hi Mark,

It *might* be easier if you jump to the bottom and read the reasoning
for the current design. The stuff in between is just me trying to
clarify some misunderstandings.

> On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote:
> > On Mon, Jul 20, 2020 at 7:28 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, Jul 15, 2020 at 09:20:52PM -0700, Saravana Kannan wrote:
>
> > > > There are Android devices that exhibit the issue in the example where
> > > > regulator-X is an LDO, device-A is a camera device and device-B and
> > > > device-C are UFS and USB. To avoid this, they have their own downstream
> > > > changes to the regulator framework.
>
> > > Can you provide any references to these bodges?
>
> > This is the best I could dig up. The site is slow as molasses. I don't
> > want to focus or critique any specific vendor's downstream code
> > though. Providing these links just to prove that this is a real issue.
>
> The issue is not that I can't see that there might be a problem.  The
> issue I have is that you seem to be starting from the point of a
> specific solution that happens to work for your systems and then working
> back from that rather than reasoning forwards from the problem to come
> to a solution with the result that I can't explain the design you're
> proposing.

I'm sorry it comes across that way. That's certainly not my intention.
Hopefully, we can walk through this together and I'll make more sense.

> Even with this version I have difficulty telling from the
> changelog that the change doesn't restrict things based on the consumers
> of a given regulator but rather based on the PMIC and I don't see why
> this is being done.

Happy to copy-paste any commit text suggestions that make this clearer.

> > Search for "proxy" here. You'll notice how that will also need changes
> > to regulator header files, etc. The 4.9 kernel is the latest publicly
> > available version AFAIK.
> > https://source.codeaurora.org/quic/la/kernel/msm-4.9/plain/drivers/regulator/core.c?h=msm-4.9
>
> Oh, the Qualcomm stuff - which looks a lot like what you've got here.

Eh... I would disagree. The similarity stops at "both solutions use a
regulator consumer to set the limits". The implementation you see
there is based on QC specific DT entries, is based on late_initcall to
"ensure" all consumers have probed, doesn't work for modules, etc.

> > > As I indicated on my previous review this doesn't seem OK, given that a
> > > huge proportion of the regulators on most systems are part of a single
> > > PMIC this means that devices won't be able to fully control regulators
> > > for the majority of the boot process, possibly quite a long time after
> > > userspace has started in systems where not all devices have drivers.
>
> ...
>
> > I think the default behavior should be for functional correctness
> > (example in the commit text) and then go for optimization (being able
> > to power off regulators before 30s into boot). Even with the timeout
> > set, this series makes it much easier for driver developers to ensure
> > functional correctness instead of playing initcall chicken between the
> > supplier and the N consumers.
>
> I don't see how any of this stuff about initcall chicken or whatever is
> relevant here.  You're replying to my concern that your change isn't
> just waiting for all the consumers of a given regulator, it's waiting
> for every other consumer of any other regulator or random other feature
> that happens to be supplied by the same PMIC but only that PMIC.  That
> is not init ordering, it just seems like an arbitrary delay even once
> all the consumers are ready and I can't see any particular logic behind
> it.  It's not just waiting for all the users of the individual resource
> to be active but it's also not waiting for the system as a whole,
> instead it's waiting for some effectively random grouping of devices
> spread over the whole system to appear.  I can't articulate a reason why
> we'd do that, it seems to be combining the worst aspects of the two
> approaches.
>
> Please engage with this issue, it is crucial but you keep on sending the
> same thing without explaining why so I'm left guessing and I really
> can't come up with anything.

Ok, at the end of the email, I'll start from a high level and then dig
into more details at each step. Hopefully that'll clarify the reasoning.

[...]

> > > I don't understand the motivation for doing things this way.  Like I
> > > said last time it really feels like this turns the whole mechanism into
> > > a very complicated way of implementing a new initcall.
>
> > Treating this as a "LATER_initcall()" has several issues.
> > 1. initcall levels set a max limit on the depth of device dependency.
> > Since DT devices are added at arch initcall level, that gives us about
> > 5 levels if you ignore the _sync ones. Adding more isn't going to
> > scale or solve the problem because in reality, the dependencies are
> > much deeper.
>
> I don't follow what you're saying here at all, sorry.  What does the
> depth of the dependency graph have to do with how late we action
> anything?

You keep raising the point "why not just add another initcall level"
to solve the problem of "sync the suppliers after all the consumers
are ready". Or you say that my patch series is just a roundabout way
of adding another initcall level. I'm explaining why adding just
another initcall level won't work.

> A lot of what you're saying here seems to be based on jumping
> to abandoning deferred probe which is a bit of a leap here.

Again, definitely misunderstanding my point. I'm saying exactly the
opposite. We need to take deferred probe into account and deferred
probe completely invalidates any attempts to guarantee ordering based
on initcall levels. I even mentioned this in point 5 in my earlier
email.

> > 6. If the answer is, "have userspace tell us when all modules are
> > loaded" -- then we are depending on userspace for functional
> > correctness AND for turning off regulators. Which IMHO is worse than
> > this patch series.
>
> > 7. If we somehow manage to add a "LATER_initcall" that doesn't have
> > the issues above, it has to work for all frameworks. So, it has to
> > come after ALL the devices in the system have probed. Not just "all
> > devices of a supplier". So, again, it's worse than this patch series,
> > at least for systems where all the drivers are present.
>
> Run the initcall when all devices in the system have a driver bound

And how is this going to work in systems where some devices don't have
drivers? Even with one missing driver, none of the devices/drivers are
going to get this initcall that you suggest. So it makes the
granularity issue worse.

> which as far as I can tell seems to be what the end result of this
> series is intended to be anyway, just implemented with much more
> complexity.

No, with sync_state(), only suppliers to devices without drivers
wouldn't get the callback. Rest of the suppliers will get the callback
and can sync their state.

>  That doesn't need any involvement from userspace, though I
> can't see why we'd want to do that rather than just work per regulator.
>
> > > >    b. If the regulator is ON at boot, a BOOT-LIMITS consumer is created
> > > >       for the regulator and an enable vote is made.
>
> > > If something was left partially set up by the bootloader this means that
> > > drivers are no longer able to remove power from the device as part of
> > > getting it into a known good state even if they are the only consumer.
>
> > If they really NEED to turn power off to get to a known good state,
> > then they really need to be exclusive consumers.
>
> There's *need* to and there's can do on systems that can support the
> thing, the same driver will often be able to handle both situations.
> This also goes to a risk of regressions, it's a fairly major
> transformation to be doing.  These issues would be massively mitigated
> if this were per regulator as I have repeatedly suggested.

Even if you do per-regulator tracking, you can have one regulator
shared by many consumers and you'll still hit this issue. So, going to
a regulator level tracking doesn't solve this.

[ ... ]

> > To be clear, I understand the cases you are mentioning and I'm not
> > discounting them. But compared to the 30 seconds of additional "on
> > time", the functional correctness issues are more important. I'm not
> > saying this series is the perfect solution, but it's certainly better
> > than what we have now and we have the default behavior to be as least
> > disruptive as possible to systems that work fine without this series.
> > And if I find incremental improvements in the future, I'll send
> > patches for that. But I'd hate to see perfect be the enemy of the
> > good.
>
> Having the requests from drivers be acted on promptly is also a
> potential correctness and robustness issue.  You seem very focused on
> the disabling of unused regulators for power reasons case

The correctness issue I care about is not power consumption. It's
about not pulling the plug on or under volting an active device before
the driver can take control of it.

> but that's
> really not the only thing going on here, there's also just the need to
> control things as part of what the device is supposed to be doing.
> Systems aren't always built to let us do all the things we're really
> supposed to do with hardware so it's often advantageous to have drivers
> that will do these things but cope gracefully, this means that it
> doesn't always make sense to use exclusive gets even where you really
> actually want things to happen as requested if they can.  Often the
> devices are in practice robust enough most of the time things are fine
> if the requests get ignored which is why systems can be built that way
> but strictly we're not supposed to do that so where we can we should
> avoid it.  Forcing consumers to be exclusive means that we need to add a
> call that lets these consumers figure out which kind of get to use and
> then have the consumer have multiple code paths for that (or more
> realistically factor that out into the core, but then we could even more
> easily just not have the system wide restrictions).

This paragraph seems to be self contradictory to me. I don't
understand how you can claim the drivers handle it gracefully without
exclusive_get(), but then still say they can't handle a regulator
being on when they request for it to be off. They just need to handle
it gracefully for the first 30s of boot with my patch series. Only
when regulator_cleanup_timeout=-1 is set is there a potential for the
regulator to be kept on forever.

> There's also issues around being able to reason about what the system is
> supposed to do and why, hence my concerns about waiting for all devices
> on the PMIC to appear.  Like I said above this is absolutely crucial.

Ok, I'll try to explain the reasoning from the top.

Firstly, the problem being solved is not specific to regulators. It's
a general issue that needs to be solved for many frameworks/suppliers.
We want a mechanism for a supplier driver to know when all the
consumers are up and managing their resources so that the supplier
driver can do a clean handoff from bootloader to the kernel.

However, we don't want each framework doing very similar but slightly
different things. That creates inconsistencies across frameworks, code
repetition, maintenance headache, etc. In addition to that, it's also
very inefficient for every framework to be trying to figure out the
dependencies from firmware and tracking them separately.

Even if you just try to answer "are all the consumers ready?" at a
resource level (eg: at a regulator level), can't depend on get()/put()
APIs provided by frameworks. Some of the reasons:
1. A consumer might do "get()" and then end up deferring further down
the probe function. If that get() was the last get() you needed to
decide "all consumers are ready", and you remove the boot limits, then
the system could crash because the last consumer isn't actually ready.
2. Not all consumers do the "get()" in their probe functions. They
might do it later for multiple reasons.
3. Not all properties in the firmware are actually used/supported by
the driver. So, you can't assume all consumers referring to the
resource will actually call get() on it.

So, even if you track at a resource level, you need to check for the
consumer readiness by looking at the device bound state. fw_devlink
adds all the supplier-consumer dependencies by creating device links
(currently only DT is supported). So, tracking consumer device
readiness is easier.

So, say you have a callback that you get for every single consumer
binding successfully. What can you do there? For every consumer, you
have to parse their firmware (Eg: DT node) to see what all resources
they use (Eg: all the -supply properties) and increment the "consumer
ref count" for those resources (Eg: regulators). And this needs to be
done by every framework that manages suppliers.

Ok, now you have a "number of consumers ready" refcount per resource.
But to know if all the consumers of a resource are ready, you'll also
need the total number of consumers per resource. How do you find that
out?

Whatever scheme you use to count the total number of consumers, you
need to make sure you also take into account consumer devices that
have not been added yet. As in device_add() hasn't been done. For
example, child devices in DT that haven't been added yet because their
parent hasn't probed.

fw_devlink again helps avoid parsing the entire DT. It creates "proxy"
device links between a supplier device and the parent device of a
consumer if the consumer hasn't been added yet.

So, when the "a consumer has probed" callback comes, apart from what
you have to do already (tracking "number of consumers ready" at a
resource level), you also need to parse all the descendant nodes (not
just direct children) of that consumer's firmware node to check and
potentially increment the "total consumers" at a per resource (Eg:
regulator) level. Again, this will need to be done for every
framework.

And we still haven't dealt with the case where a consumer binds and
unbinds. Say a resource (Eg: regulator) has N consumers. Say, only M
(< N) consumers are ready/bound so far. Then one of the M consumers
unbinds and then binds again. So, your "consumers ready" refcount per
resource becomes M + 1 when only M consumers are ready. So, at a per
resource level you have to now also track "already seen" devices or
track device unbinding. And those "devices" themselves might vanish
and come back again (Eg: DT overlays, the dwc driver that adds/removes
child devices during probe, etc). I'm not even going to try and expand
on how each framework would handle this  -- it becomes very messy.

Also, even if we do resource level tracking, it doesn't solve the
problem of a consumer that expects regulator_disable() to turn off the
regulator even when it's not using get_exclusive(). For example, the
same regulator can have more than one consumer but the consumer that
needs exclusive behavior just happens to probe first and has full
control of the regulator. With boot limits applied at a regulator
level, it'll still prevent the regulator from being turned off.

So, long story short, tracking at a resource level:
1. Will be way more complicated than tracking at a consumer device level.
2. Will use a lot more memory.
3. Will use be a lot less efficient due to all the repeated parsing
and tracking you'll have to do.
4. Doesn't really solve the "need exclusive but doesn't use
get_exclusive()" consumer problem.

But then why not just do a "boot done" initcall/callback instead of
doing it based on supplier-consumers device level dependencies?

"boot done" can be done in a couple of ways:
1. Have userspace tell that it's done loading all the modules. This is
pretty much a no go as the kernel now depends on userspace for proper
functionality.
2. Timer, but this is a configuration nightmare and doesn't work reliably.
3. Wait till every single device has been bound to a driver. The
problem with this is that even if a single device is not bound to a
driver, the entire system would be stuck in a "boot not done yet"
mode. So any device left on by the boot loader would be permanently
stuck on and can't go to states that are incompatible with their state
at boot.

So this is basically a cost (complexity) vs benefit (system vs device
vs resource level removing of boot limits) problem.

Device level granularity provides the best bang for the buck.
Hopefully that clarifies the reasoning.

Would you consider this series if the boot limit is only set when
regulator_cleanup_timeout=-1 (as opposed to always)? Then the default
behavior will stay identical to before this series and only when
regulator_cleanup_timeout=-1 will any of this code kick in.

-Saravana

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 16738 bytes --]

On Tue, Jul 28, 2020 at 02:14:52PM -0700, Saravana Kannan wrote:
> On Tue, Jul 21, 2020 at 1:18 PM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jul 20, 2020 at 08:22:15PM -0700, Saravana Kannan wrote:

Copying in Stephen having done a bunch of mailing list archive trawling
on the fw_devlink stuff since I'm re-raising some stuff he raised before
as he happened to see that stuff before it got merged - most of that
stuff is a way down in the second half of this far too long e-mail.

> > Even with this version I have difficulty telling from the
> > changelog that the change doesn't restrict things based on the consumers
> > of a given regulator but rather based on the PMIC and I don't see why
> > this is being done.

> Happy to copy-paste any commit text suggestions that make this clearer.

Well, like I keep saying I don't know how to explain the implementation
decisions here.  Simply openly stating that you do this would be a
start, things like being explicit about which devices you are talking
about for example - if you mean "the MFD associated with the regulator"
say that, when you talk about the "regulator's device" that could easily
be assumed to mean the struct device for the specific regulator.

> > I don't follow what you're saying here at all, sorry.  What does the
> > depth of the dependency graph have to do with how late we action
> > anything?

> You keep raising the point "why not just add another initcall level"
> to solve the problem of "sync the suppliers after all the consumers
> are ready". Or you say that my patch series is just a roundabout way
> of adding another initcall level. I'm explaining why adding just
> another initcall level won't work.

I am saying that this winds up being roughly the same thing in effect
just with less clarity and more complexity so therefore consider
applying the same workarounds for problems here as we do for your
current code.  Wait for devices to bind like your current code does, use
a timer in case some of them don't bind like your current code does.

> > Run the initcall when all devices in the system have a driver bound

> And how is this going to work in systems where some devices don't have
> drivers? Even with one missing driver, none of the devices/drivers are
> going to get this initcall that you suggest. So it makes the
> granularity issue worse.

I don't think either of these things is a particularly great idea, it's
just that doing things per system doesn't have materially different
effects for regulators on most systems and I can at least explain when
things happen more clearly and there'd be less random variation in
behaviour in the less common cases.

> > which as far as I can tell seems to be what the end result of this
> > series is intended to be anyway, just implemented with much more
> > complexity.

> No, with sync_state(), only suppliers to devices without drivers
> wouldn't get the callback. Rest of the suppliers will get the callback
> and can sync their state.

But with the timeout which we would need to have by default everything
will get the callback anyway.

> > > If they really NEED to turn power off to get to a known good state,
> > > then they really need to be exclusive consumers.

> > There's *need* to and there's can do on systems that can support the
> > thing, the same driver will often be able to handle both situations.
> > This also goes to a risk of regressions, it's a fairly major
> > transformation to be doing.  These issues would be massively mitigated
> > if this were per regulator as I have repeatedly suggested.

> Even if you do per-regulator tracking, you can have one regulator
> shared by many consumers and you'll still hit this issue. So, going to
> a regulator level tracking doesn't solve this.

It solves it for supplies that aren't actually shared without requiring
consumers to have separate code paths that figure out if they're using
shared supplies or not which if nothing else is effort for existing
drivers.  This would be a much less surprising implementation decision
in general, and much more likely to do what people want in a system.

> > Having the requests from drivers be acted on promptly is also a
> > potential correctness and robustness issue.  You seem very focused on
> > the disabling of unused regulators for power reasons case

> The correctness issue I care about is not power consumption. It's
> about not pulling the plug on or under volting an active device before
> the driver can take control of it.

Your comments suggest that you seem to think this is why devices would
be controlling regulators.

> > avoid it.  Forcing consumers to be exclusive means that we need to add a
> > call that lets these consumers figure out which kind of get to use and
> > then have the consumer have multiple code paths for that (or more
> > realistically factor that out into the core, but then we could even more
> > easily just not have the system wide restrictions).

> This paragraph seems to be self contradictory to me. I don't
> understand how you can claim the drivers handle it gracefully without
> exclusive_get(), but then still say they can't handle a regulator
> being on when they request for it to be off. They just need to handle
> it gracefully for the first 30s of boot with my patch series. Only
> when regulator_cleanup_timeout=-1 is set is there a potential for the
> regulator to be kept on forever.

Remember that the same device and driver can be used in different
systems with different configurations and integrations.  While some
systems might be set up so that the regulator is unshared, can be freely
controlled and even rely on that happening there may be other systems
that not have software controllable regulators at all and are instead
set up so the regulators don't need to be controlled.  Also think about
all the times you see software doing things that happen to work but
probably aren't robust or a good idea, and how we sometimes have to put
support code in there to keep them working when they do break.
Electrical engineering and general system design can be like that too.  

As a really simple example a hardware designer might decide they can
save a GPIO by not wiring up a reset signal since the device has a
dedicated power supply and software can just turn that off and on to
trigger a power on reset instead.  Other systems with the same device
might give software control of the reset GPIO but not the power and
assume that it will assert reset that way.

> > There's also issues around being able to reason about what the system is
> > supposed to do and why, hence my concerns about waiting for all devices
> > on the PMIC to appear.  Like I said above this is absolutely crucial.

> Ok, I'll try to explain the reasoning from the top.

> Firstly, the problem being solved is not specific to regulators. It's
> a general issue that needs to be solved for many frameworks/suppliers.
> We want a mechanism for a supplier driver to know when all the
> consumers are up and managing their resources so that the supplier
> driver can do a clean handoff from bootloader to the kernel.

> However, we don't want each framework doing very similar but slightly
> different things. That creates inconsistencies across frameworks, code
> repetition, maintenance headache, etc. In addition to that, it's also
> very inefficient for every framework to be trying to figure out the
> dependencies from firmware and tracking them separately.

There seem to be some very substantial assumptions in here.  The biggest
is that we can't share code if there's any subsystem knowledge of what's
going on, this is already impacting every affected subsystem with your
current implementation anyway so it's not like it's transparent to them
but you're still managing to share code.  It's also not 100% clear to me
that we want every framework to behave in exactly the same way -
resources obviously aren't completely fungible.  For example while it's
very common for regulators to be shared that's not the case for GPIOs
and that has an impact on what drivers should be expected to handle.
Consistency is obviously good but it's not the only thing to consider.

To me this doesn't explain why we would want the system to behave in
this way, it explains how some implementation details flow from other
implementation details.  At a very high level there seems to be an
unstated assumption in here about existing code outside the regulator
framework (mainly fw_devlink I think) being unmodifiable which really
should not be the case.

> Even if you just try to answer "are all the consumers ready?" at a
> resource level (eg: at a regulator level), can't depend on get()/put()
> APIs provided by frameworks. Some of the reasons:

Why would we need to or even want to depend on or use get() and put()
here?  That doesn't seem at all obvious to me.  We clearly have the
ability to relate individual resources to their consumer devices for
every subsystem that's affected here and clearly also already have the
ability to make links between consumer and supplier devices prior to
them probing.  It shouldn't be impossible to take these two things and
combine them in a way that keeps the tracking and notification code
shared.

In the case of the regulator API we already create a struct device and
have an of_node per regulator so it should be even easier to use the
existing device based stuff than with subsystems that don't have that,
it should at most be some extra code to move some links around (or add
new ones if that's easier).  Off the top of my head a call to move any
misdirected links to a given of_node to a new struct device (or add new
ones leaving the old ones in place I guess) should do the trick for
regulators without even modifying the sync_state() callback stuff, just
the link setup.  It seems like the framework is already pretty much
ideally set up for this, you could even make a case that the reason
there's not already per-resource links for regulators is that the
existing code is not doing a good job handling the DT bindings and is
misdirecting links to the wrong struct device.

> So, say you have a callback that you get for every single consumer
> binding successfully. What can you do there? For every consumer, you
> have to parse their firmware (Eg: DT node) to see what all resources
> they use (Eg: all the -supply properties)

Again off the top of my head we could also do this the other way around
and when making a link go and ask the subsystems if it's their link and
they have a better idea about where to put it.  Actually, having found
the code that adds the links we don't even need to ask the subsystems if
it's their link - we already know at the time we're doing the parsing
which subsystem a link relates to!  Perhaps we could do some of this
checking/notification at the time links are connected/satisfied rather
than at parse time, or perhaps when the suppliers register they could
look at outgoing links.

We already have a set of links and we already have the ability to figure
out which resources they're talking about, we just need to join those
two things up which shouldn't be an intractable problem.

>                                           and increment the "consumer
> ref count" for those resources (Eg: regulators).  And this needs to be
> done by every framework that manages suppliers.

With your current code every framework that manages resources that can
be shared already needs to be modified to block actions until there's a
sync_state() callback, modifying frameworks is clearly not an issue
here.  It even turns out that we already have some custom parsing code
(which shares code!) for links for individual subsystems including
regulators, unfortunately I'd not seen the regulator code until today
since while it was merged back in October it's in the of code rather
than the regulator code and was never copied to me (you were asked to do
so at the time to by Stephen Boyd, sadly the code got merged at the
version he was reviewing so there wasn't another post).  Most of that is
data driven but not all.

I also don't see why we'd need to end up open coding reference counting,
if we have code for tracking links and references let's use it,
extending it where we need to.

> fw_devlink again helps avoid parsing the entire DT. It creates "proxy"
> device links between a supplier device and the parent device of a
> consumer if the consumer hasn't been added yet.

fw_devlink is something we can modify and extend, we don't have to
completely discard it to do better than what's currently being done.
It feels like a lot of your analysis here is predicated on not being
able to change anything with that code, but obviously that reasoning
gets a bit circular.  At the minute it definitely seems to have some
assumptions in there about the encoding of the Linux device model into
DT which ought to be addressed, they seem in some fundamental way to be
at the root of my concerns.

> So, long story short, tracking at a resource level:
> 1. Will be way more complicated than tracking at a consumer device level.
> 2. Will use a lot more memory.
> 3. Will use be a lot less efficient due to all the repeated parsing
> and tracking you'll have to do.

I don't think these things need to be the case, I think we can do much
better with code reuse than you are anticipating and achieve similar
performance through that reuse.

> 4. Doesn't really solve the "need exclusive but doesn't use
> get_exclusive()" consumer problem.

It definitely does in for example the case where a regulator is intended
to be actively managed and is as a result unshared in a particular
system.

> But then why not just do a "boot done" initcall/callback instead of
> doing it based on supplier-consumers device level dependencies?

> "boot done" can be done in a couple of ways:
> 1. Have userspace tell that it's done loading all the modules. This is
> pretty much a no go as the kernel now depends on userspace for proper
> functionality.
> 2. Timer, but this is a configuration nightmare and doesn't work reliably.
> 3. Wait till every single device has been bound to a driver. The
> problem with this is that even if a single device is not bound to a
> driver, the entire system would be stuck in a "boot not done yet"
> mode. So any device left on by the boot loader would be permanently
> stuck on and can't go to states that are incompatible with their state
> at boot.

I agree that doing things per system is not great but like I keep saying
at least for regulators doing things based on the MFD is at best very
close to the same thing given that in most systems the vast majority of
controllable supplies are provided by a single PMIC.  This is why I push
for handling things per supply, it's much more what someone would expect
the system to do and much more likely to just do the right thing.  I'm
trying to understand the underlying goal of this design decision and
come up with something that might actually deliver that reliably.

One idea which was mentioned by Stephen was issues with hardware
controlled by multiple devices (eg, a display controller and a GPU) -
that might point towards doing something per system, though perhaps not
since we hopefully only need the display controller in that case until
userspace has come up and started drawing new things and obviously if
that's needed then waiting for drivers to probe isn't particularly
relevant so we'd not be gaining much.

I do think we need a timeout no matter what for something like this,
even with doing things per resource - users can configure them off if
they prefer that but there's definitely people who are going to end up
on systems with devices that don't have drivers instantiate.

> Would you consider this series if the boot limit is only set when
> regulator_cleanup_timeout=-1 (as opposed to always)? Then the default
> behavior will stay identical to before this series and only when
> regulator_cleanup_timeout=-1 will any of this code kick in.

I'm having a really hard time seeing that as a good idea, merging things
that work for some specific systems but don't scale has lead to ongoing
problems elsewhere and I don't want to repeat that mistake.  If it was a
case where we had a solid, well understood design which needed some
additional work to extend it to cover some cases then that sort of thing
could make sense but currently the basic design is still unclear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/4] regulator: core: Add basic enable/disable support for sync_state() callbacks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Tue, Aug 04, 2020 at 10:10:49PM +0100, Mark Brown wrote:
> On Tue, Jul 28, 2020 at 02:14:52PM -0700, Saravana Kannan wrote:

> > So, say you have a callback that you get for every single consumer
> > binding successfully. What can you do there? For every consumer, you
> > have to parse their firmware (Eg: DT node) to see what all resources
> > they use (Eg: all the -supply properties)

> Again off the top of my head we could also do this the other way around
> and when making a link go and ask the subsystems if it's their link and
> they have a better idea about where to put it.  Actually, having found
> the code that adds the links we don't even need to ask the subsystems if
> it's their link - we already know at the time we're doing the parsing
> which subsystem a link relates to!  Perhaps we could do some of this
> checking/notification at the time links are connected/satisfied rather
> than at parse time, or perhaps when the suppliers register they could
> look at outgoing links.

> We already have a set of links and we already have the ability to figure
> out which resources they're talking about, we just need to join those
> two things up which shouldn't be an intractable problem.

So, having taken a look at the device_link stuff in the driver core it
seems like it should be easy enough to add another parameter to
device_link_add() or a variant thereof so we can get a supplier ID of
some kind to the core (eg, a callback plus ID) along with the link so I
don't see any issue with getting the data in there.

We then need to figure out how to use that in device_links_driver_bound(),
that is currently unconditionally kicking _queue_sync_state() for every
supplier to go check if we need to do the sync_state() so would seem to
be the logical place to schedule a per subsystem callback.  It also
deletes the link if it was a _SYNC_STATE link which does make things a
bit more fun but we can always remember which link we're deleting and
pass that on when scheduling the kick.  Indeed, it occurs to me that we
could be cute here and in _queue_sync_state() have it check while
scanning the consumer links to see if we find a consumer with the same
subsystem callback information and if we don't then that must've been
the last link that just got deleted and we can call the callback.

That's not quite everything, in particular at least for any subsystem
where the core can be modular you'd need to have a layer of indirection
for the callback (it's possibly a good idea to do that anyway), but I'm
not seeing anything new with regard to locking or whatever.  It looks
like the work already done for basic sync_state() should be reusable
unless there's some nasty gotchas I'm not seeing, that was pretty much
what I was expecting to see TBH.

BTW doesn't the fact that we throw away the _SYNC_STATE_ONLY links cause
fun if the provider driver gets unbound then rebound?  We don't reparse
the DT to re-add those links.  I'm also not seeing where we ever clear
the state_synced flag that gets set which seems like it'd break things
if a supplier gets removed and reprobed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]