[PATCH 1/7] regulator: core: Properly expose requested_microamps in sysfs

[PATCH 1/7] regulator: core: Properly expose requested_microamps in sysfs

[Douglas Anderson]

The "requested_microamps" sysfs attribute was only being exposed for
"current" regulators.  This didn't make sense.  Allow it to be exposed
always.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d7ffd7b12472..ff5ca185bb8f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4565,10 +4565,6 @@ static umode_t regulator_attr_is_visible(struct kobject *kobj,
 	if (attr == &dev_attr_bypass.attr)
 		return ops->get_bypass ? mode : 0;
 
-	/* some attributes are type-specific */
-	if (attr == &dev_attr_requested_microamps.attr)
-		return rdev->desc->type == REGULATOR_CURRENT ? mode : 0;
-
 	/* constraints need specific supporting methods */
 	if (attr == &dev_attr_min_microvolts.attr ||
 	    attr == &dev_attr_max_microvolts.attr)
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 2/7] regulator: core: Don't assume always_on when is_enabled returns err

[Douglas Anderson]

At boot sometimes regulators (like qcom-rpmh-regulator) will return
-EINVAL if we don't know the enable state of the regulator.  We
shouldn't take this to mean that the regulator is an always-on
regulator, but that was what was happening since "-EINVAL" is non-zero
and a few places in the code were not properly checking for errors.

Let's resolve this.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ff5ca185bb8f..0052bbc8c531 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1622,7 +1622,7 @@ static struct regulator *create_regulator(struct regulator_dev *rdev,
 	 * enable/disable calls.
 	 */
 	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS) &&
-	    _regulator_is_enabled(rdev))
+	    _regulator_is_enabled(rdev) > 0)
 		regulator->always_on = true;
 
 	regulator_unlock(rdev);
@@ -1811,7 +1811,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	}
 
 	/* Cascade always-on state to supply */
-	if (_regulator_is_enabled(rdev)) {
+	if (_regulator_is_enabled(rdev) > 0) {
 		ret = regulator_enable(rdev->supply);
 		if (ret < 0) {
 			_regulator_put(rdev->supply);
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 3/7] regulator: core: Don't double-disable supplies in regulator_disable_deferred()

[Douglas Anderson]

In the commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for
regulators locking") disabling of the supply was moved into
_regulator_disable().  That means regulator_disable_work() shouldn't
be disabling since that double-disables the supply.

Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0052bbc8c531..63a8af1e2256 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2703,16 +2703,6 @@ static void regulator_disable_work(struct work_struct *work)
 		regulator_balance_voltage(rdev, PM_SUSPEND_ON);
 
 	regulator_unlock_dependent(rdev, &ww_ctx);
-
-	if (rdev->supply) {
-		for (i = 0; i < count; i++) {
-			ret = regulator_disable(rdev->supply);
-			if (ret != 0) {
-				rdev_err(rdev,
-					 "Supply disable failed: %d\n", ret);
-			}
-		}
-	}
 }
 
 /**
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 4/7] regulator: core: Only count load for enabled consumers

[Douglas Anderson]

In general when the consumer of a regulator requests that the
regulator be disabled it no longer will be drawing much load from the
regulator--it should just be the leakage current and that should be
very close to 0.

Up to this point the regulator framework has continued to count a
consumer's load request for disabled regulators.  This has led to code
patterns that look like this:

  enable_my_thing():
    regular_set_load(reg, load_uA)
    regulator_enable(reg)

  disable_my_thing():
    regulator_disable(reg)
    regulator_set_load(reg, 0)

Sometimgs disable_my_thing() sets a nominal (<= 100 uA) load instead
of setting a 0 uA load.  I will make the assertion that nearly all (if
not all) places where we set a nominal load of 100 uA or less we end
up with a result that is the same as if we had set a load of 0 uA.
Specifically:
- The whole point of setting the load is to help set the operating
  mode of the regulator.  Higher loads may need less efficient
  operating modes.
- The only time this matters at all is if there is another consumer of
  the regulator that wants the regulator on.  If there are no other
  consumers of the regulator then the regulator will turn off and we
  don't care about the operating mode.
- If there's another consumer that actually wants the regulator on
  then presumably it is requesting a load that makes our nominal
  <= 100 uA load insignificant.

A quick survey of the existing callers to regulator_set_load() to see
how everyone uses it:

--

Places that set a load of 0 on every disable:
  drivers/bluetooth/hci_qca.c
  drivers/net/wireless/ath/ath10k/snoc.c
  drivers/remoteproc/qcom_q6v5_mss.c
  drivers/scsi/ufs/ufshcd.c

Places that set a nominal disable load (<= 100 uA):
  drivers/gpu/drm/msm/dsi/dsi_host.c:
    git grep -A10 'struct msm_dsi_config' | grep -A10 regs | grep '{.*}'
  drivers/gpu/drm/msm/dsi/phy/dsi_phy.c:
    git grep -A10 'struct msm_dsi_phy_cfg' | grep -A10 regs | grep '{.*}'
  drivers/gpu/drm/msm/edp/edp_ctrl.c

Places that appear to continue to request load when disabled; I
believe this is not intentional and is a bug.  AKA: either the
regulator will be left in a higher power operating mode sometimes when
it doesn't need to or it is understood that there are no other
consumers of these rails and thus the regulator_set_load() shouldn't
have been needed at all (we should just force the regulator to high
power all the time in that case):
  drivers/phy/qualcomm/phy-qcom-ufs.c
  drivers/phy/qualcomm/phy-qcom-usb-hs.c
  drivers/remoteproc/qcom_q6v5_pas.c
  drivers/remoteproc/qcom_wcnss.c (*)
  drivers/remoteproc/qcom_wcnss_iris.c

(*) Quick glace makes me feel like this driver is buggy since
wcnss_start() enables regulators but wcnss_stop() doesn't disable
them.

--

Given the above argument, I propose changing the regulator core to
only consider loads from consumers that are currently enabled.  If
later we find a use case where we really need to count a non-zero load
when a consumer requests that a regulator be disabled we could just
count load like this in the system_load for the regulator.  If somehow
that's not possible I suppose we could add consider adding a new API
regulator_set_disabled_load().

In order to accomplish the above, we need to start tracking the enable
count per regulator consumer (it used to be tracked only globally
across all consumers of a given regulator).  This means:
- We can (and will) spit errors out for code that used to be invalid
  but was never caught before.  Specifically if someone leaves a
  regulator enabled and calls regulator_put() we'll yell.  We'll also
  yell if a single consumer calls more disables than enables.
- We now need to deal with "always-on" regulators slightly
  differently.  It's assumed that an always-on regulator could still
  transition between power modes, so if all of the active consumers of
  an always-on regulator don't need the regulator on we should still
  remove their requested load from the total.  The core used to have
  some optimizations where it didn't bother keeping track of the
  enable counts for always-on regulators because they were, well,
  always on.  While we could keep the optimization still for some
  cases, it's cleaner to just remove it.  A later patch will attempt
  to get some efficiency back by not propogating enables up
  unnecessarily.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Please give this patch lots of extra review.  It seems to work for me,
but I haven't done massive amounts of stress testing on it and I don't
touch every use case of the regulator core.  Thanks!

 drivers/regulator/core.c         | 190 +++++++++++++++++++++++--------
 drivers/regulator/internal.h     |   2 +
 include/linux/regulator/driver.h |   1 -
 3 files changed, 142 insertions(+), 51 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 63a8af1e2256..c5da6079e1cb 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -99,7 +99,7 @@ struct regulator_supply_alias {
 };
 
 static int _regulator_is_enabled(struct regulator_dev *rdev);
-static int _regulator_disable(struct regulator_dev *rdev);
+static int _regulator_disable(struct regulator *regulator);
 static int _regulator_get_voltage(struct regulator_dev *rdev);
 static int _regulator_get_current_limit(struct regulator_dev *rdev);
 static unsigned int _regulator_get_mode(struct regulator_dev *rdev);
@@ -764,8 +764,10 @@ static ssize_t regulator_total_uA_show(struct device *dev,
 	int uA = 0;
 
 	regulator_lock(rdev);
-	list_for_each_entry(regulator, &rdev->consumer_list, list)
-		uA += regulator->uA_load;
+	list_for_each_entry(regulator, &rdev->consumer_list, list) {
+		if (regulator->enable_count)
+			uA += regulator->uA_load;
+	}
 	regulator_unlock(rdev);
 	return sprintf(buf, "%d\n", uA);
 }
@@ -938,8 +940,10 @@ static int drms_uA_update(struct regulator_dev *rdev)
 		return -EINVAL;
 
 	/* calc total requested load */
-	list_for_each_entry(sibling, &rdev->consumer_list, list)
-		current_uA += sibling->uA_load;
+	list_for_each_entry(sibling, &rdev->consumer_list, list) {
+		if (sibling->enable_count)
+			current_uA += sibling->uA_load;
+	}
 
 	current_uA += rdev->constraints->system_load;
 
@@ -2024,6 +2028,9 @@ static void _regulator_put(struct regulator *regulator)
 
 	lockdep_assert_held_once(&regulator_list_mutex);
 
+	/* Docs say you must disable before calling regulator_put() */
+	WARN_ON(regulator->enable_count);
+
 	rdev = regulator->rdev;
 
 	debugfs_remove_recursive(regulator->debugfs);
@@ -2417,15 +2424,75 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 	return 0;
 }
 
+/**
+ * _regulator_handle_consumer_enable - handle that a consumer enabled
+ * @regulator: regulator source
+ *
+ * Some things on a regulator consumer (like the contribution towards total
+ * load on the regulator) only have an effect when the consumer wants the
+ * regulator enabled.  Explained in example with two consumers of the same
+ * regulator:
+ *   consumer A: set_load(100);       => total load = 0
+ *   consumer A: regulator_enable();  => total load = 100
+ *   consumer B: set_load(1000);      => total load = 100
+ *   consumer B: regulator_enable();  => total load = 1100
+ *   consumer A: regulator_disable(); => total_load = 1000
+ *
+ * This function (together with _regulator_handle_consumer_disable) is
+ * responsible for keeping track of the refcount for a given regulator consumer
+ * and applying / unapplying these things.
+ *
+ * Returns 0 upon no error; -error upon error.
+ */
+static int _regulator_handle_consumer_enable(struct regulator *regulator)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+
+	lockdep_assert_held_once(&rdev->mutex.base);
+
+	regulator->enable_count++;
+	if (regulator->uA_load && regulator->enable_count == 1)
+		return drms_uA_update(rdev);
+
+	return 0;
+}
+
+/**
+ * _regulator_handle_consumer_disable - handle that a consumer disabled
+ * @regulator: regulator source
+ *
+ * The opposite of _regulator_handle_consumer_enable().
+ *
+ * Returns 0 upon no error; -error upon error.
+ */
+static int _regulator_handle_consumer_disable(struct regulator *regulator)
+{
+	struct regulator_dev *rdev = regulator->rdev;
+
+	lockdep_assert_held_once(&rdev->mutex.base);
+
+	if (!regulator->enable_count) {
+		rdev_err(rdev, "Underflow of regulator enable count\n");
+		return -EINVAL;
+	}
+
+	regulator->enable_count--;
+	if (regulator->uA_load && regulator->enable_count == 0)
+		return drms_uA_update(rdev);
+
+	return 0;
+}
+
 /* locks held by regulator_enable() */
-static int _regulator_enable(struct regulator_dev *rdev)
+static int _regulator_enable(struct regulator *regulator)
 {
+	struct regulator_dev *rdev = regulator->rdev;
 	int ret;
 
 	lockdep_assert_held_once(&rdev->mutex.base);
 
 	if (rdev->supply) {
-		ret = _regulator_enable(rdev->supply->rdev);
+		ret = _regulator_enable(rdev->supply);
 		if (ret < 0)
 			return ret;
 	}
@@ -2437,9 +2504,9 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			goto err_disable_supply;
 	}
 
-	/* check voltage and requested load before enabling */
-	if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
-		drms_uA_update(rdev);
+	ret = _regulator_handle_consumer_enable(regulator);
+	if (ret < 0)
+		goto err_disable_supply;
 
 	if (rdev->use_count == 0) {
 		/* The regulator may on if it's not switchable or left on */
@@ -2448,18 +2515,18 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			if (!regulator_ops_is_valid(rdev,
 					REGULATOR_CHANGE_STATUS)) {
 				ret = -EPERM;
-				goto err_disable_supply;
+				goto err_consumer_disable;
 			}
 
 			ret = _regulator_do_enable(rdev);
 			if (ret < 0)
-				goto err_disable_supply;
+				goto err_consumer_disable;
 
 			_notifier_call_chain(rdev, REGULATOR_EVENT_ENABLE,
 					     NULL);
 		} else if (ret < 0) {
 			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
-			goto err_disable_supply;
+			goto err_consumer_disable;
 		}
 		/* Fallthrough on positive return values - already enabled */
 	}
@@ -2468,9 +2535,12 @@ static int _regulator_enable(struct regulator_dev *rdev)
 
 	return 0;
 
+err_consumer_disable:
+	_regulator_handle_consumer_disable(regulator);
+
 err_disable_supply:
 	if (rdev->supply)
-		_regulator_disable(rdev->supply->rdev);
+		_regulator_disable(rdev->supply);
 
 	return ret;
 }
@@ -2490,13 +2560,10 @@ int regulator_enable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	struct ww_acquire_ctx ww_ctx;
-	int ret = 0;
-
-	if (regulator->always_on)
-		return 0;
+	int ret;
 
 	regulator_lock_dependent(rdev, &ww_ctx);
-	ret = _regulator_enable(rdev);
+	ret = _regulator_enable(regulator);
 	regulator_unlock_dependent(rdev, &ww_ctx);
 
 	return ret;
@@ -2535,8 +2602,9 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 }
 
 /* locks held by regulator_disable() */
-static int _regulator_disable(struct regulator_dev *rdev)
+static int _regulator_disable(struct regulator *regulator)
 {
+	struct regulator_dev *rdev = regulator->rdev;
 	int ret = 0;
 
 	lockdep_assert_held_once(&rdev->mutex.base);
@@ -2571,17 +2639,17 @@ static int _regulator_disable(struct regulator_dev *rdev)
 
 		rdev->use_count = 0;
 	} else if (rdev->use_count > 1) {
-		if (regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS))
-			drms_uA_update(rdev);
-
 		rdev->use_count--;
 	}
 
+	if (ret == 0)
+		ret = _regulator_handle_consumer_disable(regulator);
+
 	if (ret == 0 && rdev->coupling_desc.n_coupled > 1)
 		ret = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
 
 	if (ret == 0 && rdev->supply)
-		ret = _regulator_disable(rdev->supply->rdev);
+		ret = _regulator_disable(rdev->supply);
 
 	return ret;
 }
@@ -2602,13 +2670,10 @@ int regulator_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	struct ww_acquire_ctx ww_ctx;
-	int ret = 0;
-
-	if (regulator->always_on)
-		return 0;
+	int ret;
 
 	regulator_lock_dependent(rdev, &ww_ctx);
-	ret = _regulator_disable(rdev);
+	ret = _regulator_disable(regulator);
 	regulator_unlock_dependent(rdev, &ww_ctx);
 
 	return ret;
@@ -2654,13 +2719,22 @@ int regulator_force_disable(struct regulator *regulator)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 	struct ww_acquire_ctx ww_ctx;
+	bool update_uA;
 	int ret;
 
 	regulator_lock_dependent(rdev, &ww_ctx);
-	regulator->uA_load = 0;
+
 	ret = _regulator_force_disable(regulator->rdev);
+
 	if (rdev->coupling_desc.n_coupled > 1)
 		regulator_balance_voltage(rdev, PM_SUSPEND_ON);
+
+	update_uA = regulator->enable_count && regulator->uA_load;
+	regulator->enable_count = 0;
+	regulator->uA_load = 0;
+	if (update_uA)
+		ret = drms_uA_update(rdev);
+
 	regulator_unlock_dependent(rdev, &ww_ctx);
 
 	if (rdev->supply)
@@ -2677,14 +2751,11 @@ static void regulator_disable_work(struct work_struct *work)
 						  disable_work.work);
 	struct ww_acquire_ctx ww_ctx;
 	int count, i, ret;
+	struct regulator *regulator;
+	int total_count = 0;
 
 	regulator_lock_dependent(rdev, &ww_ctx);
 
-	BUG_ON(!rdev->deferred_disables);
-
-	count = rdev->deferred_disables;
-	rdev->deferred_disables = 0;
-
 	/*
 	 * Workqueue functions queue the new work instance while the previous
 	 * work instance is being processed. Cancel the queued work instance
@@ -2693,11 +2764,22 @@ static void regulator_disable_work(struct work_struct *work)
 	 */
 	cancel_delayed_work(&rdev->disable_work);
 
-	for (i = 0; i < count; i++) {
-		ret = _regulator_disable(rdev);
-		if (ret != 0)
-			rdev_err(rdev, "Deferred disable failed: %d\n", ret);
+	list_for_each_entry(regulator, &rdev->consumer_list, list) {
+		count = regulator->deferred_disables;
+
+		if (!count)
+			continue;
+
+		total_count += count;
+		regulator->deferred_disables = 0;
+
+		for (i = 0; i < count; i++) {
+			ret = _regulator_disable(regulator);
+			if (ret != 0)
+				rdev_err(rdev, "Deferred disable failed: %d\n", ret);
+		}
 	}
+	WARN_ON(!total_count);
 
 	if (rdev->coupling_desc.n_coupled > 1)
 		regulator_balance_voltage(rdev, PM_SUSPEND_ON);
@@ -2721,14 +2803,11 @@ int regulator_disable_deferred(struct regulator *regulator, int ms)
 {
 	struct regulator_dev *rdev = regulator->rdev;
 
-	if (regulator->always_on)
-		return 0;
-
 	if (!ms)
 		return regulator_disable(regulator);
 
 	regulator_lock(rdev);
-	rdev->deferred_disables++;
+	regulator->deferred_disables++;
 	mod_delayed_work(system_power_efficient_wq, &rdev->disable_work,
 			 msecs_to_jiffies(ms));
 	regulator_unlock(rdev);
@@ -4135,16 +4214,30 @@ EXPORT_SYMBOL_GPL(regulator_get_error_flags);
  * DRMS will sum the total requested load on the regulator and change
  * to the most efficient operating mode if platform constraints allow.
  *
+ * NOTE: when a regulator consumer requests to have a regulator
+ * disabled then any load that consumer requested no longer counts
+ * toward the total requested load.  If the regulator is re-enabled
+ * then the previously requested load will start counting again.
+ *
+ * If a regulator is an always-on regulator then an individual consumer's
+ * load will still be removed if that consumer is fully disabled.
+ *
  * On error a negative errno is returned.
  */
 int regulator_set_load(struct regulator *regulator, int uA_load)
 {
 	struct regulator_dev *rdev = regulator->rdev;
-	int ret;
+	int old_uA_load;
+	int ret = 0;
 
 	regulator_lock(rdev);
+	old_uA_load = regulator->uA_load;
 	regulator->uA_load = uA_load;
-	ret = drms_uA_update(rdev);
+	if (regulator->enable_count && old_uA_load != uA_load) {
+		ret = drms_uA_update(rdev);
+		if (ret < 0)
+			regulator->uA_load = old_uA_load;
+	}
 	regulator_unlock(rdev);
 
 	return ret;
@@ -4315,11 +4408,8 @@ int regulator_bulk_enable(int num_consumers,
 	int ret = 0;
 
 	for (i = 0; i < num_consumers; i++) {
-		if (consumers[i].consumer->always_on)
-			consumers[i].ret = 0;
-		else
-			async_schedule_domain(regulator_bulk_enable_async,
-					      &consumers[i], &async_domain);
+		async_schedule_domain(regulator_bulk_enable_async,
+				      &consumers[i], &async_domain);
 	}
 
 	async_synchronize_full_domain(&async_domain);
diff --git a/drivers/regulator/internal.h b/drivers/regulator/internal.h
index 943926a156f2..6017f15c5d75 100644
--- a/drivers/regulator/internal.h
+++ b/drivers/regulator/internal.h
@@ -42,6 +42,8 @@ struct regulator {
 	unsigned int always_on:1;
 	unsigned int bypass:1;
 	int uA_load;
+	unsigned int enable_count;
+	unsigned int deferred_disables;
 	struct regulator_voltage voltage[REGULATOR_STATES_NUM];
 	const char *supply_name;
 	struct device_attribute dev_attr;
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 7065031f0846..389bcaf7900f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -474,7 +474,6 @@ struct regulator_dev {
 	struct regmap *regmap;
 
 	struct delayed_work disable_work;
-	int deferred_disables;
 
 	void *reg_data;		/* regulator_dev data */
 
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Douglas Anderson]

Now that consumers all keep track of their own enable count, let's add
it into the regulator_summary.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c5da6079e1cb..23e852d38b88 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -5305,8 +5305,11 @@ static void regulator_summary_show_subtree(struct seq_file *s,
 
 		switch (rdev->desc->type) {
 		case REGULATOR_VOLTAGE:
-			seq_printf(s, "%37dmA %5dmV %5dmV",
+			seq_printf(s, "%3d %33dmA%c%5dmV %5dmV",
+				   consumer->enable_count,
 				   consumer->uA_load / 1000,
+				   consumer->uA_load && !consumer->enable_count ?
+				   '*' : ' ',
 				   consumer->voltage[PM_SUSPEND_ON].min_uV / 1000,
 				   consumer->voltage[PM_SUSPEND_ON].max_uV / 1000);
 			break;
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 6/7] regulator: core: Avoid propagating to supplies when possible

[Douglas Anderson]

When we called regulator_enable() on a regulator we'd end up
propagating that call all the way up the chain every time.  This is a
bit of a waste of time.  A child regulator already refcounts its own
enables so it should avoid passing on to its parent unless the
refcount transitioned between 0 and 1.

Historically this hasn't been a huge problem since we skipped dealing
with enable for always-on regulators.  In a previous patch, however,
we removed the always-on optimization.  On one system, the debugfs
regulator_summary was now showing a "use_count" of 33 for a top-level
regulator.

Let's implement this optimization.  This turns out to be fairly
trivial with the recent reorganization of the regulator core.

NOTE: as part of this patch I'll make "always-on" regulators start
with a use count of 1.  This keeps the counts clean when recursively
resolving regulators.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 23e852d38b88..2eda87520832 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1822,6 +1822,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 			rdev->supply = NULL;
 			return ret;
 		}
+		rdev->use_count = 1;
 	}
 
 	return 0;
@@ -2491,7 +2492,7 @@ static int _regulator_enable(struct regulator *regulator)
 
 	lockdep_assert_held_once(&rdev->mutex.base);
 
-	if (rdev->supply) {
+	if (rdev->use_count == 0 && rdev->supply) {
 		ret = _regulator_enable(rdev->supply);
 		if (ret < 0)
 			return ret;
@@ -2539,7 +2540,7 @@ static int _regulator_enable(struct regulator *regulator)
 	_regulator_handle_consumer_disable(regulator);
 
 err_disable_supply:
-	if (rdev->supply)
+	if (rdev->use_count == 0 && rdev->supply)
 		_regulator_disable(rdev->supply);
 
 	return ret;
@@ -2648,7 +2649,7 @@ static int _regulator_disable(struct regulator *regulator)
 	if (ret == 0 && rdev->coupling_desc.n_coupled > 1)
 		ret = regulator_balance_voltage(rdev, PM_SUSPEND_ON);
 
-	if (ret == 0 && rdev->supply)
+	if (ret == 0 && rdev->use_count == 0 && rdev->supply)
 		ret = _regulator_disable(rdev->supply);
 
 	return ret;
-- 
2.19.1.1215.g8438c0b245-goog

[PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Douglas Anderson]

In regulator_force_disable() there was a strange loop that looked like:

  while (rdev->open_count--)
    regulator_disable(rdev->supply);

I'm not totally sure what the goal was for this loop, but it seems
wrong to me.  If anything I think maybe we should have been looping
over our use_count, but even that might be a little strange.  For now
let's just remove the code and we can add something back in if someone
can explain what's expected.

Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 2eda87520832..963081aba17a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2738,10 +2738,6 @@ int regulator_force_disable(struct regulator *regulator)
 
 	regulator_unlock_dependent(rdev, &ww_ctx);
 
-	if (rdev->supply)
-		while (rdev->open_count--)
-			regulator_disable(rdev->supply);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regulator_force_disable);
-- 
2.19.1.1215.g8438c0b245-goog

Re: [PATCH 3/7] regulator: core: Don't double-disable supplies in regulator_disable_deferred()

[Dmitry Osipenko]

On 20.11.2018 3:26, Douglas Anderson wrote:
> In the commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for
> regulators locking") disabling of the supply was moved into
> _regulator_disable().  That means regulator_disable_work() shouldn't
> be disabling since that double-disables the supply.
> 
> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/regulator/core.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 0052bbc8c531..63a8af1e2256 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2703,16 +2703,6 @@ static void regulator_disable_work(struct work_struct *work)
>  		regulator_balance_voltage(rdev, PM_SUSPEND_ON);
>  
>  	regulator_unlock_dependent(rdev, &ww_ctx);
> -
> -	if (rdev->supply) {
> -		for (i = 0; i < count; i++) {
> -			ret = regulator_disable(rdev->supply);
> -			if (ret != 0) {
> -				rdev_err(rdev,
> -					 "Supply disable failed: %d\n", ret);
> -			}
> -		}
> -	}
>  }
>  
>  /**
> 

Good catch!

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Dmitry Osipenko]

On 20.11.2018 3:26, Douglas Anderson wrote:
> In regulator_force_disable() there was a strange loop that looked like:
> 
>   while (rdev->open_count--)
>     regulator_disable(rdev->supply);
> 
> I'm not totally sure what the goal was for this loop, but it seems
> wrong to me.  If anything I think maybe we should have been looping
> over our use_count, but even that might be a little strange.  For now
> let's just remove the code and we can add something back in if someone
> can explain what's expected.
> 
> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")

Seems this "fixes" tag is incorrect, isn't it? The "ww_mutex" patch didn't touch this code.


> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/regulator/core.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 2eda87520832..963081aba17a 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -2738,10 +2738,6 @@ int regulator_force_disable(struct regulator *regulator)
>  
>  	regulator_unlock_dependent(rdev, &ww_ctx);
>  
> -	if (rdev->supply)
> -		while (rdev->open_count--)
> -			regulator_disable(rdev->supply);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(regulator_force_disable);
> 

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Doug Anderson]

Hi,

On Mon, Nov 19, 2018 at 4:58 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> On 20.11.2018 3:26, Douglas Anderson wrote:
> > In regulator_force_disable() there was a strange loop that looked like:
> >
> >   while (rdev->open_count--)
> >     regulator_disable(rdev->supply);
> >
> > I'm not totally sure what the goal was for this loop, but it seems
> > wrong to me.  If anything I think maybe we should have been looping
> > over our use_count, but even that might be a little strange.  For now
> > let's just remove the code and we can add something back in if someone
> > can explain what's expected.
> >
> > Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
>
> Seems this "fixes" tag is incorrect, isn't it? The "ww_mutex" patch didn't touch this code.

Yes, I think you're right.  Originally I added it because I thought
that the "ww_mutex" patch should have touched this too, but I'm wrong.
I'll remove the "Fixes" tag if/when I send out v2.

-Doug

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Mark Brown]

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

On Mon, Nov 19, 2018 at 04:26:54PM -0800, Douglas Anderson wrote:
> In regulator_force_disable() there was a strange loop that looked like:
> 
>   while (rdev->open_count--)
>     regulator_disable(rdev->supply);
> 
> I'm not totally sure what the goal was for this loop, but it seems
> wrong to me.  If anything I think maybe we should have been looping
> over our use_count, but even that might be a little strange.  For now
> let's just remove the code and we can add something back in if someone
> can explain what's expected.

This should be using use_count, what that loop is doing is dropping all
the enables that the regulator being force disabled had propagated up
all the enables it passed up the chain of supplies.

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

Re: [PATCH 2/7] regulator: core: Don't assume always_on when is_enabled returns err

[Mark Brown]

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

On Mon, Nov 19, 2018 at 04:26:49PM -0800, Douglas Anderson wrote:

> At boot sometimes regulators (like qcom-rpmh-regulator) will return
> -EINVAL if we don't know the enable state of the regulator.  We
> shouldn't take this to mean that the regulator is an always-on
> regulator, but that was what was happening since "-EINVAL" is non-zero
> and a few places in the code were not properly checking for errors.

This still results in breakage - if the regulator fails to read when
we're initializing then we won't flag the regualtor as always_on, the
regulator would need to tell the framework when it becomes readable.

This hardware really is fragile :(

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

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Doug Anderson]

Hi,

On Tue, Nov 20, 2018 at 7:55 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 19, 2018 at 04:26:54PM -0800, Douglas Anderson wrote:
> > In regulator_force_disable() there was a strange loop that looked like:
> >
> >   while (rdev->open_count--)
> >     regulator_disable(rdev->supply);
> >
> > I'm not totally sure what the goal was for this loop, but it seems
> > wrong to me.  If anything I think maybe we should have been looping
> > over our use_count, but even that might be a little strange.  For now
> > let's just remove the code and we can add something back in if someone
> > can explain what's expected.
>
> This should be using use_count, what that loop is doing is dropping all
> the enables that the regulator being force disabled had propagated up
> all the enables it passed up the chain of supplies.

OK, that makes much more sense then.  I can adjust it to do that.
I'll wait to see what happens with the rest of the patches in this
series and then post up a v2 of this one.  The number of disables
needed depends on how many of the patches in my series you like.  ;-)

In general it's hard for me to reason about how the system in general
should behave after regulator_force_disable() is called.  Is it
basically expected that the system will panic soon after?
Specifically other consumers of the same regulator will think it's on
but it won't actually be on.  What should happen if one of those other
consumers calls disable/enable?  Should the regulator turn back on?
...or is the regulator permanently off until the system reboots?

-Doug

Re: [PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Mark Brown]

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

On Mon, Nov 19, 2018 at 04:26:52PM -0800, Douglas Anderson wrote:
> Now that consumers all keep track of their own enable count, let's add
> it into the regulator_summary.

This patch series contains a bunch of random stuff that's not
particularly related to each other or even dependent on each other -
it's better to just send unrelated things in unrelated threads, it
makes it clear what actual dependencies exist, makes the serieses that
are sent easier to understand and stops things getting blocked on
unrelated changes earlier in the series.

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

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Mark Brown]

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

On Tue, Nov 20, 2018 at 08:04:57AM -0800, Doug Anderson wrote:

> In general it's hard for me to reason about how the system in general
> should behave after regulator_force_disable() is called.  Is it
> basically expected that the system will panic soon after?
> Specifically other consumers of the same regulator will think it's on
> but it won't actually be on.  What should happen if one of those other
> consumers calls disable/enable?  Should the regulator turn back on?
> ...or is the regulator permanently off until the system reboots?

If something has been forced off the system is in very serious trouble
and had a cricial safety problem, though the fact that there's error
handling code that did the force disable might mean that there's some
ability to recover the situation - for example, this might be part of
thermal management or something like charger management.  Other drivers
do get notified if something gets forced off so a well written system
will ensure that other users of a regulator that can get force disabled
will have handling for this as should userspace.  We don't have any such
full systems in mainline, though - it is a really uncommon case.

The usage in the Adreno drivers just looks to be another completely
out of expectation regulator API usage in the QC code.  I do wish there
were a way to flag API calls as needing review :(

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

Re: [PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Doug Anderson]

Hi,

On Tue, Nov 20, 2018 at 8:10 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 19, 2018 at 04:26:52PM -0800, Douglas Anderson wrote:
> > Now that consumers all keep track of their own enable count, let's add
> > it into the regulator_summary.
>
> This patch series contains a bunch of random stuff that's not
> particularly related to each other or even dependent on each other -
> it's better to just send unrelated things in unrelated threads, it
> makes it clear what actual dependencies exist, makes the serieses that
> are sent easier to understand and stops things getting blocked on
> unrelated changes earlier in the series.

Sure.  You want me to repost things now or wait until you've had a
chance to look things over?

Certainly patch #1 is totally self-contained.
  regulator: core: Properly expose requested_microamps in sysfs

Patch #2 is self-contained but helps with confirming patches later in
the series:
  regulator: core: Don't assume always_on when is_enabled returns err

Patch #3: self-contained but helps with confirming patches later in the series:
  regulator: core: Don't double-disable supplies in regulator_disable_deferred()

Patches #4 and #5 are intimately tied.  As per discussion, I'll squash
together for v2.
  regulator: core: Only count load for enabled consumers
  regulator: core: add enable_count for consumers to debug fs

Patch #6: wasn't originally self-contained when I wrote it 2 days ago,
but after commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for
regulators locking") it is.
  regulator: core: Avoid propagating to supplies when possible

Patch #7: Now that I know you want to loop over use_count, depends on
patch #6 which changes how many times supplies are enabled.
   regulator: core: Remove loop disabling supplies in regulator_force_disable()


I don't want to send v2 of things while you're still reviewing.  If
you have a chance give me a ping when you're ready for me to send out
v2 of things.  ...otherwise I'll plan to send it out later today or
tomorrow morning.

-Doug

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Doug Anderson]

Hi,

On Tue, Nov 20, 2018 at 8:25 AM Mark Brown <broonie@kernel.org> wrote:
>
> If something has been forced off the system is in very serious trouble
> and had a cricial safety problem, though the fact that there's error
> handling code that did the force disable might mean that there's some
> ability to recover the situation - for example, this might be part of
> thermal management or something like charger management.  Other drivers
> do get notified if something gets forced off so a well written system
> will ensure that other users of a regulator that can get force disabled
> will have handling for this as should userspace.  We don't have any such
> full systems in mainline, though - it is a really uncommon case.

OK.  I guess for now I'll just change this to disable the parent
supply as many times as this individual consumer enabled it and call
it good enough?  It can be for someone else to figure out how to make
it really usable for system recovery.

...I originally only ended up here since I was auditing all this code
while moving the enable count to the individual consumers...


> The usage in the Adreno drivers just looks to be another completely
> out of expectation regulator API usage in the QC code.

Yeah, looks like it.


> I do wish there
> were a way to flag API calls as needing review :(

Would it be worth adding a WARN_ON(1) splat here or at least a
"dev_warn" so people knew it wasn't really an API for general use?
...or is that going overboard?

-Doug

Re: [PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Mark Brown]

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

On Tue, Nov 20, 2018 at 08:52:14AM -0800, Doug Anderson wrote:

> I don't want to send v2 of things while you're still reviewing.  If
> you have a chance give me a ping when you're ready for me to send out
> v2 of things.  ...otherwise I'll plan to send it out later today or
> tomorrow morning.

I'm pretty much done for today so probably easier to just resend now,
I've got other things in my queue to review anyway!

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

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Mark Brown]

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

On Tue, Nov 20, 2018 at 08:57:32AM -0800, Doug Anderson wrote:
> On Tue, Nov 20, 2018 at 8:25 AM Mark Brown <broonie@kernel.org> wrote:

> > I do wish there
> > were a way to flag API calls as needing review :(

> Would it be worth adding a WARN_ON(1) splat here or at least a
> "dev_warn" so people knew it wasn't really an API for general use?
> ...or is that going overboard?

That might work ehre but it's not just this call, and the trouble is
that there are legitimate use cases and we can't exactly have a magic
"no, I know what I'm doing" flag that things need to pass or anything.

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

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Mark Brown]

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

On Tue, Nov 20, 2018 at 08:57:32AM -0800, Doug Anderson wrote:

> OK.  I guess for now I'll just change this to disable the parent
> supply as many times as this individual consumer enabled it and call
> it good enough?  It can be for someone else to figure out how to make
> it really usable for system recovery.

Sorry, missed the question mark here - yes.

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

Re: [PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Doug Anderson]

Hi,

On Tue, Nov 20, 2018 at 8:58 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Nov 20, 2018 at 08:52:14AM -0800, Doug Anderson wrote:
>
> > I don't want to send v2 of things while you're still reviewing.  If
> > you have a chance give me a ping when you're ready for me to send out
> > v2 of things.  ...otherwise I'll plan to send it out later today or
> > tomorrow morning.
>
> I'm pretty much done for today so probably easier to just resend now,
> I've got other things in my queue to review anyway!

OK, got it.  NOTE: I think patch #3 ("regulator: core: Don't
double-disable supplies in regulator_disable_deferred()") needs no
resend and is ready to go if you think it's OK.  I won't plan to
repost that one.

-Doug

Re: [PATCH 7/7] regulator: core: Remove loop disabling supplies in regulator_force_disable()

[Doug Anderson]

Hi,

On Mon, Nov 19, 2018 at 4:27 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> In regulator_force_disable() there was a strange loop that looked like:
>
>   while (rdev->open_count--)
>     regulator_disable(rdev->supply);
>
> I'm not totally sure what the goal was for this loop, but it seems
> wrong to me.  If anything I think maybe we should have been looping
> over our use_count, but even that might be a little strange.  For now
> let's just remove the code and we can add something back in if someone
> can explain what's expected.
>
> Fixes: f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators locking")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/regulator/core.c | 4 ----
>  1 file changed, 4 deletions(-)

For breadcrumbs: the next version of this patch is now squashed into
("[PATCH v2 2/2] regulator: core: Avoid propagating to supplies when
possible") AKA <http://lkml.kernel.org/r/20181120175255.227783-2-dianders@chromium.org>

-Doug

Re: [PATCH 5/7] regulator: core: add enable_count for consumers to debug fs

[Doug Anderson]

Hi,

On Mon, Nov 19, 2018 at 4:27 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Now that consumers all keep track of their own enable count, let's add
> it into the regulator_summary.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/regulator/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

For breadcrumbs, this patch is now squashed into ("[PATCH v2 1/2]
regulator: core: Only count load for enabled consumers") AKA
<https://lkml.kernel.org/r/20181120175255.227783-1-dianders@chromium.org>

-Doug

Re: [PATCH 2/7] regulator: core: Don't assume always_on when is_enabled returns err

[Doug Anderson]

Hi,

On Tue, Nov 20, 2018 at 8:00 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Nov 19, 2018 at 04:26:49PM -0800, Douglas Anderson wrote:
>
> > At boot sometimes regulators (like qcom-rpmh-regulator) will return
> > -EINVAL if we don't know the enable state of the regulator.  We
> > shouldn't take this to mean that the regulator is an always-on
> > regulator, but that was what was happening since "-EINVAL" is non-zero
> > and a few places in the code were not properly checking for errors.
>
> This still results in breakage - if the regulator fails to read when
> we're initializing then we won't flag the regualtor as always_on, the
> regulator would need to tell the framework when it becomes readable.

Hrm.  Are you sure it's broken?  I'll admit that I'm a little fuzzy
about why a regulator that happens to be enabled when its supplies get
resolved should be considered always-on.  Can you give me a hint about
why it works that way?

My best guess was that the code was assuming that when we were
resolving supplies it was super early and the only way the regulator
could be enabled at that point is if either:
A) the regulator ops have no is_enabled()
B) we called _regulator_do_enable() in set_machine_constraints()

If that's true then my patch definitely helps things.  Case A) isn't
an issue for qcom-rpmh-regulator.  For case B) once we see the first
call to set the enable state of the regulator then we'll definitely
return 1 from is_enabled.  That means we'll be able to tell the
difference between a regulator that should be considered always-on and
one that can't.

Did that make sense, or am I spouting nonsense again?


> This hardware really is fragile :(

No kidding.

-Doug