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

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

[Ulf Hansson]

Moving forward, platforms are going to need to execute specific "last-man"
operations before a domain idle state can be entered. 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().

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;
+
 	/* Do runtime PM to manage a hierarchical CPU toplogy. */
 	pm_runtime_put_sync_suspend(pd_dev);
 
@@ -65,10 +69,12 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
 	if (!state)
 		state = states[idx];
 
-	ret = psci_enter_state(idx, state);
+	ret = psci_cpu_suspend_enter(state) ? -1 : idx;
 
 	pm_runtime_get_sync(pd_dev);
 
+	cpu_pm_exit();
+
 	/* Clear the domain state to start fresh when back from idle. */
 	psci_set_domain_state(0);
 	return ret;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafael J. Wysocki]

On Mon, May 11, 2020 at 3:33 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Moving forward, platforms are going to need to execute specific "last-man"
> operations before a domain idle state can be entered. 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().
>
> 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>

I can pick this up provided an ACK from a maintainer of this driver.

> ---
>  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;
> +
>         /* Do runtime PM to manage a hierarchical CPU toplogy. */
>         pm_runtime_put_sync_suspend(pd_dev);
>
> @@ -65,10 +69,12 @@ static int psci_enter_domain_idle_state(struct cpuidle_device *dev,
>         if (!state)
>                 state = states[idx];
>
> -       ret = psci_enter_state(idx, state);
> +       ret = psci_cpu_suspend_enter(state) ? -1 : idx;
>
>         pm_runtime_get_sync(pd_dev);
>
> +       cpu_pm_exit();
> +
>         /* Clear the domain state to start fresh when back from idle. */
>         psci_set_domain_state(0);
>         return ret;
> --
> 2.20.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ulf Hansson]

On Thu, 14 May 2020 at 16:20, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> 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 ?

Well, at this moment this is more of a hypothetical issue as there is
no last-man notification sent/used yet.

However, typically we would call cpu_cluster_pm_enter() in the path as
when the psci_pd_power_off() is called (for psci PM domains), when a
valid domain state has been found.

This means, we would violate the cpu_cluster_pm_enter() API, as it's
clearly stated in its corresponding function header, that
cpu_pm_enter() needs to be called first (and that's also how others
are currently using it).

Note that, I am currently exploring whether we shall call
cpu_cluster_pm_enter|exit() at all, or whether we can use some other
new genpd mechanism to accomplish the similar thing.

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

This was already the case before. CPU_PM_CPU_IDLE_ENTER_PARAM() ends
up always setting "is_retention" to 0, when __CPU_PM_CPU_IDLE_ENTER()
is called.

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Thu, May 14, 2020 at 05:41:37PM +0200, Ulf Hansson wrote:
> On Thu, 14 May 2020 at 16:20, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > 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 ?
>
> Well, at this moment this is more of a hypothetical issue as there is
> no last-man notification sent/used yet.
>

Ah OK.

> However, typically we would call cpu_cluster_pm_enter() in the path as
> when the psci_pd_power_off() is called (for psci PM domains), when a
> valid domain state has been found.
>

Please don't use cpu_cluster_pm_enter. I thought the whole idea of genpd
will remove the notion of cluster and we will never need to use
cpu_cluster_pm_*

> This means, we would violate the cpu_cluster_pm_enter() API, as it's
> clearly stated in its corresponding function header, that
> cpu_pm_enter() needs to be called first (and that's also how others
> are currently using it).
>

Ah so, it was just for use of cpu_cluster_pm_* which I would like to
avoid. That was one of the motivation for using genpd I thought.

> Note that, I am currently exploring whether we shall call
> cpu_cluster_pm_enter|exit() at all, or whether we can use some other
> new genpd mechanism to accomplish the similar thing.
>

Good, you do think so.

> >
> > > 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.
>
> This was already the case before. CPU_PM_CPU_IDLE_ENTER_PARAM() ends
> up always setting "is_retention" to 0, when __CPU_PM_CPU_IDLE_ENTER()
> is called.
>

Ah OK, I recall now, sorry for the noise.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ulf Hansson]

