Message-Id: <201207061923.29748.rjw@sisk.pl>
Date:	Fri, 6 Jul 2012 19:23:29 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	preeti <preeti@...ux.vnet.ibm.com>
Cc:	Dave Hansen <dave@...ux.vnet.ibm.com>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	Linux PM mailing list <linux-pm@...r.kernel.org>,
	linux-acpi@...r.kernel.org,
	Deepthi Dharwar <deepthi@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>, Len Brown <lenb@...nel.org>
Subject: Re: [PATCH V2] Suspend,cpuidle:resume_hang fix with cpuidle

On Friday, July 06, 2012, preeti wrote:
> 
> From: Preeti U Murthy <preeti@...ux.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@...ux.vnet.ibm.com>
> Signed-off-by: Preeti U Murthy <preeti@...ux.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,
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists