On Thursday, June 28, 2012, preeti wrote: > > From: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx> > > Dave Hansen reported problems with suspend/resume when used > with intel_idle driver.More such problems were noticed. > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075. > > The reason for this could not be suspected till he reported > resume hang issue when used with acpi idle driver on his Lenovo-S10-3 > Atom netbook. > > The patch-[PM / ACPI: Fix suspend/resume regression caused by cpuidle > cleanup],fixed this issue by ensuring that acpi idle drivers prevent > cpus from going into deeper sleep states during suspend to prevent > resume hang on certain bios. > http://marc.info/?l=linux-pm&m=133958534231884&w=2 > > Commit b04e7bdb984e3b7f62fb7f44146a529f88cc7639 > (ACPI: disable lower idle C-states across suspend/resume) throws > light on the resume hang issue on certain specific bios. > > Also the following lines in drivers/idle/intel_idle.c suggest intel_idle > drivers should also ensure cpus are prevented from entering idle states > during suspend to avoid a resume hang. > > /* > * Known limitations > * [..] > * > * ACPI has a .suspend hack to turn off deep c-states during suspend > * to avoid complications with the lapic timer workaround. > * Have not seen issues with suspend, but may need same workaround here. > * > */ > This patch aims at having this fix in a place common to both the idle > drivers. > > Suspend is enabled only if ACPI is active on x86.Thus the setting of > acpi_idle_suspend during suspend is moved up to ACPI specific code with > both acpi and intel idle drivers checking if it is valid to enter deeper > idle states.The setting of acpi_idle_suspend is done via PM_SUSPEND_PREPARE > notifiers to avoid race conditions between processors entering idle states > and the ongoing process of suspend. > > The check on acpi_idle_suspend is included in the most appropriate header > so as to be visible to both the idle drivers irrespective of the > different configurations.Even if ACPI is disabled intel idle drivers can > still carry out the acpi_idle_suspend check. > > Reported-by: Dave Hansen <dave@xxxxxxxxxxxxxxxxxx> > Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx> > Reviewed-by: Deepthi Dharwar <deepthi@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx> > --- > Dave, does this patch ensure suspend/resume works > fine on your netbook if intel_idle driver is enabled? > > drivers/acpi/processor_idle.c | 46 +++++++++++++++++++---------------------- > drivers/acpi/sleep.c | 38 ++++++++++++++++++++++++++++++++++ > drivers/idle/intel_idle.c | 10 +++++++++ > include/linux/suspend.h | 24 +++++++++++++++++++++ > 4 files changed, 93 insertions(+), 25 deletions(-) > > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index c2ffd84..3ab0963 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -41,7 +41,7 @@ > #include <linux/clockchips.h> > #include <linux/cpuidle.h> > #include <linux/irqflags.h> > - > +#include <linux/suspend.h> > /* > * Include the apic definitions for x86 to have the APIC timer related defines > * available also for UP (on SMP it gets magically included via linux/smp.h). > @@ -221,10 +221,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr, > > #endif > > -/* > - * Suspend / resume control > - */ > -static int acpi_idle_suspend; > static u32 saved_bm_rld; > > static void acpi_idle_bm_rld_save(void) > @@ -243,21 +239,13 @@ static void acpi_idle_bm_rld_restore(void) > > int acpi_processor_suspend(struct acpi_device * device, pm_message_t state) > { > - if (acpi_idle_suspend == 1) > - return 0; > - > acpi_idle_bm_rld_save(); > - acpi_idle_suspend = 1; > return 0; > } So, this seems to be on top of the "PM / ACPI: Fix suspend/resume regression caused by cpuidle" patch, right? > int acpi_processor_resume(struct acpi_device * device) > { > - if (acpi_idle_suspend == 0) > - return 0; > - > acpi_idle_bm_rld_restore(); > - acpi_idle_suspend = 0; > return 0; > } > > @@ -762,11 +750,13 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev, > return -EINVAL; > > local_irq_disable(); > - > - if (acpi_idle_suspend) { > + /* > + * Do not enter idle states if you are in suspend path > + */ > + if (in_suspend_path()) { This is more than ugly. Can't you simply use the acpi_idle_suspend _variable_ here? Besides, why the name of the variable is acpi_idle_suspend, if it is used by the Intel driver too? > local_irq_enable(); > cpu_relax(); > - return -EINVAL; > + return -EBUSY; That change is made by the "PM / ACPI: Fix suspend/resume regression caused by cpuidle" patch already. > } > > lapic_timer_state_broadcast(pr, cx, 1); > @@ -838,10 +828,13 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev, > > local_irq_disable(); > > - if (acpi_idle_suspend) { > + /* > + * Do not enter idle states if you are in suspend path > + */ > + if (in_suspend_path()) { > local_irq_enable(); > cpu_relax(); > - return -EINVAL; > + return -EBUSY; > } > > if (cx->entry_method != ACPI_CSTATE_FFH) { > @@ -922,12 +915,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, > if (unlikely(!pr)) > return -EINVAL; > > - if (acpi_idle_suspend) { > - if (irqs_disabled()) > - local_irq_enable(); > - cpu_relax(); > - return -EINVAL; > - } The last version of the "PM / ACPI: Fix suspend/resume regression caused by cpuidle" patch didn't contain this hunk. > > if (!cx->bm_sts_skip && acpi_idle_bm_check()) { > if (drv->safe_state_index >= 0) { > @@ -935,13 +922,22 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, > drv, drv->safe_state_index); > } else { > local_irq_disable(); > - acpi_safe_halt(); > + if (!(in_suspend_path())) > + acpi_safe_halt(); > local_irq_enable(); > return -EINVAL; > } > } > > local_irq_disable(); > + /* > + * Do not enter idle states if you are in suspend path > + */ > + if (in_suspend_path()) { > + local_irq_enable(); > + cpu_relax(); > + return -EBUSY; > + } > > if (cx->entry_method != ACPI_CSTATE_FFH) { > current_thread_info()->status &= ~TS_POLLING; > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index 8856102..4ec77db 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -28,6 +28,11 @@ > #include "internal.h" > #include "sleep.h" > > +/* > + * Suspend/Resume control > + */ > +int acpi_idle_suspend; > + > u8 wake_sleep_flags = ACPI_NO_OPTIONAL_METHODS; > static unsigned int gts, bfs; > static int set_param_wake_flag(const char *val, struct kernel_param *kp) > @@ -905,6 +910,36 @@ static void __init acpi_gts_bfs_check(void) > } > } > > +/** > + * cpuidle_pm_callback - On some bios, resume hangs > + * if idle states are entered during suspend. > + * > + * acpi_idle_suspend is used by the x86 idle drivers > + * to decide whether to go into idle states or not. > + */ > +static int > +cpuidle_pm_callback(struct notifier_block *nb, > + unsigned long action, void *ptr) > +{ > + switch (action) { > + > + case PM_SUSPEND_PREPARE: > + if (acpi_idle_suspend == 0) > + acpi_idle_suspend = 1; > + break; > + > + case PM_POST_SUSPEND: > + if (acpi_idle_suspend == 1) > + acpi_idle_suspend = 0; > + break; > + > + default: > + return NOTIFY_DONE; > + } > + > + return NOTIFY_OK; > +} Why don't you put this notifier into cpuidle instead? > + > int __init acpi_sleep_init(void) > { > acpi_status status; > @@ -932,6 +967,9 @@ int __init acpi_sleep_init(void) > > suspend_set_ops(old_suspend_ordering ? > &acpi_suspend_ops_old : &acpi_suspend_ops); > + > + pm_notifier(cpuidle_pm_callback, 0); > + > #endif > > #ifdef CONFIG_HIBERNATION > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index d0f59c3..a595207 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -65,6 +65,7 @@ > #include <asm/cpu_device_id.h> > #include <asm/mwait.h> > #include <asm/msr.h> > +#include <linux/suspend.h> > > #define INTEL_IDLE_VERSION "0.4" > #define PREFIX "intel_idle: " > @@ -255,6 +256,15 @@ static int intel_idle(struct cpuidle_device *dev, > cstate = (((eax) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) + 1; > > /* > + * Do not enter idle states if you are in suspend path > + */ > + if (in_suspend_path()) { > + local_irq_enable(); > + cpu_relax(); > + return -EBUSY; > + } > + > + /* > * leave_mm() to avoid costly and often unnecessary wakeups > * for flushing the user TLB's associated with the active mm. > */ > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index 0c808d7..eb96670 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -38,6 +38,30 @@ typedef int __bitwise suspend_state_t; > #define PM_SUSPEND_MEM ((__force suspend_state_t) 3) > #define PM_SUSPEND_MAX ((__force suspend_state_t) 4) > > +extern int acpi_idle_suspend; > + > +/** > + * in_suspend_path - X86 idle drivers make a call > + * to this function before entering idle states. > + * > + * Entering idle states is prevented if it is in suspend > + * path. > + */ > +#ifdef CONFIG_ACPI Why #ifdef CONFIG_ACPI? > +static inline int in_suspend_path(void) > +{ > + if (acpi_idle_suspend == 1) > + return 1; > + else > + return 0; > +} > +#else > +static inline int in_suspend_path(void) > +{ > + return 0; > +} > +#endif > + > enum suspend_stat_step { > SUSPEND_FREEZE = 1, > SUSPEND_PREPARE, Thanks, Rafael
