2006-02-07 22:43:42

by Nigel Cunningham

[permalink] [raw]
Subject: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

Hi all.

This patch is designed to help with diagnosing and fixing the cause of
problems in suspending/resuming, due to drivers wrongly re-enabling
interrupts in their .suspend or .resume methods.

I nearly forgot about it in sending patches in suspend2 that might help
where swsusp fails.

Signed-off-by: Nigel Cunningham <[email protected]>

power/resume.c | 5 +++++
power/suspend.c | 5 +++++
sys.c | 37 +++++++++++++++++++++++++++++++++++--
3 files changed, 45 insertions(+), 2 deletions(-)
diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/power/resume.c
8010-driver-model-debug.patch-new/drivers/base/power/resume.c
--- 8010-driver-model-debug.patch-old/drivers/base/power/resume.c
2006-01-19 21:27:39.000000000 +1000
+++ 8010-driver-model-debug.patch-new/drivers/base/power/resume.c
2006-01-31 19:54:52.000000000 +1000
@@ -35,6 +35,11 @@ int resume_device(struct device * dev)
if (dev->bus && dev->bus->resume) {
dev_dbg(dev,"resuming\n");
error = dev->bus->resume(dev);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming device
%s.\n",
+ kobject_name(&dev->kobj));
+ local_irq_disable();
+ }
}
up(&dev->sem);
return error;
diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/power/suspend.c
8010-driver-model-debug.patch-new/drivers/base/power/suspend.c
--- 8010-driver-model-debug.patch-old/drivers/base/power/suspend.c
2006-01-19 21:27:39.000000000 +1000
+++ 8010-driver-model-debug.patch-new/drivers/base/power/suspend.c
2006-01-31 19:54:44.000000000 +1000
@@ -58,6 +58,11 @@ int suspend_device(struct device * dev,
if (dev->bus && dev->bus->suspend && !dev->power.power_state.event) {
dev_dbg(dev, "suspending\n");
error = dev->bus->suspend(dev, state);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
device %s.\n",
+ kobject_name(&dev->kobj));
+ local_irq_disable();
+ }
}
up(&dev->sem);
return error;
diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/sys.c
8010-driver-model-debug.patch-new/drivers/base/sys.c
--- 8010-driver-model-debug.patch-old/drivers/base/sys.c 2006-01-19
21:27:39.000000000 +1000
+++ 8010-driver-model-debug.patch-new/drivers/base/sys.c 2006-01-31
19:54:09.000000000 +1000
@@ -298,16 +298,34 @@ static void __sysdev_resume(struct sys_d
if (cls->resume)
cls->resume(dev);

+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
class specific driver %s.\n",
+ kobject_name(&dev->kobj));
+ local_irq_disable();
+ }
+
/* Call auxillary drivers next. */
list_for_each_entry(drv, &cls->drivers, entry) {
- if (drv->resume)
+ if (drv->resume) {
drv->resume(dev);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
class driver %s.\n",
+ kobject_name(&dev->kobj));
+ local_irq_disable();
+ }
+ }
}

/* Call global drivers. */
list_for_each_entry(drv, &sysdev_drivers, entry) {
- if (drv->resume)
+ if (drv->resume) {
drv->resume(dev);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
driver %s.\n",
+ kobject_name(&dev->kobj));
+ local_irq_disable();
+ }
+ }
}
}

@@ -346,6 +364,11 @@ int sysdev_suspend(pm_message_t state)
list_for_each_entry(drv, &sysdev_drivers, entry) {
if (drv->suspend) {
ret = drv->suspend(sysdev, state);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
sysdev driver %s.\n",
+ kobject_name(&sysdev->kobj));
+ local_irq_disable();
+ }
if (ret)
goto gbl_driver;
}
@@ -355,6 +378,11 @@ int sysdev_suspend(pm_message_t state)
list_for_each_entry(drv, &cls->drivers, entry) {
if (drv->suspend) {
ret = drv->suspend(sysdev, state);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
sysdev class driver %s.\n",
+ kobject_name(&sysdev->kobj));
+ local_irq_disable();
+ }
if (ret)
goto aux_driver;
}
@@ -363,6 +391,11 @@ int sysdev_suspend(pm_message_t state)
/* Now call the generic one */
if (cls->suspend) {
ret = cls->suspend(sysdev, state);
+ if (!irqs_disabled()) {
+ printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
class driver %s.\n",
+ kobject_name(&sysdev->kobj));
+ local_irq_disable();
+ }
if (ret)
goto cls_driver;
}


Attachments:
(No filename) (4.44 kB)
(No filename) (189.00 B)
Download all attachments

2006-02-07 23:59:28

by Russell King

[permalink] [raw]
Subject: Re: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

On Tue, Feb 07, 2006 at 07:06:48PM +1000, Nigel Cunningham wrote:
> This patch is designed to help with diagnosing and fixing the cause of
> problems in suspending/resuming, due to drivers wrongly re-enabling
> interrupts in their .suspend or .resume methods.

Sorry, aren't interrupts enabled at this point anyway? They are if
you issue a suspend to RAM via sysfs, eg:

state_store->enter_state->suspend_prepare->device_suspend->suspend_device

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core

2006-02-08 03:47:12

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

On Tue, 2006-02-07 at 19:06 +1000, Nigel Cunningham wrote:
> Hi all.
>
> This patch is designed to help with diagnosing and fixing the cause of
> problems in suspending/resuming, due to drivers wrongly re-enabling
> interrupts in their .suspend or .resume methods.
>
> I nearly forgot about it in sending patches in suspend2 that might help
> where swsusp fails.

Ugh ? Interrupts aren't supposed to be off during resume()...

Ben.

> Signed-off-by: Nigel Cunningham <[email protected]>
>
> power/resume.c | 5 +++++
> power/suspend.c | 5 +++++
> sys.c | 37 +++++++++++++++++++++++++++++++++++--
> 3 files changed, 45 insertions(+), 2 deletions(-)
> diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/power/resume.c
> 8010-driver-model-debug.patch-new/drivers/base/power/resume.c
> --- 8010-driver-model-debug.patch-old/drivers/base/power/resume.c
> 2006-01-19 21:27:39.000000000 +1000
> +++ 8010-driver-model-debug.patch-new/drivers/base/power/resume.c
> 2006-01-31 19:54:52.000000000 +1000
> @@ -35,6 +35,11 @@ int resume_device(struct device * dev)
> if (dev->bus && dev->bus->resume) {
> dev_dbg(dev,"resuming\n");
> error = dev->bus->resume(dev);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming device
> %s.\n",
> + kobject_name(&dev->kobj));
> + local_irq_disable();
> + }
> }
> up(&dev->sem);
> return error;
> diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/power/suspend.c
> 8010-driver-model-debug.patch-new/drivers/base/power/suspend.c
> --- 8010-driver-model-debug.patch-old/drivers/base/power/suspend.c
> 2006-01-19 21:27:39.000000000 +1000
> +++ 8010-driver-model-debug.patch-new/drivers/base/power/suspend.c
> 2006-01-31 19:54:44.000000000 +1000
> @@ -58,6 +58,11 @@ int suspend_device(struct device * dev,
> if (dev->bus && dev->bus->suspend && !dev->power.power_state.event) {
> dev_dbg(dev, "suspending\n");
> error = dev->bus->suspend(dev, state);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
> device %s.\n",
> + kobject_name(&dev->kobj));
> + local_irq_disable();
> + }
> }
> up(&dev->sem);
> return error;
> diff -ruNp 8010-driver-model-debug.patch-old/drivers/base/sys.c
> 8010-driver-model-debug.patch-new/drivers/base/sys.c
> --- 8010-driver-model-debug.patch-old/drivers/base/sys.c 2006-01-19
> 21:27:39.000000000 +1000
> +++ 8010-driver-model-debug.patch-new/drivers/base/sys.c 2006-01-31
> 19:54:09.000000000 +1000
> @@ -298,16 +298,34 @@ static void __sysdev_resume(struct sys_d
> if (cls->resume)
> cls->resume(dev);
>
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
> class specific driver %s.\n",
> + kobject_name(&dev->kobj));
> + local_irq_disable();
> + }
> +
> /* Call auxillary drivers next. */
> list_for_each_entry(drv, &cls->drivers, entry) {
> - if (drv->resume)
> + if (drv->resume) {
> drv->resume(dev);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
> class driver %s.\n",
> + kobject_name(&dev->kobj));
> + local_irq_disable();
> + }
> + }
> }
>
> /* Call global drivers. */
> list_for_each_entry(drv, &sysdev_drivers, entry) {
> - if (drv->resume)
> + if (drv->resume) {
> drv->resume(dev);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while resuming sysdev
> driver %s.\n",
> + kobject_name(&dev->kobj));
> + local_irq_disable();
> + }
> + }
> }
> }
>
> @@ -346,6 +364,11 @@ int sysdev_suspend(pm_message_t state)
> list_for_each_entry(drv, &sysdev_drivers, entry) {
> if (drv->suspend) {
> ret = drv->suspend(sysdev, state);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
> sysdev driver %s.\n",
> + kobject_name(&sysdev->kobj));
> + local_irq_disable();
> + }
> if (ret)
> goto gbl_driver;
> }
> @@ -355,6 +378,11 @@ int sysdev_suspend(pm_message_t state)
> list_for_each_entry(drv, &cls->drivers, entry) {
> if (drv->suspend) {
> ret = drv->suspend(sysdev, state);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
> sysdev class driver %s.\n",
> + kobject_name(&sysdev->kobj));
> + local_irq_disable();
> + }
> if (ret)
> goto aux_driver;
> }
> @@ -363,6 +391,11 @@ int sysdev_suspend(pm_message_t state)
> /* Now call the generic one */
> if (cls->suspend) {
> ret = cls->suspend(sysdev, state);
> + if (!irqs_disabled()) {
> + printk(KERN_EMERG "WARNING: Interrupts reenabled while suspending
> class driver %s.\n",
> + kobject_name(&sysdev->kobj));
> + local_irq_disable();
> + }
> if (ret)
> goto cls_driver;
> }

2006-02-08 05:40:45

by Dmitry Torokhov

[permalink] [raw]
Subject: Re: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

On Tuesday 07 February 2006 04:06, Nigel Cunningham wrote:
> Hi all.
>
> This patch is designed to help with diagnosing and fixing the cause of
> problems in suspending/resuming, due to drivers wrongly re-enabling
> interrupts in their .suspend or .resume methods.
>
> I nearly forgot about it in sending patches in suspend2 that might help
> where swsusp fails.
>

Only sysdevs are guaranteed to be suspebded/resumed with interrupts off,
other devices are suspended with interrupts on (at least on first pass
over device list).

--
Dmitry

2006-02-08 06:45:57

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

On Wednesday 08 February 2006 06:40, Dmitry Torokhov wrote:
> On Tuesday 07 February 2006 04:06, Nigel Cunningham wrote:
> > Hi all.
> >
> > This patch is designed to help with diagnosing and fixing the cause of
> > problems in suspending/resuming, due to drivers wrongly re-enabling
> > interrupts in their .suspend or .resume methods.
> >
> > I nearly forgot about it in sending patches in suspend2 that might help
> > where swsusp fails.
> >
>
> Only sysdevs are guaranteed to be suspebded/resumed with interrupts off,
> other devices are suspended with interrupts on (at least on first pass
> over device list).

Yes, and AFAICT this is how it's supposed to be. [There was an LKML thread
last year that finished with this conclusion. I think I can find it for reference,
if necessary.]

Greetings,
Rafael

2006-02-09 00:02:17

by Nigel Cunningham

[permalink] [raw]
Subject: Re: [PATCH] Complain if driver reenables interrupts during drivers_[suspend|resume] & re-disable

Hi Dmitry et al.

On Wednesday 08 February 2006 15:40, Dmitry Torokhov wrote:
> On Tuesday 07 February 2006 04:06, Nigel Cunningham wrote:
> > Hi all.
> >
> > This patch is designed to help with diagnosing and fixing the cause of
> > problems in suspending/resuming, due to drivers wrongly re-enabling
> > interrupts in their .suspend or .resume methods.
> >
> > I nearly forgot about it in sending patches in suspend2 that might help
> > where swsusp fails.
> >
>
> Only sysdevs are guaranteed to be suspebded/resumed with interrupts off,
> other devices are suspended with interrupts on (at least on first pass
> over device list).

Ok. I guess I missed that outcome of that discussion. Sorry for the
bogusness :(. Is the sysdev bit useful at all?

Regards,

Nigel


Attachments:
(No filename) (773.00 B)
(No filename) (189.00 B)
Download all attachments