[PATCH] regulator: core: Fix s2idle

[PATCH] regulator: core: Fix s2idle

[Geert Uytterhoeven]

Systems using regulators can no longer be frozen:

$ echo 0 > /sys/module/printk/parameters/console_suspend
$ echo freeze > /sys/power/state

    PM: suspend entry (s2idle)
    PM: Syncing filesystems ... done.
    Freezing user space processes ... (elapsed 0.011 seconds) done.
    OOM killer disabled.
    Freezing remaining freezable tasks ... (elapsed 0.009 seconds) done.
    dpm_run_callback(): regulator_suspend_late+0x0/0x44 returns -22
    PM: Device regulator.7 failed to suspend late: error -22
    PM: late suspend of devices failed

Fix this by adding the missing case for s2idle.  Treat s2idle the same
as standby, as there are no separate constraints defined for s2idle.

Fixes: f7efad10b5c49289 ("regulator: add PM suspend and resume hooks")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
regulator_check_states() also considers PM_SUSPEND_TO_IDLE an invalid
state:

    /* return 0 if the state is valid */
    static int regulator_check_states(suspend_state_t state)
    {
	    return (state > PM_SUSPEND_MAX || state == PM_SUSPEND_TO_IDLE);
    }

But I have no idea what impact it has.  So far it doesn't seem to hurt,
though.

 drivers/regulator/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 42681c10cbe4fa9f..5f411b0cca9aa838 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -333,6 +333,7 @@ regulator_get_suspend_state(struct regulator_dev *rdev, suspend_state_t state)
 		return NULL;
 
 	switch (state) {
+	case PM_SUSPEND_TO_IDLE:
 	case PM_SUSPEND_STANDBY:
 		return &rdev->constraints->state_standby;
 	case PM_SUSPEND_MEM:
-- 
2.7.4

Re: [PATCH] regulator: core: Fix s2idle

[Mark Brown]

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

On Tue, Jan 30, 2018 at 11:32:26AM +0100, Geert Uytterhoeven wrote:

> But I have no idea what impact it has.  So far it doesn't seem to hurt,
> though.

I suspect that instead what we should be doing is changing the
set_suspend_state() check to the below - if we don't have any
constraints or there's otherwise nothing to do just let the suspend
proceed.

Can you give this a spin and confirm if it fixes things?

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 42681c10cbe4..dd4708c58480 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -758,7 +758,7 @@ static int suspend_set_state(struct regulator_dev *rdev,
 
 	rstate = regulator_get_suspend_state(rdev, state);
 	if (rstate == NULL)
-		return -EINVAL;
+		return 0;
 
 	/* If we have no suspend mode configration don't set anything;
 	 * only warn if the driver implements set_suspend_voltage or

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

Re: [PATCH] regulator: core: Fix s2idle

[Geert Uytterhoeven]

Hi Mark,

On Tue, Jan 30, 2018 at 1:03 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jan 30, 2018 at 11:32:26AM +0100, Geert Uytterhoeven wrote:
>> But I have no idea what impact it has.  So far it doesn't seem to hurt,
>> though.
>
> I suspect that instead what we should be doing is changing the
> set_suspend_state() check to the below - if we don't have any
> constraints or there's otherwise nothing to do just let the suspend
> proceed.
>
> Can you give this a spin and confirm if it fixes things?

Sure. Works, too.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 42681c10cbe4..dd4708c58480 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -758,7 +758,7 @@ static int suspend_set_state(struct regulator_dev *rdev,
>
>         rstate = regulator_get_suspend_state(rdev, state);
>         if (rstate == NULL)
> -               return -EINVAL;
> +               return 0;
>
>         /* If we have no suspend mode configration don't set anything;
>          * only warn if the driver implements set_suspend_voltage or

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Applied "regulator: Fix suspend to idle" to the regulator tree

[Mark Brown]

The patch

   regulator: Fix suspend to idle

has been applied to the regulator tree at

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

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

>From 57a0dd187956ea04870f4bbbf25a63c425ee7cad Mon Sep 17 00:00:00 2001
From: Mark Brown <broonie@kernel.org>
Date: Tue, 30 Jan 2018 12:16:31 +0000
Subject: [PATCH] regulator: Fix suspend to idle

When suspending to idle with the new suspend mode configuration support
we go through the suspend callbacks with a state of PM_SUSPEND_TO_IDLE
which we don't have regulator constraints for, causing an error.  Avoid
this and similar errors by treating missing constraints as a noop.

Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 42681c10cbe4..dd4708c58480 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -758,7 +758,7 @@ static int suspend_set_state(struct regulator_dev *rdev,
 
 	rstate = regulator_get_suspend_state(rdev, state);
 	if (rstate == NULL)
-		return -EINVAL;
+		return 0;
 
 	/* If we have no suspend mode configration don't set anything;
 	 * only warn if the driver implements set_suspend_voltage or
-- 
2.15.1