Re: [RFC PATCH RESEND] cpuidle: undelaying cpuidle in dpm_{suspend|resume}()

[Rafael J. Wysocki]

On Wednesday, October 30, 2019 3:21:05 AM CET Ikjoon Jang wrote:
> cpuidle is paused only during dpm_suspend_noirq() ~ dpm_resume_noirq().
> But some device drivers need random sized IOs in dpm_{suspend|resume}()
> stage (e.g. re-downloading firmware in resume).
> And with such a device, cpuidle's latencies could be critical to
> response time of system suspend/resume.
> 
> To minimize those latencies, we could apply pm_qos to such device drivers,
> but simply undelaying cpuidle from dpm_suspend_noirq() to dpm suspend()
> seems no harm.

While the patch is generally acceptable, the changelog is not.

First, what does "undelying" mean?

Second, you seem to be talking about the cases in which exit latencies of
idle states are not small relative to the system suspend/resume time, so
without any specific examples this is not really convincing.

Also, potentially, there is another reason to make this change, which is
that on some systems i2c (or similar) controllers may be requisite for
idle state entry and exit, so it may make sense in general to prevent
cpuidle from being used over the entire suspend and resume of the
system.  However, without any example of a system in which that matters
it still is not convincing enough IMO.

> Signed-off-by: Ikjoon Jang <ikjn@xxxxxxxxxxxx>
> ---
>  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 134a8af51511..5928dd2139e8 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -773,8 +773,6 @@ void dpm_resume_noirq(pm_message_t state)
>  
>  	resume_device_irqs();
>  	device_wakeup_disarm_wake_irqs();
> -
> -	cpuidle_resume();
>  }
>  
>  static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
> @@ -1069,6 +1067,7 @@ void dpm_resume(pm_message_t state)
>  
>  	cpufreq_resume();
>  	devfreq_resume();
> +	cpuidle_resume();
>  	trace_suspend_resume(TPS("dpm_resume"), state.event, false);
>  }
>  
> @@ -1411,8 +1410,6 @@ int dpm_suspend_noirq(pm_message_t state)
>  {
>  	int ret;
>  
> -	cpuidle_pause();
> -
>  	device_wakeup_arm_wake_irqs();
>  	suspend_device_irqs();
>  
> @@ -1830,6 +1827,7 @@ int dpm_suspend(pm_message_t state)
>  	trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
>  	might_sleep();
>  
> +	cpuidle_pause();
>  	devfreq_suspend();
>  	cpufreq_suspend();
>  
>