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

[Ikjoon Jang]

On Fri, Nov 8, 2019 at 7:22 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
>
> 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?

You're right, that should be fixed,
actually I used 'undelaying' from commit: 8651f97bd951d
(PM / cpuidle: System resume hang fix with cpuidle),
when the first time cpuidle_{pause|resume} is introduced:

"Since we are dealing with drivers it seems best to call this function
during dpm_suspend(). Delaying the call till dpm_suspend_noirq() does
no harm, as long as it is before cpu_hotplug_begin() to avoid race
conditions with cpu hotpulg operations."

Delaying does no harm, but I think that there had been no specific
reason of this
delay from the beginning. Undelaying does no harm too.

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

Currently I've got only one specific device for examples.
Maybe this patch needs more generalized examples for applying to all
other machines.

Thanks!

> > 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();
> >
> >
>
>
>
>