| Message ID | 20210929144451.113334-3-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpuidle: Fix runtime PM based cpuidle for s2idle | expand |
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.
On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > 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. Yes, I agree. Although, I am not really changing the behaviour in regards to this. cpuidle_pause() is already being called in dpm_suspend_noirq(), for everybody today. > > 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(). cpuidle_suspend() would then need to go and fetch the cpuidle driver instance, which in some cases is one driver per CPU. Doesn't that get rather messy? Additionally, since find_deepest_state() is being called for cpuidle_enter_s2idle() too, we would need to treat the new CPUIDLE_STATE_DISABLED_ flag in a special way, right? Is this really what we want? > > 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. Avoiding to pause cpuidle when it's not needed makes perfect sense. Although, it looks to me that we could also implement that on top of $subject patch. Unless you insist on the CPUIDLE_STATE_DISABLED_ way, I would probably explore an option to let a cpuidle driver to set a global cpuidle flag during ->probe(). Depending if this flag is set, we can simply skip calling cpuidle_pause() during system suspend. What do you think? Kind regards Uffe
On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > 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. > > Yes, I agree. > > Although, I am not really changing the behaviour in regards to this. > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > everybody today. Yes, it is, but pausing it earlier will cause more energy to be spent, potentially. That said, there are not too many users of suspend_late callbacks in the tree, so it may not matter too much. > > > > 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(). > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > instance, which in some cases is one driver per CPU. Doesn't that get > rather messy? Per-CPU variables are used for that, so it is quite straightforward. > Additionally, since find_deepest_state() is being called for > cpuidle_enter_s2idle() too, we would need to treat the new > CPUIDLE_STATE_DISABLED_ flag in a special way, right? No, it already checks "disabled". > Is this really what we want? > > > > > 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. > > Avoiding to pause cpuidle when it's not needed makes perfect sense. > Although, it looks to me that we could also implement that on top of > $subject patch. Yes, it could. > Unless you insist on the CPUIDLE_STATE_DISABLED_ way, I would probably > explore an option to let a cpuidle driver to set a global cpuidle flag > during ->probe(). Depending if this flag is set, we can simply skip > calling cpuidle_pause() during system suspend. > > What do you think? Well, which driver in particular is in question here?
On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > 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. > > > > Yes, I agree. > > > > Although, I am not really changing the behaviour in regards to this. > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > everybody today. > > Yes, it is, but pausing it earlier will cause more energy to be spent, > potentially. > > That said, there are not too many users of suspend_late callbacks in > the tree, so it may not matter too much. > > > > > > > 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(). > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > instance, which in some cases is one driver per CPU. Doesn't that get > > rather messy? > > Per-CPU variables are used for that, so it is quite straightforward. > > > Additionally, since find_deepest_state() is being called for > > cpuidle_enter_s2idle() too, we would need to treat the new > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > No, it already checks "disabled". Yes, but that would be wrong. The use case I want to support, for cpuidle-psci, is to allow all idle states in suspend-to-idle, but prevent those that rely on runtime PM (after it has been disabled) for the regular idle path. > > > Is this really what we want? > > > > > > > > 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. > > > > Avoiding to pause cpuidle when it's not needed makes perfect sense. > > Although, it looks to me that we could also implement that on top of > > $subject patch. > > Yes, it could. > > > Unless you insist on the CPUIDLE_STATE_DISABLED_ way, I would probably > > explore an option to let a cpuidle driver to set a global cpuidle flag > > during ->probe(). Depending if this flag is set, we can simply skip > > calling cpuidle_pause() during system suspend. > > > > What do you think? > > Well, which driver in particular is in question here? Honestly, I don't know. It has not been my goal to entirely prevent calling cpuidle_pause(). In any case, it was introduced in the below commit, perhaps that can give us a hint when this is still needed? commit 8651f97bd951d0bb1c10fa24e3fa3455193f3548 Author: Preeti U Murthy <preeti@linux.vnet.ibm.com> Date: Mon Jul 9 10:12:56 2012 +0200 PM / cpuidle: System resume hang fix with cpuidle Kind regards Uffe
On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > 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. > > > > > > Yes, I agree. > > > > > > Although, I am not really changing the behaviour in regards to this. > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > everybody today. > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > potentially. > > > > That said, there are not too many users of suspend_late callbacks in > > the tree, so it may not matter too much. > > > > > > > > > > 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(). > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > rather messy? > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > Additionally, since find_deepest_state() is being called for > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > No, it already checks "disabled". > > Yes, but that would be wrong. Hmmm. > The use case I want to support, for cpuidle-psci, is to allow all idle > states in suspend-to-idle, So does PM-runtime work in suspend-to-idle? How? > but prevent those that rely on runtime PM > (after it has been disabled) for the regular idle path. Do you have a special suspend-to-idle handling of those states that doesn't require PM-runtime? > > > > > Is this really what we want? > > > > > > > > > > > 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. > > > > > > Avoiding to pause cpuidle when it's not needed makes perfect sense. > > > Although, it looks to me that we could also implement that on top of > > > $subject patch. > > > > Yes, it could. > > > > > Unless you insist on the CPUIDLE_STATE_DISABLED_ way, I would probably > > > explore an option to let a cpuidle driver to set a global cpuidle flag > > > during ->probe(). Depending if this flag is set, we can simply skip > > > calling cpuidle_pause() during system suspend. > > > > > > What do you think? > > > > Well, which driver in particular is in question here? > > Honestly, I don't know. It has not been my goal to entirely prevent > calling cpuidle_pause(). > > In any case, it was introduced in the below commit, perhaps that can > give us a hint when this is still needed? > > commit 8651f97bd951d0bb1c10fa24e3fa3455193f3548 > Author: Preeti U Murthy <preeti@linux.vnet.ibm.com> > Date: Mon Jul 9 10:12:56 2012 +0200 > PM / cpuidle: System resume hang fix with cpuidle Yes, I remember that.
On Thu, Oct 21, 2021 at 5:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > 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. > > > > > > > > Yes, I agree. > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > everybody today. > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > potentially. > > > > > > That said, there are not too many users of suspend_late callbacks in > > > the tree, so it may not matter too much. > > > > > > > > > > > > > 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(). > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > rather messy? > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > Additionally, since find_deepest_state() is being called for > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > No, it already checks "disabled". > > > > Yes, but that would be wrong. > > Hmmm. > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > states in suspend-to-idle, > > So does PM-runtime work in suspend-to-idle? How? > > > but prevent those that rely on runtime PM > > (after it has been disabled) for the regular idle path. > > Do you have a special suspend-to-idle handling of those states that > doesn't require PM-runtime? Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't make sense at all, so this needs to be taken care of in the first place. The problem with PM-runtime being unavailable after dpm_suspend() needs to be addressed in a different way IMO, because it only affects one specific use case.
On Thu, 21 Oct 2021 at 17:09, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > 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. > > > > > > > > Yes, I agree. > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > everybody today. > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > potentially. > > > > > > That said, there are not too many users of suspend_late callbacks in > > > the tree, so it may not matter too much. > > > > > > > > > > > > > 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(). > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > rather messy? > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > Additionally, since find_deepest_state() is being called for > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > No, it already checks "disabled". > > > > Yes, but that would be wrong. > > Hmmm. > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > states in suspend-to-idle, > > So does PM-runtime work in suspend-to-idle? How? No it doesn't. See below. > > > but prevent those that rely on runtime PM > > (after it has been disabled) for the regular idle path. > > Do you have a special suspend-to-idle handling of those states that > doesn't require PM-runtime? Yes. Feel free to have a look in __psci_enter_domain_idle_state(). In principle, when running the s2idle path, we call dev_pm_genpd_suspend|resume(), rather than pm_runtime_get|put*. This let genpd manage the reference counting (hierarchically too) and it also ignores the genpd governor in this stage, which also is needed to enter the deepest state. Quite similar to how cpuidle works. [...] Kind regards Uffe
On Thu, 21 Oct 2021 at 17:46, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 5:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > 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. > > > > > > > > > > Yes, I agree. > > > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > > everybody today. > > > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > > potentially. > > > > > > > > That said, there are not too many users of suspend_late callbacks in > > > > the tree, so it may not matter too much. > > > > > > > > > > > > > > > > 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(). > > > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > > rather messy? > > > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > No, it already checks "disabled". > > > > > > Yes, but that would be wrong. > > > > Hmmm. > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > states in suspend-to-idle, > > > > So does PM-runtime work in suspend-to-idle? How? > > > > > but prevent those that rely on runtime PM > > > (after it has been disabled) for the regular idle path. > > > > Do you have a special suspend-to-idle handling of those states that > > doesn't require PM-runtime? > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't > make sense at all, so this needs to be taken care of in the first > place. Right, I do agree, don't get me wrong. But, do we really want to treat s2-to-idle differently, compared to s2-to-ram in regards to this? Wouldn't it be a lot easier to let cpuidle drivers to opt-out for cpuidle_pause|resume(), no matter whether it's for s2-to-idle or s2-to-ram? > > The problem with PM-runtime being unavailable after dpm_suspend() > needs to be addressed in a different way IMO, because it only affects > one specific use case. It's one specific case so far, but we have the riscv driver on its way, which would suffer from the same problem. Anyway, an option is to figure out what platforms and cpuidle drivers, that really needs cpuidle_pause|resume() at this point and make an opt-in solution instead. This could then be used by runtime PM based cpuidle drivers as well. Would that be a way forward? Kind regards Uffe
On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 17:09, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > 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. > > > > > > > > > > Yes, I agree. > > > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > > everybody today. > > > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > > potentially. > > > > > > > > That said, there are not too many users of suspend_late callbacks in > > > > the tree, so it may not matter too much. > > > > > > > > > > > > > > > > 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(). > > > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > > rather messy? > > > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > No, it already checks "disabled". > > > > > > Yes, but that would be wrong. > > > > Hmmm. > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > states in suspend-to-idle, > > > > So does PM-runtime work in suspend-to-idle? How? > > No it doesn't. See below. > > > > > > but prevent those that rely on runtime PM > > > (after it has been disabled) for the regular idle path. > > > > Do you have a special suspend-to-idle handling of those states that > > doesn't require PM-runtime? > > Yes. Feel free to have a look in __psci_enter_domain_idle_state(). So in theory you could check the pm_runtime_put_sync_suspend() return value and fall back to something like WFI if that's an error code. > In principle, when running the s2idle path, we call > dev_pm_genpd_suspend|resume(), rather than pm_runtime_get|put*. > > This let genpd manage the reference counting (hierarchically too) and > it also ignores the genpd governor in this stage, which also is needed > to enter the deepest state. Quite similar to how cpuidle works. OK
On Thu, Oct 21, 2021 at 6:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 17:46, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 5:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > 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. > > > > > > > > > > > > Yes, I agree. > > > > > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > > > everybody today. > > > > > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > > > potentially. > > > > > > > > > > That said, there are not too many users of suspend_late callbacks in > > > > > the tree, so it may not matter too much. > > > > > > > > > > > > > > > > > > > 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(). > > > > > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > > > rather messy? > > > > > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > > > No, it already checks "disabled". > > > > > > > > Yes, but that would be wrong. > > > > > > Hmmm. > > > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > > states in suspend-to-idle, > > > > > > So does PM-runtime work in suspend-to-idle? How? > > > > > > > but prevent those that rely on runtime PM > > > > (after it has been disabled) for the regular idle path. > > > > > > Do you have a special suspend-to-idle handling of those states that > > > doesn't require PM-runtime? > > > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't > > make sense at all, so this needs to be taken care of in the first > > place. > > Right, I do agree, don't get me wrong. But, do we really want to treat > s2-to-idle differently, compared to s2-to-ram in regards to this? > > Wouldn't it be a lot easier to let cpuidle drivers to opt-out for > cpuidle_pause|resume(), no matter whether it's for s2-to-idle or > s2-to-ram? I don't think so. Suspend-to-idle resume cpuidle after pausing it which is just plain confusing and waste of energy and the fact that the system-wide suspend flow interferes with using PM-runtime for implementing cpuidle callbacks at the low level really is an orthogonal problem. > > > > The problem with PM-runtime being unavailable after dpm_suspend() > > needs to be addressed in a different way IMO, because it only affects > > one specific use case. > > It's one specific case so far, but we have the riscv driver on its > way, which would suffer from the same problem. So perhaps they should be advised about this issue. > Anyway, an option is to figure out what platforms and cpuidle drivers, > that really needs cpuidle_pause|resume() at this point and make an > opt-in solution instead. None of them need to pause cpuidle for suspend-to-idle AFAICS. Some may want it in the non-s2idle suspend path, but I'm not sure about the exact point where cpuidle needs to be paused in this case. Possibly before offlining the nonboot CPUs. > This could then be used by runtime PM based > cpuidle drivers as well. Would that be a way forward? The PM-runtime case should be addressed directly IMO, we only need to figure out how to do that. I'm wondering how you are dealing with the case when user space prevents pd_dev from suspending via sysfs, for that matter.
On Thu, Oct 21, 2021 at 6:41 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 6:29 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 17:46, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 5:09 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > Yes, I agree. > > > > > > > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > > > > everybody today. > > > > > > > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > > > > potentially. > > > > > > > > > > > > That said, there are not too many users of suspend_late callbacks in > > > > > > the tree, so it may not matter too much. > > > > > > > > > > > > > > > > > > > > > > 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(). > > > > > > > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > > > > rather messy? > > > > > > > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > > > > > No, it already checks "disabled". > > > > > > > > > > Yes, but that would be wrong. > > > > > > > > Hmmm. > > > > > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > > > states in suspend-to-idle, > > > > > > > > So does PM-runtime work in suspend-to-idle? How? > > > > > > > > > but prevent those that rely on runtime PM > > > > > (after it has been disabled) for the regular idle path. > > > > > > > > Do you have a special suspend-to-idle handling of those states that > > > > doesn't require PM-runtime? > > > > > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't > > > make sense at all, so this needs to be taken care of in the first > > > place. > > > > Right, I do agree, don't get me wrong. But, do we really want to treat > > s2-to-idle differently, compared to s2-to-ram in regards to this? > > > > Wouldn't it be a lot easier to let cpuidle drivers to opt-out for > > cpuidle_pause|resume(), no matter whether it's for s2-to-idle or > > s2-to-ram? > > I don't think so. > > Suspend-to-idle resume cpuidle after pausing it which is just plain > confusing and waste of energy and the fact that the system-wide > suspend flow interferes with using PM-runtime for implementing cpuidle > callbacks at the low level really is an orthogonal problem. > > > > > > > The problem with PM-runtime being unavailable after dpm_suspend() > > > needs to be addressed in a different way IMO, because it only affects > > > one specific use case. > > > > It's one specific case so far, but we have the riscv driver on its > > way, which would suffer from the same problem. > > So perhaps they should be advised about this issue. > > > Anyway, an option is to figure out what platforms and cpuidle drivers, > > that really needs cpuidle_pause|resume() at this point and make an > > opt-in solution instead. > > None of them need to pause cpuidle for suspend-to-idle AFAICS. > > Some may want it in the non-s2idle suspend path, but I'm not sure > about the exact point where cpuidle needs to be paused in this case. > Possibly before offlining the nonboot CPUs. > > > This could then be used by runtime PM based > > cpuidle drivers as well. Would that be a way forward? > > The PM-runtime case should be addressed directly IMO, we only need to > figure out how to do that. > > I'm wondering how you are dealing with the case when user space > prevents pd_dev from suspending via sysfs, for that matter. Or what happens if rpm_suspend() returns -EAGAIN, because someone has started to resume the device right after its reference counter went down to 0. It looks to me like the problem is there regardless of the whole interference with system suspend.
On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 17:09, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 4:05 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Thu, 21 Oct 2021 at 15:45, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 1:49 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > On Wed, 20 Oct 2021 at 20:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > 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. > > > > > > > > > > > > Yes, I agree. > > > > > > > > > > > > Although, I am not really changing the behaviour in regards to this. > > > > > > cpuidle_pause() is already being called in dpm_suspend_noirq(), for > > > > > > everybody today. > > > > > > > > > > Yes, it is, but pausing it earlier will cause more energy to be spent, > > > > > potentially. > > > > > > > > > > That said, there are not too many users of suspend_late callbacks in > > > > > the tree, so it may not matter too much. > > > > > > > > > > > > > > > > > > > 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(). > > > > > > > > > > > > cpuidle_suspend() would then need to go and fetch the cpuidle driver > > > > > > instance, which in some cases is one driver per CPU. Doesn't that get > > > > > > rather messy? > > > > > > > > > > Per-CPU variables are used for that, so it is quite straightforward. > > > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > > > No, it already checks "disabled". > > > > > > > > Yes, but that would be wrong. > > > > > > Hmmm. > > > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > > states in suspend-to-idle, > > > > > > So does PM-runtime work in suspend-to-idle? How? > > > > No it doesn't. See below. > > > > > > > > > but prevent those that rely on runtime PM > > > > (after it has been disabled) for the regular idle path. > > > > > > Do you have a special suspend-to-idle handling of those states that > > > doesn't require PM-runtime? > > > > Yes. Feel free to have a look in __psci_enter_domain_idle_state(). > > So in theory you could check the pm_runtime_put_sync_suspend() return > value and fall back to something like WFI if that's an error code. I have already tried that, but it simply got too complicated. The main issue was that runtime PM could become disabled for the device in the middle of executing the ->enter() callback. For example, if pm_runtime_get_sync() fails, I still need to make sure the reference counting in genpd becomes correct - and I can't do that using dev_pm_genpd_resume(). That's because it's not designed to be called in this "unknown" suspend phase, but should be called after the noirq phase and be properly balanced with dev_pm_genpd_suspend(). In other words, the error path didn't work out for me. [...] Kind regards Uffe
[...] > > > > > > > > > > > > > Additionally, since find_deepest_state() is being called for > > > > > > > cpuidle_enter_s2idle() too, we would need to treat the new > > > > > > > CPUIDLE_STATE_DISABLED_ flag in a special way, right? > > > > > > > > > > > > No, it already checks "disabled". > > > > > > > > > > Yes, but that would be wrong. > > > > > > > > Hmmm. > > > > > > > > > The use case I want to support, for cpuidle-psci, is to allow all idle > > > > > states in suspend-to-idle, > > > > > > > > So does PM-runtime work in suspend-to-idle? How? > > > > > > > > > but prevent those that rely on runtime PM > > > > > (after it has been disabled) for the regular idle path. > > > > > > > > Do you have a special suspend-to-idle handling of those states that > > > > doesn't require PM-runtime? > > > > > > Regardless, pausing cpuidle in the suspend-to-idle path simply doesn't > > > make sense at all, so this needs to be taken care of in the first > > > place. > > > > Right, I do agree, don't get me wrong. But, do we really want to treat > > s2-to-idle differently, compared to s2-to-ram in regards to this? > > > > Wouldn't it be a lot easier to let cpuidle drivers to opt-out for > > cpuidle_pause|resume(), no matter whether it's for s2-to-idle or > > s2-to-ram? > > I don't think so. > > Suspend-to-idle resume cpuidle after pausing it which is just plain > confusing and waste of energy and the fact that the system-wide > suspend flow interferes with using PM-runtime for implementing cpuidle > callbacks at the low level really is an orthogonal problem. It's certainly an orthogonal problem, I agree. However, trying to solve it in two different ways, may not really be worth the effort, in my opinion. As I kind of pointed out in the earlier reply, I am not sure there are any other relatively easy solutions available, to fix the problem for runtime PM based cpuidle drivers. We probably need to call cpuidle_pause() (or similar) in some way. > > > > > > > The problem with PM-runtime being unavailable after dpm_suspend() > > > needs to be addressed in a different way IMO, because it only affects > > > one specific use case. > > > > It's one specific case so far, but we have the riscv driver on its > > way, which would suffer from the same problem. > > So perhaps they should be advised about this issue. Yes, I will let them know - and hopefully I will soon also be able to provide them with a fix. :-) > > > Anyway, an option is to figure out what platforms and cpuidle drivers, > > that really needs cpuidle_pause|resume() at this point and make an > > opt-in solution instead. > > None of them need to pause cpuidle for suspend-to-idle AFAICS. I assume so too, otherwise things would have been broken when cpuidle_resume() is called in s2idle_enter(). But, it's still a bit unclear. > > Some may want it in the non-s2idle suspend path, but I'm not sure > about the exact point where cpuidle needs to be paused in this case. > Possibly before offlining the nonboot CPUs. Okay. Note that, I assume it would be okay to also pause cpuidle a bit earlier in these cases, like in dpm_suspend() for example. The point is, it's really a limited short period of time for when cpuidle would be paused, so I doubt it would have any impact on the consumed energy. Right? > > > This could then be used by runtime PM based > > cpuidle drivers as well. Would that be a way forward? > > The PM-runtime case should be addressed directly IMO, we only need to > figure out how to do that. If you have any other suggestions, I am listening. :-) > > I'm wondering how you are dealing with the case when user space > prevents pd_dev from suspending via sysfs, for that matter. That should work fine during runtime - because runtime PM is enabled for the device. Kind regards Uffe
[...] > > The PM-runtime case should be addressed directly IMO, we only need to > > figure out how to do that. > > > > I'm wondering how you are dealing with the case when user space > > prevents pd_dev from suspending via sysfs, for that matter. > > Or what happens if rpm_suspend() returns -EAGAIN, because someone has > started to resume the device right after its reference counter went > down to 0. That would mean that the pm_runtime_put_sync() call fails to runtime suspend the device. In other words, the corresponding genpd stays powered on, which prevents idle states from being selected by the genpd governor. So, yes, this should work fine. > > It looks to me like the problem is there regardless of the whole > interference with system suspend. I don't think so, but I may be overlooking some points. Kind regards Uffe
On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: [cut] > > So in theory you could check the pm_runtime_put_sync_suspend() return > > value and fall back to something like WFI if that's an error code. > > I have already tried that, but it simply got too complicated. The main > issue was that runtime PM could become disabled for the device in the > middle of executing the ->enter() callback. So IIUC the problem is that you cannot resume after suspending in that case. IOW, you need to guarantee that if the suspend is successful, the resume also will take place, but if the suspend fails, you basically don't care. > For example, if pm_runtime_get_sync() fails, I still need to make sure > the reference counting in genpd becomes correct - and I can't do that > using dev_pm_genpd_resume(). That's because it's not designed to be > called in this "unknown" suspend phase, but should be called after the > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > In other words, the error path didn't work out for me. It should be sufficient to call wake_up_all_idle_cpus() in the suspend path before dpm_suspend_late(), because system suspend acquires a PM-runtime reference on every device. IOW, it won't let any devices runtime-suspend, so if your power domain devices are resumed in that path, they will never suspend again in it and the pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() becomes a reference counter management call which works regardless of whether or not PM runtime is disabled.
On Thu, 21 Oct 2021 at 21:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > [cut] > > > > So in theory you could check the pm_runtime_put_sync_suspend() return > > > value and fall back to something like WFI if that's an error code. > > > > I have already tried that, but it simply got too complicated. The main > > issue was that runtime PM could become disabled for the device in the > > middle of executing the ->enter() callback. > > So IIUC the problem is that you cannot resume after suspending in that case. > > IOW, you need to guarantee that if the suspend is successful, the > resume also will take place, but if the suspend fails, you basically > don't care. Exactly. > > > For example, if pm_runtime_get_sync() fails, I still need to make sure > > the reference counting in genpd becomes correct - and I can't do that > > using dev_pm_genpd_resume(). That's because it's not designed to be > > called in this "unknown" suspend phase, but should be called after the > > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > > > In other words, the error path didn't work out for me. > > It should be sufficient to call wake_up_all_idle_cpus() in the suspend > path before dpm_suspend_late(), because system suspend acquires a > PM-runtime reference on every device. IOW, it won't let any devices > runtime-suspend, so if your power domain devices are resumed in that > path, they will never suspend again in it and the > pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() > becomes a reference counter management call which works regardless of > whether or not PM runtime is disabled. That sounds like a great idea, this should work too! Then the question is, how to make that call to wake_up_all_idle_cpus() to become optional - or only invoked for the cpuidle drivers that need it. In any case, I will try this out, thanks for the suggestion! Kind regards Uffe
On Thu, 21 Oct 2021 at 21:56, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 21:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > [cut] > > > > > > So in theory you could check the pm_runtime_put_sync_suspend() return > > > > value and fall back to something like WFI if that's an error code. > > > > > > I have already tried that, but it simply got too complicated. The main > > > issue was that runtime PM could become disabled for the device in the > > > middle of executing the ->enter() callback. > > > > So IIUC the problem is that you cannot resume after suspending in that case. > > > > IOW, you need to guarantee that if the suspend is successful, the > > resume also will take place, but if the suspend fails, you basically > > don't care. > > Exactly. > > > > > > For example, if pm_runtime_get_sync() fails, I still need to make sure > > > the reference counting in genpd becomes correct - and I can't do that > > > using dev_pm_genpd_resume(). That's because it's not designed to be > > > called in this "unknown" suspend phase, but should be called after the > > > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > > > > > In other words, the error path didn't work out for me. > > > > It should be sufficient to call wake_up_all_idle_cpus() in the suspend > > path before dpm_suspend_late(), because system suspend acquires a > > PM-runtime reference on every device. IOW, it won't let any devices > > runtime-suspend, so if your power domain devices are resumed in that > > path, they will never suspend again in it and the > > pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() > > becomes a reference counter management call which works regardless of > > whether or not PM runtime is disabled. > > That sounds like a great idea, this should work too! Then the question > is, how to make that call to wake_up_all_idle_cpus() to become > optional - or only invoked for the cpuidle drivers that need it. > > In any case, I will try this out, thanks for the suggestion! I now recall that I have already tried this, but unfortunately it doesn't work. The problem is that the dev->power.syscore flag is set for the device, which makes device_prepare() to bail out early and skip calling pm_runtime_get_noresume(). Kind regards Uffe
On Fri, Oct 22, 2021 at 12:18 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 21 Oct 2021 at 21:56, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 21:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > [cut] > > > > > > > > So in theory you could check the pm_runtime_put_sync_suspend() return > > > > > value and fall back to something like WFI if that's an error code. > > > > > > > > I have already tried that, but it simply got too complicated. The main > > > > issue was that runtime PM could become disabled for the device in the > > > > middle of executing the ->enter() callback. > > > > > > So IIUC the problem is that you cannot resume after suspending in that case. > > > > > > IOW, you need to guarantee that if the suspend is successful, the > > > resume also will take place, but if the suspend fails, you basically > > > don't care. > > > > Exactly. > > > > > > > > > For example, if pm_runtime_get_sync() fails, I still need to make sure > > > > the reference counting in genpd becomes correct - and I can't do that > > > > using dev_pm_genpd_resume(). That's because it's not designed to be > > > > called in this "unknown" suspend phase, but should be called after the > > > > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > > > > > > > In other words, the error path didn't work out for me. > > > > > > It should be sufficient to call wake_up_all_idle_cpus() in the suspend > > > path before dpm_suspend_late(), because system suspend acquires a > > > PM-runtime reference on every device. IOW, it won't let any devices > > > runtime-suspend, so if your power domain devices are resumed in that > > > path, they will never suspend again in it and the > > > pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() > > > becomes a reference counter management call which works regardless of > > > whether or not PM runtime is disabled. > > > > That sounds like a great idea, this should work too! Then the question > > is, how to make that call to wake_up_all_idle_cpus() to become > > optional - or only invoked for the cpuidle drivers that need it. It need not be optional. For suspend-to-idle it doesn't matter, because all CPUs will be woken up from idle shortly anyway. For other suspend variants this doesn't matter, because all secondary CPUs will be taken offline shortly and the primary CPU will call into the platform "sleep" handler. > > > > In any case, I will try this out, thanks for the suggestion! > > I now recall that I have already tried this, but unfortunately it doesn't work. > > The problem is that the dev->power.syscore flag is set for the device, > which makes device_prepare() to bail out early and skip calling > pm_runtime_get_noresume(). This needs to be fixed, then.
On Fri, 22 Oct 2021 at 14:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Oct 22, 2021 at 12:18 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 21 Oct 2021 at 21:56, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 21 Oct 2021 at 21:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > [cut] > > > > > > > > > > So in theory you could check the pm_runtime_put_sync_suspend() return > > > > > > value and fall back to something like WFI if that's an error code. > > > > > > > > > > I have already tried that, but it simply got too complicated. The main > > > > > issue was that runtime PM could become disabled for the device in the > > > > > middle of executing the ->enter() callback. > > > > > > > > So IIUC the problem is that you cannot resume after suspending in that case. > > > > > > > > IOW, you need to guarantee that if the suspend is successful, the > > > > resume also will take place, but if the suspend fails, you basically > > > > don't care. > > > > > > Exactly. > > > > > > > > > > > > For example, if pm_runtime_get_sync() fails, I still need to make sure > > > > > the reference counting in genpd becomes correct - and I can't do that > > > > > using dev_pm_genpd_resume(). That's because it's not designed to be > > > > > called in this "unknown" suspend phase, but should be called after the > > > > > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > > > > > > > > > In other words, the error path didn't work out for me. > > > > > > > > It should be sufficient to call wake_up_all_idle_cpus() in the suspend > > > > path before dpm_suspend_late(), because system suspend acquires a > > > > PM-runtime reference on every device. IOW, it won't let any devices > > > > runtime-suspend, so if your power domain devices are resumed in that > > > > path, they will never suspend again in it and the > > > > pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() > > > > becomes a reference counter management call which works regardless of > > > > whether or not PM runtime is disabled. > > > > > > That sounds like a great idea, this should work too! Then the question > > > is, how to make that call to wake_up_all_idle_cpus() to become > > > optional - or only invoked for the cpuidle drivers that need it. > > It need not be optional. > > For suspend-to-idle it doesn't matter, because all CPUs will be woken > up from idle shortly anyway. > > For other suspend variants this doesn't matter, because all secondary > CPUs will be taken offline shortly and the primary CPU will call into > the platform "sleep" handler. > > > > > > > In any case, I will try this out, thanks for the suggestion! > > > > I now recall that I have already tried this, but unfortunately it doesn't work. > > > > The problem is that the dev->power.syscore flag is set for the device, > > which makes device_prepare() to bail out early and skip calling > > pm_runtime_get_noresume(). > > This needs to be fixed, then. So bumping the usage count even if the dev->power.syscore is set, should be fine? (And of course dropping it in the complete phase). I can work with that, let me try! Kind regards Uffe
On Fri, Oct 22, 2021 at 2:57 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Fri, 22 Oct 2021 at 14:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Fri, Oct 22, 2021 at 12:18 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 21 Oct 2021 at 21:56, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > On Thu, 21 Oct 2021 at 21:02, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 21, 2021 at 8:12 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > > > On Thu, 21 Oct 2021 at 18:33, Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > On Thu, Oct 21, 2021 at 6:17 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > > > > > [cut] > > > > > > > > > > > > So in theory you could check the pm_runtime_put_sync_suspend() return > > > > > > > value and fall back to something like WFI if that's an error code. > > > > > > > > > > > > I have already tried that, but it simply got too complicated. The main > > > > > > issue was that runtime PM could become disabled for the device in the > > > > > > middle of executing the ->enter() callback. > > > > > > > > > > So IIUC the problem is that you cannot resume after suspending in that case. > > > > > > > > > > IOW, you need to guarantee that if the suspend is successful, the > > > > > resume also will take place, but if the suspend fails, you basically > > > > > don't care. > > > > > > > > Exactly. > > > > > > > > > > > > > > > For example, if pm_runtime_get_sync() fails, I still need to make sure > > > > > > the reference counting in genpd becomes correct - and I can't do that > > > > > > using dev_pm_genpd_resume(). That's because it's not designed to be > > > > > > called in this "unknown" suspend phase, but should be called after the > > > > > > noirq phase and be properly balanced with dev_pm_genpd_suspend(). > > > > > > > > > > > > In other words, the error path didn't work out for me. > > > > > > > > > > It should be sufficient to call wake_up_all_idle_cpus() in the suspend > > > > > path before dpm_suspend_late(), because system suspend acquires a > > > > > PM-runtime reference on every device. IOW, it won't let any devices > > > > > runtime-suspend, so if your power domain devices are resumed in that > > > > > path, they will never suspend again in it and the > > > > > pm_runtime_put_sync_suspend() in __psci_enter_domain_idle_state() > > > > > becomes a reference counter management call which works regardless of > > > > > whether or not PM runtime is disabled. > > > > > > > > That sounds like a great idea, this should work too! Then the question > > > > is, how to make that call to wake_up_all_idle_cpus() to become > > > > optional - or only invoked for the cpuidle drivers that need it. > > > > It need not be optional. > > > > For suspend-to-idle it doesn't matter, because all CPUs will be woken > > up from idle shortly anyway. > > > > For other suspend variants this doesn't matter, because all secondary > > CPUs will be taken offline shortly and the primary CPU will call into > > the platform "sleep" handler. > > > > > > > > > > In any case, I will try this out, thanks for the suggestion! > > > > > > I now recall that I have already tried this, but unfortunately it doesn't work. > > > > > > The problem is that the dev->power.syscore flag is set for the device, > > > which makes device_prepare() to bail out early and skip calling > > > pm_runtime_get_noresume(). > > > > This needs to be fixed, then. > > So bumping the usage count even if the dev->power.syscore is set, > should be fine? (And of course dropping it in the complete phase). Yes, please see https://patchwork.kernel.org/project/linux-pm/patch/5773062.lOV4Wx5bFT@kreacher/ It should have been done this way from the outset, but I messed up a merge (and said that it was "trivial" :-/). > I can work with that, let me try!
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;
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(-)