Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <2230995.ElGaqSPkdT@kreacher> <1813297.tdWV9SEqCh@kreacher>
In-Reply-To: <1813297.tdWV9SEqCh@kreacher>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Mon, 25 Oct 2021 16:23:49 +0200
Message-ID: <CAPDyKFpGm-f79bmdpipmTB_ug1XGPcQEihVqHHcmSK-EprW5ww@mail.gmail.com>
Subject: Re: [PATCH 1/2] PM: suspend: Do not pause cpuidle in the
 suspend-to-idle path
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc:     Linux PM <linux-pm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 22 Oct 2021 at 18:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> It is pointless to pause cpuidle in the suspend-to-idle path,
> because it is going to be resumed in the same path later and
> pausing it does not serve any particular purpose in that case.
>
> Rework the code to avoid doing that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Tested-by: Ulf Hansson <ulf.hansson@linaro.org>

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:
>
>
>
>
>
>
>