[BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Stanislaw Gruszka]

Hi,

We have this bug report on our bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=1058268

In short:
WARNING: CPU: 0 PID: 6733 at drivers/base/syscore.c:104 syscore_resume+0x9a/0xe0()
Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

Perhaps I'm wrong, but I think sysops->resume() callback should be
atomic i.e. can not use mutexes or kmalloc(,GFP_KERNEL), what is not
true regarding xen_acpi_processor_resume(). That callback was introduced
by commit 3fac10145b766a2244422788f62dc35978613fd8. Fixing that will
not be easy IMHO, but maybe you have some ideas ? :-)

Stanislaw

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Konrad Rzeszutek Wilk]

On Tue, Jan 28, 2014 at 04:08:49PM +0100, Stanislaw Gruszka wrote:
> Hi,

Hey!
> 
> We have this bug report on our bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=1058268
> 
> In short:
> WARNING: CPU: 0 PID: 6733 at drivers/base/syscore.c:104 syscore_resume+0x9a/0xe0()
> Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]
> 
> Perhaps I'm wrong, but I think sysops->resume() callback should be
> atomic i.e. can not use mutexes or kmalloc(,GFP_KERNEL), what is not
> true regarding xen_acpi_processor_resume(). That callback was introduced
> by commit 3fac10145b766a2244422788f62dc35978613fd8. Fixing that will
> not be easy IMHO, but maybe you have some ideas ? :-)

If that is the case, then perhaps the patch below will fix it?

(I didn't compile tested it)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 7231859..7602229 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -46,7 +46,7 @@ module_param_named(off, no_hypercall, int, 0400);
  */
 static unsigned int nr_acpi_bits;
 /* Mutex to protect the acpi_ids_done - for CPU hotplug use. */
-static DEFINE_MUTEX(acpi_ids_mutex);
+static DEFINE_SPINLOCK(acpi_ids_lock);
 /* Which ACPI ID we have processed from 'struct acpi_processor'. */
 static unsigned long *acpi_ids_done;
 /* Which ACPI ID exist in the SSDT/DSDT processor definitions. */
@@ -68,7 +68,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
 	int ret = 0;
 
 	dst_cx_states = kcalloc(_pr->power.count,
-				sizeof(struct xen_processor_cx), GFP_KERNEL);
+				sizeof(struct xen_processor_cx), GFP_ATOMIC);
 	if (!dst_cx_states)
 		return -ENOMEM;
 
@@ -149,7 +149,7 @@ xen_copy_pss_data(struct acpi_processor *_pr,
 		     sizeof(struct acpi_processor_px));
 
 	dst_states = kcalloc(_pr->performance->state_count,
-			     sizeof(struct xen_processor_px), GFP_KERNEL);
+			     sizeof(struct xen_processor_px), GFP_ATOMIC);
 	if (!dst_states)
 		return ERR_PTR(-ENOMEM);
 
@@ -275,9 +275,9 @@ static int upload_pm_data(struct acpi_processor *_pr)
 {
 	int err = 0;
 
-	mutex_lock(&acpi_ids_mutex);
+	spin_lock(&acpi_ids_lock);
 	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
-		mutex_unlock(&acpi_ids_mutex);
+		spin_unlock(&acpi_ids_lock);
 		return -EBUSY;
 	}
 	if (_pr->flags.power)
@@ -286,7 +286,7 @@ static int upload_pm_data(struct acpi_processor *_pr)
 	if (_pr->performance && _pr->performance->states)
 		err |= push_pxx_to_hypervisor(_pr);
 
-	mutex_unlock(&acpi_ids_mutex);
+	spin_unlock(&acpi_ids_lock);
 	return err;
 }
 static unsigned int __init get_max_acpi_id(void)

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[David Rientjes]

On Tue, 28 Jan 2014, Konrad Rzeszutek Wilk wrote:

> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 7231859..7602229 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -46,7 +46,7 @@ module_param_named(off, no_hypercall, int, 0400);
>   */
>  static unsigned int nr_acpi_bits;
>  /* Mutex to protect the acpi_ids_done - for CPU hotplug use. */
> -static DEFINE_MUTEX(acpi_ids_mutex);
> +static DEFINE_SPINLOCK(acpi_ids_lock);
>  /* Which ACPI ID we have processed from 'struct acpi_processor'. */
>  static unsigned long *acpi_ids_done;
>  /* Which ACPI ID exist in the SSDT/DSDT processor definitions. */
> @@ -68,7 +68,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
>  	int ret = 0;
>  
>  	dst_cx_states = kcalloc(_pr->power.count,
> -				sizeof(struct xen_processor_cx), GFP_KERNEL);
> +				sizeof(struct xen_processor_cx), GFP_ATOMIC);
>  	if (!dst_cx_states)
>  		return -ENOMEM;
>  
> @@ -149,7 +149,7 @@ xen_copy_pss_data(struct acpi_processor *_pr,
>  		     sizeof(struct acpi_processor_px));
>  
>  	dst_states = kcalloc(_pr->performance->state_count,
> -			     sizeof(struct xen_processor_px), GFP_KERNEL);
> +			     sizeof(struct xen_processor_px), GFP_ATOMIC);
>  	if (!dst_states)
>  		return ERR_PTR(-ENOMEM);
>  
> @@ -275,9 +275,9 @@ static int upload_pm_data(struct acpi_processor *_pr)
>  {
>  	int err = 0;
>  
> -	mutex_lock(&acpi_ids_mutex);
> +	spin_lock(&acpi_ids_lock);
>  	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
> -		mutex_unlock(&acpi_ids_mutex);
> +		spin_unlock(&acpi_ids_lock);
>  		return -EBUSY;
>  	}
>  	if (_pr->flags.power)
> @@ -286,7 +286,7 @@ static int upload_pm_data(struct acpi_processor *_pr)
>  	if (_pr->performance && _pr->performance->states)
>  		err |= push_pxx_to_hypervisor(_pr);
>  
> -	mutex_unlock(&acpi_ids_mutex);
> +	spin_unlock(&acpi_ids_lock);
>  	return err;
>  }
>  static unsigned int __init get_max_acpi_id(void)

Looks incomplete, what about the kzalloc() in 
xen_upload_processor_pm_data() and kcalloc()s in check_acpi_ids()?

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Stanislaw Gruszka]

(Cc: correct Rafael email)

On Tue, Jan 28, 2014 at 09:24:57PM -0800, David Rientjes wrote:
> On Tue, 28 Jan 2014, Konrad Rzeszutek Wilk wrote:
> 
> > diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> > index 7231859..7602229 100644
> > --- a/drivers/xen/xen-acpi-processor.c
> > +++ b/drivers/xen/xen-acpi-processor.c
> > @@ -46,7 +46,7 @@ module_param_named(off, no_hypercall, int, 0400);
> >   */
> >  static unsigned int nr_acpi_bits;
> >  /* Mutex to protect the acpi_ids_done - for CPU hotplug use. */
> > -static DEFINE_MUTEX(acpi_ids_mutex);
> > +static DEFINE_SPINLOCK(acpi_ids_lock);
> >  /* Which ACPI ID we have processed from 'struct acpi_processor'. */
> >  static unsigned long *acpi_ids_done;
> >  /* Which ACPI ID exist in the SSDT/DSDT processor definitions. */
> > @@ -68,7 +68,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
> >  	int ret = 0;
> >  
> >  	dst_cx_states = kcalloc(_pr->power.count,
> > -				sizeof(struct xen_processor_cx), GFP_KERNEL);
> > +				sizeof(struct xen_processor_cx), GFP_ATOMIC);
> >  	if (!dst_cx_states)
> >  		return -ENOMEM;
> >  
> > @@ -149,7 +149,7 @@ xen_copy_pss_data(struct acpi_processor *_pr,
> >  		     sizeof(struct acpi_processor_px));
> >  
> >  	dst_states = kcalloc(_pr->performance->state_count,
> > -			     sizeof(struct xen_processor_px), GFP_KERNEL);
> > +			     sizeof(struct xen_processor_px), GFP_ATOMIC);
> >  	if (!dst_states)
> >  		return ERR_PTR(-ENOMEM);
> >  
> > @@ -275,9 +275,9 @@ static int upload_pm_data(struct acpi_processor *_pr)
> >  {
> >  	int err = 0;
> >  
> > -	mutex_lock(&acpi_ids_mutex);
> > +	spin_lock(&acpi_ids_lock);
> >  	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
> > -		mutex_unlock(&acpi_ids_mutex);
> > +		spin_unlock(&acpi_ids_lock);
> >  		return -EBUSY;
> >  	}
> >  	if (_pr->flags.power)
> > @@ -286,7 +286,7 @@ static int upload_pm_data(struct acpi_processor *_pr)
> >  	if (_pr->performance && _pr->performance->states)
> >  		err |= push_pxx_to_hypervisor(_pr);
> >  
> > -	mutex_unlock(&acpi_ids_mutex);
> > +	spin_unlock(&acpi_ids_lock);
> >  	return err;
> >  }
> >  static unsigned int __init get_max_acpi_id(void)
> 
> Looks incomplete, what about the kzalloc() in 
> xen_upload_processor_pm_data() and kcalloc()s in check_acpi_ids()?

Indeed and additionally from check_acpi_ids() we call
acpi_walk_namespace(), which also take mutexes. Hence unfortunately
making xen_upload_processor_pm_data() atomic is not easy, but possibly
can be done by saving some data in memory after initialization.

Or perhaps this problem can be solved differently, by not using 
yscore_ops->resume(), but some other resume callback from core, which
allow to sleep. Than can require registering dummy device or sysfs
class, but maybe there are simpler solutions.

Thanks
Stanislaw 
 

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Stanislaw Gruszka]

On Wed, Jan 29, 2014 at 09:25:21AM +0100, Stanislaw Gruszka wrote:
> > Looks incomplete, what about the kzalloc() in 
> > xen_upload_processor_pm_data() and kcalloc()s in check_acpi_ids()?
> 
> Indeed and additionally from check_acpi_ids() we call
> acpi_walk_namespace(), which also take mutexes. Hence unfortunately
> making xen_upload_processor_pm_data() atomic is not easy, but possibly
> can be done by saving some data in memory after initialization.
> 
> Or perhaps this problem can be solved differently, by not using 
> yscore_ops->resume(), but some other resume callback from core, which
> allow to sleep. Than can require registering dummy device or sysfs
> class, but maybe there are simpler solutions.

Eventually work_struct could be used and scheduled from ->resume()
callback, if there is no dependency between uploading processor PM
data to hypervisor and other actions performed after
syscore_ops->resume();

Thanks
Stanislaw

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Konrad Rzeszutek Wilk]

On Wed, Jan 29, 2014 at 09:25:22AM +0100, Stanislaw Gruszka wrote:
> (Cc: correct Rafael email)
> 
> On Tue, Jan 28, 2014 at 09:24:57PM -0800, David Rientjes wrote:
> > On Tue, 28 Jan 2014, Konrad Rzeszutek Wilk wrote:
> > 
> > > diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> > > index 7231859..7602229 100644
> > > --- a/drivers/xen/xen-acpi-processor.c
> > > +++ b/drivers/xen/xen-acpi-processor.c
> > > @@ -46,7 +46,7 @@ module_param_named(off, no_hypercall, int, 0400);
> > >   */
> > >  static unsigned int nr_acpi_bits;
> > >  /* Mutex to protect the acpi_ids_done - for CPU hotplug use. */
> > > -static DEFINE_MUTEX(acpi_ids_mutex);
> > > +static DEFINE_SPINLOCK(acpi_ids_lock);
> > >  /* Which ACPI ID we have processed from 'struct acpi_processor'. */
> > >  static unsigned long *acpi_ids_done;
> > >  /* Which ACPI ID exist in the SSDT/DSDT processor definitions. */
> > > @@ -68,7 +68,7 @@ static int push_cxx_to_hypervisor(struct acpi_processor *_pr)
> > >  	int ret = 0;
> > >  
> > >  	dst_cx_states = kcalloc(_pr->power.count,
> > > -				sizeof(struct xen_processor_cx), GFP_KERNEL);
> > > +				sizeof(struct xen_processor_cx), GFP_ATOMIC);
> > >  	if (!dst_cx_states)
> > >  		return -ENOMEM;
> > >  
> > > @@ -149,7 +149,7 @@ xen_copy_pss_data(struct acpi_processor *_pr,
> > >  		     sizeof(struct acpi_processor_px));
> > >  
> > >  	dst_states = kcalloc(_pr->performance->state_count,
> > > -			     sizeof(struct xen_processor_px), GFP_KERNEL);
> > > +			     sizeof(struct xen_processor_px), GFP_ATOMIC);
> > >  	if (!dst_states)
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > > @@ -275,9 +275,9 @@ static int upload_pm_data(struct acpi_processor *_pr)
> > >  {
> > >  	int err = 0;
> > >  
> > > -	mutex_lock(&acpi_ids_mutex);
> > > +	spin_lock(&acpi_ids_lock);
> > >  	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
> > > -		mutex_unlock(&acpi_ids_mutex);
> > > +		spin_unlock(&acpi_ids_lock);
> > >  		return -EBUSY;
> > >  	}
> > >  	if (_pr->flags.power)
> > > @@ -286,7 +286,7 @@ static int upload_pm_data(struct acpi_processor *_pr)
> > >  	if (_pr->performance && _pr->performance->states)
> > >  		err |= push_pxx_to_hypervisor(_pr);
> > >  
> > > -	mutex_unlock(&acpi_ids_mutex);
> > > +	spin_unlock(&acpi_ids_lock);
> > >  	return err;
> > >  }
> > >  static unsigned int __init get_max_acpi_id(void)
> > 
> > Looks incomplete, what about the kzalloc() in 
> > xen_upload_processor_pm_data() and kcalloc()s in check_acpi_ids()?
> 
> Indeed and additionally from check_acpi_ids() we call
> acpi_walk_namespace(), which also take mutexes. Hence unfortunately
> making xen_upload_processor_pm_data() atomic is not easy, but possibly
> can be done by saving some data in memory after initialization.
> 
> Or perhaps this problem can be solved differently, by not using 
> yscore_ops->resume(), but some other resume callback from core, which
> allow to sleep. Than can require registering dummy device or sysfs
> class, but maybe there are simpler solutions.

Perhaps by using 'subsys_system_register' and stick it there?

Again, untested and uncompiled:


diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 7231859..a2524f6 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -27,7 +27,6 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
-#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 #include <xen/xen.h>
@@ -495,13 +494,15 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static void xen_acpi_processor_resume(void)
+static int xen_acpi_processor_resume(struct device *dev)
 {
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	xen_upload_processor_pm_data();
+	return xen_upload_processor_pm_data();
 }
 
-static struct syscore_ops xap_syscore_ops = {
+static struct bus_type xap_bus = {
+	.name	= "xen-acpi-processor",
+	.dev_name = "xen-acpi-processor",
 	.resume	= xen_acpi_processor_resume,
 };
 
@@ -555,7 +556,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	register_syscore_ops(&xap_syscore_ops);
+	subsys_system_register(&xap_bus);
 
 	return 0;
 err_unregister:
@@ -574,7 +575,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	unregister_syscore_ops(&xap_syscore_ops);
+	bus_unregister(&xap_bus);
 	kfree(acpi_ids_done);
 	kfree(acpi_id_present);
 	kfree(acpi_id_cst_present);
> 
> Thanks
> Stanislaw 
>  

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Stanislaw Gruszka]

On Fri, Jan 31, 2014 at 11:01:40AM -0500, Konrad Rzeszutek Wilk wrote:
> Perhaps by using 'subsys_system_register' and stick it there?

This will not call ->resume callback as it is only called for
devices, so additional dummy device is needed, for example:

struct device xap_dev = {
        .init_name = "xen-acpi-processor-dev",
        .bus = &xap_bus,
};
...
subsys_system_register(&xap_bus, NULL);
device_register(&xap_dev);

But I'm not sure if that is good solution. It crate some not necessery
sysfs directories and files. Additionaly it can restore CPU C-states
after some other drivers resume, which prehaps require proper C-states.

Hence maybe adding direct notify from xen core resume will be better
idea (proposed patch below). Plese let me know what you think, I'll
provide solution which you choose to bug reporters for testing.

Thanks
Stanislaw

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 624e8dc..96e4173 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -13,6 +13,7 @@
 #include <linux/freezer.h>
 #include <linux/syscore_ops.h>
 #include <linux/export.h>
+#include <linux/notifier.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -46,6 +47,20 @@ struct suspend_info {
 	void (*post)(int cancelled);
 };
 