On Thu, 14 May 2020 at 19:28, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, May 14, 2020 at 05:41:37PM +0200, Ulf Hansson wrote:
> > On Thu, 14 May 2020 at 16:20, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > 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 ?
> >
> > Well, at this moment this is more of a hypothetical issue as there is
> > no last-man notification sent/used yet.
> >
>
> Ah OK.
>
> > However, typically we would call cpu_cluster_pm_enter() in the path as
> > when the psci_pd_power_off() is called (for psci PM domains), when a
> > valid domain state has been found.
> >
>
> Please don't use cpu_cluster_pm_enter. I thought the whole idea of genpd
> will remove the notion of cluster and we will never need to use
> cpu_cluster_pm_*
>
> > This means, we would violate the cpu_cluster_pm_enter() API, as it's
> > clearly stated in its corresponding function header, that
> > cpu_pm_enter() needs to be called first (and that's also how others
> > are currently using it).
> >
>
> Ah so, it was just for use of cpu_cluster_pm_* which I would like to
> avoid. That was one of the motivation for using genpd I thought.
>
> > Note that, I am currently exploring whether we shall call
> > cpu_cluster_pm_enter|exit() at all, or whether we can use some other
> > new genpd mechanism to accomplish the similar thing.
> >
>
> Good, you do think so.

Okay, let's see where things go. I will keep you posted.

> > >
> > > > 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.
> >
> > This was already the case before. CPU_PM_CPU_IDLE_ENTER_PARAM() ends
> > up always setting "is_retention" to 0, when __CPU_PM_CPU_IDLE_ENTER()
> > is called.
> >
>
> Ah OK, I recall now, sorry for the noise.

No worries, thanks for reviewing.

That said, are you fine with Rafel queuing this then?

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Thu, May 14, 2020 at 09:11:50PM +0200, Ulf Hansson wrote:
>
> No worries, thanks for reviewing.
>

You are welcome.

> That said, are you fine with Rafel queuing this then?
>

I am fine with that. However I told if you need fixes tags as there are
no users of the notification yet in the kernel. Though this is trivial,
but do we need this backported to stable kernel. I don't have strong
opinion and leave it to you and Rafael.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ulf Hansson]

On Fri, 15 May 2020 at 12:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, May 14, 2020 at 09:11:50PM +0200, Ulf Hansson wrote:
> >
> > No worries, thanks for reviewing.
> >
>
> You are welcome.
>
> > That said, are you fine with Rafel queuing this then?
> >
>
> I am fine with that. However I told if you need fixes tags as there are
> no users of the notification yet in the kernel. Though this is trivial,
> but do we need this backported to stable kernel. I don't have strong
> opinion and leave it to you and Rafael.

I wanted to add the fixes tag, to make it obvious that there is an
error being fixed.

On the other hand, no need to tag this for stable, nor to need to send
it as fix for 5.7,

>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Fri, May 15, 2020 at 01:29:27PM +0200, Ulf Hansson wrote:
> On Fri, 15 May 2020 at 12:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, May 14, 2020 at 09:11:50PM +0200, Ulf Hansson wrote:
> > >
> > > No worries, thanks for reviewing.
> > >
> >
> > You are welcome.
> >
> > > That said, are you fine with Rafel queuing this then?
> > >
> >
> > I am fine with that. However I told if you need fixes tags as there are
> > no users of the notification yet in the kernel. Though this is trivial,
> > but do we need this backported to stable kernel. I don't have strong
> > opinion and leave it to you and Rafael.
> 
> I wanted to add the fixes tag, to make it obvious that there is an
> error being fixed.
> 
> On the other hand, no need to tag this for stable, nor to need to send
> it as fix for 5.7,
> 

Sometimes bots pick up patches for stable based on Fixes tag, hence I
thought of mentioning that.

-- 
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rafael J. Wysocki]

On Fri, May 15, 2020 at 1:30 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 15 May 2020 at 12:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, May 14, 2020 at 09:11:50PM +0200, Ulf Hansson wrote:
> > >
> > > No worries, thanks for reviewing.
> > >
> >
> > You are welcome.
> >
> > > That said, are you fine with Rafel queuing this then?
> > >
> >
> > I am fine with that. However I told if you need fixes tags as there are
> > no users of the notification yet in the kernel. Though this is trivial,
> > but do we need this backported to stable kernel. I don't have strong
> > opinion and leave it to you and Rafael.
>
> I wanted to add the fixes tag, to make it obvious that there is an
> error being fixed.
>
> On the other hand, no need to tag this for stable, nor to need to send
> it as fix for 5.7,
>
> >
> > Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> Thanks!

So applied as 5.8 material, thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel