Re: [PATCH 1/2] cpuidle: Avoid calls to cpuidle_resume|pause() for s2idle

Ulf Hansson
2021-10-11T10:05:26Z

On Sat, 9 Oct 2021 at 17:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, September 29, 2021 4:44:50 PM CEST Ulf Hansson wrote:
> > In s2idle_enter(), cpuidle_resume|pause() are invoked to re-allow calls to
> > the cpuidle callbacks during s2idle operations. This is needed because
> > cpuidle is paused in-between in dpm_suspend_noirq() and dpm_resume_noirq().
>
> Well, in fact, doing that last thing for s2idle is pointless, because cpuidle
> is going to be resumed eventually anyway in that case and the breakage expected
> to be prevented by the pausing will still occur.
>
> So I would rather do something like the patch below (untested).

Hi Rafael,

From a standalone change point of view, what you suggest seems reasonable to me.

However, the main issue I am really trying to fix in this series is
being done in patch2/2. And unfortunately, the below change doesn't
really fit with what I suggest in patch2/2. Can you please have a look
at patch2 as well?

If you think it may be better, I squash the two patches?

Kind regards
Uffe

>
> ---
> drivers/base/power/main.c | 11 ++++++-----
> kernel/power/suspend.c | 8 ++++++--
> 2 files changed, 12 insertions(+), 7 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -747,8 +747,6 @@ void dpm_resume_noirq(pm_message_t state
>
> resume_device_irqs();
> device_wakeup_disarm_wake_irqs();
> -
> - cpuidle_resume();
> }
>
> /**
> @@ -881,6 +879,7 @@ void dpm_resume_early(pm_message_t state
> void dpm_resume_start(pm_message_t state)
> {
> dpm_resume_noirq(state);
> + cpuidle_resume();
> dpm_resume_early(state);
> }
> EXPORT_SYMBOL_GPL(dpm_resume_start);
> @@ -1336,8 +1335,6 @@ int dpm_suspend_noirq(pm_message_t state
> {
> int ret;
>
> - cpuidle_pause();
> -
> device_wakeup_arm_wake_irqs();
> suspend_device_irqs();
>
> @@ -1521,9 +1518,13 @@ int dpm_suspend_end(pm_message_t state)
> if (error)
> goto out;
>
> + cpuidle_pause();
> +
> error = dpm_suspend_noirq(state);
> - if (error)
> + if (error) {
> + cpuidle_resume();
> dpm_resume_early(resume_event(state));
> + }
>
> out:
> dpm_show_time(starttime, state, error, "end");
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -97,7 +97,6 @@ static void s2idle_enter(void)
> raw_spin_unlock_irq(&s2idle_lock);
>
> cpus_read_lock();
> - cpuidle_resume();
>
> /* Push all the CPUs into the idle loop. */
> wake_up_all_idle_cpus();
> @@ -105,7 +104,6 @@ static void s2idle_enter(void)
> swait_event_exclusive(s2idle_wait_head,
> s2idle_state == S2IDLE_STATE_WAKE);
>
> - cpuidle_pause();
> cpus_read_unlock();
>
> raw_spin_lock_irq(&s2idle_lock);
> @@ -405,6 +403,9 @@ static int suspend_enter(suspend_state_t
> if (error)
> goto Devices_early_resume;
>
> + if (state != PM_SUSPEND_TO_IDLE)
> + cpuidle_pause();
> +
> error = dpm_suspend_noirq(PMSG_SUSPEND);
> if (error) {
> pr_err("noirq suspend of devices failed\n");
> @@ -459,6 +460,9 @@ static int suspend_enter(suspend_state_t
> dpm_resume_noirq(PMSG_RESUME);
>
> Platform_early_resume:
> + if (state != PM_SUSPEND_TO_IDLE)
> + cpuidle_resume();
> +
> platform_resume_early(state);
>
> Devices_early_resume:
>