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: <20210929144451.113334-1-ulf.hansson@linaro.org> <20210929144451.113334-3-ulf.hansson@linaro.org>
In-Reply-To: <20210929144451.113334-3-ulf.hansson@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 20 Oct 2021 20:18:11 +0200
Message-ID: <CAJZ5v0hgdQeJ+6mLMLQcvnM_+EiyDBERj54aT2cL=HiTO9nMNQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] PM: sleep: Fix runtime PM based cpuidle support
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Maulik Shah <mkshah@codeaurora.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Len Brown <len.brown@intel.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 29, 2021 at 4:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> In the cpuidle-psci case, runtime PM in combination with the generic PM
> domain (genpd), may be used when entering/exiting an idlestate. More
> precisely, genpd relies on runtime PM to be enabled for the attached device
> (in this case it belongs to a CPU), to properly manage the reference
> counting of its PM domain.
>
> This works fine most of the time, but during system suspend in the
> dpm_suspend_late() phase, the PM core disables runtime PM for all devices.
> Beyond this point and until runtime PM becomes re-enabled in the
> dpm_resume_early() phase, calls to pm_runtime_get|put*() will fail.
>
> To make sure the reference counting in genpd becomes correct, we need to
> prevent cpuidle-psci from using runtime PM when it has been disabled for
> the device. Therefore, let's move the call to cpuidle_pause() from
> dpm_suspend_noirq() to dpm_suspend_late() - and cpuidle_resume() from
> dpm_resume_noirq() into dpm_resume_early().
>
> Diagnosed-by: Maulik Shah <mkshah@codeaurora.org>
> Suggested-by: Maulik Shah <mkshah@codeaurora.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index cbea78e79f3d..1c753b651272 100644
> --- a/drivers/base/power/main.c
> +++ b/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();
>  }
>
>  /**
> @@ -870,6 +868,7 @@ void dpm_resume_early(pm_message_t state)
>         }
>         mutex_unlock(&dpm_list_mtx);
>         async_synchronize_full();
> +       cpuidle_resume();
>         dpm_show_time(starttime, state, 0, "early");
>         trace_suspend_resume(TPS("dpm_resume_early"), state.event, false);
>  }
> @@ -1336,8 +1335,6 @@ int dpm_suspend_noirq(pm_message_t state)
>  {
>         int ret;
>
> -       cpuidle_pause();
> -
>         device_wakeup_arm_wake_irqs();
>         suspend_device_irqs();
>
> @@ -1467,6 +1464,7 @@ int dpm_suspend_late(pm_message_t state)
>         int error = 0;
>
>         trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
> +       cpuidle_pause();
>         mutex_lock(&dpm_list_mtx);
>         pm_transition = state;
>         async_error = 0;
> --

Well, this is somewhat heavy-handed and it affects even the systems
that don't really need to pause cpuidle at all in the suspend path.

Also, IIUC you don't need to pause cpuidle completely, but make it
temporarily avoid idle states potentially affected by this issue.  An
additional CPUIDLE_STATE_DISABLED_ flag could be used for that I
suppose and it could be set via cpuidle_suspend() called from the core
next to cpufreq_suspend().

The other guys who rely on the cpuidle pausing today could be switched
over to this new mechanism later and it would be possible to get rid
of the pausing from the system suspend path completely.