[PATCH v1 1/2] genirq: Fix error path for resuming irqs

[PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Derek Basehore]

In the case of a late abort to suspend/hibernate, irqs marked with
IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
called on these paths.

This can happen with a pm test for platform, a late wakeup irq, and other
instances. This change removes the function from syscore and calls it explicitly
in suspend, hibernate, etc.

This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/base/power/main.c |  5 ++++-
 drivers/xen/manage.c      |  5 ++++-
 include/linux/interrupt.h |  1 +
 include/linux/pm.h        |  2 +-
 kernel/irq/pm.c           | 17 +++--------------
 kernel/kexec.c            |  2 +-
 kernel/power/hibernate.c  |  6 +++---
 kernel/power/suspend.c    |  2 +-
 8 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index bf41296..a087473 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
  * dpm_resume_start - Execute "noirq" and "early" device callbacks.
  * @state: PM transition of the system being carried out.
  */
-void dpm_resume_start(pm_message_t state)
+void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
 {
+	if (enable_early_irqs)
+		early_resume_device_irqs();
 	dpm_resume_noirq(state);
 	dpm_resume_early(state);
 }
@@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
 	if (error) {
 		suspend_stats.failed_suspend_noirq++;
 		dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
+		early_resume_device_irqs();
 		dpm_resume_noirq(resume_event(state));
 	} else {
 		dpm_show_time(starttime, state, "noirq");
diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index c3667b2..d387cdf 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -68,6 +68,7 @@ static int xen_suspend(void *data)
 	err = syscore_suspend();
 	if (err) {
 		pr_err("%s: system core suspend failed: %d\n", __func__, err);
+		early_resume_device_irqs();
 		return err;
 	}
 
@@ -92,6 +93,8 @@ static int xen_suspend(void *data)
 		xen_timer_resume();
 	}
 
+	early_resume_device_irqs();
+
 	syscore_resume();
 
 	return 0;
@@ -137,7 +140,7 @@ static void do_suspend(void)
 
 	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
 
-	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
+	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
 
 	if (err) {
 		pr_err("failed to start xen_suspend: %d\n", err);
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 698ad05..7f390e3 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
 /* The following three functions are for the core kernel use only. */
 extern void suspend_device_irqs(void);
 extern void resume_device_irqs(void);
+extern void early_resume_device_irqs(void);
 #ifdef CONFIG_PM_SLEEP
 extern int check_wakeup_irqs(void);
 #else
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 72c0fe0..ae5b26a 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -677,7 +677,7 @@ struct dev_pm_domain {
 
 #ifdef CONFIG_PM_SLEEP
 extern void device_pm_lock(void);
-extern void dpm_resume_start(pm_message_t state);
+extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
 extern void dpm_resume_end(pm_message_t state);
 extern void dpm_resume(pm_message_t state);
 extern void dpm_complete(pm_message_t state);
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index abcd6ca..b07dc9c 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
 }
 
 /**
- * irq_pm_syscore_ops - enable interrupt lines early
+ * early_resume_device_irqs - enable interrupt lines early
  *
  * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
  */
-static void irq_pm_syscore_resume(void)
+void early_resume_device_irqs(void)
 {
 	resume_irqs(true);
 }
-
-static struct syscore_ops irq_pm_syscore_ops = {
-	.resume		= irq_pm_syscore_resume,
-};
-
-static int __init irq_pm_init_ops(void)
-{
-	register_syscore_ops(&irq_pm_syscore_ops);
-	return 0;
-}
-
-device_initcall(irq_pm_init_ops);
+EXPORT_SYMBOL_GPL(early_resume_device_irqs);
 
 /**
  * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
diff --git a/kernel/kexec.c b/kernel/kexec.c
index 369f41a..272853b 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1700,7 +1700,7 @@ int kernel_kexec(void)
 		local_irq_enable();
  Enable_cpus:
 		enable_nonboot_cpus();
-		dpm_resume_start(PMSG_RESTORE);
+		dpm_resume_start(PMSG_RESTORE, true);
  Resume_devices:
 		dpm_resume_end(PMSG_RESTORE);
  Resume_console:
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index fcc2611..1d6dd56 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -325,7 +325,7 @@ static int create_image(int platform_mode)
 	platform_finish(platform_mode);
 
 	dpm_resume_start(in_suspend ?
-		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
+		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
 
 	return error;
 }
@@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
  Cleanup:
 	platform_restore_cleanup(platform_mode);
 
-	dpm_resume_start(PMSG_RECOVER);
+	dpm_resume_start(PMSG_RECOVER, true);
 
 	return error;
 }
@@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
  Platform_finish:
 	hibernation_ops->finish();
 
-	dpm_resume_start(PMSG_RESTORE);
+	dpm_resume_start(PMSG_RESTORE, true);
 
  Resume_devices:
 	entering_platform_hibernation = false;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 4dd8822..3597c72 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
 	if (need_suspend_ops(state) && suspend_ops->wake)
 		suspend_ops->wake();
 
-	dpm_resume_start(PMSG_RESUME);
+	dpm_resume_start(PMSG_RESUME, true);
 
  Platform_finish:
 	if (need_suspend_ops(state) && suspend_ops->finish)
-- 
2.0.0.526.g5318336

[PATCH v1 2/2] Revert "irq: Enable all irqs unconditionally in irq_resume"

[Derek Basehore]

This reverts the fix to IRQF_EARLY_RESUME irqs staying disabled after a suspend
failure. It incorrectly stated that Xen is the only platform that uses this
feature. Some rtc drivers such as rtc-as3722.c use the feature and can have its
irq permanently enabled with the change. The driver does disable/enable the irq
for the rtc alarm, so it needs a different fix which is in "genirq: Fix error
path for resuming irqs"

We should also keep correct enable/disable parity for irqs.

This reverts commit ac01810c9d2814238f08a227062e66a35a0e1ea2.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 kernel/irq/pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index b07dc9c..a5eaf1f 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -50,7 +50,7 @@ static void resume_irqs(bool want_early)
 		bool is_early = desc->action &&
 			desc->action->flags & IRQF_EARLY_RESUME;
 
-		if (!is_early && want_early)
+		if (is_early != want_early)
 			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
-- 
2.0.0.526.g5318336

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Konrad Rzeszutek Wilk]

On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
> In the case of a late abort to suspend/hibernate, irqs marked with
> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
> called on these paths.
> 
> This can happen with a pm test for platform, a late wakeup irq, and other
> instances. This change removes the function from syscore and calls it explicitly
> in suspend, hibernate, etc.
> 
> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

on the Xen side.
> ---
>  drivers/base/power/main.c |  5 ++++-
>  drivers/xen/manage.c      |  5 ++++-
>  include/linux/interrupt.h |  1 +
>  include/linux/pm.h        |  2 +-
>  kernel/irq/pm.c           | 17 +++--------------
>  kernel/kexec.c            |  2 +-
>  kernel/power/hibernate.c  |  6 +++---
>  kernel/power/suspend.c    |  2 +-
>  8 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index bf41296..a087473 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
>   * @state: PM transition of the system being carried out.
>   */
> -void dpm_resume_start(pm_message_t state)
> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
>  {
> +	if (enable_early_irqs)
> +		early_resume_device_irqs();
>  	dpm_resume_noirq(state);
>  	dpm_resume_early(state);
>  }
> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
>  	if (error) {
>  		suspend_stats.failed_suspend_noirq++;
>  		dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
> +		early_resume_device_irqs();
>  		dpm_resume_noirq(resume_event(state));
>  	} else {
>  		dpm_show_time(starttime, state, "noirq");
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index c3667b2..d387cdf 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
>  	err = syscore_suspend();
>  	if (err) {
>  		pr_err("%s: system core suspend failed: %d\n", __func__, err);
> +		early_resume_device_irqs();
>  		return err;
>  	}
>  
> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
>  		xen_timer_resume();
>  	}
>  
> +	early_resume_device_irqs();
> +
>  	syscore_resume();
>  
>  	return 0;
> @@ -137,7 +140,7 @@ static void do_suspend(void)
>  
>  	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
>  
> -	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
> +	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
>  
>  	if (err) {
>  		pr_err("failed to start xen_suspend: %d\n", err);
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 698ad05..7f390e3 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
>  /* The following three functions are for the core kernel use only. */
>  extern void suspend_device_irqs(void);
>  extern void resume_device_irqs(void);
> +extern void early_resume_device_irqs(void);
>  #ifdef CONFIG_PM_SLEEP
>  extern int check_wakeup_irqs(void);
>  #else
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 72c0fe0..ae5b26a 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -677,7 +677,7 @@ struct dev_pm_domain {
>  
>  #ifdef CONFIG_PM_SLEEP
>  extern void device_pm_lock(void);
> -extern void dpm_resume_start(pm_message_t state);
> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
>  extern void dpm_resume_end(pm_message_t state);
>  extern void dpm_resume(pm_message_t state);
>  extern void dpm_complete(pm_message_t state);
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index abcd6ca..b07dc9c 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
>  }
>  
>  /**
> - * irq_pm_syscore_ops - enable interrupt lines early
> + * early_resume_device_irqs - enable interrupt lines early
>   *
>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
>   */
> -static void irq_pm_syscore_resume(void)
> +void early_resume_device_irqs(void)
>  {
>  	resume_irqs(true);
>  }
> -
> -static struct syscore_ops irq_pm_syscore_ops = {
> -	.resume		= irq_pm_syscore_resume,
> -};
> -
> -static int __init irq_pm_init_ops(void)
> -{
> -	register_syscore_ops(&irq_pm_syscore_ops);
> -	return 0;
> -}
> -
> -device_initcall(irq_pm_init_ops);
> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
>  
>  /**
>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 369f41a..272853b 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
>  		local_irq_enable();
>   Enable_cpus:
>  		enable_nonboot_cpus();
> -		dpm_resume_start(PMSG_RESTORE);
> +		dpm_resume_start(PMSG_RESTORE, true);
>   Resume_devices:
>  		dpm_resume_end(PMSG_RESTORE);
>   Resume_console:
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index fcc2611..1d6dd56 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
>  	platform_finish(platform_mode);
>  
>  	dpm_resume_start(in_suspend ?
> -		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> +		(error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
>  
>  	return error;
>  }
> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
>   Cleanup:
>  	platform_restore_cleanup(platform_mode);
>  
> -	dpm_resume_start(PMSG_RECOVER);
> +	dpm_resume_start(PMSG_RECOVER, true);
>  
>  	return error;
>  }
> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
>   Platform_finish:
>  	hibernation_ops->finish();
>  
> -	dpm_resume_start(PMSG_RESTORE);
> +	dpm_resume_start(PMSG_RESTORE, true);
>  
>   Resume_devices:
>  	entering_platform_hibernation = false;
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4dd8822..3597c72 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>  	if (need_suspend_ops(state) && suspend_ops->wake)
>  		suspend_ops->wake();
>  
> -	dpm_resume_start(PMSG_RESUME);
> +	dpm_resume_start(PMSG_RESUME, true);
>  
>   Platform_finish:
>  	if (need_suspend_ops(state) && suspend_ops->finish)
> -- 
> 2.0.0.526.g5318336
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[dbasehore .]

Adding maintainers for affected systems to this CL for review.

On Mon, Jul 7, 2014 at 8:33 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
>> In the case of a late abort to suspend/hibernate, irqs marked with
>> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
>> called on these paths.
>>
>> This can happen with a pm test for platform, a late wakeup irq, and other
>> instances. This change removes the function from syscore and calls it explicitly
>> in suspend, hibernate, etc.
>>
>> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
>>
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> on the Xen side.
>> ---
>>  drivers/base/power/main.c |  5 ++++-
>>  drivers/xen/manage.c      |  5 ++++-
>>  include/linux/interrupt.h |  1 +
>>  include/linux/pm.h        |  2 +-
>>  kernel/irq/pm.c           | 17 +++--------------
>>  kernel/kexec.c            |  2 +-
>>  kernel/power/hibernate.c  |  6 +++---
>>  kernel/power/suspend.c    |  2 +-
>>  8 files changed, 18 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index bf41296..a087473 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
>>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
>>   * @state: PM transition of the system being carried out.
>>   */
>> -void dpm_resume_start(pm_message_t state)
>> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
>>  {
>> +     if (enable_early_irqs)
>> +             early_resume_device_irqs();
>>       dpm_resume_noirq(state);
>>       dpm_resume_early(state);
>>  }
>> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
>>       if (error) {
>>               suspend_stats.failed_suspend_noirq++;
>>               dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
>> +             early_resume_device_irqs();
>>               dpm_resume_noirq(resume_event(state));
>>       } else {
>>               dpm_show_time(starttime, state, "noirq");
>> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
>> index c3667b2..d387cdf 100644
>> --- a/drivers/xen/manage.c
>> +++ b/drivers/xen/manage.c
>> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
>>       err = syscore_suspend();
>>       if (err) {
>>               pr_err("%s: system core suspend failed: %d\n", __func__, err);
>> +             early_resume_device_irqs();
>>               return err;
>>       }
>>
>> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
>>               xen_timer_resume();
>>       }
>>
>> +     early_resume_device_irqs();
>> +
>>       syscore_resume();
>>
>>       return 0;
>> @@ -137,7 +140,7 @@ static void do_suspend(void)
>>
>>       raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
>>
>> -     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>> +     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
>>
>>       if (err) {
>>               pr_err("failed to start xen_suspend: %d\n", err);
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index 698ad05..7f390e3 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
>>  /* The following three functions are for the core kernel use only. */
>>  extern void suspend_device_irqs(void);
>>  extern void resume_device_irqs(void);
>> +extern void early_resume_device_irqs(void);
>>  #ifdef CONFIG_PM_SLEEP
>>  extern int check_wakeup_irqs(void);
>>  #else
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index 72c0fe0..ae5b26a 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -677,7 +677,7 @@ struct dev_pm_domain {
>>
>>  #ifdef CONFIG_PM_SLEEP
>>  extern void device_pm_lock(void);
>> -extern void dpm_resume_start(pm_message_t state);
>> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
>>  extern void dpm_resume_end(pm_message_t state);
>>  extern void dpm_resume(pm_message_t state);
>>  extern void dpm_complete(pm_message_t state);
>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>> index abcd6ca..b07dc9c 100644
>> --- a/kernel/irq/pm.c
>> +++ b/kernel/irq/pm.c
>> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
>>  }
>>
>>  /**
>> - * irq_pm_syscore_ops - enable interrupt lines early
>> + * early_resume_device_irqs - enable interrupt lines early
>>   *
>>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
>>   */
>> -static void irq_pm_syscore_resume(void)
>> +void early_resume_device_irqs(void)
>>  {
>>       resume_irqs(true);
>>  }
>> -
>> -static struct syscore_ops irq_pm_syscore_ops = {
>> -     .resume         = irq_pm_syscore_resume,
>> -};
>> -
>> -static int __init irq_pm_init_ops(void)
>> -{
>> -     register_syscore_ops(&irq_pm_syscore_ops);
>> -     return 0;
>> -}
>> -
>> -device_initcall(irq_pm_init_ops);
>> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
>>
>>  /**
>>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
>> diff --git a/kernel/kexec.c b/kernel/kexec.c
>> index 369f41a..272853b 100644
>> --- a/kernel/kexec.c
>> +++ b/kernel/kexec.c
>> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
>>               local_irq_enable();
>>   Enable_cpus:
>>               enable_nonboot_cpus();
>> -             dpm_resume_start(PMSG_RESTORE);
>> +             dpm_resume_start(PMSG_RESTORE, true);
>>   Resume_devices:
>>               dpm_resume_end(PMSG_RESTORE);
>>   Resume_console:
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index fcc2611..1d6dd56 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
>>       platform_finish(platform_mode);
>>
>>       dpm_resume_start(in_suspend ?
>> -             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
>> +             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
>>
>>       return error;
>>  }
>> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
>>   Cleanup:
>>       platform_restore_cleanup(platform_mode);
>>
>> -     dpm_resume_start(PMSG_RECOVER);
>> +     dpm_resume_start(PMSG_RECOVER, true);
>>
>>       return error;
>>  }
>> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
>>   Platform_finish:
>>       hibernation_ops->finish();
>>
>> -     dpm_resume_start(PMSG_RESTORE);
>> +     dpm_resume_start(PMSG_RESTORE, true);
>>
>>   Resume_devices:
>>       entering_platform_hibernation = false;
>> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>> index 4dd8822..3597c72 100644
>> --- a/kernel/power/suspend.c
>> +++ b/kernel/power/suspend.c
>> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>>       if (need_suspend_ops(state) && suspend_ops->wake)
>>               suspend_ops->wake();
>>
>> -     dpm_resume_start(PMSG_RESUME);
>> +     dpm_resume_start(PMSG_RESUME, true);
>>
>>   Platform_finish:
>>       if (need_suspend_ops(state) && suspend_ops->finish)
>> --
>> 2.0.0.526.g5318336
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Rafael J. Wysocki]

On Wednesday, October 01, 2014 01:48:39 PM dbasehore . wrote:
> Adding maintainers for affected systems to this CL for review.
> 
> On Mon, Jul 7, 2014 at 8:33 AM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
> >> In the case of a late abort to suspend/hibernate, irqs marked with
> >> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
> >> called on these paths.
> >>
> >> This can happen with a pm test for platform, a late wakeup irq, and other
> >> instances. This change removes the function from syscore and calls it explicitly
> >> in suspend, hibernate, etc.
> >>
> >> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
> >>
> >> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> >
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > on the Xen side.
> >> ---
> >>  drivers/base/power/main.c |  5 ++++-
> >>  drivers/xen/manage.c      |  5 ++++-
> >>  include/linux/interrupt.h |  1 +
> >>  include/linux/pm.h        |  2 +-
> >>  kernel/irq/pm.c           | 17 +++--------------
> >>  kernel/kexec.c            |  2 +-
> >>  kernel/power/hibernate.c  |  6 +++---
> >>  kernel/power/suspend.c    |  2 +-
> >>  8 files changed, 18 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index bf41296..a087473 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
> >>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
> >>   * @state: PM transition of the system being carried out.
> >>   */
> >> -void dpm_resume_start(pm_message_t state)
> >> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
> >>  {
> >> +     if (enable_early_irqs)
> >> +             early_resume_device_irqs();

This conflicts with some changes I've already queued up for merging.

Why don't you do that in dpm_resume_noirq() directly?  Also why do we need
the extra argument here?  The only case when we pass 'false' apprears to be
the Xen one, so perhaps we can use a special PM_EVENT_XEN flag or something
similar to indicate that?  Honestly, I don't want to litter the regular suspend
code with Xen-specific stuff like this.

> >>       dpm_resume_noirq(state);
> >>       dpm_resume_early(state);
> >>  }
> >> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
> >>       if (error) {
> >>               suspend_stats.failed_suspend_noirq++;
> >>               dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
> >> +             early_resume_device_irqs();
> >>               dpm_resume_noirq(resume_event(state));
> >>       } else {
> >>               dpm_show_time(starttime, state, "noirq");
> >> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> >> index c3667b2..d387cdf 100644
> >> --- a/drivers/xen/manage.c
> >> +++ b/drivers/xen/manage.c
> >> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
> >>       err = syscore_suspend();
> >>       if (err) {
> >>               pr_err("%s: system core suspend failed: %d\n", __func__, err);
> >> +             early_resume_device_irqs();
> >>               return err;
> >>       }
> >>
> >> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
> >>               xen_timer_resume();
> >>       }
> >>
> >> +     early_resume_device_irqs();
> >> +
> >>       syscore_resume();
> >>
> >>       return 0;
> >> @@ -137,7 +140,7 @@ static void do_suspend(void)
> >>
> >>       raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> >>
> >> -     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
> >> +     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
> >>
> >>       if (err) {
> >>               pr_err("failed to start xen_suspend: %d\n", err);
> >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> >> index 698ad05..7f390e3 100644
> >> --- a/include/linux/interrupt.h
> >> +++ b/include/linux/interrupt.h
> >> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
> >>  /* The following three functions are for the core kernel use only. */
> >>  extern void suspend_device_irqs(void);
> >>  extern void resume_device_irqs(void);
> >> +extern void early_resume_device_irqs(void);
> >>  #ifdef CONFIG_PM_SLEEP
> >>  extern int check_wakeup_irqs(void);
> >>  #else
> >> diff --git a/include/linux/pm.h b/include/linux/pm.h
> >> index 72c0fe0..ae5b26a 100644
> >> --- a/include/linux/pm.h
> >> +++ b/include/linux/pm.h
> >> @@ -677,7 +677,7 @@ struct dev_pm_domain {
> >>
> >>  #ifdef CONFIG_PM_SLEEP
> >>  extern void device_pm_lock(void);
> >> -extern void dpm_resume_start(pm_message_t state);
> >> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
> >>  extern void dpm_resume_end(pm_message_t state);
> >>  extern void dpm_resume(pm_message_t state);
> >>  extern void dpm_complete(pm_message_t state);
> >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> >> index abcd6ca..b07dc9c 100644
> >> --- a/kernel/irq/pm.c
> >> +++ b/kernel/irq/pm.c
> >> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
> >>  }
> >>
> >>  /**
> >> - * irq_pm_syscore_ops - enable interrupt lines early
> >> + * early_resume_device_irqs - enable interrupt lines early
> >>   *
> >>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
> >>   */
> >> -static void irq_pm_syscore_resume(void)
> >> +void early_resume_device_irqs(void)
> >>  {
> >>       resume_irqs(true);
> >>  }
> >> -
> >> -static struct syscore_ops irq_pm_syscore_ops = {
> >> -     .resume         = irq_pm_syscore_resume,
> >> -};
> >> -
> >> -static int __init irq_pm_init_ops(void)
> >> -{
> >> -     register_syscore_ops(&irq_pm_syscore_ops);
> >> -     return 0;
> >> -}
> >> -
> >> -device_initcall(irq_pm_init_ops);
> >> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
> >>
> >>  /**
> >>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
> >> diff --git a/kernel/kexec.c b/kernel/kexec.c
> >> index 369f41a..272853b 100644
> >> --- a/kernel/kexec.c
> >> +++ b/kernel/kexec.c
> >> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
> >>               local_irq_enable();
> >>   Enable_cpus:
> >>               enable_nonboot_cpus();
> >> -             dpm_resume_start(PMSG_RESTORE);
> >> +             dpm_resume_start(PMSG_RESTORE, true);
> >>   Resume_devices:
> >>               dpm_resume_end(PMSG_RESTORE);
> >>   Resume_console:
> >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >> index fcc2611..1d6dd56 100644
> >> --- a/kernel/power/hibernate.c
> >> +++ b/kernel/power/hibernate.c
> >> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
> >>       platform_finish(platform_mode);
> >>
> >>       dpm_resume_start(in_suspend ?
> >> -             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> >> +             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
> >>
> >>       return error;
> >>  }
> >> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
> >>   Cleanup:
> >>       platform_restore_cleanup(platform_mode);
> >>
> >> -     dpm_resume_start(PMSG_RECOVER);
> >> +     dpm_resume_start(PMSG_RECOVER, true);
> >>
> >>       return error;
> >>  }
> >> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
> >>   Platform_finish:
> >>       hibernation_ops->finish();
> >>
> >> -     dpm_resume_start(PMSG_RESTORE);
> >> +     dpm_resume_start(PMSG_RESTORE, true);
> >>
> >>   Resume_devices:
> >>       entering_platform_hibernation = false;
> >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> >> index 4dd8822..3597c72 100644
> >> --- a/kernel/power/suspend.c
> >> +++ b/kernel/power/suspend.c
> >> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> >>       if (need_suspend_ops(state) && suspend_ops->wake)
> >>               suspend_ops->wake();
> >>
> >> -     dpm_resume_start(PMSG_RESUME);
> >> +     dpm_resume_start(PMSG_RESUME, true);
> >>
> >>   Platform_finish:
> >>       if (need_suspend_ops(state) && suspend_ops->finish)
> >> --
> >> 2.0.0.526.g5318336
> >>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Rafael J. Wysocki]

On Thursday, October 02, 2014 12:25:08 AM Rafael J. Wysocki wrote:
> On Wednesday, October 01, 2014 01:48:39 PM dbasehore . wrote:
> > Adding maintainers for affected systems to this CL for review.
> > 
> > On Mon, Jul 7, 2014 at 8:33 AM, Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com> wrote:
> > > On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
> > >> In the case of a late abort to suspend/hibernate, irqs marked with
> > >> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
> > >> called on these paths.
> > >>
> > >> This can happen with a pm test for platform, a late wakeup irq, and other
> > >> instances. This change removes the function from syscore and calls it explicitly
> > >> in suspend, hibernate, etc.
> > >>
> > >> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
> > >>
> > >> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > >
> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >
> > > on the Xen side.
> > >> ---
> > >>  drivers/base/power/main.c |  5 ++++-
> > >>  drivers/xen/manage.c      |  5 ++++-
> > >>  include/linux/interrupt.h |  1 +
> > >>  include/linux/pm.h        |  2 +-
> > >>  kernel/irq/pm.c           | 17 +++--------------
> > >>  kernel/kexec.c            |  2 +-
> > >>  kernel/power/hibernate.c  |  6 +++---
> > >>  kernel/power/suspend.c    |  2 +-
> > >>  8 files changed, 18 insertions(+), 22 deletions(-)
> > >>
> > >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > >> index bf41296..a087473 100644
> > >> --- a/drivers/base/power/main.c
> > >> +++ b/drivers/base/power/main.c
> > >> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
> > >>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
> > >>   * @state: PM transition of the system being carried out.
> > >>   */
> > >> -void dpm_resume_start(pm_message_t state)
> > >> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
> > >>  {
> > >> +     if (enable_early_irqs)
> > >> +             early_resume_device_irqs();
> 
> This conflicts with some changes I've already queued up for merging.
> 
> Why don't you do that in dpm_resume_noirq() directly?  Also why do we need
> the extra argument here?  The only case when we pass 'false' apprears to be
> the Xen one, so perhaps we can use a special PM_EVENT_XEN flag or something
> similar to indicate that?  Honestly, I don't want to litter the regular suspend
> code with Xen-specific stuff like this.

BTW, is dpm_resume_noirq() early enough?  What about the time we bring up
the non-boot CPUs?  Don't we need to call the early_resume_device_irqs()
thing before that?

> > >>       dpm_resume_noirq(state);
> > >>       dpm_resume_early(state);
> > >>  }
> > >> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
> > >>       if (error) {
> > >>               suspend_stats.failed_suspend_noirq++;
> > >>               dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
> > >> +             early_resume_device_irqs();
> > >>               dpm_resume_noirq(resume_event(state));
> > >>       } else {
> > >>               dpm_show_time(starttime, state, "noirq");
> > >> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > >> index c3667b2..d387cdf 100644
> > >> --- a/drivers/xen/manage.c
> > >> +++ b/drivers/xen/manage.c
> > >> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
> > >>       err = syscore_suspend();
> > >>       if (err) {
> > >>               pr_err("%s: system core suspend failed: %d\n", __func__, err);
> > >> +             early_resume_device_irqs();
> > >>               return err;
> > >>       }
> > >>
> > >> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
> > >>               xen_timer_resume();
> > >>       }
> > >>
> > >> +     early_resume_device_irqs();
> > >> +
> > >>       syscore_resume();
> > >>
> > >>       return 0;
> > >> @@ -137,7 +140,7 @@ static void do_suspend(void)
> > >>
> > >>       raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> > >>
> > >> -     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
> > >> +     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
> > >>
> > >>       if (err) {
> > >>               pr_err("failed to start xen_suspend: %d\n", err);
> > >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > >> index 698ad05..7f390e3 100644
> > >> --- a/include/linux/interrupt.h
> > >> +++ b/include/linux/interrupt.h
> > >> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
> > >>  /* The following three functions are for the core kernel use only. */
> > >>  extern void suspend_device_irqs(void);
> > >>  extern void resume_device_irqs(void);
> > >> +extern void early_resume_device_irqs(void);
> > >>  #ifdef CONFIG_PM_SLEEP
> > >>  extern int check_wakeup_irqs(void);
> > >>  #else
> > >> diff --git a/include/linux/pm.h b/include/linux/pm.h
> > >> index 72c0fe0..ae5b26a 100644
> > >> --- a/include/linux/pm.h
> > >> +++ b/include/linux/pm.h
> > >> @@ -677,7 +677,7 @@ struct dev_pm_domain {
> > >>
> > >>  #ifdef CONFIG_PM_SLEEP
> > >>  extern void device_pm_lock(void);
> > >> -extern void dpm_resume_start(pm_message_t state);
> > >> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
> > >>  extern void dpm_resume_end(pm_message_t state);
> > >>  extern void dpm_resume(pm_message_t state);
> > >>  extern void dpm_complete(pm_message_t state);
> > >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > >> index abcd6ca..b07dc9c 100644
> > >> --- a/kernel/irq/pm.c
> > >> +++ b/kernel/irq/pm.c
> > >> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
> > >>  }
> > >>
> > >>  /**
> > >> - * irq_pm_syscore_ops - enable interrupt lines early
> > >> + * early_resume_device_irqs - enable interrupt lines early
> > >>   *
> > >>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
> > >>   */
> > >> -static void irq_pm_syscore_resume(void)
> > >> +void early_resume_device_irqs(void)
> > >>  {
> > >>       resume_irqs(true);
> > >>  }
> > >> -
> > >> -static struct syscore_ops irq_pm_syscore_ops = {
> > >> -     .resume         = irq_pm_syscore_resume,
> > >> -};
> > >> -
> > >> -static int __init irq_pm_init_ops(void)
> > >> -{
> > >> -     register_syscore_ops(&irq_pm_syscore_ops);
> > >> -     return 0;
> > >> -}
> > >> -
> > >> -device_initcall(irq_pm_init_ops);
> > >> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
> > >>
> > >>  /**
> > >>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
> > >> diff --git a/kernel/kexec.c b/kernel/kexec.c
> > >> index 369f41a..272853b 100644
> > >> --- a/kernel/kexec.c
> > >> +++ b/kernel/kexec.c
> > >> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
> > >>               local_irq_enable();
> > >>   Enable_cpus:
> > >>               enable_nonboot_cpus();
> > >> -             dpm_resume_start(PMSG_RESTORE);
> > >> +             dpm_resume_start(PMSG_RESTORE, true);
> > >>   Resume_devices:
> > >>               dpm_resume_end(PMSG_RESTORE);
> > >>   Resume_console:
> > >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > >> index fcc2611..1d6dd56 100644
> > >> --- a/kernel/power/hibernate.c
> > >> +++ b/kernel/power/hibernate.c
> > >> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
> > >>       platform_finish(platform_mode);
> > >>
> > >>       dpm_resume_start(in_suspend ?
> > >> -             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> > >> +             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
> > >>
> > >>       return error;
> > >>  }
> > >> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
> > >>   Cleanup:
> > >>       platform_restore_cleanup(platform_mode);
> > >>
> > >> -     dpm_resume_start(PMSG_RECOVER);
> > >> +     dpm_resume_start(PMSG_RECOVER, true);
> > >>
> > >>       return error;
> > >>  }
> > >> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
> > >>   Platform_finish:
> > >>       hibernation_ops->finish();
> > >>
> > >> -     dpm_resume_start(PMSG_RESTORE);
> > >> +     dpm_resume_start(PMSG_RESTORE, true);
> > >>
> > >>   Resume_devices:
> > >>       entering_platform_hibernation = false;
> > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > >> index 4dd8822..3597c72 100644
> > >> --- a/kernel/power/suspend.c
> > >> +++ b/kernel/power/suspend.c
> > >> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> > >>       if (need_suspend_ops(state) && suspend_ops->wake)
> > >>               suspend_ops->wake();
> > >>
> > >> -     dpm_resume_start(PMSG_RESUME);
> > >> +     dpm_resume_start(PMSG_RESUME, true);
> > >>
> > >>   Platform_finish:
> > >>       if (need_suspend_ops(state) && suspend_ops->finish)
> > >> --
> > >> 2.0.0.526.g5318336
> > >>
> > >>
> > >> _______________________________________________
> > >> Xen-devel mailing list
> > >> Xen-devel@lists.xen.org
> > >> http://lists.xen.org/xen-devel
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[dbasehore .]

dpm_resume_noirq is not early enough for the Xen stuff, but should be
early enough for other stuff. This patch is mostly just a bandage on
top of the broken IRQF_EARLY_RESUME code.

We may consider getting rid of IRQF_EARLY_RESUME and having Xen
register its own syscore resume function to enable whatever irq it
needs. I'd be fine with that since some rtc drivers started using
IRQF_EARLY_RESUME. I can't think of any reason those drivers would
need to be resumed early. This way, the flag wouldn't even be there
for people to mistakenly add.

On Wed, Oct 1, 2014 at 3:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, October 02, 2014 12:25:08 AM Rafael J. Wysocki wrote:
>> On Wednesday, October 01, 2014 01:48:39 PM dbasehore . wrote:
>> > Adding maintainers for affected systems to this CL for review.
>> >
>> > On Mon, Jul 7, 2014 at 8:33 AM, Konrad Rzeszutek Wilk
>> > <konrad.wilk@oracle.com> wrote:
>> > > On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
>> > >> In the case of a late abort to suspend/hibernate, irqs marked with
>> > >> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
>> > >> called on these paths.
>> > >>
>> > >> This can happen with a pm test for platform, a late wakeup irq, and other
>> > >> instances. This change removes the function from syscore and calls it explicitly
>> > >> in suspend, hibernate, etc.
>> > >>
>> > >> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
>> > >>
>> > >> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> > >
>> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > >
>> > > on the Xen side.
>> > >> ---
>> > >>  drivers/base/power/main.c |  5 ++++-
>> > >>  drivers/xen/manage.c      |  5 ++++-
>> > >>  include/linux/interrupt.h |  1 +
>> > >>  include/linux/pm.h        |  2 +-
>> > >>  kernel/irq/pm.c           | 17 +++--------------
>> > >>  kernel/kexec.c            |  2 +-
>> > >>  kernel/power/hibernate.c  |  6 +++---
>> > >>  kernel/power/suspend.c    |  2 +-
>> > >>  8 files changed, 18 insertions(+), 22 deletions(-)
>> > >>
>> > >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> > >> index bf41296..a087473 100644
>> > >> --- a/drivers/base/power/main.c
>> > >> +++ b/drivers/base/power/main.c
>> > >> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
>> > >>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
>> > >>   * @state: PM transition of the system being carried out.
>> > >>   */
>> > >> -void dpm_resume_start(pm_message_t state)
>> > >> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
>> > >>  {
>> > >> +     if (enable_early_irqs)
>> > >> +             early_resume_device_irqs();
>>
>> This conflicts with some changes I've already queued up for merging.
>>
>> Why don't you do that in dpm_resume_noirq() directly?  Also why do we need
>> the extra argument here?  The only case when we pass 'false' apprears to be
>> the Xen one, so perhaps we can use a special PM_EVENT_XEN flag or something
>> similar to indicate that?  Honestly, I don't want to litter the regular suspend
>> code with Xen-specific stuff like this.
>
> BTW, is dpm_resume_noirq() early enough?  What about the time we bring up
> the non-boot CPUs?  Don't we need to call the early_resume_device_irqs()
> thing before that?
>
>> > >>       dpm_resume_noirq(state);
>> > >>       dpm_resume_early(state);
>> > >>  }
>> > >> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
>> > >>       if (error) {
>> > >>               suspend_stats.failed_suspend_noirq++;
>> > >>               dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
>> > >> +             early_resume_device_irqs();
>> > >>               dpm_resume_noirq(resume_event(state));
>> > >>       } else {
>> > >>               dpm_show_time(starttime, state, "noirq");
>> > >> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
>> > >> index c3667b2..d387cdf 100644
>> > >> --- a/drivers/xen/manage.c
>> > >> +++ b/drivers/xen/manage.c
>> > >> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
>> > >>       err = syscore_suspend();
>> > >>       if (err) {
>> > >>               pr_err("%s: system core suspend failed: %d\n", __func__, err);
>> > >> +             early_resume_device_irqs();
>> > >>               return err;
>> > >>       }
>> > >>
>> > >> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
>> > >>               xen_timer_resume();
>> > >>       }
>> > >>
>> > >> +     early_resume_device_irqs();
>> > >> +
>> > >>       syscore_resume();
>> > >>
>> > >>       return 0;
>> > >> @@ -137,7 +140,7 @@ static void do_suspend(void)
>> > >>
>> > >>       raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
>> > >>
>> > >> -     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>> > >> +     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
>> > >>
>> > >>       if (err) {
>> > >>               pr_err("failed to start xen_suspend: %d\n", err);
>> > >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> > >> index 698ad05..7f390e3 100644
>> > >> --- a/include/linux/interrupt.h
>> > >> +++ b/include/linux/interrupt.h
>> > >> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
>> > >>  /* The following three functions are for the core kernel use only. */
>> > >>  extern void suspend_device_irqs(void);
>> > >>  extern void resume_device_irqs(void);
>> > >> +extern void early_resume_device_irqs(void);
>> > >>  #ifdef CONFIG_PM_SLEEP
>> > >>  extern int check_wakeup_irqs(void);
>> > >>  #else
>> > >> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> > >> index 72c0fe0..ae5b26a 100644
>> > >> --- a/include/linux/pm.h
>> > >> +++ b/include/linux/pm.h
>> > >> @@ -677,7 +677,7 @@ struct dev_pm_domain {
>> > >>
>> > >>  #ifdef CONFIG_PM_SLEEP
>> > >>  extern void device_pm_lock(void);
>> > >> -extern void dpm_resume_start(pm_message_t state);
>> > >> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
>> > >>  extern void dpm_resume_end(pm_message_t state);
>> > >>  extern void dpm_resume(pm_message_t state);
>> > >>  extern void dpm_complete(pm_message_t state);
>> > >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>> > >> index abcd6ca..b07dc9c 100644
>> > >> --- a/kernel/irq/pm.c
>> > >> +++ b/kernel/irq/pm.c
>> > >> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
>> > >>  }
>> > >>
>> > >>  /**
>> > >> - * irq_pm_syscore_ops - enable interrupt lines early
>> > >> + * early_resume_device_irqs - enable interrupt lines early
>> > >>   *
>> > >>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
>> > >>   */
>> > >> -static void irq_pm_syscore_resume(void)
>> > >> +void early_resume_device_irqs(void)
>> > >>  {
>> > >>       resume_irqs(true);
>> > >>  }
>> > >> -
>> > >> -static struct syscore_ops irq_pm_syscore_ops = {
>> > >> -     .resume         = irq_pm_syscore_resume,
>> > >> -};
>> > >> -
>> > >> -static int __init irq_pm_init_ops(void)
>> > >> -{
>> > >> -     register_syscore_ops(&irq_pm_syscore_ops);
>> > >> -     return 0;
>> > >> -}
>> > >> -
>> > >> -device_initcall(irq_pm_init_ops);
>> > >> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
>> > >>
>> > >>  /**
>> > >>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
>> > >> diff --git a/kernel/kexec.c b/kernel/kexec.c
>> > >> index 369f41a..272853b 100644
>> > >> --- a/kernel/kexec.c
>> > >> +++ b/kernel/kexec.c
>> > >> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
>> > >>               local_irq_enable();
>> > >>   Enable_cpus:
>> > >>               enable_nonboot_cpus();
>> > >> -             dpm_resume_start(PMSG_RESTORE);
>> > >> +             dpm_resume_start(PMSG_RESTORE, true);
>> > >>   Resume_devices:
>> > >>               dpm_resume_end(PMSG_RESTORE);
>> > >>   Resume_console:
>> > >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> > >> index fcc2611..1d6dd56 100644
>> > >> --- a/kernel/power/hibernate.c
>> > >> +++ b/kernel/power/hibernate.c
>> > >> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
>> > >>       platform_finish(platform_mode);
>> > >>
>> > >>       dpm_resume_start(in_suspend ?
>> > >> -             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
>> > >> +             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
>> > >>
>> > >>       return error;
>> > >>  }
>> > >> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
>> > >>   Cleanup:
>> > >>       platform_restore_cleanup(platform_mode);
>> > >>
>> > >> -     dpm_resume_start(PMSG_RECOVER);
>> > >> +     dpm_resume_start(PMSG_RECOVER, true);
>> > >>
>> > >>       return error;
>> > >>  }
>> > >> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
>> > >>   Platform_finish:
>> > >>       hibernation_ops->finish();
>> > >>
>> > >> -     dpm_resume_start(PMSG_RESTORE);
>> > >> +     dpm_resume_start(PMSG_RESTORE, true);
>> > >>
>> > >>   Resume_devices:
>> > >>       entering_platform_hibernation = false;
>> > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
>> > >> index 4dd8822..3597c72 100644
>> > >> --- a/kernel/power/suspend.c
>> > >> +++ b/kernel/power/suspend.c
>> > >> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
>> > >>       if (need_suspend_ops(state) && suspend_ops->wake)
>> > >>               suspend_ops->wake();
>> > >>
>> > >> -     dpm_resume_start(PMSG_RESUME);
>> > >> +     dpm_resume_start(PMSG_RESUME, true);
>> > >>
>> > >>   Platform_finish:
>> > >>       if (need_suspend_ops(state) && suspend_ops->finish)
>> > >> --
>> > >> 2.0.0.526.g5318336
>> > >>
>> > >>
>> > >> _______________________________________________
>> > >> Xen-devel mailing list
>> > >> Xen-devel@lists.xen.org
>> > >> http://lists.xen.org/xen-devel
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Rafael J. Wysocki]

On Wednesday, October 01, 2014 04:01:33 PM dbasehore . wrote:
> dpm_resume_noirq is not early enough for the Xen stuff, but should be
> early enough for other stuff. This patch is mostly just a bandage on
> top of the broken IRQF_EARLY_RESUME code.
> 
> We may consider getting rid of IRQF_EARLY_RESUME and having Xen
> register its own syscore resume function to enable whatever irq it
> needs.

I'd very much prefer that.

> I'd be fine with that since some rtc drivers started using
> IRQF_EARLY_RESUME. I can't think of any reason those drivers would
> need to be resumed early. This way, the flag wouldn't even be there
> for people to mistakenly add.

There seem to be some ordering issues they are trying to solve this way,
but I think those issues can and should be addressed differently.

> On Wed, Oct 1, 2014 at 3:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, October 02, 2014 12:25:08 AM Rafael J. Wysocki wrote:
> >> On Wednesday, October 01, 2014 01:48:39 PM dbasehore . wrote:
> >> > Adding maintainers for affected systems to this CL for review.
> >> >
> >> > On Mon, Jul 7, 2014 at 8:33 AM, Konrad Rzeszutek Wilk
> >> > <konrad.wilk@oracle.com> wrote:
> >> > > On Fri, Jun 27, 2014 at 05:04:24PM -0700, Derek Basehore wrote:
> >> > >> In the case of a late abort to suspend/hibernate, irqs marked with
> >> > >> IRQF_EARLY_RESUME will not be enabled. This is due to syscore_resume not getting
> >> > >> called on these paths.
> >> > >>
> >> > >> This can happen with a pm test for platform, a late wakeup irq, and other
> >> > >> instances. This change removes the function from syscore and calls it explicitly
> >> > >> in suspend, hibernate, etc.
> >> > >>
> >> > >> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"
> >> > >>
> >> > >> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> >> > >
> >> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> > >
> >> > > on the Xen side.
> >> > >> ---
> >> > >>  drivers/base/power/main.c |  5 ++++-
> >> > >>  drivers/xen/manage.c      |  5 ++++-
> >> > >>  include/linux/interrupt.h |  1 +
> >> > >>  include/linux/pm.h        |  2 +-
> >> > >>  kernel/irq/pm.c           | 17 +++--------------
> >> > >>  kernel/kexec.c            |  2 +-
> >> > >>  kernel/power/hibernate.c  |  6 +++---
> >> > >>  kernel/power/suspend.c    |  2 +-
> >> > >>  8 files changed, 18 insertions(+), 22 deletions(-)
> >> > >>
> >> > >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> > >> index bf41296..a087473 100644
> >> > >> --- a/drivers/base/power/main.c
> >> > >> +++ b/drivers/base/power/main.c
> >> > >> @@ -712,8 +712,10 @@ static void dpm_resume_early(pm_message_t state)
> >> > >>   * dpm_resume_start - Execute "noirq" and "early" device callbacks.
> >> > >>   * @state: PM transition of the system being carried out.
> >> > >>   */
> >> > >> -void dpm_resume_start(pm_message_t state)
> >> > >> +void dpm_resume_start(pm_message_t state, bool enable_early_irqs)
> >> > >>  {
> >> > >> +     if (enable_early_irqs)
> >> > >> +             early_resume_device_irqs();
> >>
> >> This conflicts with some changes I've already queued up for merging.
> >>
> >> Why don't you do that in dpm_resume_noirq() directly?  Also why do we need
> >> the extra argument here?  The only case when we pass 'false' apprears to be
> >> the Xen one, so perhaps we can use a special PM_EVENT_XEN flag or something
> >> similar to indicate that?  Honestly, I don't want to litter the regular suspend
> >> code with Xen-specific stuff like this.
> >
> > BTW, is dpm_resume_noirq() early enough?  What about the time we bring up
> > the non-boot CPUs?  Don't we need to call the early_resume_device_irqs()
> > thing before that?
> >
> >> > >>       dpm_resume_noirq(state);
> >> > >>       dpm_resume_early(state);
> >> > >>  }
> >> > >> @@ -1132,6 +1134,7 @@ static int dpm_suspend_noirq(pm_message_t state)
> >> > >>       if (error) {
> >> > >>               suspend_stats.failed_suspend_noirq++;
> >> > >>               dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
> >> > >> +             early_resume_device_irqs();
> >> > >>               dpm_resume_noirq(resume_event(state));
> >> > >>       } else {
> >> > >>               dpm_show_time(starttime, state, "noirq");
> >> > >> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> >> > >> index c3667b2..d387cdf 100644
> >> > >> --- a/drivers/xen/manage.c
> >> > >> +++ b/drivers/xen/manage.c
> >> > >> @@ -68,6 +68,7 @@ static int xen_suspend(void *data)
> >> > >>       err = syscore_suspend();
> >> > >>       if (err) {
> >> > >>               pr_err("%s: system core suspend failed: %d\n", __func__, err);
> >> > >> +             early_resume_device_irqs();
> >> > >>               return err;
> >> > >>       }
> >> > >>
> >> > >> @@ -92,6 +93,8 @@ static int xen_suspend(void *data)
> >> > >>               xen_timer_resume();
> >> > >>       }
> >> > >>
> >> > >> +     early_resume_device_irqs();
> >> > >> +
> >> > >>       syscore_resume();
> >> > >>
> >> > >>       return 0;
> >> > >> @@ -137,7 +140,7 @@ static void do_suspend(void)
> >> > >>
> >> > >>       raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> >> > >>
> >> > >> -     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
> >> > >> +     dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE, false);
> >> > >>
> >> > >>       if (err) {
> >> > >>               pr_err("failed to start xen_suspend: %d\n", err);
> >> > >> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> >> > >> index 698ad05..7f390e3 100644
> >> > >> --- a/include/linux/interrupt.h
> >> > >> +++ b/include/linux/interrupt.h
> >> > >> @@ -193,6 +193,7 @@ extern void irq_wake_thread(unsigned int irq, void *dev_id);
> >> > >>  /* The following three functions are for the core kernel use only. */
> >> > >>  extern void suspend_device_irqs(void);
> >> > >>  extern void resume_device_irqs(void);
> >> > >> +extern void early_resume_device_irqs(void);
> >> > >>  #ifdef CONFIG_PM_SLEEP
> >> > >>  extern int check_wakeup_irqs(void);
> >> > >>  #else
> >> > >> diff --git a/include/linux/pm.h b/include/linux/pm.h
> >> > >> index 72c0fe0..ae5b26a 100644
> >> > >> --- a/include/linux/pm.h
> >> > >> +++ b/include/linux/pm.h
> >> > >> @@ -677,7 +677,7 @@ struct dev_pm_domain {
> >> > >>
> >> > >>  #ifdef CONFIG_PM_SLEEP
> >> > >>  extern void device_pm_lock(void);
> >> > >> -extern void dpm_resume_start(pm_message_t state);
> >> > >> +extern void dpm_resume_start(pm_message_t state, bool enable_early_irqs);
> >> > >>  extern void dpm_resume_end(pm_message_t state);
> >> > >>  extern void dpm_resume(pm_message_t state);
> >> > >>  extern void dpm_complete(pm_message_t state);
> >> > >> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> >> > >> index abcd6ca..b07dc9c 100644
> >> > >> --- a/kernel/irq/pm.c
> >> > >> +++ b/kernel/irq/pm.c
> >> > >> @@ -60,26 +60,15 @@ static void resume_irqs(bool want_early)
> >> > >>  }
> >> > >>
> >> > >>  /**
> >> > >> - * irq_pm_syscore_ops - enable interrupt lines early
> >> > >> + * early_resume_device_irqs - enable interrupt lines early
> >> > >>   *
> >> > >>   * Enable all interrupt lines with %IRQF_EARLY_RESUME set.
> >> > >>   */
> >> > >> -static void irq_pm_syscore_resume(void)
> >> > >> +void early_resume_device_irqs(void)
> >> > >>  {
> >> > >>       resume_irqs(true);
> >> > >>  }
> >> > >> -
> >> > >> -static struct syscore_ops irq_pm_syscore_ops = {
> >> > >> -     .resume         = irq_pm_syscore_resume,
> >> > >> -};
> >> > >> -
> >> > >> -static int __init irq_pm_init_ops(void)
> >> > >> -{
> >> > >> -     register_syscore_ops(&irq_pm_syscore_ops);
> >> > >> -     return 0;
> >> > >> -}
> >> > >> -
> >> > >> -device_initcall(irq_pm_init_ops);
> >> > >> +EXPORT_SYMBOL_GPL(early_resume_device_irqs);
> >> > >>
> >> > >>  /**
> >> > >>   * resume_device_irqs - enable interrupt lines disabled by suspend_device_irqs()
> >> > >> diff --git a/kernel/kexec.c b/kernel/kexec.c
> >> > >> index 369f41a..272853b 100644
> >> > >> --- a/kernel/kexec.c
> >> > >> +++ b/kernel/kexec.c
> >> > >> @@ -1700,7 +1700,7 @@ int kernel_kexec(void)
> >> > >>               local_irq_enable();
> >> > >>   Enable_cpus:
> >> > >>               enable_nonboot_cpus();
> >> > >> -             dpm_resume_start(PMSG_RESTORE);
> >> > >> +             dpm_resume_start(PMSG_RESTORE, true);
> >> > >>   Resume_devices:
> >> > >>               dpm_resume_end(PMSG_RESTORE);
> >> > >>   Resume_console:
> >> > >> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> >> > >> index fcc2611..1d6dd56 100644
> >> > >> --- a/kernel/power/hibernate.c
> >> > >> +++ b/kernel/power/hibernate.c
> >> > >> @@ -325,7 +325,7 @@ static int create_image(int platform_mode)
> >> > >>       platform_finish(platform_mode);
> >> > >>
> >> > >>       dpm_resume_start(in_suspend ?
> >> > >> -             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE);
> >> > >> +             (error ? PMSG_RECOVER : PMSG_THAW) : PMSG_RESTORE, true);
> >> > >>
> >> > >>       return error;
> >> > >>  }
> >> > >> @@ -482,7 +482,7 @@ static int resume_target_kernel(bool platform_mode)
> >> > >>   Cleanup:
> >> > >>       platform_restore_cleanup(platform_mode);
> >> > >>
> >> > >> -     dpm_resume_start(PMSG_RECOVER);
> >> > >> +     dpm_resume_start(PMSG_RECOVER, true);
> >> > >>
> >> > >>       return error;
> >> > >>  }
> >> > >> @@ -574,7 +574,7 @@ int hibernation_platform_enter(void)
> >> > >>   Platform_finish:
> >> > >>       hibernation_ops->finish();
> >> > >>
> >> > >> -     dpm_resume_start(PMSG_RESTORE);
> >> > >> +     dpm_resume_start(PMSG_RESTORE, true);
> >> > >>
> >> > >>   Resume_devices:
> >> > >>       entering_platform_hibernation = false;
> >> > >> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> >> > >> index 4dd8822..3597c72 100644
> >> > >> --- a/kernel/power/suspend.c
> >> > >> +++ b/kernel/power/suspend.c
> >> > >> @@ -281,7 +281,7 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> >> > >>       if (need_suspend_ops(state) && suspend_ops->wake)
> >> > >>               suspend_ops->wake();
> >> > >>
> >> > >> -     dpm_resume_start(PMSG_RESUME);
> >> > >> +     dpm_resume_start(PMSG_RESUME, true);
> >> > >>
> >> > >>   Platform_finish:
> >> > >>       if (need_suspend_ops(state) && suspend_ops->finish)
> >> > >> --
> >> > >> 2.0.0.526.g5318336
> >> > >>
> >> > >>
> >> > >> _______________________________________________
> >> > >> Xen-devel mailing list
> >> > >> Xen-devel@lists.xen.org
> >> > >> http://lists.xen.org/xen-devel
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at  http://www.tux.org/lkml/
> >>
> >>
> >
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Xen-devel] [PATCH v1 1/2] genirq: Fix error path for resuming irqs

[Thomas Gleixner]

On Wed, 1 Oct 2014, dbasehore . wrote:
> On Wed, Oct 1, 2014 at 3:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

Please stop top-posting and trim the quoted text. This is not your
$corp mail drop.

> dpm_resume_noirq is not early enough for the Xen stuff, but should be
> early enough for other stuff. This patch is mostly just a bandage on
> top of the broken IRQF_EARLY_RESUME code.

You are getting it really backwards.

The IRQF_EARLY_RESUME stuff was introduced on behalf of XEN and now
you are claiming in your changelog that:
 
> >> > >> This regression was introduced in 9bab0b7f "genirq: Add IRQF_RESUME_EARLY"

So you are fixing a 3+ years old regression here? Do you have any
prove that the code worked before that commit 9bab0b7f went in during
the 3.2 merge window?

No, you don't. Simply because the XEN suspend/resume stuff was not
working at all before that. So it's not a regression.

It either was a wrong design decision back then which did not take an
error path into account or the things have changed in a way that this
mechanism does not work anymore.

I agree with Rafael that this should be solved differently. Though I'm
not really convinced of the proposed syscore solution, simply because
it will introduce another "tolerated" way for people to work around
totally unrelated shortcomings of other parts of the suspend/resume
machinery.

You noticed yourself that

> .... some rtc drivers started using IRQF_EARLY_RESUME. I can't think
> of any reason those drivers would need to be resumed early. This
> way, the flag wouldn't even be there for people to mistakenly add.

Now the question is WHY XEN is so special that it needs all these
extra features and mechanisms all over the place?

I have not looked into the guts of XEN that deep that I can judge that
and I have no intention to do so as I want to preserve my mental
sanity, but I have not seen any reasonable explanation WHY:

> dpm_resume_noirq is not early enough for the Xen stuff

Is it just because XEN works that way and XEN folks are not willing or
able to make it different? Or is there any fundamental and reasonable
technical reason why XEN needs to have the extra treatment while the
rest of the world does not?

Thanks,

	tglx