s2ram: warn when interrupts should be disabled but are not

s2ram: warn when interrupts should be disabled but are not

[Pavel Machek]

sysdevs should be suspended/resumed with IRQs disabled; if something
reenables them by mistake, we want to know early.

Signed-off-by: Pavel Machek <pavel@suse.cz>

---
commit fc3bccaf69e6c05464e7154369639bd5c96ccdc1
tree bf960093ddb840242a00b4ec456ec79472b1c2ad
parent 308e9d5309f378209e931f63f729806ec0a4d9f1
author Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
committer Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200

 drivers/ata/ahci.c |    2 +-
 drivers/base/sys.c |    3 +++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/base/sys.c b/drivers/base/sys.c
index 8e13fd9..e214467 100644
--- a/drivers/base/sys.c
+++ b/drivers/base/sys.c
@@ -22,6 +22,7 @@ #include <linux/string.h>
 #include <linux/pm.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
+#include <linux/hardirq.h>
 
 #include "base.h"
 
@@ -367,6 +368,7 @@ int sysdev_suspend(pm_message_t state)
 			/* Call auxillary drivers first */
 			list_for_each_entry(drv, &cls->drivers, entry) {
 				if (drv->suspend) {
+					WARN_ON(!irqs_disabled());
 					ret = drv->suspend(sysdev, state);
 					if (ret)
 						goto aux_driver;
@@ -442,6 +444,7 @@ int sysdev_resume(void)
 		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
 			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
 
+			WARN_ON(!irqs_disabled());
 			__sysdev_resume(sysdev);
 		}
 	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] s2ram: warn when interrupts should be disabled but are not

[Alan Stern]

On Sat, 12 Apr 2008, Pavel Machek wrote:

> ---
> commit fc3bccaf69e6c05464e7154369639bd5c96ccdc1
> tree bf960093ddb840242a00b4ec456ec79472b1c2ad
> parent 308e9d5309f378209e931f63f729806ec0a4d9f1
> author Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> committer Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> 
>  drivers/ata/ahci.c |    2 +-
>  drivers/base/sys.c |    3 +++
>  2 files changed, 4 insertions(+), 1 deletions(-)

What happened to the drivers/ata/ahci.c part of the patch?

> diff --git a/drivers/base/sys.c b/drivers/base/sys.c
> index 8e13fd9..e214467 100644
> --- a/drivers/base/sys.c
> +++ b/drivers/base/sys.c
> @@ -22,6 +22,7 @@ #include <linux/string.h>
>  #include <linux/pm.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
> +#include <linux/hardirq.h>
>  
>  #include "base.h"
>  
> @@ -367,6 +368,7 @@ int sysdev_suspend(pm_message_t state)
>  			/* Call auxillary drivers first */
>  			list_for_each_entry(drv, &cls->drivers, entry) {
>  				if (drv->suspend) {
> +					WARN_ON(!irqs_disabled());
>  					ret = drv->suspend(sysdev, state);
>  					if (ret)
>  						goto aux_driver;
> @@ -442,6 +444,7 @@ int sysdev_resume(void)
>  		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
>  			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
>  
> +			WARN_ON(!irqs_disabled());
>  			__sysdev_resume(sysdev);
>  		}
>  	}

Alan Stern

Re: [linux-pm] s2ram: warn when interrupts should be disabled but are not

[Pavel Machek]

On Sat 2008-04-12 11:36:32, Alan Stern wrote:
> On Sat, 12 Apr 2008, Pavel Machek wrote:
> 
> > ---
> > commit fc3bccaf69e6c05464e7154369639bd5c96ccdc1
> > tree bf960093ddb840242a00b4ec456ec79472b1c2ad
> > parent 308e9d5309f378209e931f63f729806ec0a4d9f1
> > author Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> > committer Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> > 
> >  drivers/ata/ahci.c |    2 +-
> >  drivers/base/sys.c |    3 +++
> >  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> What happened to the drivers/ata/ahci.c part of the patch?

Sorry, bad diffstat (but correct patch).

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] s2ram: warn when interrupts should be disabled but are not

[David Brownell]

On Saturday 12 April 2008, Pavel Machek wrote:
> +                                       WARN_ON(!irqs_disabled());
>                                         ret = drv->suspend(sysdev, state);

Wouldn't it be better to assert the warning AFTER the
driver had a chance to screw it up?  (On the resume
side of things too.)

We know IRQs were disabled on entry to the loop.
If something went wrong it was some goofy driver.
Best warn about it ASAP instead of *maybe* getting
a warning before the next driver (if there is one).

And if you're concerned about such issues, I'd think
similar warnings would be done in suspend_late() and
resume_early() support...

- Dave

Re: s2ram: warn when interrupts should be disabled but are not

[Andrew Morton]

On Sat, 12 Apr 2008 11:53:11 +0200 Pavel Machek <pavel@ucw.cz> wrote:

> 
> sysdevs should be suspended/resumed with IRQs disabled; if something
> reenables them by mistake, we want to know early.
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> ---
> commit fc3bccaf69e6c05464e7154369639bd5c96ccdc1
> tree bf960093ddb840242a00b4ec456ec79472b1c2ad
> parent 308e9d5309f378209e931f63f729806ec0a4d9f1
> author Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> committer Pavel <pavel@amd.ucw.cz> Sat, 12 Apr 2008 11:52:05 +0200
> 
>  drivers/ata/ahci.c |    2 +-
>  drivers/base/sys.c |    3 +++
>  2 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/base/sys.c b/drivers/base/sys.c
> index 8e13fd9..e214467 100644
> --- a/drivers/base/sys.c
> +++ b/drivers/base/sys.c
> @@ -22,6 +22,7 @@ #include <linux/string.h>
>  #include <linux/pm.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
> +#include <linux/hardirq.h>
>  
>  #include "base.h"
>  
> @@ -367,6 +368,7 @@ int sysdev_suspend(pm_message_t state)
>  			/* Call auxillary drivers first */
>  			list_for_each_entry(drv, &cls->drivers, entry) {
>  				if (drv->suspend) {
> +					WARN_ON(!irqs_disabled());
>  					ret = drv->suspend(sysdev, state);
>  					if (ret)
>  						goto aux_driver;

Is this warning ever triggers I expect we'll really really wish that
we'd done the check _after_ calling drv->suspend, and that we had also
done a print_symbol(drv->suspend).  No?


> @@ -442,6 +444,7 @@ int sysdev_resume(void)
>  		list_for_each_entry(sysdev, &cls->kset.list, kobj.entry) {
>  			pr_debug(" %s\n", kobject_name(&sysdev->kobj));
>  
> +			WARN_ON(!irqs_disabled());
>  			__sysdev_resume(sysdev);
>  		}
>  	}

Perhaps here too.