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

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

[Konrad Rzeszutek Wilk]

This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.

As it actually never gets called on the initial domain when
resuming. That is after we suspend and go in resume, the
do_suspend (from manage.c) is never called (it is if it
was running as a guest)- so the 'resume' functionality of the driver
was never called.

Which means that this whole patch was pointless (well, it did
remove the WARNING splat).

This patch reverts the patch and allows the C and P states to
be uploaded to the hypervisor on ACPI S3 resume of the
initial domain. It sadly brings back the WARNING splat which
will have to be dealt with at some point.

CC: <sgruszka@redhat.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/manage.c             | 16 ----------------
 drivers/xen/xen-acpi-processor.c | 15 +++++++--------
 include/xen/xen-ops.h            |  4 ----
 3 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/drivers/xen/manage.c b/drivers/xen/manage.c
index f8bb36f..05d4126 100644
--- a/drivers/xen/manage.c
+++ b/drivers/xen/manage.c
@@ -43,20 +43,6 @@ struct suspend_info {
 	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 int xen_suspend(void *data)
 {
@@ -133,8 +119,6 @@ static void do_suspend(void)
 	if (!si.cancelled)
 		xen_console_resume();
 
-	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 59fc190..2d727ab 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,15 +495,14 @@ static int xen_upload_processor_pm_data(void)
 	return rc;
 }
 
-static int xen_acpi_processor_resume(struct notifier_block *nb,
-				     unsigned long action, void *data)
+static void xen_acpi_processor_resume(void)
 {
 	bitmap_zero(acpi_ids_done, nr_acpi_bits);
-	return xen_upload_processor_pm_data();
+	xen_upload_processor_pm_data();
 }
 
-struct notifier_block xen_acpi_processor_resume_nb = {
-	.notifier_call = xen_acpi_processor_resume,
+static struct syscore_ops xap_syscore_ops = {
+	.resume	= xen_acpi_processor_resume,
 };
 
 static int __init xen_acpi_processor_init(void)
@@ -556,7 +555,7 @@ static int __init xen_acpi_processor_init(void)
 	if (rc)
 		goto err_unregister;
 
-	xen_resume_notifier_register(&xen_acpi_processor_resume_nb);
+	register_syscore_ops(&xap_syscore_ops);
 
 	return 0;
 err_unregister:
@@ -575,7 +574,7 @@ static void __exit xen_acpi_processor_exit(void)
 {
 	int i;
 
-	xen_resume_notifier_unregister(&xen_acpi_processor_resume_nb);
+	unregister_syscore_ops(&xap_syscore_ops);
 	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 7491ee5..b4637b8 100644
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -2,7 +2,6 @@
 #define INCLUDE_XEN_OPS_H
 
 #include <linux/percpu.h>
-#include <linux/notifier.h>
 #include <linux/efi.h>
 #include <asm/xen/interface.h>
 
@@ -14,9 +13,6 @@ void xen_arch_post_suspend(int suspend_cancelled);
 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.9.3

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

[David Vrabel]

On 29/09/14 15:51, Konrad Rzeszutek Wilk wrote:
> This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.
> 
> As it actually never gets called on the initial domain when
> resuming. That is after we suspend and go in resume, the
> do_suspend (from manage.c) is never called (it is if it
> was running as a guest)- so the 'resume' functionality of the driver
> was never called.
> 
> Which means that this whole patch was pointless (well, it did
> remove the WARNING splat).
> 
> This patch reverts the patch and allows the C and P states to
> be uploaded to the hypervisor on ACPI S3 resume of the
> initial domain. It sadly brings back the WARNING splat which
> will have to be dealt with at some point.

Incorrectly enabling interrupts in contexts where this is not permitted
is not just harmless "WARNING splat".

This has been broken since 3.15-rc1 without anyone else noticing so I
think we can afford to take a bit more time and fix the original bug
properly.

David

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

[Konrad Rzeszutek Wilk]

On Mon, Sep 29, 2014 at 03:56:14PM +0100, David Vrabel wrote:
> On 29/09/14 15:51, Konrad Rzeszutek Wilk wrote:
> > This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.
> > 
> > As it actually never gets called on the initial domain when
> > resuming. That is after we suspend and go in resume, the
> > do_suspend (from manage.c) is never called (it is if it
> > was running as a guest)- so the 'resume' functionality of the driver
> > was never called.
> > 
> > Which means that this whole patch was pointless (well, it did
> > remove the WARNING splat).
> > 
> > This patch reverts the patch and allows the C and P states to
> > be uploaded to the hypervisor on ACPI S3 resume of the
> > initial domain. It sadly brings back the WARNING splat which
> > will have to be dealt with at some point.
> 
> Incorrectly enabling interrupts in contexts where this is not permitted
> is not just harmless "WARNING splat".
> 
> This has been broken since 3.15-rc1 without anyone else noticing so I
> think we can afford to take a bit more time and fix the original bug
> properly.

That certainly can be done (perhaps as a next patch). But that won't get
done by for a good month or so (-EOVERLOADED).
> 
> David

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

[David Vrabel]

On 29/09/14 16:07, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:56:14PM +0100, David Vrabel wrote:
>> On 29/09/14 15:51, Konrad Rzeszutek Wilk wrote:
>>> This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.
>>>
>>> As it actually never gets called on the initial domain when
>>> resuming. That is after we suspend and go in resume, the
>>> do_suspend (from manage.c) is never called (it is if it
>>> was running as a guest)- so the 'resume' functionality of the driver
>>> was never called.
>>>
>>> Which means that this whole patch was pointless (well, it did
>>> remove the WARNING splat).
>>>
>>> This patch reverts the patch and allows the C and P states to
>>> be uploaded to the hypervisor on ACPI S3 resume of the
>>> initial domain. It sadly brings back the WARNING splat which
>>> will have to be dealt with at some point.
>>
>> Incorrectly enabling interrupts in contexts where this is not permitted
>> is not just harmless "WARNING splat".
>>
>> This has been broken since 3.15-rc1 without anyone else noticing so I
>> think we can afford to take a bit more time and fix the original bug
>> properly.
> 
> That certainly can be done (perhaps as a next patch). But that won't get
> done by for a good month or so (-EOVERLOADED).

I think we can wait for a month or two for a proper fix.

David

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

[Konrad Rzeszutek Wilk]

On Mon, Sep 29, 2014 at 03:56:14PM +0100, David Vrabel wrote:
> On 29/09/14 15:51, Konrad Rzeszutek Wilk wrote:
> > This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.
> > 
> > As it actually never gets called on the initial domain when
> > resuming. That is after we suspend and go in resume, the
> > do_suspend (from manage.c) is never called (it is if it
> > was running as a guest)- so the 'resume' functionality of the driver
> > was never called.
> > 
> > Which means that this whole patch was pointless (well, it did
> > remove the WARNING splat).
> > 
> > This patch reverts the patch and allows the C and P states to
> > be uploaded to the hypervisor on ACPI S3 resume of the
> > initial domain. It sadly brings back the WARNING splat which
> > will have to be dealt with at some point.
> 
> Incorrectly enabling interrupts in contexts where this is not permitted
> is not just harmless "WARNING splat".
> 
> This has been broken since 3.15-rc1 without anyone else noticing so I
> think we can afford to take a bit more time and fix the original bug
> properly.

This patch should be a good start to discussing the fix.

>From 3544815f7c44508e2c9a0c55caf4a32cc8283685 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 29 Sep 2014 13:48:57 -0400
Subject: [PATCH] xen-acpi-processor: Use spinlock and GFP_ATOMIC to deal with
 resume

hitting the IRQs being enabled during resume.

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]

Call Trace:
 [<ffffffff81667a8b>] dump_stack+0x45/0x56
 [<ffffffff8106921d>] warn_slowpath_common+0x7d/0xa0
 [<ffffffff8106928c>] warn_slowpath_fmt+0x4c/0x50
 [<ffffffffa0261bb0>] ? xen_upload_processor_pm_data+0x300/0x300 [xen_acpi_processor]
 [<ffffffff814055fa>] syscore_resume+0x9a/0xe0
 [<ffffffff810aef42>] suspend_devices_and_enter+0x402/0x470
 [<ffffffff810af128>] pm_suspend+0x178/0x260

Converting the mutex to a spinlock and all of the GPF_KERNEL
to GFP_ATOMIC take care of that.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/xen/xen-acpi-processor.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 2d727ab..8c76767 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -45,8 +45,8 @@ module_param_named(off, no_hypercall, int, 0400);
  * which is dynamically computed based on the MADT or x2APIC table.
  */
 static unsigned int nr_acpi_bits;
-/* Mutex to protect the acpi_ids_done - for CPU hotplug use. */
-static DEFINE_MUTEX(acpi_ids_mutex);
+/* Spinlock to protect the acpi_ids_done - for CPU hotplug use. */
+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);
 
@@ -273,11 +273,12 @@ err_free:
 }
 static int upload_pm_data(struct acpi_processor *_pr)
 {
+	unsigned long flags;
 	int err = 0;
 
-	mutex_lock(&acpi_ids_mutex);
+	spin_lock_irqsave(&acpi_ids_lock, flags);
 	if (__test_and_set_bit(_pr->acpi_id, acpi_ids_done)) {
-		mutex_unlock(&acpi_ids_mutex);
+		spin_unlock_irqrestore(&acpi_ids_lock, flags);
 		return -EBUSY;
 	}
 	if (_pr->flags.power)
@@ -286,7 +287,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_lock_irqsave(&acpi_ids_lock, flags);
 	return err;
 }
 static unsigned int __init get_max_acpi_id(void)
@@ -395,11 +396,11 @@ static int check_acpi_ids(struct acpi_processor *pr_backup)
 	/* All online CPUs have been processed at this stage. Now verify
 	 * whether in fact "online CPUs" == physical CPUs.
 	 */
-	acpi_id_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_KERNEL);
+	acpi_id_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_ATOMIC);
 	if (!acpi_id_present)
 		return -ENOMEM;
 
-	acpi_id_cst_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_KERNEL);
+	acpi_id_cst_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_ATOMIC);
 	if (!acpi_id_cst_present) {
 		kfree(acpi_id_present);
 		return -ENOMEM;
@@ -482,7 +483,7 @@ static int xen_upload_processor_pm_data(void)
 			continue;
 
 		if (!pr_backup) {
-			pr_backup = kzalloc(sizeof(struct acpi_processor), GFP_KERNEL);
+			pr_backup = kzalloc(sizeof(struct acpi_processor), GFP_ATOMIC);
 			if (pr_backup)
 				memcpy(pr_backup, _pr, sizeof(struct acpi_processor));
 		}
@@ -514,7 +515,7 @@ static int __init xen_acpi_processor_init(void)
 		return rc;
 
 	nr_acpi_bits = get_max_acpi_id() + 1;
-	acpi_ids_done = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_KERNEL);
+	acpi_ids_done = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned long), GFP_ATOMIC);
 	if (!acpi_ids_done)
 		return -ENOMEM;
 
@@ -527,7 +528,7 @@ static int __init xen_acpi_processor_init(void)
 	for_each_possible_cpu(i) {
 		if (!zalloc_cpumask_var_node(
 			&per_cpu_ptr(acpi_perf_data, i)->shared_cpu_map,
-			GFP_KERNEL, cpu_to_node(i))) {
+			GFP_ATOMIC, cpu_to_node(i))) {
 			rc = -ENOMEM;
 			goto err_out;
 		}
-- 
1.9.3

> 
> David

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

[David Vrabel]

On 29/09/14 18:55, Konrad Rzeszutek Wilk wrote:
> 
> This patch should be a good start to discussing the fix.
> 
> From 3544815f7c44508e2c9a0c55caf4a32cc8283685 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 29 Sep 2014 13:48:57 -0400
> Subject: [PATCH] xen-acpi-processor: Use spinlock and GFP_ATOMIC to deal with
>  resume
> 
> hitting the IRQs being enabled during resume.
> 
> 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]
> 
> Call Trace:
>  [<ffffffff81667a8b>] dump_stack+0x45/0x56
>  [<ffffffff8106921d>] warn_slowpath_common+0x7d/0xa0
>  [<ffffffff8106928c>] warn_slowpath_fmt+0x4c/0x50
>  [<ffffffffa0261bb0>] ? xen_upload_processor_pm_data+0x300/0x300 [xen_acpi_processor]
>  [<ffffffff814055fa>] syscore_resume+0x9a/0xe0
>  [<ffffffff810aef42>] suspend_devices_and_enter+0x402/0x470
>  [<ffffffff810af128>] pm_suspend+0x178/0x260
> 
> Converting the mutex to a spinlock and all of the GPF_KERNEL
> to GFP_ATOMIC take care of that.

Do we need to load this into Xen so early?  I'd much prefer not adding a
bunch of GFP_ATOMIC allocations into the resume path.

I'd probably just schedule some work in the syscore resume to do the load.

David

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

[Stanislaw Gruszka]

On Mon, Sep 29, 2014 at 01:55:17PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 29, 2014 at 03:56:14PM +0100, David Vrabel wrote:
> > On 29/09/14 15:51, Konrad Rzeszutek Wilk wrote:
> > > This reverts commit cd979883b9ede90643e019f33cb317933eb867b4.
> > > 
> > > As it actually never gets called on the initial domain when
> > > resuming. That is after we suspend and go in resume, the
> > > do_suspend (from manage.c) is never called (it is if it
> > > was running as a guest)- so the 'resume' functionality of the driver
> > > was never called.
> > > 
> > > Which means that this whole patch was pointless (well, it did
> > > remove the WARNING splat).
> > > 
> > > This patch reverts the patch and allows the C and P states to
> > > be uploaded to the hypervisor on ACPI S3 resume of the
> > > initial domain. It sadly brings back the WARNING splat which
> > > will have to be dealt with at some point.
> > 
> > Incorrectly enabling interrupts in contexts where this is not permitted
> > is not just harmless "WARNING splat".
> > 
> > This has been broken since 3.15-rc1 without anyone else noticing so I
> > think we can afford to take a bit more time and fix the original bug
> > properly.
> 
> This patch should be a good start to discussing the fix.
> 
> From 3544815f7c44508e2c9a0c55caf4a32cc8283685 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 29 Sep 2014 13:48:57 -0400
> Subject: [PATCH] xen-acpi-processor: Use spinlock and GFP_ATOMIC to deal with
>  resume
> 
> hitting the IRQs being enabled during resume.
> 
> 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]
> 
> Call Trace:
>  [<ffffffff81667a8b>] dump_stack+0x45/0x56
>  [<ffffffff8106921d>] warn_slowpath_common+0x7d/0xa0
>  [<ffffffff8106928c>] warn_slowpath_fmt+0x4c/0x50
>  [<ffffffffa0261bb0>] ? xen_upload_processor_pm_data+0x300/0x300 [xen_acpi_processor]
>  [<ffffffff814055fa>] syscore_resume+0x9a/0xe0
>  [<ffffffff810aef42>] suspend_devices_and_enter+0x402/0x470
>  [<ffffffff810af128>] pm_suspend+0x178/0x260
> 
> Converting the mutex to a spinlock and all of the GPF_KERNEL
> to GFP_ATOMIC take care of that.

This is not enough to make xen_upload_processor_pm_data() run in atomic
context. Via check_acpi_ids() it call acpi_walk_namespace() and
acpi_walk_namespace(), which are internal ACPICA functions that
take internal ACPICA  mutexes.

Stanislaw