On 07/06/2012 01:12 PM, preeti wrote: > > From: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx> > > On certain bios,resume hangs if cpus are allowed to enter idle states > during suspend[1] > > This was fixed in apci idle driver[2].But intel_idle driver does not > have this fix.Thus instead of replicating the fix in both the idle > drivers,or in more platform specific idle drivers if needed,the > more general cpuidle infrastructure could handle this. > > A suspend callback in cpuidle_driver could handle this fix.But > a cpuidle_driver provides only basic functionalities like platform idle > state detection capability and mechanisms to support entry and exit > into CPU idle states.All other cpuidle functions are found in the > cpuidle generic infrastructure for good reason that all cpuidle > drivers,irrepective of their platforms will support these functions. > > One option therefore would be to register a suspend callback in cpuidle > which handles this fix.This could be called through a PM_SUSPEND_PREPARE > notifier.But this is too generic a notfier for a driver to handle. > > Also,ideally the job of cpuidle is not to handle side effects of suspend. > It should expose the interfaces which "handle cpuidle 'during' suspend" > or any other operation,which the subsystems call during that respective > operation. > > The fix demands that during suspend,no cpus should be allowed to enter > deep C-states.The interface cpuidle_uninstall_idle_handler() in cpuidle > ensures that.Not just that it also kicks all the cpus which are already > in idle out of their idle states which was being done during cpu hotplug > through a CPU_DYING_FROZEN callbacks. > > Now the question arises about when during suspend should > cpuidle_uninstall_idle_handler() be called.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.In dpm_suspend_noirq(),it would be wise to place this call > before suspend_device_irqs() to avoid ugly interactions with the same. > > Ananlogously,during resume. > > References: > 1.https://bugs.launchpad.net/ubuntu/+source/linux/+bug/674075. > 2.http://marc.info/?l=linux-pm&m=133958534231884&w=2 > > Reported-by: Dave Hansen <dave@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx> Much better design. Good to avoid all those ugly flags and checks :-) Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx> Regards, Srivatsa S. Bhat > --- > > drivers/acpi/processor_idle.c | 30 +----------------------------- > drivers/base/power/main.c | 4 +++- > drivers/cpuidle/cpuidle.c | 16 ++++++++++++++++ > include/linux/cpuidle.h | 2 ++ > 4 files changed, 22 insertions(+), 30 deletions(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 6d9ec3e..b894627 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -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 device *dev) > { > - if (acpi_idle_suspend == 1) > - return 0; > - > acpi_idle_bm_rld_save(); > - acpi_idle_suspend = 1; > return 0; > } > > int acpi_processor_resume(struct device *dev) > { > - if (acpi_idle_suspend == 0) > - return 0; > - > acpi_idle_bm_rld_restore(); > - acpi_idle_suspend = 0; > return 0; > } > > @@ -763,11 +751,6 @@ static int acpi_idle_enter_c1(struct cpuidle_device *dev, > > local_irq_disable(); > > - if (acpi_idle_suspend) { > - local_irq_enable(); > - cpu_relax(); > - return -EBUSY; > - } > > lapic_timer_state_broadcast(pr, cx, 1); > kt1 = ktime_get_real(); > @@ -838,11 +821,6 @@ static int acpi_idle_enter_simple(struct cpuidle_device *dev, > > local_irq_disable(); > > - if (acpi_idle_suspend) { > - local_irq_enable(); > - cpu_relax(); > - return -EBUSY; > - } > > if (cx->entry_method != ACPI_CSTATE_FFH) { > current_thread_info()->status &= ~TS_POLLING; > @@ -928,8 +906,7 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, > drv, drv->safe_state_index); > } else { > local_irq_disable(); > - if (!acpi_idle_suspend) > - acpi_safe_halt(); > + acpi_safe_halt(); > local_irq_enable(); > return -EBUSY; > } > @@ -937,11 +914,6 @@ static int acpi_idle_enter_bm(struct cpuidle_device *dev, > > local_irq_disable(); > > - if (acpi_idle_suspend) { > - local_irq_enable(); > - cpu_relax(); > - return -EBUSY; > - } > > if (cx->entry_method != ACPI_CSTATE_FFH) { > current_thread_info()->status &= ~TS_POLLING; > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index df5f41d..d791950 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -28,7 +28,7 @@ > #include <linux/sched.h> > #include <linux/async.h> > #include <linux/suspend.h> > - > +#include <linux/cpuidle.h> > #include "../base.h" > #include "power.h" > > @@ -467,6 +467,7 @@ static void dpm_resume_noirq(pm_message_t state) > mutex_unlock(&dpm_list_mtx); > dpm_show_time(starttime, state, "noirq"); > resume_device_irqs(); > + cpuidle_resume(); > } > > /** > @@ -867,6 +868,7 @@ static int dpm_suspend_noirq(pm_message_t state) > ktime_t starttime = ktime_get(); > int error = 0; > > + cpuidle_pause(); > suspend_device_irqs(); > mutex_lock(&dpm_list_mtx); > while (!list_empty(&dpm_late_early_list)) { > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 0132706..d6a533e 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -201,6 +201,22 @@ void cpuidle_resume_and_unlock(void) > > EXPORT_SYMBOL_GPL(cpuidle_resume_and_unlock); > > +/* Currently used in suspend/resume path to suspend cpuidle */ > +void cpuidle_pause(void) > +{ > + mutex_lock(&cpuidle_lock); > + cpuidle_uninstall_idle_handler(); > + mutex_unlock(&cpuidle_lock); > +} > + > +/* Currently used in suspend/resume path to resume cpuidle */ > +void cpuidle_resume(void) > +{ > + mutex_lock(&cpuidle_lock); > + cpuidle_install_idle_handler(); > + mutex_unlock(&cpuidle_lock); > +} > + > /** > * cpuidle_wrap_enter - performs timekeeping and irqen around enter function > * @dev: pointer to a valid cpuidle_device object > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index 8684a0d..d83b27e 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -146,6 +146,8 @@ extern void cpuidle_unregister_device(struct cpuidle_device *dev); > > extern void cpuidle_pause_and_lock(void); > extern void cpuidle_resume_and_unlock(void); > +extern void cpuidle_pause(void); > +extern void cpuidle_resume(void); > extern int cpuidle_enable_device(struct cpuidle_device *dev); > extern void cpuidle_disable_device(struct cpuidle_device *dev); > extern int cpuidle_wrap_enter(struct cpuidle_device *dev, > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