+static RAW_NOTIFIER_HEAD(xen_resume_notifier);
+
+void xen_resume_notifier_register(struct notifier_block *nb)
+{
+	raw_notifier_chain_register(&xen_resume_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(xen_resume_notifier_register);
+
+void xen_resume_notifier_unregister(struct notifier_block *nb)
+{
+	raw_notifier_chain_unregister(&xen_resume_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(xen_resume_notifier_unregister);
+
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 static void xen_hvm_post_suspend(int cancelled)
 {
@@ -152,6 +167,8 @@ static void do_suspend(void)
 
 	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
+	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
+
 	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	if (err) {
diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 7231859..82358d1 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -27,10 +27,10 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
-#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 #include <xen/xen.h>
+#include <xen/xen-ops.h>
 #include <xen/interface/platform.h>
 #include <asm/xen/hypercall.h>
 
@@ -495,14 +495,15 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static void xen_acpi_processor_resume(void)
+static int xen_acpi_processor_resume(struct notifier_block *nb,
+				     unsigned long action, void *data)
 {
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	xen_upload_processor_pm_data();
+	return xen_upload_processor_pm_data();
 }
 
-static struct syscore_ops xap_syscore_ops = {
-	.resume	= xen_acpi_processor_resume,
+struct notifier_block xen_acpi_processor_resume_nb = {
+	.notifier_call = xen_acpi_processor_resume,
 };
 
 static int __init xen_acpi_processor_init(void)
@@ -555,7 +556,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	register_syscore_ops(&xap_syscore_ops);
+	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
 
 	return 0;
 err_unregister:
@@ -574,7 +575,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	unregister_syscore_ops(&xap_syscore_ops);
+	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
 	kfree(acpi_ids_done);
 	kfree(acpi_id_present);
 	kfree(acpi_id_cst_present);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index fb2ea8f..6412358 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -16,6 +16,9 @@ void xen_mm_unpin_all(void);
 void xen_timer_resume(void);
 void xen_arch_resume(void);
 
+void xen_resume_notifier_register(struct notifier_block *nb);
+void xen_resume_notifier_unregister(struct notifier_block *nb);
+
 int xen_setup_shutdown_event(void);
 
 extern unsigned long *xen_contiguous_bitmap;

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Konrad Rzeszutek Wilk]

On Mon, Feb 03, 2014 at 11:12:16AM +0100, Stanislaw Gruszka wrote:
> On Fri, Jan 31, 2014 at 11:01:40AM -0500, Konrad Rzeszutek Wilk wrote:
> > Perhaps by using 'subsys_system_register' and stick it there?
> 
> This will not call ->resume callback as it is only called for
> devices, so additional dummy device is needed, for example:
> 
> struct device xap_dev = {
>         .init_name = "xen-acpi-processor-dev",
>         .bus = &xap_bus,
> };
> ...
> subsys_system_register(&xap_bus, NULL);
> device_register(&xap_dev);
> 
> But I'm not sure if that is good solution. It crate some not necessery
> sysfs directories and files. Additionaly it can restore CPU C-states
> after some other drivers resume, which prehaps require proper C-states.

Yes.
> 
> Hence maybe adding direct notify from xen core resume will be better
> idea (proposed patch below). Plese let me know what you think, I'll
> provide solution which you choose to bug reporters for testing.

Let me think about it for a day or so.

Thanks!
> 
> Thanks
> Stanislaw
> 
> diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> index 624e8dc..96e4173 100644
> --- a/drivers/xen/manage.c
> +++ b/drivers/xen/manage.c
> @@ -13,6 +13,7 @@
>  #include <linux/freezer.h>
>  #include <linux/syscore_ops.h>
>  #include <linux/export.h>
> +#include <linux/notifier.h>
>  
>  #include <xen/xen.h>
>  #include <xen/xenbus.h>
> @@ -46,6 +47,20 @@ struct suspend_info {
>  	void (*post)(int cancelled);
>  };
>  
> +static RAW_NOTIFIER_HEAD(xen_resume_notifier);
> +
> +void xen_resume_notifier_register(struct notifier_block *nb)
> +{
> +	raw_notifier_chain_register(&xen_resume_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(xen_resume_notifier_register);
> +
> +void xen_resume_notifier_unregister(struct notifier_block *nb)
> +{
> +	raw_notifier_chain_unregister(&xen_resume_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(xen_resume_notifier_unregister);
> +
>  #ifdef CONFIG_HIBERNATE_CALLBACKS
>  static void xen_hvm_post_suspend(int cancelled)
>  {
> @@ -152,6 +167,8 @@ static void do_suspend(void)
>  
>  	err = stop_machine(xen_suspend, &si, cpumask_of(0));
>  
> +	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> +
>  	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
>  
>  	if (err) {
> diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> index 7231859..82358d1 100644
> --- a/drivers/xen/xen-acpi-processor.c
> +++ b/drivers/xen/xen-acpi-processor.c
> @@ -27,10 +27,10 @@
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
> -#include <linux/syscore_ops.h>
>  #include <linux/acpi.h>
>  #include <acpi/processor.h>
>  #include <xen/xen.h>
> +#include <xen/xen-ops.h>
>  #include <xen/interface/platform.h>
>  #include <asm/xen/hypercall.h>
>  
> @@ -495,14 +495,15 @@ static int xen_upload_processor_pm_data(void)
>  	return rc;
>  }
>  
> -static void xen_acpi_processor_resume(void)
> +static int xen_acpi_processor_resume(struct notifier_block *nb,
> +				     unsigned long action, void *data)
>  {
>  	bitmap_zero(acpi_ids_done, nr_acpi_bits);
> -	xen_upload_processor_pm_data();
> +	return xen_upload_processor_pm_data();
>  }
>  
> -static struct syscore_ops xap_syscore_ops = {
> -	.resume	= xen_acpi_processor_resume,
> +struct notifier_block xen_acpi_processor_resume_nb = {
> +	.notifier_call = xen_acpi_processor_resume,
>  };
>  
>  static int __init xen_acpi_processor_init(void)
> @@ -555,7 +556,7 @@ static int __init xen_acpi_processor_init(void)
>  	if (rc)
>  		goto err_unregister;
>  
> -	register_syscore_ops(&xap_syscore_ops);
> +	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
>  
>  	return 0;
>  err_unregister:
> @@ -574,7 +575,7 @@ static void __exit xen_acpi_processor_exit(void)
>  {
>  	int i;
>  
> -	unregister_syscore_ops(&xap_syscore_ops);
> +	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
>  	kfree(acpi_ids_done);
>  	kfree(acpi_id_present);
>  	kfree(acpi_id_cst_present);
> diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> index fb2ea8f..6412358 100644
> --- a/include/xen/xen-ops.h
> +++ b/include/xen/xen-ops.h
> @@ -16,6 +16,9 @@ void xen_mm_unpin_all(void);
>  void xen_timer_resume(void);
>  void xen_arch_resume(void);
>  
> +void xen_resume_notifier_register(struct notifier_block *nb);
> +void xen_resume_notifier_unregister(struct notifier_block *nb);
> +
>  int xen_setup_shutdown_event(void);
>  
>  extern unsigned long *xen_contiguous_bitmap;

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Konrad Rzeszutek Wilk]

On Mon, Feb 03, 2014 at 09:14:29AM -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Feb 03, 2014 at 11:12:16AM +0100, Stanislaw Gruszka wrote:
> > On Fri, Jan 31, 2014 at 11:01:40AM -0500, Konrad Rzeszutek Wilk wrote:
> > > Perhaps by using 'subsys_system_register' and stick it there?
> > 
> > This will not call ->resume callback as it is only called for
> > devices, so additional dummy device is needed, for example:
> > 
> > struct device xap_dev = {
> >         .init_name = "xen-acpi-processor-dev",
> >         .bus = &xap_bus,
> > };
> > ...
> > subsys_system_register(&xap_bus, NULL);
> > device_register(&xap_dev);
> > 
> > But I'm not sure if that is good solution. It crate some not necessery
> > sysfs directories and files. Additionaly it can restore CPU C-states
> > after some other drivers resume, which prehaps require proper C-states.
> 
> Yes.
> > 
> > Hence maybe adding direct notify from xen core resume will be better
> > idea (proposed patch below). Plese let me know what you think, I'll
> > provide solution which you choose to bug reporters for testing.
> 
> Let me think about it for a day or so.

Sorry for the delay. I think this is fine.
> 
> Thanks!
> > 
> > Thanks
> > Stanislaw
> > 
> > diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
> > index 624e8dc..96e4173 100644
> > --- a/drivers/xen/manage.c
> > +++ b/drivers/xen/manage.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/freezer.h>
> >  #include <linux/syscore_ops.h>
> >  #include <linux/export.h>
> > +#include <linux/notifier.h>
> >  
> >  #include <xen/xen.h>
> >  #include <xen/xenbus.h>
> > @@ -46,6 +47,20 @@ struct suspend_info {
> >  	void (*post)(int cancelled);
> >  };
> >  
> > +static RAW_NOTIFIER_HEAD(xen_resume_notifier);
> > +
> > +void xen_resume_notifier_register(struct notifier_block *nb)
> > +{
> > +	raw_notifier_chain_register(&xen_resume_notifier, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(xen_resume_notifier_register);
> > +
> > +void xen_resume_notifier_unregister(struct notifier_block *nb)
> > +{
> > +	raw_notifier_chain_unregister(&xen_resume_notifier, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(xen_resume_notifier_unregister);
> > +
> >  #ifdef CONFIG_HIBERNATE_CALLBACKS
> >  static void xen_hvm_post_suspend(int cancelled)
> >  {
> > @@ -152,6 +167,8 @@ static void do_suspend(void)
> >  
> >  	err = stop_machine(xen_suspend, &si, cpumask_of(0));
> >  
> > +	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
> > +
> >  	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
> >  
> >  	if (err) {
> > diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
> > index 7231859..82358d1 100644
> > --- a/drivers/xen/xen-acpi-processor.c
> > +++ b/drivers/xen/xen-acpi-processor.c
> > @@ -27,10 +27,10 @@
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> >  #include <linux/types.h>
> > -#include <linux/syscore_ops.h>
> >  #include <linux/acpi.h>
> >  #include <acpi/processor.h>
> >  #include <xen/xen.h>
> > +#include <xen/xen-ops.h>
> >  #include <xen/interface/platform.h>
> >  #include <asm/xen/hypercall.h>
> >  
> > @@ -495,14 +495,15 @@ static int xen_upload_processor_pm_data(void)
> >  	return rc;
> >  }
> >  
> > -static void xen_acpi_processor_resume(void)
> > +static int xen_acpi_processor_resume(struct notifier_block *nb,
> > +				     unsigned long action, void *data)
> >  {
> >  	bitmap_zero(acpi_ids_done, nr_acpi_bits);
> > -	xen_upload_processor_pm_data();
> > +	return xen_upload_processor_pm_data();
> >  }
> >  
> > -static struct syscore_ops xap_syscore_ops = {
> > -	.resume	= xen_acpi_processor_resume,
> > +struct notifier_block xen_acpi_processor_resume_nb = {
> > +	.notifier_call = xen_acpi_processor_resume,
> >  };
> >  
> >  static int __init xen_acpi_processor_init(void)
> > @@ -555,7 +556,7 @@ static int __init xen_acpi_processor_init(void)
> >  	if (rc)
> >  		goto err_unregister;
> >  
> > -	register_syscore_ops(&xap_syscore_ops);
> > +	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
> >  
> >  	return 0;
> >  err_unregister:
> > @@ -574,7 +575,7 @@ static void __exit xen_acpi_processor_exit(void)
> >  {
> >  	int i;
> >  
> > -	unregister_syscore_ops(&xap_syscore_ops);
> > +	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
> >  	kfree(acpi_ids_done);
> >  	kfree(acpi_id_present);
> >  	kfree(acpi_id_cst_present);
> > diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
> > index fb2ea8f..6412358 100644
> > --- a/include/xen/xen-ops.h
> > +++ b/include/xen/xen-ops.h
> > @@ -16,6 +16,9 @@ void xen_mm_unpin_all(void);
> >  void xen_timer_resume(void);
> >  void xen_arch_resume(void);
> >  
> > +void xen_resume_notifier_register(struct notifier_block *nb);
> > +void xen_resume_notifier_unregister(struct notifier_block *nb);
> > +
> >  int xen_setup_shutdown_event(void);
> >  
> >  extern unsigned long *xen_contiguous_bitmap;

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Stanislaw Gruszka]

On Mon, Feb 10, 2014 at 09:37:27AM -0500, Konrad Rzeszutek Wilk wrote:
> > > But I'm not sure if that is good solution. It crate some not necessery
> > > sysfs directories and files. Additionaly it can restore CPU C-states
> > > after some other drivers resume, which prehaps require proper C-states.
> > 
> > Yes.
> > > 
> > > Hence maybe adding direct notify from xen core resume will be better
> > > idea (proposed patch below). Plese let me know what you think, I'll
> > > provide solution which you choose to bug reporters for testing.
> > 
> > Let me think about it for a day or so.
> 
> Sorry for the delay. I think this is fine.

I'm sorry for delay too. I provided test kernel with the patch to bug
reporter, but have not get any answer since two weeks now.

I'll post the patch in the next email, I hope someone can test it (I'm
not xen user). I'm pretty sure it fixes the problem, but not sure if it
does not cause some crash. To test is enough to perform suspend/resume
cycle (thought I'm not sure if suspended should be just guest or whole
system with hypervisor).

Thanks
Stanislaw

[PATCH] xen/acpi-processor: fix enabling interrupts on syscore_resume

[Stanislaw Gruszka]

syscore->resume() callback is expected to do not enable interrupts,
it generates warning like below otherwise:

[ 9386.365390] WARNING: CPU: 0 PID: 6733 at drivers/base/syscore.c:104 syscore_resume+0x9a/0xe0()
[ 9386.365403] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]
...
[ 9386.365429] Call Trace:
[ 9386.365434]  [<ffffffff81667a8b>] dump_stack+0x45/0x56
[ 9386.365437]  [<ffffffff8106921d>] warn_slowpath_common+0x7d/0xa0
[ 9386.365439]  [<ffffffff8106928c>] warn_slowpath_fmt+0x4c/0x50
[ 9386.365442]  [<ffffffffa0261bb0>] ? xen_upload_processor_pm_data+0x300/0x300 [xen_acpi_processor]
[ 9386.365443]  [<ffffffff814055fa>] syscore_resume+0x9a/0xe0
[ 9386.365445]  [<ffffffff810aef42>] suspend_devices_and_enter+0x402/0x470
[ 9386.365447]  [<ffffffff810af128>] pm_suspend+0x178/0x260

On xen_acpi_processor_resume() we call various procedures, which are
non atomic and can enable interrupts. To prevent the issue introduce
separate resume notify called after we enable interrupts on resume
and before we call other drivers resume callbacks.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
Patch was only compiled, not tested otherwise.

 drivers/xen/manage.c             | 17 +++++++++++++++++
 drivers/xen/xen-acpi-processor.c | 15 ++++++++-------
 include/xen/xen-ops.h            |  3 +++
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index 624e8dc..96e4173 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -13,6 +13,7 @@
 #include <linux/freezer.h>
 #include <linux/syscore_ops.h>
 #include <linux/export.h>
+#include <linux/notifier.h>
 
 #include <xen/xen.h>
 #include <xen/xenbus.h>
@@ -46,6 +47,20 @@ struct suspend_info {
 	void (*post)(int cancelled);
 };
 
+static RAW_NOTIFIER_HEAD(xen_resume_notifier);
+
+void xen_resume_notifier_register(struct notifier_block *nb)
+{
+	raw_notifier_chain_register(&xen_resume_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(xen_resume_notifier_register);
+
+void xen_resume_notifier_unregister(struct notifier_block *nb)
+{
+	raw_notifier_chain_unregister(&xen_resume_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(xen_resume_notifier_unregister);
+
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 static void xen_hvm_post_suspend(int cancelled)
 {
@@ -152,6 +167,8 @@ static void do_suspend(void)
 
 	err = stop_machine(xen_suspend, &si, cpumask_of(0));
 
+	raw_notifier_call_chain(&xen_resume_notifier, 0, NULL);
+
 	dpm_resume_start(si.cancelled ? PMSG_THAW : PMSG_RESTORE);
 
 	if (err) {
diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 7231859..82358d1 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -27,10 +27,10 @@
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/types.h>
-#include <linux/syscore_ops.h>
 #include <linux/acpi.h>
 #include <acpi/processor.h>
 #include <xen/xen.h>
+#include <xen/xen-ops.h>
 #include <xen/interface/platform.h>
 #include <asm/xen/hypercall.h>
 
@@ -495,14 +495,15 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static void xen_acpi_processor_resume(void)
+static int xen_acpi_processor_resume(struct notifier_block *nb,
+				     unsigned long action, void *data)
 {
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	xen_upload_processor_pm_data();
+	return xen_upload_processor_pm_data();
 }
 
-static struct syscore_ops xap_syscore_ops = {
-	.resume	= xen_acpi_processor_resume,
+struct notifier_block xen_acpi_processor_resume_nb = {
+	.notifier_call = xen_acpi_processor_resume,
 };
 
 static int __init xen_acpi_processor_init(void)
@@ -555,7 +556,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	register_syscore_ops(&xap_syscore_ops);
+	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
 
 	return 0;
 err_unregister:
@@ -574,7 +575,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	unregister_syscore_ops(&xap_syscore_ops);
+	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
 	kfree(acpi_ids_done);
 	kfree(acpi_id_present);
 	kfree(acpi_id_cst_present);
diff --git a/include/xen/xen-ops.h b/include/xen/xen-ops.h
index fb2ea8f..6412358 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -16,6 +16,9 @@ void xen_mm_unpin_all(void);
 void xen_timer_resume(void);
 void xen_arch_resume(void);
 
+void xen_resume_notifier_register(struct notifier_block *nb);
+void xen_resume_notifier_unregister(struct notifier_block *nb);
+
 int xen_setup_shutdown_event(void);
 
 extern unsigned long *xen_contiguous_bitmap;
-- 
1.7.11.7

Re: [BUG?] Interrupts enabled after xen_acpi_processor_resume+0x0/0x34 [xen_acpi_processor]

[Konrad Rzeszutek Wilk]

On Wed, Feb 26, 2014 at 11:26:21AM +0100, Stanislaw Gruszka wrote:
> On Mon, Feb 10, 2014 at 09:37:27AM -0500, Konrad Rzeszutek Wilk wrote:
> > > > But I'm not sure if that is good solution. It crate some not necessery
> > > > sysfs directories and files. Additionaly it can restore CPU C-states
> > > > after some other drivers resume, which prehaps require proper C-states.
> > > 
> > > Yes.
> > > > 
> > > > Hence maybe adding direct notify from xen core resume will be better
> > > > idea (proposed patch below). Plese let me know what you think, I'll
> > > > provide solution which you choose to bug reporters for testing.
> > > 
> > > Let me think about it for a day or so.
> > 
> > Sorry for the delay. I think this is fine.
> 
> I'm sorry for delay too. I provided test kernel with the patch to bug
> reporter, but have not get any answer since two weeks now.

I tested it and it works great.
> 
> I'll post the patch in the next email, I hope someone can test it (I'm
> not xen user). I'm pretty sure it fixes the problem, but not sure if it
> does not cause some crash. To test is enough to perform suspend/resume
> cycle (thought I'm not sure if suspended should be just guest or whole
> system with hypervisor).

I put your put in the queue for 3.15.

Thanks!
> 
> Thanks
> Stanislaw