| Message ID | 20200511133346.21706-1-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | cpuidle: psci: Fixup execution order when entering a domain idle state | expand |
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 >
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.
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
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
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
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
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
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.
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!
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;
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(-)