* [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume
2018-09-03 14:49 ` [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16 Marek Szyprowski
@ 2018-09-03 14:49 ` Marek Szyprowski
2018-09-03 15:09 ` Mark Brown
2018-09-03 14:49 ` [PATCH 2/2] regulator: Fix 'do-nothing' value for regulators without suspend state Marek Szyprowski
2018-09-03 15:16 ` [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16 Mark Brown
2 siblings, 1 reply; 6+ messages in thread
From: Marek Szyprowski @ 2018-09-03 14:49 UTC (permalink / raw)
To: linux-kernel, Mark Brown, Chunyan Zhang
Cc: Marek Szyprowski, Liam Girdwood, Bartlomiej Zolnierkiewicz,
linux-samsung-soc
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(®ulator_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(®ulator_class, NULL, &state,
- _regulator_resume);
-}
-
#else /* !CONFIG_SUSPEND */
#define regulator_suspend NULL
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume
2018-09-03 14:49 ` [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume Marek Szyprowski
@ 2018-09-03 15:09 ` Mark Brown
2018-09-04 8:39 ` Marek Szyprowski
0 siblings, 1 reply; 6+ messages in thread
From: Mark Brown @ 2018-09-03 15:09 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, Chunyan Zhang, Liam Girdwood,
Bartlomiej Zolnierkiewicz, linux-samsung-soc
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume
2018-09-03 15:09 ` Mark Brown
@ 2018-09-04 8:39 ` Marek Szyprowski
0 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2018-09-04 8:39 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Chunyan Zhang, Liam Girdwood,
Bartlomiej Zolnierkiewicz, linux-samsung-soc
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] regulator: Fix 'do-nothing' value for regulators without suspend state
2018-09-03 14:49 ` [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16 Marek Szyprowski
2018-09-03 14:49 ` [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume Marek Szyprowski
@ 2018-09-03 14:49 ` Marek Szyprowski
2018-09-03 15:16 ` [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16 Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Marek Szyprowski @ 2018-09-03 14:49 UTC (permalink / raw)
To: linux-kernel, Mark Brown, Chunyan Zhang
Cc: Marek Szyprowski, Liam Girdwood, Bartlomiej Zolnierkiewicz,
linux-samsung-soc
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16
2018-09-03 14:49 ` [PATCH 0/2] regualtors: Fix suspend/resume issues since v4.16 Marek Szyprowski
2018-09-03 14:49 ` [PATCH 1/2] regulator: Fix useless O^2 complexity in suspend/resume Marek Szyprowski
2018-09-03 14:49 ` [PATCH 2/2] regulator: Fix 'do-nothing' value for regulators without suspend state Marek Szyprowski
@ 2018-09-03 15:16 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2018-09-03 15:16 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-kernel, Chunyan Zhang, Liam Girdwood,
Bartlomiej Zolnierkiewicz, linux-samsung-soc
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread