[PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16

[PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16

[Marek Szyprowski]

Hi All,

While working on suspend/resume support for Samsung Exynos5433-based TM2
board (arch/arm64/boot/dts/exynos/exynos5433-tm2.dts), I've noticed that
v4.16 kernel release introduced some serious problems with regulator
configuration in system suspend state. Further investigation revealed
that the following 2 commits are responsible for my issues:

1. f7efad10b5c4: regulator: add PM suspend and resume hooks
2. 72069f9957a1: regulator: leave one item to record whether regulator is enabled

I've been really surprised that no-one noticed those issues for almost
4 kernel releases. Please review my fixes and apply to v4.19-rcX if
possible.

Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (2):
  regulator: Fix useless O^2 complexity in suspend/resume
  regulator: Fix 'do-nothing' value for regulators without suspend state

 drivers/regulator/core.c          | 41 +++++++++----------------------
 drivers/regulator/of_regulator.c  |  2 --
 include/linux/regulator/machine.h |  6 ++---
 3 files changed, 15 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume

[Marek Szyprowski]

regulator_pm_ops with regulator_suspend and regulator_resume functions are
assigned to every regulator device registered in the system, so there is no
need to iterate over all again in them. Replace class_for_each_device()
construction with direct operation on the rdev embedded in the given
regulator device. This saves a lots of useless operations in suspend and
resume paths.

Fixes: f7efad10b5c4: regulator: add PM suspend and resume hooks
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index bb1324f93143..71ba040c7c5b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4455,19 +4455,6 @@ void regulator_unregister(struct regulator_dev *rdev)
 EXPORT_SYMBOL_GPL(regulator_unregister);
 
 #ifdef CONFIG_SUSPEND
-static int _regulator_suspend(struct device *dev, void *data)
-{
-	struct regulator_dev *rdev = dev_to_rdev(dev);
-	suspend_state_t *state = data;
-	int ret;
-
-	regulator_lock(rdev);
-	ret = suspend_set_state(rdev, *state);
-	regulator_unlock(rdev);
-
-	return ret;
-}
-
 /**
  * regulator_suspend - prepare regulators for system wide suspend
  * @state: system suspend state
@@ -4476,20 +4463,25 @@ static int _regulator_suspend(struct device *dev, void *data)
  */
 static int regulator_suspend(struct device *dev)
 {
+	struct regulator_dev *rdev = dev_to_rdev(dev);
 	suspend_state_t state = pm_suspend_target_state;
+	int ret;
+
+	regulator_lock(rdev);
+	ret = suspend_set_state(rdev, state);
+	regulator_unlock(rdev);
 
-	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_suspend);
+	return ret;
 }
 
-static int _regulator_resume(struct device *dev, void *data)
+static int regulator_resume(struct device *dev)
 {
-	int ret = 0;
+	suspend_state_t state = pm_suspend_target_state;
 	struct regulator_dev *rdev = dev_to_rdev(dev);
-	suspend_state_t *state = data;
 	struct regulator_state *rstate;
+	int ret = 0;
 
-	rstate = regulator_get_suspend_state(rdev, *state);
+	rstate = regulator_get_suspend_state(rdev, state);
 	if (rstate == NULL)
 		return 0;
 
@@ -4504,15 +4496,6 @@ static int _regulator_resume(struct device *dev, void *data)
 
 	return ret;
 }
-
-static int regulator_resume(struct device *dev)
-{
-	suspend_state_t state = pm_suspend_target_state;
-
-	return class_for_each_device(&regulator_class, NULL, &state,
-				     _regulator_resume);
-}
-
 #else /* !CONFIG_SUSPEND */
 
 #define regulator_suspend	NULL
-- 
2.17.1

[PATCH 2/2] regulator: Fix 'do-nothing' value for regulators without suspend state

[Marek Szyprowski]

Some regulators don't have all states defined and in such cases regulator
core should not assume anything. However in current implementation
of of_get_regulation_constraints() DO_NOTHING_IN_SUSPEND enable value was
set only for regulators which had suspend node defined, otherwise the
default 0 value was used, what means DISABLE_IN_SUSPEND. This lead to
broken system suspend/resume on boards, which had simple regulator
constraints definition (without suspend state nodes).

To avoid further mismatches between the default and uninitialized values
of the suspend enabled/disabled states, change the values of the them,
so default '0' means DO_NOTHING_IN_SUSPEND.

Fixes: 72069f9957a1: regulator: leave one item to record whether regulator is enabled
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/regulator/core.c          | 2 +-
 drivers/regulator/of_regulator.c  | 2 --
 include/linux/regulator/machine.h | 6 +++---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 71ba040c7c5b..d1485ed98509 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3161,7 +3161,7 @@ static inline int regulator_suspend_toggle(struct regulator_dev *rdev,
 	if (!rstate->changeable)
 		return -EPERM;
 
-	rstate->enabled = en;
+	rstate->enabled = (en) ? ENABLE_IN_SUSPEND : DISABLE_IN_SUSPEND;
 
 	return 0;
 }
diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index 638f17d4c848..210fc20f7de7 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -213,8 +213,6 @@ static void of_get_regulation_constraints(struct device_node *np,
 		else if (of_property_read_bool(suspend_np,
 					"regulator-off-in-suspend"))
 			suspend_state->enabled = DISABLE_IN_SUSPEND;
-		else
-			suspend_state->enabled = DO_NOTHING_IN_SUSPEND;
 
 		if (!of_property_read_u32(np, "regulator-suspend-min-microvolt",
 					  &pval))
diff --git a/include/linux/regulator/machine.h b/include/linux/regulator/machine.h
index 3468703d663a..a459a5e973a7 100644
--- a/include/linux/regulator/machine.h
+++ b/include/linux/regulator/machine.h
@@ -48,9 +48,9 @@ struct regulator;
  * DISABLE_IN_SUSPEND	- turn off regulator in suspend states
  * ENABLE_IN_SUSPEND	- keep regulator on in suspend states
  */
-#define DO_NOTHING_IN_SUSPEND	(-1)
-#define DISABLE_IN_SUSPEND	0
-#define ENABLE_IN_SUSPEND	1
+#define DO_NOTHING_IN_SUSPEND	0
+#define DISABLE_IN_SUSPEND	1
+#define ENABLE_IN_SUSPEND	2
 
 /* Regulator active discharge flags */
 enum regulator_active_discharge {
-- 
2.17.1

Re: [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume

[Mark Brown]

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

On Mon, Sep 03, 2018 at 04:49:36PM +0200, Marek Szyprowski wrote:
> regulator_pm_ops with regulator_suspend and regulator_resume functions are
> assigned to every regulator device registered in the system, so there is no
> need to iterate over all again in them. Replace class_for_each_device()
> construction with direct operation on the rdev embedded in the given
> regulator device. This saves a lots of useless operations in suspend and
> resume paths.

This would've been better as the second patch since it's an optimization
and not so urgent for stable.

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

Re: [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16

[Mark Brown]

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

On Mon, Sep 03, 2018 at 04:49:35PM +0200, Marek Szyprowski wrote:

> I've been really surprised that no-one noticed those issues for almost
> 4 kernel releases. Please review my fixes and apply to v4.19-rcX if
> possible.

I suspect this is down to relatively few systems having devices with
suspend operations, and fewer actually ever suspending in use.

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

Re: [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume

[Marek Szyprowski]

Hi Mark,

On 2018-09-03 17:09, Mark Brown wrote:
> On Mon, Sep 03, 2018 at 04:49:36PM +0200, Marek Szyprowski wrote:
>> regulator_pm_ops with regulator_suspend and regulator_resume functions are
>> assigned to every regulator device registered in the system, so there is no
>> need to iterate over all again in them. Replace class_for_each_device()
>> construction with direct operation on the rdev embedded in the given
>> regulator device. This saves a lots of useless operations in suspend and
>> resume paths.
> This would've been better as the second patch since it's an optimization
> and not so urgent for stable.

Frankly, the slow down caused by this O^2 in case of 40 regulators in the
system (not that unusual in nowadays mobiles) is noticeable and quite
annoying. IMHO this is still a good candidate for stable.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland