From: Rafael J. Wysocki
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 1:14 pm 2008

On Wednesday, 20 of February 2008, Jesse Barnes wrote:
> On Wednesday, February 20, 2008 11:18 am Jesse Barnes wrote:
> > On Wednesday, February 20, 2008 11:10 am Jeff Chua wrote:
> > > On Feb 21, 2008 2:53 AM, Jesse Barnes <jesse.barnes@intel.com> wrote:
> > > > > So, next I'll try "shutdown" to see if it work. I was using
> > > > > "platform".
> > > >
> > > > Ok, that would be good to try.
> > >
> > > "shutdown" does power down properly. But still green on resume.
> >
> > Ok, so Linus' theory about something later in the resume path trying to
> > touch video is looking good.
> >
> > Rafael, is there anyway to prevent the device shutdown in the hibernate
> > path?
> 
> Given the way the PM core works, do we need to set a flag like this?  I really 
> hope there's a better way of doing this...

I think we should export the target sleep state somehow.

> diff --git a/drivers/char/drm/i915_drv.c b/drivers/char/drm/i915_drv.c
> index 4048f39..a2d6242 100644
> --- a/drivers/char/drm/i915_drv.c
> +++ b/drivers/char/drm/i915_drv.c
> @@ -238,6 +238,13 @@ static void i915_restore_vga(struct drm_device *dev)
>  
>  }
>  
> +/*
> + * If we're doing a suspend to disk, we don't want to power off the device.
> + * Unfortunately, the PM core doesn't tell us if we're headed for a regular
> + * S3 state or that it's about to shut down the machine, so we use this flag.
> + */
> +static int i915_hibernate;
> +
>  static int i915_suspend(struct drm_device *dev, pm_message_t state)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -252,6 +259,9 @@ static int i915_suspend(struct drm_device *dev, 
> pm_message_t state)
>  	if (state.event == PM_EVENT_PRETHAW)
>  		return 0;
>  
> +	if (state.event == PM_EVENT_FREEZE)
> +		i915_hibernate = 1;
> +
>  	pci_save_state(dev->pdev);
>  	pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
>  
> @@ -366,7 +376,7 @@ static int i915_suspend(struct drm_device *dev, 
> pm_message_t state)
>  
>  	i915_save_vga(dev);
>  
> -	if (state.event == PM_EVENT_SUSPEND) {
> +	if (!i915_hibernate) {
>  		/* Shut down the device */
>  		pci_disable_device(dev->pdev);
>  		pci_set_power_state(dev->pdev, PCI_D3hot);
> @@ -385,6 +395,8 @@ static int i915_resume(struct drm_device *dev)
>  	if (pci_enable_device(dev->pdev))
>  		return -1;
>  
> +	i915_hibernate = 0;
> +
>  	pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
>  
>  	/* Pipe & plane A info */

Then, the .resume() called after the image creation will clear the flag and I
don't think it's safe to allow it to survive i915_resume() ...

Thanks,
Rafael
--


From: Linus Torvalds
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 1:29 pm 2008

On Wed, 20 Feb 2008, Rafael J. Wysocki wrote:
> 
> I think we should export the target sleep state somehow.

Yeah. By *not* using "->suspend()" for freezing or hibernate.

Please, Rafael - just make the f*cking suspend-to-disk use other routines 
already. 99% of all hardware needs to do exactly *nothing* on 
suspend-to-disk, and the ones that really do need things tend to need to 
not do a whole lot.

For example, the "freeze" action for USB (which is one of the hardest 
things to suspend) should literally be something like just setting the 
controller STOP bit, and waiting for it to have stopped. The "unfreeze" 
should be to just clear the stop bit, while the "restart" should be just a 
controller reset to use the current memory image.

NONE OF THIS HAS ABSOLUTELY ANYTHING TO DO WITH SUSPEND.

It never did. I've told people so for years. Maybe actually seeing the 
problems will make people realize.

So please, we shouldn't call "->suspend[_late]" or "->resume[_early]" at 
all. Not with PMSG_FREEZE, not with PMSG_*anything*.

Can we please get this fixed some day? 

		Linus
--


From: Jesse Barnes
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 1:41 pm 2008

On Wednesday, February 20, 2008 12:29 pm Linus Torvalds wrote:
> On Wed, 20 Feb 2008, Rafael J. Wysocki wrote:
> > I think we should export the target sleep state somehow.
>
> Yeah. By *not* using "->suspend()" for freezing or hibernate.
>
> Please, Rafael - just make the f*cking suspend-to-disk use other routines
> already. 99% of all hardware needs to do exactly *nothing* on
> suspend-to-disk, and the ones that really do need things tend to need to
> not do a whole lot.

In talking with Rafael on IRC about this, I think we're agreed that we need 
separate entry points.  Even with a kexec based hibernate, we'll probably 
want ->hibernate callbacks so we don't end up shutting down the device.

The current callback system looks like this (according to Rafael and the last 
time I looked):
  ->suspend(PMSG_FREEZE)
  ->resume()
  ->suspend(PMSG_SUSPEND)
  *enter S3 or power off*
  ->resume()
The fact that we get suspend/resume called once before suspend again in the 
hibernate case is somewhat obnoxious, but it's even worse that we don't know 
what we're about to enter after ->suspend(PMSG_SUSPEND).  So in the short 
term it would be nice to at least get the target state exported.

And in the long term we could have:
  ->suspend()
  *enter S3*
  ->resume()
or:
  ->hibernate()
  *kexec to another kernel to save image*
  *power off*
  ->return_from_hibernate() (or somesuch)

Jesse
--


From: Linus Torvalds
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 2:13 pm 2008

On Wed, 20 Feb 2008, Jesse Barnes wrote:
> 
> The current callback system looks like this (according to Rafael and the last 
> time I looked):
>   ->suspend(PMSG_FREEZE)
>   ->resume()
>   ->suspend(PMSG_SUSPEND)
>   *enter S3 or power off*
>   ->resume()

Yes, it's very messy.

It's messy for a few different reasons:

 - the one you hit: a driver actually has a really hard time telling what 
   PMSG_SUSPEND really means.

 - more importantly, we generally don't want to "suspend/resume" the 
   hardware at all around a power-off, because we're going to resume with 
   the state at the time of the PMSG_FREEZE, which means that the hardware 
   has actually *changed* and been used in between!

that second case is very fundamental for things like USB devices, which in 
theory you can hold alive over a real suspend event (ie a STR event), but 
which absolutely MUST NOT be resumed over a suspend-to-disk event, because 
all the low-level request state is bogus!

So the "->resume" really isn't a resume at all. It's much closer to a 
"->reset".

Of course, the "solution" to this all right now is that we have to reset 
everything even if it *is* a suspend event, so it basically means that STR 
ends up using the much weaker model that snapshot-to-disk uses.

The fundamental problem being that the two really have nothing 
what-so-ever to do with each other. They aren't even similar. Never were.

> And in the long term we could have:
>   ->suspend()
>   *enter S3*
>   ->resume()

Yes, apart from all the complexities (suspend_late/resume_early). So in 
reality it's more than that, but the suspend/resume things are clearly 
nesting, and they have the potential to actually keep state around 
(because we *know* this machine is not going to mess with the devices in 
between).

IOW, here we actually can have as an option "assume the device is there 
when you return".

> or:
>   ->hibernate()
>   *kexec to another kernel to save image*
>   *power off*
>   ->return_from_hibernate() (or somesuch)

Enough people don't trust kexec that I suspect the right thing simply is

	->freeze()		// stop dma, synchronize device state
	*snapshot*
	->unfreeze();		// resume dma
	*save image*
	[ optionally ->poweroff() ]	// do we really care? I'd say no
	*power off*
	->restore()		// reset device to the frozen one

which may have four entry-points that can be illogically mapped to the 
suspend/resume ones like we do now, but they really have nothing to do 
with suspending/resuming.

And notice how while "freeze/restore" kind of pairs like a 
"suspend/resume", it really shouldn't be expected to realistically restore 
the same state at all. The "restore" part is generally much better seen as 
a "reset hardware" than a "resume" thing. Because we literally cannot 
trust *anything* about the state since we froze it - we might have booted 
a different OS in between etc. Very different from suspend/resume.

			Linus
--

From: Rafael J. Wysocki
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 3:36 pm 2008

On Wednesday, 20 of February 2008, Jesse Barnes wrote:
> On Wednesday, February 20, 2008 1:13 pm Linus Torvalds wrote:
> > On Wed, 20 Feb 2008, Jesse Barnes wrote:
> > > The current callback system looks like this (according to Rafael and the
> > > last time I looked):
> > >   ->suspend(PMSG_FREEZE)
> > >   ->resume()
> > >   ->suspend(PMSG_SUSPEND)
> > >   *enter S3 or power off*
> > >   ->resume()
> >
> > Yes, it's very messy.
> >
> > It's messy for a few different reasons:
> >
> >  - the one you hit: a driver actually has a really hard time telling what
> >    PMSG_SUSPEND really means.

In fact the driver can find out in which state to put the device into,
depending on the target ACPI state which is known.

> >  - more importantly, we generally don't want to "suspend/resume" the
> >    hardware at all around a power-off, because we're going to resume with
> >    the state at the time of the PMSG_FREEZE, which means that the hardware
> >    has actually *changed* and been used in between!
> 
> Exactly.
> 
> > So the "->resume" really isn't a resume at all. It's much closer to a
> > "->reset".
> 
> Yeah, in the hibernate case this is definitely true.

Agreed.

> > Of course, the "solution" to this all right now is that we have to reset
> > everything even if it *is* a suspend event, so it basically means that STR
> > ends up using the much weaker model that snapshot-to-disk uses.
> >
> > The fundamental problem being that the two really have nothing
> > what-so-ever to do with each other. They aren't even similar. Never were.
> >
> > > And in the long term we could have:
> > >   ->suspend()
> > >   *enter S3*
> > >   ->resume()
> >
> > Yes, apart from all the complexities (suspend_late/resume_early). So in
> > reality it's more than that, but the suspend/resume things are clearly
> > nesting, and they have the potential to actually keep state around
> > (because we *know* this machine is not going to mess with the devices in
> > between).
> 
> Really, in the simple s3 case we still need early/late stuff?

Yes, we do.  There are devices that need to be suspended with interrupts off.

> > IOW, here we actually can have as an option "assume the device is there
> > when you return".

That is, unless the user pulls out that pendrive while suspended, no?

> > > or:
> > >   ->hibernate()
> > >   *kexec to another kernel to save image*
> > >   *power off*
> > >   ->return_from_hibernate() (or somesuch)
> >
> > Enough people don't trust kexec that I suspect the right thing simply is
> >
> > 	->freeze()		// stop dma, synchronize device state
> > 	*snapshot*
> > 	->unfreeze();		// resume dma
> > 	*save image*
> > 	[ optionally ->poweroff() ]	// do we really care? I'd say no

We do, if there are devices that wake us up from S4 and don't wake us up from
S5, for example.  Plus this f*cking fan in my box that doesn't work after the
resume if we don't do ->poweroff() ...

> > 	*power off*
> > 	->restore()		// reset device to the frozen one
> >
> > which may have four entry-points that can be illogically mapped to the
> > suspend/resume ones like we do now, but they really have nothing to do
> > with suspending/resuming.

Apart from putting devices into the right low power states, that is.

> Well, it seems like we'll have to fix drivers in either case, and isn't a 
> kexec approach fundamentally more sound and simple, design-wise?  Rafael 
> pointed out some problems with properly setting wakeup states, but I think 
> that could be overcome...

Your honor, I would like to register a differing opinion ...
 
> > And notice how while "freeze/restore" kind of pairs like a
> > "suspend/resume", it really shouldn't be expected to realistically restore
> > the same state at all. The "restore" part is generally much better seen as
> > a "reset hardware" than a "resume" thing.

That's absolutely correct.

> > Because we literally cannot trust *anything* about the state since we froze
> > it - we might have booted a different OS in between etc. Very different from
> > suspend/resume. 
> 
> Yeah, definitely.  It has to be much more robust and deal with configuration 
> changes, etc. (within reason).

Agreed.

Thanks,
Rafael
--


From: Linus Torvalds
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 4:13 pm 2008

On Wed, 20 Feb 2008, Rafael J. Wysocki wrote:
> > >
> > > which may have four entry-points that can be illogically mapped to the
> > > suspend/resume ones like we do now, but they really have nothing to do
> > > with suspending/resuming.
> 
> Apart from putting devices into the right low power states, that is.

And by "right low power states" you mean "wrong low-power states", right?

The thing is, they really *are* the wrong states for 99% of all hardware.

If you really have a piece of hardware that you want to have the 
"->poweroff()" thing do the same as "->suspend()", then hey, just use the 
same function (or better yet, use two different functions with a call to a
shared part).

Because IT IS NOT TRUE that ->suspend() puts the devices in the "right 
power state". The power states are likely to be totally different for S3 
and for poweroff, and they are going to differ in different ways depending 
on the device type.

One example would be the one that started this version of the whole 
discussion (shock horror! We're on subject!) ie when you do a system 
shutdown, you generally do not even *want* to put individual devices into 
low-power states at all, because the actual "power off the system" thing 
will take care of it for you much better.

So to take just something as simple as VGA as an example: you really do 
not want to suspend that device, because you want to see the poweroff 
messages until the very end. 

So that final device ->poweroff function really has absolutely *nothing* 
in common with the device ->suspend[_late] functions, simply because 
almost any sane driver would decide to do different things.

Of course, we can continue to do the insane thing and just continue to use 
inappropriate and misleadign function callback names, and then encodign 
what the *real* action should be in the argument and/or in magic 
system-wide state parameters.

So in that sense, it's certainly totally the same thing whether we call it 
->shutdown or ->poweroff or ->eat_a_banana, since you could always just 
look at the argument and other clues, and decide that *this* time, for 
*this* kind of device, the "eat a banana" callback actually means that we 
should power it off, but wouldn't it be a lot more logical to just make it 
clear in the first place that they aren't called for the same reason at 
all?

I'd claim that it's much easier for everybody (and _especially_ for device 
driver writers) to have

	static int my_shutdown(struct pci_device *dev, int state)
	{
		.. do something ..
	}

	static int my_suspend(struct pci_device *dev, int state)
	{
		.. do something ..
	}

	...
	.shutdown = my_shutdown,
	.suspend = my_suspend,
	...	

than to have

	static int my_suspend(struct pci_device *dev, state)
	{
		.. common code ..
		if (state == XYZZY)
			..special code..
		else
			..other case code..
	}

	...
	.suspend = my_suspend
	...

even if the latter might be fewer lines. It doesn't really matter if it's 
fewer, does it, if the alternate version is more obvious about what it 
does?

The other issue is that I've long wanted to make sure that when people fix 
suspend-to-ram, they don't screw up suspend-to-disk by mistake and vice 
versa. When a driver writer makes changes, he shouldn't have the kind of 
illogical "oops, unintended consequences" issues in general. It should be 
pretty damn obvious when he changes suspend code vs when he changes 
snapshot/restore code.

We've somewhat untangled that on the "core kernel" layer, but we've left 
the driver confusion alone.

		Linus
--


From: Rafael J. Wysocki
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 4:35 pm 2008

On Thursday, 21 of February 2008, Linus Torvalds wrote:
> 
> On Wed, 20 Feb 2008, Rafael J. Wysocki wrote:
> > > >
> > > > which may have four entry-points that can be illogically mapped to the
> > > > suspend/resume ones like we do now, but they really have nothing to do
> > > > with suspending/resuming.
> > 
> > Apart from putting devices into the right low power states, that is.
> 
> And by "right low power states" you mean "wrong low-power states", right?

No, I don't.

> The thing is, they really *are* the wrong states for 99% of all hardware.
> 
> If you really have a piece of hardware that you want to have the 
> "->poweroff()" thing do the same as "->suspend()", then hey, just use the 
> same function (or better yet, use two different functions with a call to a
> shared part).
> 
> Because IT IS NOT TRUE that ->suspend() puts the devices in the "right 
> power state". The power states are likely to be totally different for S3 
> and for poweroff, and they are going to differ in different ways depending 
> on the device type.

In fact we have acpi_pci_choose_state() that tells the driver which power
state to put the device into in ->suspend().  If that is used, the device ends
up in the state expected by to BIOS for S4.

> One example would be the one that started this version of the whole 
> discussion (shock horror! We're on subject!) ie when you do a system 
> shutdown, you generally do not even *want* to put individual devices into 
> low-power states at all, because the actual "power off the system" thing 
> will take care of it for you much better.

No.  Again, if there are devices that wake us up from S4, but not from S5,
they need to be handled differently in the *enter S4* case (hibernation) and
in the *enter S5* case (powering off the system).
 
> So to take just something as simple as VGA as an example: you really do 
> not want to suspend that device, because you want to see the poweroff 
> messages until the very end. 
> 
> So that final device ->poweroff function really has absolutely *nothing* 
> in common with the device ->suspend[_late] functions, simply because 
> almost any sane driver would decide to do different things.

Yes, it would.  Still, the common thing is, it (ie. ->poweroff) _may_ want to
put the device into a low power state different from D3.

> Of course, we can continue to do the insane thing and just continue to use 
> inappropriate and misleadign function callback names, and then encodign 
> what the *real* action should be in the argument and/or in magic 
> system-wide state parameters.

To clarify, I agree that we should use different callbacks for hibernation.
I'm only saying that _in_ _general_ we may need the ->poweroff callback.

> So in that sense, it's certainly totally the same thing whether we call it 
> ->shutdown or ->poweroff or ->eat_a_banana, since you could always just 
> look at the argument and other clues, and decide that *this* time, for 
> *this* kind of device, the "eat a banana" callback actually means that we 
> should power it off, but wouldn't it be a lot more logical to just make it 
> clear in the first place that they aren't called for the same reason at 
> all?
> 
> I'd claim that it's much easier for everybody (and _especially_ for device 
> driver writers) to have
> 
> 	static int my_shutdown(struct pci_device *dev, int state)
> 	{
> 		.. do something ..
> 	}
> 
> 	static int my_suspend(struct pci_device *dev, int state)
> 	{
> 		.. do something ..
> 	}
> 
> 	...
> 	.shutdown = my_shutdown,
> 	.suspend = my_suspend,
> 	...	
> 
> than to have
> 
> 	static int my_suspend(struct pci_device *dev, state)
> 	{
> 		.. common code ..
> 		if (state == XYZZY)
> 			..special code..
> 		else
> 			..other case code..
> 	}
> 
> 	...
> 	.suspend = my_suspend
> 	...
> 
> even if the latter might be fewer lines. It doesn't really matter if it's 
> fewer, does it, if the alternate version is more obvious about what it 
> does?
> 
> The other issue is that I've long wanted to make sure that when people fix 
> suspend-to-ram, they don't screw up suspend-to-disk by mistake and vice 
> versa. When a driver writer makes changes, he shouldn't have the kind of 
> illogical "oops, unintended consequences" issues in general. It should be 
> pretty damn obvious when he changes suspend code vs when he changes 
> snapshot/restore code.
> 
> We've somewhat untangled that on the "core kernel" layer, but we've left 
> the driver confusion alone.

Well, I agree with that.

As I said before, that's mainly because I've been busy with other stuff
recently.  Now, with the Alex's help, I'm hoping to take care of it soon.

Thanks,
Rafael
--


From: Linus Torvalds
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 5:00 pm 2008

On Thu, 21 Feb 2008, Rafael J. Wysocki wrote:
> 
> In fact we have acpi_pci_choose_state() that tells the driver which power
> state to put the device into in ->suspend().  If that is used, the device ends
> up in the state expected by to BIOS for S4.

First off, nobody should *ever* use that directly anyway.

Secondly, the one that people should use ("pci_choose_state()") doesn't 
actually do what you claim it does. It does all kinds of wrong things, and 
doesn't even take the target state into account at all. So look again.

> No.  Again, if there are devices that wake us up from S4, but not from S5,
> they need to be handled differently in the *enter S4* case (hibernation) and
> in the *enter S5* case (powering off the system).

And again, what does this have to do with (the example I used) the 
graphics hardware? Answer: nothing. The example I gave you we simply DO 
THE WRONG THING FOR.

Same thing for things like USB devices - where pci_choose_state() doesn't 
work to begin with. Why do we call "suspend()" on such a thing when we 
don't want to suspend it? We shouldn't. We should call "freeze/unfreeze" 
(which are no-ops) and then finally perhaps "poweroff", and that final 
stage might want to spin things down or similar.

But *none* of it has anything to do with suspend, and none of it has 
anything to do with pci_choose_state() (much less acpi_pci_choose_state)

The fact is, we should let the driver decide, and we should make it clear 
to the driver writer what he is deciding about - rather than basically lie 
and say "suspend the device and put it into D3" even when that's the last 
thing it should ever do.

			Linus
--


From: Rafael J. Wysocki
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 5:13 pm 2008

On Thursday, 21 of February 2008, Linus Torvalds wrote:
> 
> On Thu, 21 Feb 2008, Rafael J. Wysocki wrote:
> > 
> > In fact we have acpi_pci_choose_state() that tells the driver which power
> > state to put the device into in ->suspend().  If that is used, the device ends
> > up in the state expected by to BIOS for S4.
> 
> First off, nobody should *ever* use that directly anyway.

Yes, sorry.

> Secondly, the one that people should use ("pci_choose_state()") doesn't 
> actually do what you claim it does. It does all kinds of wrong things, and 
> doesn't even take the target state into account at all. So look again.

Well, if platform_pci_choose_state() is defined, pci_choose_state() returns
its result and on ACPI systems that points to acpi_pci_choose_state(), so in
fact it does what I said (apart from the error path). 

> > No.  Again, if there are devices that wake us up from S4, but not from S5,
> > they need to be handled differently in the *enter S4* case (hibernation) and
> > in the *enter S5* case (powering off the system).
> 
> And again, what does this have to do with (the example I used) the 
> graphics hardware? Answer: nothing. The example I gave you we simply DO 
> THE WRONG THING FOR.
> 
> Same thing for things like USB devices - where pci_choose_state() doesn't 
> work to begin with. Why do we call "suspend()" on such a thing when we 
> don't want to suspend it? We shouldn't. We should call "freeze/unfreeze" 
> (which are no-ops) and then finally perhaps "poweroff", and that final 
> stage might want to spin things down or similar.

I'm already convinced, really. :-)

Thanks,
Rafael
--


From: Linus Torvalds
Subject: Re: 2.6.25-rc2 System no longer powers off after suspend-to-disk. Screen becomes green.
Date: Feb 20, 5:25 pm 2008

On Thu, 21 Feb 2008, Rafael J. Wysocki wrote:
> 
> > Secondly, the one that people should use ("pci_choose_state()") doesn't 
> > actually do what you claim it does. It does all kinds of wrong things, and 
> > doesn't even take the target state into account at all. So look again.
> 
> Well, if platform_pci_choose_state() is defined, pci_choose_state() returns
> its result and on ACPI systems that points to acpi_pci_choose_state(), so in
> fact it does what I said (apart from the error path). 

Did you check closer?

I repeat: acpi_pci_choose_state() (when called from pci_choose_state()) 
doesn't even look at the target 'state'. It just blindly assumes that you 
want the deepest sleep-state you can have.

Which happens to be correct for normal suspend, but means that if you want 
to test other states (through '/sys/devices/.../power'), that sounds 
broken.

I didn't check any closer, but go check it yourself. The short and sweet: 
acpi_pci_choose_state() totally ignores its 'state' argument. Do you 
really think that's correct? But yes, "pci_choose_state()' effectively 
does that too, apart from PM_EVENT_ON, which is never used.

(But the whole and only point of pci_choose_state() was to do the 
PM_EVENT_FREEZE thing differently, which it doesn't do, so I think the 
real issue here is that the interface is really rather mis-designed)

I suspect most people who ever really looked and worked on this code had a 
specific device in mind, and I'm sure that all of the code individually 
always ends up making sense from the standpoint of some specific device 
driver. It's just that it never seems to make sense from a bigger issues 
standpoint, and often seems senseless from the standpoint of other devices 
of other types.

			Linus
--