* WARNING: at kernel/resource.c:189 __release_resource @ 2007-11-22 21:41 Jiri Slaby 2007-11-27 6:05 ` Andrew Morton 0 siblings, 1 reply; 22+ messages in thread From: Jiri Slaby @ 2007-11-22 21:41 UTC (permalink / raw) To: Matthew Wilcox Cc: Andrew Morton, Linux kernel mailing list, Greg KH, linux-usb-devel, linux-pm Hi, 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 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 # LANG=en ll /sys/devices/pnp0/00:06/ total 0 lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial -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 regards, -- Jiri Slaby (jirislaby@gmail.com) Faculty of Informatics, Masaryk University ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-22 21:41 WARNING: at kernel/resource.c:189 __release_resource Jiri Slaby @ 2007-11-27 6:05 ` Andrew Morton 2007-11-27 12:38 ` Matthew Wilcox 2007-11-29 23:40 ` Bjorn Helgaas 0 siblings, 2 replies; 22+ messages in thread From: Andrew Morton @ 2007-11-27 6:05 UTC (permalink / raw) To: Jiri Slaby Cc: Matthew Wilcox, Linux kernel mailing list, Greg KH, linux-usb-devel, linux-pm, Bjorn Helgaas On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <jirislaby@gmail.com> wrote: > Hi, > > 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? > 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 > > # LANG=en ll /sys/devices/pnp0/00:06/ > total 0 > lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial > -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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-27 6:05 ` Andrew Morton @ 2007-11-27 12:38 ` Matthew Wilcox 2007-11-29 23:40 ` Bjorn Helgaas 1 sibling, 0 replies; 22+ messages in thread From: Matthew Wilcox @ 2007-11-27 12:38 UTC (permalink / raw) To: Andrew Morton Cc: Jiri Slaby, Linux kernel mailing list, Greg KH, linux-usb-devel, linux-pm, Bjorn Helgaas On Mon, Nov 26, 2007 at 10:05:38PM -0800, Andrew Morton wrote: > On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <jirislaby@gmail.com> 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. -- 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." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-27 6:05 ` Andrew Morton 2007-11-27 12:38 ` Matthew Wilcox @ 2007-11-29 23:40 ` Bjorn Helgaas 2007-11-30 0:42 ` Andrew Morton 2007-12-01 21:01 ` WARNING: at kernel/resource.c:189 __release_resource Pierre Ossman 1 sibling, 2 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-11-29 23:40 UTC (permalink / raw) To: Andrew Morton Cc: Jiri Slaby, Matthew Wilcox, Linux kernel mailing list, Greg KH, linux-usb-devel, linux-pm, Pierre Ossman On Monday 26 November 2007 11:05:38 pm Andrew Morton wrote: > On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <jirislaby@gmail.com> 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 > > > > # LANG=en ll /sys/devices/pnp0/00:06/ > > total 0 > > lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial > > -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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-29 23:40 ` Bjorn Helgaas @ 2007-11-30 0:42 ` Andrew Morton 2007-11-30 21:08 ` Bjorn Helgaas 2007-12-01 21:01 ` WARNING: at kernel/resource.c:189 __release_resource Pierre Ossman 1 sibling, 1 reply; 22+ messages in thread From: Andrew Morton @ 2007-11-30 0:42 UTC (permalink / raw) To: Bjorn Helgaas Cc: jirislaby, matthew, linux-kernel, gregkh, linux-usb-devel, linux-pm, drzeus On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: > On Monday 26 November 2007 11:05:38 pm Andrew Morton wrote: > > On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <jirislaby@gmail.com> 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 > > > > > > # LANG=en ll /sys/devices/pnp0/00:06/ > > > total 0 > > > lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial > > > -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 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-30 0:42 ` Andrew Morton @ 2007-11-30 21:08 ` Bjorn Helgaas 2007-11-30 22:49 ` Jiri Slaby 0 siblings, 1 reply; 22+ messages in thread From: Bjorn Helgaas @ 2007-11-30 21:08 UTC (permalink / raw) To: Andrew Morton Cc: jirislaby, matthew, linux-kernel, gregkh, linux-usb-devel, linux-pm, drzeus On Thursday 29 November 2007 05:42:07 pm Andrew Morton wrote: > On Thu, 29 Nov 2007 16:40:37 -0700 > Bjorn Helgaas <bjorn.helgaas@hp.com> wrote: > > > On Monday 26 November 2007 11:05:38 pm Andrew Morton wrote: > > > On Thu, 22 Nov 2007 22:41:16 +0100 Jiri Slaby <jirislaby@gmail.com> 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 > > > > > > > > # LANG=en ll /sys/devices/pnp0/00:06/ > > > > total 0 > > > > lrwxrwxrwx 1 root root 0 Nov 22 22:35 driver -> ../../../bus/pnp/drivers/serial > > > > -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 > > 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 @@ -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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-30 21:08 ` Bjorn Helgaas @ 2007-11-30 22:49 ` Jiri Slaby 2007-11-30 22:58 ` Bjorn Helgaas 0 siblings, 1 reply; 22+ messages in thread From: Jiri Slaby @ 2007-11-30 22:49 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus On 11/30/2007 10:08 PM, Bjorn Helgaas wrote: > On Thursday 29 November 2007 05:42:07 pm Andrew Morton wrote: >> On Thu, 29 Nov 2007 16:40:37 -0700 >>> 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 > =================================================================== > --- 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 > @@ -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); > No, it breaks suspend. regards, -- Jiri Slaby (jirislaby@gmail.com) Faculty of Informatics, Masaryk University ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-30 22:49 ` Jiri Slaby @ 2007-11-30 22:58 ` Bjorn Helgaas 2007-12-01 8:12 ` Jiri Slaby 0 siblings, 1 reply; 22+ messages in thread From: Bjorn Helgaas @ 2007-11-30 22:58 UTC (permalink / raw) To: Jiri Slaby Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus On Friday 30 November 2007 03:49:55 pm Jiri Slaby wrote: > On 11/30/2007 10:08 PM, Bjorn Helgaas wrote: > > On Thursday 29 November 2007 05:42:07 pm Andrew Morton wrote: > >> On Thu, 29 Nov 2007 16:40:37 -0700 > >>> 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 > > =================================================================== > > --- 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 > > @@ -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); > > > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-30 22:58 ` Bjorn Helgaas @ 2007-12-01 8:12 ` Jiri Slaby 2007-12-01 12:00 ` Jiri Slaby 0 siblings, 1 reply; 22+ messages in thread From: Jiri Slaby @ 2007-12-01 8:12 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus On 11/30/2007 11:58 PM, Bjorn Helgaas wrote: > On Friday 30 November 2007 03:49:55 pm Jiri Slaby wrote: >> On 11/30/2007 10:08 PM, Bjorn Helgaas wrote: >>> On Thursday 29 November 2007 05:42:07 pm Andrew Morton wrote: >>>> On Thu, 29 Nov 2007 16:40:37 -0700 >>>>> 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 >>> =================================================================== >>> --- 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 >>> @@ -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); >>> >> 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. 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, -- Jiri Slaby (jirislaby@gmail.com) Faculty of Informatics, Masaryk University ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-12-01 8:12 ` Jiri Slaby @ 2007-12-01 12:00 ` Jiri Slaby 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas 0 siblings, 1 reply; 22+ messages in thread From: Jiri Slaby @ 2007-12-01 12:00 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus On 12/01/2007 09:12 AM, Jiri Slaby wrote: > On 11/30/2007 11:58 PM, Bjorn Helgaas wrote: >> On Friday 30 November 2007 03:49:55 pm Jiri Slaby wrote: >>> On 11/30/2007 10:08 PM, Bjorn Helgaas wrote: >>>> On Thursday 29 November 2007 05:42:07 pm Andrew Morton wrote: >>>>> On Thu, 29 Nov 2007 16:40:37 -0700 >>>>>> 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 >>>> =================================================================== >>>> --- 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 >>>> @@ -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); >>>> >>> 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. > > 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: diff --git a/drivers/pnp/driver.c b/drivers/pnp/driver.c index f5b64ee..b0fc3ee 100644 --- a/drivers/pnp/driver.c +++ b/drivers/pnp/driver.c @@ -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; ^ permalink raw reply related [flat|nested] 22+ messages in thread
* RFC: PNP: do not stop/start devices in suspend/resume path 2007-12-01 12:00 ` Jiri Slaby @ 2007-12-05 18:24 ` Bjorn Helgaas 2007-12-05 18:50 ` Matthew Wilcox ` (4 more replies) 0 siblings, 5 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-12-05 18:24 UTC (permalink / raw) To: Jiri Slaby Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown 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: > 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.] Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com> 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 @@ -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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: RFC: PNP: do not stop/start devices in suspend/resume path 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas @ 2007-12-05 18:50 ` Matthew Wilcox 2007-12-06 1:07 ` Rafael J. Wysocki ` (3 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Matthew Wilcox @ 2007-12-05 18:50 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, Andrew Morton, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown On Wed, Dec 05, 2007 at 11:24:18AM -0700, Bjorn Helgaas wrote: > 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. -- 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." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: RFC: PNP: do not stop/start devices in suspend/resume path 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas 2007-12-05 18:50 ` Matthew Wilcox @ 2007-12-06 1:07 ` Rafael J. Wysocki 2007-12-06 23:25 ` Bjorn Helgaas ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Rafael J. Wysocki @ 2007-12-06 1:07 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown On Wednesday, 5 of December 2007, Bjorn Helgaas wrote: > 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: > > 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.] 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. > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com> > > 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 > @@ -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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > -- "Premature optimization is the root of all evil." - Donald Knuth ^ permalink raw reply [flat|nested] 22+ messages in thread
* PNP: do not stop/start devices in suspend/resume path 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas 2007-12-05 18:50 ` Matthew Wilcox 2007-12-06 1:07 ` Rafael J. Wysocki @ 2007-12-06 23:25 ` Bjorn Helgaas 2007-12-12 8:16 ` Andrew Morton 2007-12-13 8:26 ` Pierre Ossman 2007-12-07 7:13 ` RFC: " Shaohua Li 2007-12-24 1:43 ` do not stop/start devices in suspend/resume path: the SCSI case Stephane Ascoet 4 siblings, 2 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-12-06 23:25 UTC (permalink / raw) To: Jiri Slaby Cc: Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown, Kristen Carlson Accardi, Rafael J. Wysocki On Wednesday 05 December 2007 11:24:18 am Bjorn Helgaas wrote: > 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: > > 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 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). - 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 <bjorn.helgaas@hp.com> 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 @@ -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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: PNP: do not stop/start devices in suspend/resume path 2007-12-06 23:25 ` Bjorn Helgaas @ 2007-12-12 8:16 ` Andrew Morton 2007-12-12 16:29 ` Bjorn Helgaas 2007-12-13 8:26 ` Pierre Ossman 1 sibling, 1 reply; 22+ messages in thread From: Andrew Morton @ 2007-12-12 8:16 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown, Kristen Carlson Accardi, Rafael J. Wysocki On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: PNP: do not stop/start devices in suspend/resume path 2007-12-12 8:16 ` Andrew Morton @ 2007-12-12 16:29 ` Bjorn Helgaas 0 siblings, 0 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-12-12 16:29 UTC (permalink / raw) To: Andrew Morton Cc: Jiri Slaby, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown, Kristen Carlson Accardi, Rafael J. Wysocki On Wednesday 12 December 2007 01:16:10 am Andrew Morton wrote: > On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> 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. > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: PNP: do not stop/start devices in suspend/resume path 2007-12-06 23:25 ` Bjorn Helgaas 2007-12-12 8:16 ` Andrew Morton @ 2007-12-13 8:26 ` Pierre Ossman 1 sibling, 0 replies; 22+ messages in thread From: Pierre Ossman @ 2007-12-13 8:26 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown, Kristen Carlson Accardi, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 1402 bytes --] On Thu, 6 Dec 2007 16:25:57 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> 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). > > - 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 <bjorn.helgaas@hp.com> > Tested-by: Pierre Ossman <drzeus@drzeus.cx> No noticeable issues with suspend or hibernate using this patch. Rgds Pierre [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: RFC: PNP: do not stop/start devices in suspend/resume path 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas ` (2 preceding siblings ...) 2007-12-06 23:25 ` Bjorn Helgaas @ 2007-12-07 7:13 ` Shaohua Li 2007-12-10 23:26 ` Bjorn Helgaas 2007-12-24 1:43 ` do not stop/start devices in suspend/resume path: the SCSI case Stephane Ascoet 4 siblings, 1 reply; 22+ messages in thread From: Shaohua Li @ 2007-12-07 7:13 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Len Brown On Thu, 2007-12-06 at 02:24 +0800, Bjorn Helgaas wrote: > 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: > > 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.] > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com> > > 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 > @@ -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; > - } > - I'd suggest keep pnp_start_dev here to prevent BIOS not or assign different resources after a resume. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: RFC: PNP: do not stop/start devices in suspend/resume path 2007-12-07 7:13 ` RFC: " Shaohua Li @ 2007-12-10 23:26 ` Bjorn Helgaas 0 siblings, 0 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-12-10 23:26 UTC (permalink / raw) To: Shaohua Li Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Len Brown On Friday 07 December 2007 12:13:35 am Shaohua Li wrote: > On Thu, 2007-12-06 at 02:24 +0800, Bjorn Helgaas wrote: > > 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 > > @@ -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; > > - } > > - > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: do not stop/start devices in suspend/resume path: the SCSI case 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas ` (3 preceding siblings ...) 2007-12-07 7:13 ` RFC: " Shaohua Li @ 2007-12-24 1:43 ` Stephane Ascoet 2007-12-24 3:21 ` Bjorn Helgaas 4 siblings, 1 reply; 22+ messages in thread From: Stephane Ascoet @ 2007-12-24 1:43 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown Bjorn Helgaas a écrit : > 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)? -- Bien cordialement, Stephane Ascoet, conseils pour que la technologie soit un progres respectueux de la vie. Album photo:<http://www.flickr.com/photos/stephaneascoet/> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: do not stop/start devices in suspend/resume path: the SCSI case 2007-12-24 1:43 ` do not stop/start devices in suspend/resume path: the SCSI case Stephane Ascoet @ 2007-12-24 3:21 ` Bjorn Helgaas 0 siblings, 0 replies; 22+ messages in thread From: Bjorn Helgaas @ 2007-12-24 3:21 UTC (permalink / raw) To: Stephane Ascoet Cc: Jiri Slaby, Andrew Morton, matthew, Linux Kernel Mailing List, linux-pm, drzeus, linux-acpi, Adam Belay, Matthieu Castet, Li Shaohua, Len Brown On Sunday 23 December 2007 6:43:49 pm Stephane Ascoet wrote: > Bjorn Helgaas a écrit : > > 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: WARNING: at kernel/resource.c:189 __release_resource 2007-11-29 23:40 ` Bjorn Helgaas 2007-11-30 0:42 ` Andrew Morton @ 2007-12-01 21:01 ` Pierre Ossman 1 sibling, 0 replies; 22+ messages in thread From: Pierre Ossman @ 2007-12-01 21:01 UTC (permalink / raw) To: Bjorn Helgaas Cc: Andrew Morton, Jiri Slaby, Matthew Wilcox, Linux kernel mailing list, Greg KH, linux-usb-devel, linux-pm [-- Attachment #1: Type: text/plain, Size: 598 bytes --] On Thu, 29 Nov 2007 16:40:37 -0700 Bjorn Helgaas <bjorn.helgaas@hp.com> 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2007-12-24 3:23 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-11-22 21:41 WARNING: at kernel/resource.c:189 __release_resource Jiri Slaby 2007-11-27 6:05 ` Andrew Morton 2007-11-27 12:38 ` Matthew Wilcox 2007-11-29 23:40 ` Bjorn Helgaas 2007-11-30 0:42 ` Andrew Morton 2007-11-30 21:08 ` Bjorn Helgaas 2007-11-30 22:49 ` Jiri Slaby 2007-11-30 22:58 ` Bjorn Helgaas 2007-12-01 8:12 ` Jiri Slaby 2007-12-01 12:00 ` Jiri Slaby 2007-12-05 18:24 ` RFC: PNP: do not stop/start devices in suspend/resume path Bjorn Helgaas 2007-12-05 18:50 ` Matthew Wilcox 2007-12-06 1:07 ` Rafael J. Wysocki 2007-12-06 23:25 ` Bjorn Helgaas 2007-12-12 8:16 ` Andrew Morton 2007-12-12 16:29 ` Bjorn Helgaas 2007-12-13 8:26 ` Pierre Ossman 2007-12-07 7:13 ` RFC: " Shaohua Li 2007-12-10 23:26 ` Bjorn Helgaas 2007-12-24 1:43 ` do not stop/start devices in suspend/resume path: the SCSI case Stephane Ascoet 2007-12-24 3:21 ` Bjorn Helgaas 2007-12-01 21:01 ` WARNING: at kernel/resource.c:189 __release_resource Pierre Ossman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).