Re: WARNING: at kernel/resource.c:189 __release_resource

Re: WARNING: at kernel/resource.c:189 __release_resource

[Andrew Morton]

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.

Re: WARNING: at kernel/resource.c:189 __release_resource

[Matthew Wilcox]

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."

Re: WARNING: at kernel/resource.c:189 __release_resource

[Bjorn Helgaas]

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

Re: WARNING: at kernel/resource.c:189 __release_resource

[Andrew Morton]

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.

Re: WARNING: at kernel/resource.c:189 __release_resource

[Bjorn Helgaas]

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);
 

Re: WARNING: at kernel/resource.c:189 __release_resource

[Jiri Slaby]

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

Re: WARNING: at kernel/resource.c:189 __release_resource

[Bjorn Helgaas]

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

Re: WARNING: at kernel/resource.c:189 __release_resource

[Jiri Slaby]

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

Re: WARNING: at kernel/resource.c:189 __release_resource

[Jiri Slaby]

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;

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

[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:
> 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);
 

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

[Matthew Wilcox]

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."

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

[Rafael J. Wysocki]

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

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

[Bjorn Helgaas]

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);
 

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

[Andrew Morton]

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?

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

[Bjorn Helgaas]

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

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

[Pierre Ossman]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Shaohua Li]

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.

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

[Bjorn Helgaas]

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

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

[Stephane Ascoet]

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/>

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

[Bjorn Helgaas]

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

Re: WARNING: at kernel/resource.c:189 __release_resource

[Pierre Ossman]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

WARNING: at kernel/resource.c:189 __release_resource

[Jiri Slaby]

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