Re: BUG: using smp_processor_id() during suspend with 2.6.25-rc8

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday, 8 of April 2008, Andi Kleen wrote:
> On Tue, Apr 08, 2008 at 12:29:30AM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, 8 of April 2008, Andi Kleen wrote:
> > > On Tue, Apr 08, 2008 at 12:11:17AM +0200, Jiri Kosina wrote:
> > > > On Tue, 8 Apr 2008, Andi Kleen wrote:
> > > > 
> > > > > > I know. However preempt_count is a little bit inconsistent in such cases 
> > > > > > though.
> > > > > And? interrupts off beats preempt count anyways. Why did you write the 
> > > > > patch? Was there a (incorrect) warning triggered?
> > > > 
> > > > Reported at http://lkml.org/lkml/2008/4/7/130
> > > > 
> > > > BTW is also mce_init() (called from mce_resume()) guaranteed to run with 
> > > > IRQs off?
> > > 
> > > [cc rafael]
> > > 
> > > The mce resume is a sysdev.
> > > 
> > > sysdevs were always supposed to run completely with interrupts off. If they 
> > > don't anymore that's some kind of higher level resume code bug which you need 
> > > to fix there, not hack around in the low level code.
> > 
> > They are executed with interrupts disabled, on one CPU.
> 
> Well then someone enables them incorrectly, see the report above.

Hm, well.  I really don't think that ACPI or the core does it, but anyway
Zdenek, could you please apply the appended patch and see if it
produces any additional warnings?

If it doesn't, then probably one of the sysdevs does it in its ->resume()
routine, which also will be easy to check against.

> > 
> > > If it does that it likely broke more code too.
> > > 
> > > Obviously turning on preemption anywhere around the machine check is
> > > fatal because it touches CPU state and if you reschedule you could
> > > switch to another CPU and change or access the wrong CPU's state.
> > 
> > FWIW, at the point when sysdevs are resumed we are single-threaded.
> 
> You mean single CPUed? Even a single thread could switch to another CPU.

Signle CPUed too.

Thanks,
Rafael

---
---
 drivers/acpi/sleep/main.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -141,9 +141,13 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 	}
 
+	WARN_ON(!irqs_disabled());
+
 	/* Reprogram control registers and execute _BFS */
 	acpi_leave_sleep_state_prep(acpi_state);
 
+	WARN_ON(!irqs_disabled());
+
 	/* ACPI 3.0 specs (P62) says that it's the responsibility
 	 * of the OSPM to clear the status bit [ implying that the
 	 * POWER_BUTTON event should not reach userspace ]
@@ -158,6 +162,8 @@ static int acpi_pm_enter(suspend_state_t
 	 */
 	acpi_hw_disable_all_gpes();
 
+	WARN_ON(!irqs_disabled());
+
 	local_irq_restore(flags);
 	printk(KERN_DEBUG "Back to C!\n");
 
@@ -165,6 +171,8 @@ static int acpi_pm_enter(suspend_state_t
 	if (acpi_state == ACPI_STATE_S3)
 		acpi_restore_state_mem();
 
+	WARN_ON(!irqs_disabled());
+
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }