Re: [PATCH] cpuidle: psci: Fixup execution order when entering a domain idle state

From: Sudeep Holla <sudeep.holla@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>,
	linux-pm@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Benjamin Gaignard <benjamin.gaignard@st.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] cpuidle: psci: Fixup execution order when entering a domain idle state
Date: Thu, 14 May 2020 15:20:15 +0100	[thread overview]
Message-ID: <20200514142015.GA23401@bogus> (raw)
In-Reply-To: <20200511133346.21706-1-ulf.hansson@linaro.org>

On Mon, May 11, 2020 at 03:33:46PM +0200, Ulf Hansson wrote:
> Moving forward, platforms are going to need to execute specific "last-man"
> operations before a domain idle state can be entered.

I need to dig the thread details, but I remember commenting on one of
the similar discussion. It was something to do with voting which wasn't
necessary at all. I am interested in the details here.

> In one way or the other, these operations needs to be triggered while
> walking the  hierarchical topology via runtime PM and genpd, as it's at that
> point the last-man becomes known.
>
> Moreover, executing last-man operations needs to be done after the CPU PM
> notifications are sent through cpu_pm_enter(), as otherwise it's likely
> that some notifications would fail. Therefore, let's re-order the sequence
> in psci_enter_domain_idle_state(), so cpu_pm_enter() gets called prior
> pm_runtime_put_sync().
>

More details on why notifications fail ?

> Fixes: ce85aef570df ("cpuidle: psci: Manage runtime PM in the idle path")
> Reported-by: Lina Iyer <ilina@codeaurora.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/cpuidle/cpuidle-psci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index bae9140a65a5..d0fb585073c6 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -58,6 +58,10 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>  	u32 state;
>  	int ret;
>  
> +	ret = cpu_pm_enter();
> +	if (ret)
> +		return -1;
> +

This change is ignoring the retention case psci_enter_state handles and
this may affect performance by doing unnecessary save/restore.

-- 
Regards,
Sudeep