WARNING: at kernel/resource.c:189 __release_resource

Quoted 8 lines · Andrew Morton

Quoted 4 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:
Step aside. What's the purpose of having two similar patches for one issue, it then warns about the same thing twice:
make-sure-nobodys-leaking-resources.patch releasing-resources-with-children.patch
Oh well. It's better than having none. Matthew, could you have think about something for mainline please?

I submitted it for mainline. I was never quite sure why you only took it into -mm. I'll take a look at what you have in -mm these days.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Bjorn Helgaas

2007-11-29 23:40 UTC #5 lore raw

Quoted 20 lines · Andrew Morton

Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:

Ok, I hit the bug, suspend of 00:06 device complains about it:

WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83

Quoted 12 lines

Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.

We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,

03f8-03ff : 00:06

    03f8-03ff : serial

I think this problem happens because pnp_bus_suspend() calls serial_pnp_suspend(), which suspends the driver but does nothing with the resources. Then it calls pnp_stop_dev(), which releases the 00:06 resource, which still has a serial child resource.

The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.

Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().

Bjorn

[1] http://lkml.org/lkml/2005/11/30/39

Bjorn Helgaas

2007-11-29 23:40 UTC #6 lore raw

Quoted 20 lines · Andrew Morton

Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:

Ok, I hit the bug, suspend of 00:06 device complains about it:

WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83

Quoted 12 lines

Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.

We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,

03f8-03ff : 00:06

    03f8-03ff : serial

Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().

Bjorn

[1] http://lkml.org/lkml/2005/11/30/39

Andrew Morton

2007-11-30 00:42 UTC #7 lore raw

Quoted 52 lines · On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <[email protected]> wrote:

Quoted 20 lines · Andrew Morton
Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:
Ok, I hit the bug, suspend of 00:06 device complains about it:
WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83
Quoted 12 lines
Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.
We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,
03f8-03ff : 00:06
    03f8-03ff : serial
I think this problem happens because pnp_bus_suspend() calls serial_pnp_suspend(), which suspends the driver but does nothing with the resources. Then it calls pnp_stop_dev(), which releases the 00:06 resource, which still has a serial child resource.
The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.

Quoted 7 lines

Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39

So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?

Thanks.

Andrew Morton

2007-11-30 00:42 UTC #8 lore raw

Quoted 52 lines · On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <[email protected]> wrote:

Quoted 20 lines · Andrew Morton
Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:
Ok, I hit the bug, suspend of 00:06 device complains about it:
WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83
Quoted 12 lines
Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.
We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,
03f8-03ff : 00:06
    03f8-03ff : serial
I think this problem happens because pnp_bus_suspend() calls serial_pnp_suspend(), which suspends the driver but does nothing with the resources. Then it calls pnp_stop_dev(), which releases the 00:06 resource, which still has a serial child resource.
The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.

Quoted 7 lines

Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39

So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?

Thanks.

Bjorn Helgaas

2007-11-30 21:08 UTC #9 lore raw

Quoted 59 lines · Andrew Morton

Quoted 56 lines · On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <[email protected]> wrote:
Quoted 20 lines · Andrew Morton
Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:
Ok, I hit the bug, suspend of 00:06 device complains about it:
WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83
Quoted 12 lines
Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.
We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,
03f8-03ff : 00:06
    03f8-03ff : serial
I think this problem happens because pnp_bus_suspend() calls serial_pnp_suspend(), which suspends the driver but does nothing with the resources. Then it calls pnp_stop_dev(), which releases the 00:06 resource, which still has a serial child resource.
The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().

Quoted 7 lines

Quoted 3 lines
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?

I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.

Jiri, can you try the additional patch below, please?

drivers/pnp/driver.c +0 ~~-13~~

Index: linux-mm/drivers/pnp/driver.c
===================================================================
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

Bjorn Helgaas

2007-11-30 21:09 UTC #10 lore raw

Quoted 59 lines · Andrew Morton

Quoted 56 lines · On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <[email protected]> wrote:
Quoted 20 lines · Andrew Morton
Quoted 19 lines · On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <[email protected]> wrote:
Ok, I hit the bug, suspend of 00:06 device complains about it:
WARNING: at .../kernel/resource.c:185 __release_resource()

Call Trace:
 [<ffffffff8023f7b5>] release_resource+0xb5/0xf0
 [<ffffffff8036cda0>] pnp_release_resources+0x70/0x130
 [<ffffffff8036db85>] pnp_stop_dev+0x45/0x90
 [<ffffffff8036c942>] pnp_bus_suspend+0x92/0xb0
 [<ffffffff803b9f73>] suspend_device+0x113/0x180
 [<ffffffff803ba330>] device_suspend+0x200/0x320
 [<ffffffff80266905>] suspend_devices_and_enter+0xa5/0x170
 [<ffffffff80266bd9>] enter_state+0x209/0x270
 [<ffffffff80266cef>] state_store+0xaf/0xf0
 [<ffffffff8032ca67>] kobj_attr_store+0x17/0x20
 [<ffffffff802e459e>] sysfs_write_file+0xce/0x140
 [<ffffffff80299cc7>] vfs_write+0xc7/0x170
 [<ffffffff8029a360>] sys_write+0x50/0x90
 [<ffffffff8020bcde>] system_call+0x7e/0x83
Quoted 12 lines
Quoted 10 lines
# LANG=en ll /sys/devices/pnp0/00:06/
total 0
lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial
(quoted diff) +0 -2
-r--r--r-- 1 root root 4096 Nov 22 22:35 id
-r--r--r-- 1 root root 4096 Nov 22 22:35 options
drwxr-xr-x 2 root root 0 Nov 22 22:35 power
-rw-r--r-- 1 root root 4096 Nov 22 22:35 resources
lrwxrwxrwx 1 root root 0 Nov 22 22:35 subsystem -> ../../../bus/pnp
drwxr-xr-x 3 root root 0 Nov 22 22:35 tty
-rw-r--r-- 1 root root 4096 Nov 22 22:35 uevent
I suppose that's a genuine leak, presumably in 8250_pnp.
We used to have only the serial driver resource reservation. We now have an additional 00:06 resource that is the parent of the serial resource, e.g.,
03f8-03ff : 00:06
    03f8-03ff : serial
I think this problem happens because pnp_bus_suspend() calls serial_pnp_suspend(), which suspends the driver but does nothing with the resources. Then it calls pnp_stop_dev(), which releases the 00:06 resource, which still has a serial child resource.
The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().

Quoted 7 lines

Quoted 3 lines
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?

I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.

Jiri, can you try the additional patch below, please?

drivers/pnp/driver.c +0 ~~-13~~

Index: linux-mm/drivers/pnp/driver.c
===================================================================
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

Jiri Slaby

author

2007-11-30 22:49 UTC #11 lore raw

Quoted 39 lines · Bjorn Helgaas

Quoted 11 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 7 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
drivers/pnp/driver.c +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);

drivers/pnp/driver.c line 183

Quoted 10 lines

drivers/pnp/driver.c +0 -6

 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

No, it breaks suspend.

regards,

Signature · 2 lines

Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University

Jiri Slaby

author

2007-11-30 22:50 UTC #12 lore raw

Quoted 39 lines · Bjorn Helgaas

Quoted 11 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 7 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
drivers/pnp/driver.c +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);

drivers/pnp/driver.c line 183

Quoted 10 lines

drivers/pnp/driver.c +0 -6

 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

No, it breaks suspend.

regards,

Signature · 2 lines

Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University

Bjorn Helgaas

2007-11-30 22:58 UTC #13 lore raw

Quoted 41 lines · Jiri Slaby

Quoted 40 lines · Bjorn Helgaas
Quoted 11 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 7 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
drivers/pnp/driver.c +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 

Quoted 11 lines

drivers/pnp/driver.c line 183

Quoted 9 lines

drivers/pnp/driver.c +0 -6

-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

No, it breaks suspend.

Thanks for trying it. What are the symptoms? I'd like to understand why we need to stop the devices before suspend.

Bjorn

Jiri Slaby

author

2007-12-01 08:12 UTC #14 lore raw

Quoted 52 lines · Bjorn Helgaas

Quoted 40 lines · Jiri Slaby
Quoted 39 lines · Bjorn Helgaas
Quoted 11 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 7 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
drivers/pnp/driver.c +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
Quoted 10 lines
drivers/pnp/driver.c line 183
Quoted 9 lines
drivers/pnp/driver.c +0 -6
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
No, it breaks suspend.

Quoted 2 lines

Thanks for trying it. What are the symptoms? I'd like to understand why we need to stop the devices before suspend.

Ho hum, it's not so easy, it's kind of nondeterministic now. Maybe some other issue. If I remove 8250* modules from the kernel, it works. Otherwise it locks in the middle of suspend after disks and graphics go down no matter if the patch has been applied or not. Trying to investigate this further...

regards,

Signature · 2 lines

Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University

Jiri Slaby

author

2007-12-01 08:12 UTC #15 lore raw

Quoted 52 lines · Bjorn Helgaas

Quoted 40 lines · Jiri Slaby
Quoted 39 lines · Bjorn Helgaas
Quoted 11 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 7 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
drivers/pnp/driver.c +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
Quoted 10 lines
drivers/pnp/driver.c line 183
Quoted 9 lines
drivers/pnp/driver.c +0 -6
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
No, it breaks suspend.

Quoted 2 lines

Thanks for trying it. What are the symptoms? I'd like to understand why we need to stop the devices before suspend.

regards,

Signature · 2 lines

Jiri Slaby ([email protected])
Faculty of Informatics, Masaryk University

Jiri Slaby

author

2007-12-01 12:00 UTC #16 lore raw

Quoted 58 lines · Jiri Slaby

Quoted 54 lines · Bjorn Helgaas
Quoted 43 lines · Jiri Slaby
Quoted 42 lines · Bjorn Helgaas
Quoted 5 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 4 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Quoted 7 lines
Quoted 3 lines
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is
it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
--- linux-mm.orig/drivers/pnp/driver.c 2007-11-30
13:58:25.000000000 -0700
+++ linux-mm/drivers/pnp/driver.c 2007-11-30 13:59:37.000000000
-0700
(quoted diff) +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
Quoted 10 lines
Review line 183
Quoted 9 lines
(quoted diff) +0 -6
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
No, it breaks suspend.
Quoted 2 lines
Thanks for trying it. What are the symptoms? I'd like to understand why we need to stop the devices before suspend.

Quoted 7 lines

Ho hum, it's not so easy, it's kind of nondeterministic now. Maybe some other
issue. If I remove 8250* modules from the kernel, it works. Otherwise it locks
in the middle of suspend after disks and graphics go down no matter if the patch
has been applied or not. Trying to investigate this further...

I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone. Just a note, fold this -fix into it:

drivers/pnp/driver.c +0 -1

index f5b64ee..b0fc3ee 100644
@@ -170,7 +170,6 @@ static int pnp_bus_resume(struct device *dev)
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

Jiri Slaby

author

2007-12-01 12:14 UTC #17 lore raw

In reply to Jiri Slaby

Quoted 58 lines · Jiri Slaby

Quoted 54 lines · Bjorn Helgaas
Quoted 43 lines · Jiri Slaby
Quoted 42 lines · Bjorn Helgaas
Quoted 5 lines · Andrew Morton
On Thu, 29 Nov 2007 16:40:37 -0700
Quoted 4 lines
Maybe we could either remove the pnp_{stop,start}_dev() calls from the suspend/resume path, or move the PNP resource management out of pnp_{start,stop}_dev().
Quoted 7 lines
Quoted 3 lines
Bjorn
[1] http://lkml.org/lkml/2005/11/30/39
So was this particular problem caused/exposed by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, or is
it in mainline?
I'm pretty sure this problem is caused by that patch, so we we shouldn't see this in mainline.
Jiri, can you try the additional patch below, please?
Index: linux-mm/drivers/pnp/driver.c
===================================================================
--- linux-mm.orig/drivers/pnp/driver.c 2007-11-30
13:58:25.000000000 -0700
+++ linux-mm/drivers/pnp/driver.c 2007-11-30 13:59:37.000000000
-0700
(quoted diff) +0 -7
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;
@@ -185,12 +178,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
Quoted 10 lines
Review line 183
Quoted 9 lines
(quoted diff) +0 -6
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
No, it breaks suspend.
Quoted 2 lines
Thanks for trying it. What are the symptoms? I'd like to understand why we need to stop the devices before suspend.

Quoted 7 lines

Ho hum, it's not so easy, it's kind of nondeterministic now. Maybe some other
issue. If I remove 8250* modules from the kernel, it works. Otherwise it locks
in the middle of suspend after disks and graphics go down no matter if the patch
has been applied or not. Trying to investigate this further...

drivers/pnp/driver.c +0 -1

index f5b64ee..b0fc3ee 100644
@@ -170,7 +170,6 @@ static int pnp_bus_resume(struct device *dev)
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

Bjorn Helgaas

2007-12-05 18:24 UTC #18 lore raw

RFC: PNP: do not stop/start devices in suspend/resume path

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110

Quoted 3 lines · Jiri Slaby

I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.

Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.

RFC: PNP: do not stop/start devices in suspend/resume path

Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.

This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:

03f8-03ff : 00:06

    03f8-03ff : serial

pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.

However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because

- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.

This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.

[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]

drivers/pnp/driver.c +0 ~~-13~~

Index: linux-mm/drivers/pnp/driver.c
===================================================================
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -177,7 +170,6 @@
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

@@ -185,12 +177,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}

Matthew Wilcox

2007-12-05 18:50 UTC #19 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 10 lines · Bjorn Helgaas

This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.

So, I put the warning in there because I genuinely didn't know if we were doing this, and if so what the semantics should be.

I'm quite happy to see resources with children being released -- I just want to know what we should do with those children. As the first person to come across this problem, I think you get to decide the semantics.

It seems to me from this description that all we want to do is reparent the children -- which is easy enough to do; just a matter of twizzling all the ->parent pointers of the children; the ->sibling of the resource before the one being released and the ->sibling of the last child of the one being released.

If you think it's a genuine bug that happens to have been caught by this check, I'm happy to see the check being useful, and we don't need to allow the removal of resources.

The only thing is, if you're going to be re-acquiring these resources upon resume, you'll need to use insert_resource() instead of request_region() because the child will now be in that spot.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Matthew Wilcox

2007-12-05 18:50 UTC #20 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 10 lines · Bjorn Helgaas

This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.

So, I put the warning in there because I genuinely didn't know if we were doing this, and if so what the semantics should be.

If you think it's a genuine bug that happens to have been caught by this check, I'm happy to see the check being useful, and we don't need to allow the removal of resources.

The only thing is, if you're going to be re-acquiring these resources upon resume, you'll need to use insert_resource() instead of request_region() because the child will now be in that spot.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Rafael J. Wysocki

2007-12-06 00:49 UTC #21 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 48 lines · Bjorn Helgaas

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110
Quoted 3 lines · Jiri Slaby
I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
RFC: PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.
However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

Quoted 7 lines

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]

The only comment I can make is that this change is along with the lines of what PCI drivers do, so it looks reasonable.

In any case it's worthy of checking if removing the pnp_stop(start)_dev() invocations from the suspend/resume code paths will cause any problems to appear, IMO.

Quoted 48 lines

Index: linux-mm/drivers/pnp/driver.c
===================================================================

drivers/pnp/driver.c +0 ~~-15~~

@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -177,7 +170,6 @@
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

@@ -185,12 +177,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line

"Premature optimization is the root of all evil." - Donald Knuth

Rafael J. Wysocki

2007-12-06 01:07 UTC #22 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 48 lines · Bjorn Helgaas

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110
Quoted 3 lines · Jiri Slaby
I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
RFC: PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.
However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

Quoted 7 lines

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]

The only comment I can make is that this change is along with the lines of what PCI drivers do, so it looks reasonable.

In any case it's worthy of checking if removing the pnp_stop(start)_dev() invocations from the suspend/resume code paths will cause any problems to appear, IMO.

Quoted 48 lines

Index: linux-mm/drivers/pnp/driver.c
===================================================================

drivers/pnp/driver.c +0 ~~-15~~

@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -177,7 +170,6 @@
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

@@ -185,12 +177,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);
 
--

Signature · 1 line

"Premature optimization is the root of all evil." - Donald Knuth

Bjorn Helgaas

2007-12-06 23:25 UTC #23 lore raw

PNP: do not stop/start devices in suspend/resume path

Quoted 5 lines · Bjorn Helgaas

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110
On Saturday 01 December 2007 05:00:34 am Jiri Slaby wrote:

Quoted 8 lines

Quoted 3 lines
I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.

I checked with a Windows kernel person, who said that when preparing to sleep, Windows puts devices in S3 state (I think he meant D3) and does not use _DIS.

I think this patch is the right thing to do. It's possible we'll trip over some BIOS issue, but in the long term, we should try to do it the same way Windows does. Andrew, can you add this before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch?

Thanks,
Bjorn

PNP: do not stop/start devices in suspend/resume path

I'm told that Windows puts devices in a low-power state (Linux does this in the protocol suspend method), but does not use _DIS in the suspend path. Other relevant references:

- This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.

drivers/pnp/driver.c +0 ~~-14~~

Index: linux-mm/drivers/pnp/driver.c
===================================================================
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -177,7 +170,6 @@
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

@@ -185,12 +177,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

Andrew Morton

2007-12-12 08:16 UTC #24 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 6 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:

Andrew, can you add this before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch?
...
PNP: do not stop/start devices in suspend/resume path

I did, but I also temporarily dropped pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch.

Is it expected that this patch will fix pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch? Should I bring it back?

Andrew Morton

2007-12-12 08:16 UTC #25 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 6 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:

Andrew, can you add this before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch?
...
PNP: do not stop/start devices in suspend/resume path

I did, but I also temporarily dropped pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch.

Is it expected that this patch will fix pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch? Should I bring it back?

Bjorn Helgaas

2007-12-12 16:29 UTC #26 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:
Andrew, can you add this before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch?
...
PNP: do not stop/start devices in suspend/resume path
I did, but I also temporarily dropped pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch.

Thanks.

Quoted 3 lines

Is it expected that this patch will fix pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch? Should I bring it back?

No, not yet. The "do not stop/start" patch should fix the kernel/resource.c warning, but I don't understand the "acpi reboots machine" (critical temp reached) problem yet.

Bjorn

Bjorn Helgaas

2007-12-12 16:29 UTC #27 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:
Andrew, can you add this before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch?
...
PNP: do not stop/start devices in suspend/resume path
I did, but I also temporarily dropped pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch.

Thanks.

Quoted 3 lines

Is it expected that this patch will fix pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch? Should I bring it back?

No, not yet. The "do not stop/start" patch should fix the kernel/resource.c warning, but I don't understand the "acpi reboots machine" (critical temp reached) problem yet.

Bjorn

Pierre Ossman

Tested-by

2007-12-13 08:26 UTC #28 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 21 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:

PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
I'm told that Windows puts devices in a low-power state (Linux does this in the protocol suspend method), but does not use _DIS in the suspend path. Other relevant references:
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
- This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

Quoted 6 lines

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
Signed-off-by Bjorn Helgaas <[email protected]>

Tested-by Pierre Ossman <[email protected]>

No noticeable issues with suspend or hibernate using this patch.

Rgds
Pierre

Pierre Ossman

Tested-by

2007-12-13 08:26 UTC #29 lore raw

Re: PNP: do not stop/start devices in suspend/resume path

Quoted 21 lines · On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <[email protected]> wrote:

PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
I'm told that Windows puts devices in a low-power state (Linux does this in the protocol suspend method), but does not use _DIS in the suspend path. Other relevant references:
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
- This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

Quoted 6 lines

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
Signed-off-by Bjorn Helgaas <[email protected]>

Tested-by Pierre Ossman <[email protected]>

No noticeable issues with suspend or hibernate using this patch.

Rgds
Pierre

Shaohua Li

2007-12-07 07:13 UTC #30 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 88 lines · Bjorn Helgaas

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110
Quoted 1 line · Jiri Slaby
I didn't get it. Maybe some trolls poking around or something (maybe
the
Quoted 2 lines
ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
RFC: PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.
However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).
- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]
Signed-off-by Bjorn Helgaas <[email protected]>
Index: linux-mm/drivers/pnp/driver.c
===================================================================
--- linux-mm.orig/drivers/pnp/driver.c 2007-11-30 13:58:25.000000000 -0700
+++ linux-mm/drivers/pnp/driver.c 2007-12-03 09:58:35.000000000
-0700
drivers/pnp/driver.c +0 -8
@@ -161,13 +161,6 @@
                        return error;
        }
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-           pnp_can_disable(pnp_dev)) {
-               error = pnp_stop_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
        if (pnp_dev->protocol && pnp_dev->protocol->suspend)
                pnp_dev->protocol->suspend(pnp_dev, state);
        return 0;
@@ -177,7 +170,6 @@
 {
        struct pnp_dev *pnp_dev = to_pnp_dev(dev);
        struct pnp_driver *pnp_drv = pnp_dev->driver;
-       int error;
 
        if (!pnp_drv)
                return 0;

drivers/pnp/driver.c line 193

Quoted 9 lines

drivers/pnp/driver.c +0 -6

@@ -185,12 +177,6 @@
        if (pnp_dev->protocol && pnp_dev->protocol->resume)
                pnp_dev->protocol->resume(pnp_dev);
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-               error = pnp_start_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-

I'd suggest keep pnp_start_dev here to prevent BIOS not or assign different resources after a resume.

Shaohua Li

2007-12-07 07:20 UTC #31 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 88 lines · Bjorn Helgaas

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110
Quoted 1 line · Jiri Slaby
I didn't get it. Maybe some trolls poking around or something (maybe
the
Quoted 2 lines
ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
RFC: PNP: do not stop/start devices in suspend/resume path
Do not disable PNP devices in the suspend path. We still call the driver's suspend method, which should prevent further use of the device, and the protocol suspend method, which may put the device in a low-power state.
This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:
03f8-03ff : 00:06
    03f8-03ff : serial
pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.
However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because
- In the ACPI 3.0b spec, I can't find any mention of _DIS in connection with sleep. And Device Object Notifications, Section 5.6.3, Table 5-43, says we should get a bus check after awakening if hardware was removed while we slept.
This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).
- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.
[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]
Signed-off-by Bjorn Helgaas <[email protected]>
Index: linux-mm/drivers/pnp/driver.c
===================================================================
--- linux-mm.orig/drivers/pnp/driver.c 2007-11-30 13:58:25.000000000 -0700
+++ linux-mm/drivers/pnp/driver.c 2007-12-03 09:58:35.000000000
-0700
drivers/pnp/driver.c +0 -8
@@ -161,13 +161,6 @@
                        return error;
        }
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-           pnp_can_disable(pnp_dev)) {
-               error = pnp_stop_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
        if (pnp_dev->protocol && pnp_dev->protocol->suspend)
                pnp_dev->protocol->suspend(pnp_dev, state);
        return 0;
@@ -177,7 +170,6 @@
 {
        struct pnp_dev *pnp_dev = to_pnp_dev(dev);
        struct pnp_driver *pnp_drv = pnp_dev->driver;
-       int error;
 
        if (!pnp_drv)
                return 0;

drivers/pnp/driver.c line 193

Quoted 9 lines

drivers/pnp/driver.c +0 -6

@@ -185,12 +177,6 @@
        if (pnp_dev->protocol && pnp_dev->protocol->resume)
                pnp_dev->protocol->resume(pnp_dev);
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-               error = pnp_start_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-

I'd suggest keep pnp_start_dev here to prevent BIOS not or assign different resources after a resume.

Bjorn Helgaas

2007-12-10 23:26 UTC #32 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

Quoted 33 lines · Shaohua Li

Quoted 32 lines · Bjorn Helgaas

Index: linux-mm/drivers/pnp/driver.c
===================================================================
--- linux-mm.orig/drivers/pnp/driver.c 2007-11-30 13:58:25.000000000 -0700
+++ linux-mm/drivers/pnp/driver.c 2007-12-03 09:58:35.000000000
-0700

drivers/pnp/driver.c +0 -8

@@ -161,13 +161,6 @@
                        return error;
        }
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-           pnp_can_disable(pnp_dev)) {
-               error = pnp_stop_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
        if (pnp_dev->protocol && pnp_dev->protocol->suspend)
                pnp_dev->protocol->suspend(pnp_dev, state);
        return 0;

@@ -177,7 +170,6 @@
 {
        struct pnp_dev *pnp_dev = to_pnp_dev(dev);
        struct pnp_driver *pnp_drv = pnp_dev->driver;
-       int error;
 
        if (!pnp_drv)
                return 0;

@@ -185,12 +177,6 @@
        if (pnp_dev->protocol && pnp_dev->protocol->resume)
                pnp_dev->protocol->resume(pnp_dev);

Quoted 8 lines

drivers/pnp/driver.c
Quoted 6 lines
drivers/pnp/driver.c +0 -6
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-               error = pnp_start_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
I'd suggest keep pnp_start_dev here to prevent BIOS not or assign different resources after a resume.

The patch I currently have in -mm (http://lkml.org/lkml/2007/10/29/412)
merely requests resources in pnp_start_dev() and releases them in pnp_stop_dev(). So if we remove pnp_stop_dev() but keep pnp_start_dev(), I have to fix that patch to deal with things that may already be reserved.

But I don't see any mention in the spec of running _SRS in the sleep/wakup path, so I'm not convinced it's really necessary. Section 7.4 mentions _TTS, _PTS, _GTS, etc., but not _SRS.

For devices, it looks like the intent is that BIOS should generate notifications that cause OSPM to re-enumerate devices that might have changed. I'm pretty sure Linux is missing some of that code, though, so I could believe that _SRS might help paper over that deficiency.

What I'd really like to do is figure out how Windows uses _SRS and do the same thing.

Bjorn

Bjorn Helgaas

2007-12-10 23:26 UTC #33 lore raw

Re: RFC: PNP: do not stop/start devices in suspend/resume path

In reply to Shaohua Li

Quoted 33 lines · Shaohua Li

Quoted 32 lines · Bjorn Helgaas

drivers/pnp/driver.c +0 -8

@@ -161,13 +161,6 @@
                        return error;
        }
 
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-           pnp_can_disable(pnp_dev)) {
-               error = pnp_stop_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
        if (pnp_dev->protocol && pnp_dev->protocol->suspend)
                pnp_dev->protocol->suspend(pnp_dev, state);
        return 0;

@@ -177,7 +170,6 @@
 {
        struct pnp_dev *pnp_dev = to_pnp_dev(dev);
        struct pnp_driver *pnp_drv = pnp_dev->driver;
-       int error;
 
        if (!pnp_drv)
                return 0;

@@ -185,12 +177,6 @@
        if (pnp_dev->protocol && pnp_dev->protocol->resume)
                pnp_dev->protocol->resume(pnp_dev);

Quoted 8 lines

drivers/pnp/driver.c
Quoted 6 lines
drivers/pnp/driver.c +0 -6
-       if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-               error = pnp_start_dev(pnp_dev);
-               if (error)
-                       return error;
-       }
-
I'd suggest keep pnp_start_dev here to prevent BIOS not or assign different resources after a resume.

But I don't see any mention in the spec of running _SRS in the sleep/wakup path, so I'm not convinced it's really necessary. Section 7.4 mentions _TTS, _PTS, _GTS, etc., but not _SRS.

What I'd really like to do is figure out how Windows uses _SRS and do the same thing.

Bjorn

Stephane Ascoet

2007-12-23 23:39 UTC #34 lore raw

Re: do not stop/start devices in suspend/resume path: the SCSI case

In reply to Stephane Ascoet

Quoted 4 lines · Bjorn Helgaas

Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.

Hi, is there a chance for SCSI sleeping being managed by ACPI(it seems to be the case for IDE)?

Signature · 6 lines

Bien cordialement, Stephane Ascoet, conseils pour que la technologie soit un progres respectueux de la vie.
Album photo:<http://www.flickr.com/photos/stephaneascoet/>
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bjorn Helgaas

2007-12-24 03:21 UTC #35 lore raw

Re: do not stop/start devices in suspend/resume path: the SCSI case

Quoted 8 lines · Stephane Ascoet

Quoted 4 lines · Bjorn Helgaas
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
Hi, is there a chance for SCSI sleeping being managed by ACPI(it seems to be the case for IDE)?

I'm sorry, I don't know the answer to this.

Bjorn

Bjorn Helgaas

2007-12-24 03:23 UTC #36 lore raw

Re: do not stop/start devices in suspend/resume path: the SCSI case

Quoted 8 lines · Stephane Ascoet

Quoted 4 lines · Bjorn Helgaas
Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.
Hi, is there a chance for SCSI sleeping being managed by ACPI(it seems to be the case for IDE)?

I'm sorry, I don't know the answer to this.

Bjorn

-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephane Ascoet

2007-12-24 01:43 UTC #37 lore raw

Re: do not stop/start devices in suspend/resume path: the SCSI case

Quoted 4 lines · Bjorn Helgaas

Good. It's not clear to me whether it is safe to leave devices enabled while we sleep. I don't see an actual problem, but there might be something related to hotplug while we're asleep or something. So I'll cc: some additional people who might have some insight.

Hi, is there a chance for SCSI sleeping being managed by ACPI(it seems to be the case for IDE)?

Signature · 2 lines

Bien cordialement, Stephane Ascoet, conseils pour que la technologie soit un progres respectueux de la vie.
Album photo:<http://www.flickr.com/photos/stephaneascoet/>

Bjorn Helgaas

2007-12-05 18:24 UTC #38 lore raw

RFC: PNP: do not stop/start devices in suspend/resume path

In reply to Jiri Slaby

Re: warning on suspend-to-RAM caused by pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch, thread here: http://lkml.org/lkml/2007/11/22/110

Quoted 3 lines · Jiri Slaby

I didn't get it. Maybe some trolls poking around or something (maybe the ext3 breakage which fsck fixed). It works after recompilation of the whole tree. And the important part -- the warning has gone.

RFC: PNP: do not stop/start devices in suspend/resume path

This means we will not disable the device and release its resources. The driver suspend method typically does not release its resources in the suspend path. For example, if we have:

03f8-03ff : 00:06

    03f8-03ff : serial

pnp_stop_dev() would release the 00:06 region, which still has a child. This causes a warning from __release_resource and corrupts /proc/ioports.

However, we should do this the same way Windows does, so if Windows disables devices before going to sleep, we should, too. It doesn't *look* necessary to me because

This: http://msdn2.microsoft.com/en-us/library/ms810079.aspx
makes a similar point about how the OS re-enumerates devices as a result of a power state change (3rd last paragraph of text).

- This: http://msdn2.microsoft.com/en-us/library/aa489874.aspx
suggests that Windows only stops a device to rebalance hardware resources.

[This should go before pnp-request-ioport-and-iomem-resources-used-by-active-devices.patch for best bisect-ability.]

drivers/pnp/driver.c +0 ~~-14~~

Index: linux-mm/drivers/pnp/driver.c
===================================================================
@@ -161,13 +161,6 @@
 			return error;
 	}
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE) &&
-	    pnp_can_disable(pnp_dev)) {
-		error = pnp_stop_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_dev->protocol && pnp_dev->protocol->suspend)
 		pnp_dev->protocol->suspend(pnp_dev, state);
 	return 0;

@@ -177,7 +170,6 @@
 {
 	struct pnp_dev *pnp_dev = to_pnp_dev(dev);
 	struct pnp_driver *pnp_drv = pnp_dev->driver;
-	int error;
 
 	if (!pnp_drv)
 		return 0;

@@ -185,12 +177,6 @@
 	if (pnp_dev->protocol && pnp_dev->protocol->resume)
 		pnp_dev->protocol->resume(pnp_dev);
 
-	if (!(pnp_drv->flags & PNP_DRIVER_RES_DO_NOT_CHANGE)) {
-		error = pnp_start_dev(pnp_dev);
-		if (error)
-			return error;
-	}
-
 	if (pnp_drv->resume)
 		return pnp_drv->resume(pnp_dev);

Pierre Ossman

2007-12-01 21:01 UTC #39 lore raw

Quoted 7 lines · On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <[email protected]> wrote:

The corresponding PCI code in pci_device_suspend() does not do any generic device disable or resource release. I don't know why PNP disables the device on suspend. I glanced through the ACPI spec but didn't see a requirement for it. Maybe Pierre [1] remembers.

Afraid not. There was a reason for it, but my mind draws a blank as to what that was... I have a slight recollection of bad BIOS interaction during STR, but I'm not sure it was related to this specific patch.

Rgds
Pierre

Pierre Ossman

2007-12-01 21:01 UTC #40 lore raw