* Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle
[not found] ` <4FF696D4.4040905@linux.vnet.ibm.com>
@ 2012-07-06 17:23 ` Rafael J. Wysocki
2012-07-06 17:21 ` Dave Hansen
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2012-07-06 17:23 UTC (permalink / raw)
To: preeti
Cc: Dave Hansen, Srivatsa S. Bhat, Linux PM mailing list, linux-acpi,
Deepthi Dharwar, LKML, Len Brown
On Friday, July 06, 2012, preeti wrote:
>
> From: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>
> 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@linux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
OK, this looks good to me. Queuing up in the linux-next branch of the
linux-pm.git tree. If no problems with it are reported, I'll move it to the
pm-cpuidle branch in a couple of days.
Thanks,
Rafael
> ---
>
> 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,
>
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread