Messages in this thread | | | From | Ulf Hansson <> | Date | Mon, 11 Oct 2021 12:04:44 +0200 | Subject | Re: [PATCH 1/2] cpuidle: Avoid calls to cpuidle_resume|pause() for s2idle |
| |
On Sat, 9 Oct 2021 at 17:39, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, September 29, 2021 4:44:50 PM CEST Ulf Hansson wrote: > > In s2idle_enter(), cpuidle_resume|pause() are invoked to re-allow calls to > > the cpuidle callbacks during s2idle operations. This is needed because > > cpuidle is paused in-between in dpm_suspend_noirq() and dpm_resume_noirq(). > > Well, in fact, doing that last thing for s2idle is pointless, because cpuidle > is going to be resumed eventually anyway in that case and the breakage expected > to be prevented by the pausing will still occur. > > So I would rather do something like the patch below (untested).
Hi Rafael,
From a standalone change point of view, what you suggest seems reasonable to me.
However, the main issue I am really trying to fix in this series is being done in patch2/2. And unfortunately, the below change doesn't really fit with what I suggest in patch2/2. Can you please have a look at patch2 as well?
If you think it may be better, I squash the two patches?
Kind regards Uffe
> > --- > drivers/base/power/main.c | 11 ++++++----- > kernel/power/suspend.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 7 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/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(); > } > > /** > @@ -881,6 +879,7 @@ void dpm_resume_early(pm_message_t state > void dpm_resume_start(pm_message_t state) > { > dpm_resume_noirq(state); > + cpuidle_resume(); > dpm_resume_early(state); > } > EXPORT_SYMBOL_GPL(dpm_resume_start); > @@ -1336,8 +1335,6 @@ int dpm_suspend_noirq(pm_message_t state > { > int ret; > > - cpuidle_pause(); > - > device_wakeup_arm_wake_irqs(); > suspend_device_irqs(); > > @@ -1521,9 +1518,13 @@ int dpm_suspend_end(pm_message_t state) > if (error) > goto out; > > + cpuidle_pause(); > + > error = dpm_suspend_noirq(state); > - if (error) > + if (error) { > + cpuidle_resume(); > dpm_resume_early(resume_event(state)); > + } > > out: > dpm_show_time(starttime, state, error, "end"); > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -97,7 +97,6 @@ static void s2idle_enter(void) > raw_spin_unlock_irq(&s2idle_lock); > > cpus_read_lock(); > - cpuidle_resume(); > > /* Push all the CPUs into the idle loop. */ > wake_up_all_idle_cpus(); > @@ -105,7 +104,6 @@ static void s2idle_enter(void) > swait_event_exclusive(s2idle_wait_head, > s2idle_state == S2IDLE_STATE_WAKE); > > - cpuidle_pause(); > cpus_read_unlock(); > > raw_spin_lock_irq(&s2idle_lock); > @@ -405,6 +403,9 @@ static int suspend_enter(suspend_state_t > if (error) > goto Devices_early_resume; > > + if (state != PM_SUSPEND_TO_IDLE) > + cpuidle_pause(); > + > error = dpm_suspend_noirq(PMSG_SUSPEND); > if (error) { > pr_err("noirq suspend of devices failed\n"); > @@ -459,6 +460,9 @@ static int suspend_enter(suspend_state_t > dpm_resume_noirq(PMSG_RESUME); > > Platform_early_resume: > + if (state != PM_SUSPEND_TO_IDLE) > + cpuidle_resume(); > + > platform_resume_early(state); > > Devices_early_resume: >
| |