Re: [Linux 2.6.29-rc2] BUG: using smp_processor_id() in preemptible

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

On Tuesday 27 January 2009, Ingo Molnar wrote:
> 
> * Rafael J. Wysocki <rjw@sisk.pl> wrote:
> 
> > > In fact whatever check you put in it's _always_ going to be 
> > > fundamentally more fragile than direct instrumentation: you cannot 
> > > possibly check all possible places that enable interrupts. (they could 
> > > be disabling interrupts as a _restore_irqs() sequence for example)
> > 
> > In this particular case, I'm not really interested in that.  What I'm 
> > interested in is which driver's ->suspend_late() or ->resume_early() (or 
> > the equivalents for sysdevs) has enabled interrupts, which is quite easy 
> > to check directly.
> 
> But this is exactly what it does - without any need for debug checks 
> spread around!
> 
> You'll get a _full stack dump_ from the very driver that is enabling 
> interrupts! You dont get a trace - you get a stack dump of the very place 
> that is buggy. It does not get any better than that.

I'm not going to argue.

Nevertheless, IMO something like the patch below should be sufficient to catch
these bugs.

Thanks,
Rafael


---
 drivers/base/power/main.c |   12 ++++++++++++
 drivers/base/sys.c        |   21 ++++++++++++++++-----
 include/linux/pm.h        |   18 ++++++++++++++++++
 3 files changed, 46 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -186,12 +186,14 @@ static int pm_noirq_op(struct device *de
 		if (ops->suspend_noirq) {
 			error = ops->suspend_noirq(dev);
 			suspend_report_result(ops->suspend_noirq, error);
+			pm_check_irqs_after(ops->suspend_noirq);
 		}
 		break;
 	case PM_EVENT_RESUME:
 		if (ops->resume_noirq) {
 			error = ops->resume_noirq(dev);
 			suspend_report_result(ops->resume_noirq, error);
+			pm_check_irqs_after(ops->resume_noirq);
 		}
 		break;
 #endif /* CONFIG_SUSPEND */
@@ -201,12 +203,14 @@ static int pm_noirq_op(struct device *de
 		if (ops->freeze_noirq) {
 			error = ops->freeze_noirq(dev);
 			suspend_report_result(ops->freeze_noirq, error);
+			pm_check_irqs_after(ops->freeze_noirq);
 		}
 		break;
 	case PM_EVENT_HIBERNATE:
 		if (ops->poweroff_noirq) {
 			error = ops->poweroff_noirq(dev);
 			suspend_report_result(ops->poweroff_noirq, error);
+			pm_check_irqs_after(ops->poweroff_noirq);
 		}
 		break;
 	case PM_EVENT_THAW:
@@ -214,12 +218,14 @@ static int pm_noirq_op(struct device *de
 		if (ops->thaw_noirq) {
 			error = ops->thaw_noirq(dev);
 			suspend_report_result(ops->thaw_noirq, error);
+			pm_check_irqs_after(ops->thaw_noirq);
 		}
 		break;
 	case PM_EVENT_RESTORE:
 		if (ops->restore_noirq) {
 			error = ops->restore_noirq(dev);
 			suspend_report_result(ops->restore_noirq, error);
+			pm_check_irqs_after(ops->restore_noirq);
 		}
 		break;
 #endif /* CONFIG_HIBERNATION */
@@ -292,6 +298,7 @@ static int resume_device_noirq(struct de
 	} else if (dev->bus->resume_early) {
 		pm_dev_dbg(dev, state, "legacy EARLY ");
 		error = dev->bus->resume_early(dev);
+		pm_check_irqs_after(dev->bus->resume_early);
 	}
  End:
 	TRACE_RESUME(error);
@@ -311,6 +318,8 @@ static void dpm_power_up(pm_message_t st
 {
 	struct device *dev;
 
+	pm_check_irqs();
+
 	list_for_each_entry(dev, &dpm_list, power.entry)
 		if (dev->power.status > DPM_OFF) {
 			int error;
@@ -551,6 +560,7 @@ static int suspend_device_noirq(struct d
 		pm_dev_dbg(dev, state, "legacy LATE ");
 		error = dev->bus->suspend_late(dev, state);
 		suspend_report_result(dev->bus->suspend_late, error);
+		pm_check_irqs_after(dev->bus->suspend_late);
 	}
 	return error;
 }
@@ -569,6 +579,8 @@ int device_power_down(pm_message_t state
 	struct device *dev;
 	int error = 0;
 
+	pm_check_irqs();
+
 	list_for_each_entry_reverse(dev, &dpm_list, power.entry) {
 		error = suspend_device_noirq(dev, state);
 		if (error) {
Index: linux-2.6/include/linux/pm.h
===================================================================
--- linux-2.6.orig/include/linux/pm.h
+++ linux-2.6/include/linux/pm.h
@@ -396,6 +396,20 @@ extern void __suspend_report_result(cons
 		__suspend_report_result(__func__, fn, ret);		\
 	} while (0)
 
+#define pm_check_irqs()							\
+	do {								\
+		WARN_ONCE(!irqs_disabled(),				\
+			"PM: Interrupts enabled in %s\n",		\
+			__FUNCTION__);					\
+	} while (0)
+
+#define pm_check_irqs_after(callback)					\
+	do {								\
+		WARN_ONCE(!irqs_disabled(),				\
+			"PM: Interrupts enabled by %pF\n",		\
+			(callback));					\
+	} while (0)
+
 #else /* !CONFIG_PM_SLEEP */
 
 static inline int device_suspend(pm_message_t state)
@@ -405,6 +419,10 @@ static inline int device_suspend(pm_mess
 
 #define suspend_report_result(fn, ret)		do {} while (0)
 
+#define pm_check_irqs()				do {} while (0)
+
+#define pm_check_irqs_after(callback)		do {} while (0)
+
 #endif /* !CONFIG_PM_SLEEP */
 
 /*
Index: linux-2.6/drivers/base/sys.c
===================================================================
--- linux-2.6.orig/drivers/base/sys.c
+++ linux-2.6/drivers/base/sys.c
@@ -341,14 +341,17 @@ static void __sysdev_resume(struct sys_d
 	struct sysdev_driver *drv;
 
 	/* First, call the class-specific one */
-	if (cls->resume)
+	if (cls->resume) {
 		cls->resume(dev);
+		pm_check_irqs_after(cls->resume);
+	}
 
 	/* Call auxillary drivers next. */
-	list_for_each_entry(drv, &cls->drivers, entry) {
-		if (drv->resume)
+	list_for_each_entry(drv, &cls->drivers, entry)
+		if (drv->resume) {
 			drv->resume(dev);
-	}
+			pm_check_irqs_after(drv->resume);
+		}
 }
 
 /**
@@ -373,6 +376,8 @@ int sysdev_suspend(pm_message_t state)
 
 	pr_debug("Suspending System Devices\n");
 
+	pm_check_irqs();
+
 	list_for_each_entry_reverse(cls, &system_kset->list, kset.kobj.entry) {
 		pr_debug("Suspending type '%s':\n",
 			 kobject_name(&cls->kset.kobj));
@@ -384,6 +389,7 @@ int sysdev_suspend(pm_message_t state)
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->suspend) {
 					ret = drv->suspend(sysdev, state);
+					pm_check_irqs_after(drv->suspend);
 					if (ret)
 						goto aux_driver;
 				}
@@ -392,6 +398,7 @@ int sysdev_suspend(pm_message_t state)
 			/* Now call the generic one */
 			if (cls->suspend) {
 				ret = cls->suspend(sysdev, state);
+				pm_check_irqs_after(cls->suspend);
 				if (ret)
 					goto cls_driver;
 			}
@@ -411,8 +418,10 @@ aux_driver:
 	list_for_each_entry(err_drv, &cls->drivers, entry) {
 		if (err_drv == drv)
 			break;
-		if (err_drv->resume)
+		if (err_drv->resume) {
 			err_drv->resume(sysdev);
+			pm_check_irqs_after(err_drv->resume);
+		}
 	}
 
 	/* resume other sysdevs in current class */
@@ -449,6 +458,8 @@ int sysdev_resume(void)
 
 	pr_debug("Resuming System Devices\n");
 
+	pm_check_irqs();
+
 	list_for_each_entry(cls, &system_kset->list, kset.kobj.entry) {
 		struct sys_device * sysdev;