[RFC 3/3] Runtime PM support for named power states

[RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2225 bytes --]

This patch adds support for named power states to the USB device drivers.  
It's fairly simple and well-contained because USB devices have only two 
states: on and suspended.  Even so, the confusion over which field in 
dev->power.power_state should represent the true state remains.

Alan Stern

P.S.:  I forgot the mention this in the original cover message.  This 
entire series of patches is meant to go on top of 2.6.14-rc2-git6 plus

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-all-2.6.14-rc2-git6.patch



Index: usb-2.6/drivers/usb/core/usb.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/usb.c
+++ usb-2.6/drivers/usb/core/usb.c
@@ -58,6 +58,8 @@ static int nousb;	/* Disable USB when bu
 
 static DECLARE_RWSEM(usb_all_devices_rwsem);
 
+const char *(usb_states[3]) = { pm_name_ON, pm_name_SUSPEND, NULL };
+
 
 static int generic_probe (struct device *dev)
 {
@@ -739,6 +741,7 @@ usb_alloc_dev(struct usb_device *parent,
 	dev->dev.driver_data = &usb_generic_driver_data;
 	dev->dev.driver = &usb_generic_driver;
 	dev->dev.release = usb_release_dev;
+	dev->dev.power.states = usb_states;
 	dev->state = USB_STATE_ATTACHED;
 
 	INIT_LIST_HEAD(&dev->ep0.urb_list);
Index: usb-2.6/drivers/usb/core/usb.h
===================================================================
--- usb-2.6.orig/drivers/usb/core/usb.h
+++ usb-2.6/drivers/usb/core/usb.h
@@ -29,6 +29,7 @@ extern void usb_major_cleanup(void);
 extern int usb_host_init(void);
 extern void usb_host_cleanup(void);
 
+extern const char *(usb_states[]);
 extern int usb_suspend_device(struct usb_device *dev);
 extern int usb_resume_device(struct usb_device *dev);
 
Index: usb-2.6/drivers/usb/core/message.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/message.c
+++ usb-2.6/drivers/usb/core/message.c
@@ -1393,6 +1393,7 @@ free_interfaces:
 			intf->dev.dma_mask = dev->dev.dma_mask;
 			intf->dev.release = release_interface;
 			device_initialize (&intf->dev);
+			intf->dev.power.states = usb_states;
 			mark_quiesced(intf);
 			sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
 				 dev->bus->busnum, dev->devpath,


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

Re: [RFC 3/3] Runtime PM support for named power states

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

> This patch adds support for named power states to the USB device drivers.  
> It's fairly simple and well-contained because USB devices have only two 
> states: on and suspended.  Even so, the confusion over which field in 
> dev->power.power_state should represent the true state remains.

Hmm, what confusion is that?  You may have mentioned it elsewhere.

There are two different notions of "device" inside USB.  One is that
of a "usb_interface" ... something that doesn't actually have any
kind of power state, it's only got "driver has quiesced".

The other is that of "usb_device", which can have real power states
when CONFIG_USB_SUSPEND is defined.  But otherwise, it's got the
same "quiesced or not" dimension as a usb_interface.

- Dave


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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2081 bytes --]

On Mon, 3 Oct 2005, David Brownell wrote:

> > This patch adds support for named power states to the USB device drivers.  
> > It's fairly simple and well-contained because USB devices have only two 
> > states: on and suspended.  Even so, the confusion over which field in 
> > dev->power.power_state should represent the true state remains.
> 
> Hmm, what confusion is that?  You may have mentioned it elsewhere.

The confusion is over whether the current power state is indicated by 
dev->power.power_state.state or dev->power.power_state.name.  (Never mind 
the possibility that the actual power level might be indicated by 
something else entirely, like pcidev->current_state or usbdev->state!)

There are many places in the PM core and in usbcore where decisions about 
whether or not to call suspend or resume methods are based on the value of 
dev->power.power_state.state.  For example, if power_state.state is 0, the 
runtime_resume function in runtime.c won't do anything.

But with named states, it makes more sense to look at power_state.name
instead.  The name field does a much better job of representing the state
the device is currently in, whereas the event field really represents the
reason for entering that state (i.e., the event that triggered the state
transition).

The problem is made even worse by the way runtime_resume() and 
runtime_suspend() assign their own values to dev->power.power_state, 
overwriting whatever the driver may have put there.  I should have taken 
out those assignments; they really don't do any good.

> There are two different notions of "device" inside USB.  One is that
> of a "usb_interface" ... something that doesn't actually have any
> kind of power state, it's only got "driver has quiesced".
> 
> The other is that of "usb_device", which can have real power states
> when CONFIG_USB_SUSPEND is defined.  But otherwise, it's got the
> same "quiesced or not" dimension as a usb_interface.

That's not what I was talking about.  The distinction between "device" and 
"interface" doesn't matter to the PM core.

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]

> > > 	  Even so, the confusion over which field in 
> > > dev->power.power_state should represent the true state remains.
> > 
> > Hmm, what confusion is that?  You may have mentioned it elsewhere.
>
> The confusion is over whether the current power state is indicated by 
> dev->power.power_state.state or dev->power.power_state.name.  (Never mind 
> the possibility that the actual power level might be indicated by 
> something else entirely, like pcidev->current_state or usbdev->state!)

Oh, that confusion.  :)


> There are many places in the PM core and in usbcore where decisions about 
> whether or not to call suspend or resume methods are based on the value of 
> dev->power.power_state.state.  For example, if power_state.state is 0, the 
> runtime_resume function in runtime.c won't do anything.

And suspend.c::device_suspend(msg) only checks for "event" nonzero...


> But with named states, it makes more sense to look at power_state.name
> instead.  The name field does a much better job of representing the state
> the device is currently in, whereas the event field really represents the
> reason for entering that state (i.e., the event that triggered the state
> transition).

Let's forget about the names for a moment (which I certainly prefer
in sysfs, especially with driver-controlled values).

The "confusion" is that there are two notions of state:

 - One of them is for system sleep transitions, and in practice it's
   nothing more than "I successfully called suspend_device()".

 - The other notion is for runtime power management, and it needs
   a richer notion.  Part of that is that the states are defined
   by the driver (or bus).

My theory is that the PM core doesn't need any notion that's more
complex than "moved device from <this> device list to <that> one".
Suspend and resume just move devices between lists.

One nice consequence of that theory is that there's nothing to
prevent completly ripping out the other notions of power state.
Or -- equivalently, but less painfully! -- repurposing all that
infrastructure to the exclusive use of runtime PM.  Which I'd
argue only really needs one cookie identifying the device state.
Be it a string constant, or whatever.


> The problem is made even worse by the way runtime_resume() and 
> runtime_suspend() assign their own values to dev->power.power_state, 
> overwriting whatever the driver may have put there.  I should have taken 
> out those assignments; they really don't do any good.

Well, virtually no drivers set that value themselves.  I think
that suspend.c::suspend_device() should probably set the value,
at least for devices that don't change it themselves, ditto with
the corresponding resume path.

(IMO it's clearly wrong to clobber a power_state value that the
driver set ... it surely knows more about itself than the pm core.)

- Dave



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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5152 bytes --]

On Tue, 4 Oct 2005, David Brownell wrote:

> Let's forget about the names for a moment (which I certainly prefer
> in sysfs, especially with driver-controlled values).
> 
> The "confusion" is that there are two notions of state:
> 
>  - One of them is for system sleep transitions, and in practice it's
>    nothing more than "I successfully called suspend_device()".
> 
>  - The other notion is for runtime power management, and it needs
>    a richer notion.  Part of that is that the states are defined
>    by the driver (or bus).

In the early stages of the PM development, the developers thought a device
was either ON or SUSPENDed and imagined this would suffice for runtime 
PM.  So there was originally just one notion of state, which has since 
split into two.

> My theory is that the PM core doesn't need any notion that's more
> complex than "moved device from <this> device list to <that> one".
> Suspend and resume just move devices between lists.

Sounds reasonable.  There's one thing to consider, though.  System sleep
transitions are supposed to work even when devices are already in a
low-power state, which some drivers may not be able to handle.  That's the
main reason why dev->power.power_state exists today -- so that the PM core
can see if a device needs to be set back to full power before suspending
it.

IMO we could avoid the need for power_state by removing the guarantee
about never calling ->suspend when a device is already in a low-power
state.  The system sleep code could issue an optimistic ->suspend call,
and if that fails then do ->resume followed by ->suspend.

> One nice consequence of that theory is that there's nothing to
> prevent completly ripping out the other notions of power state.
> Or -- equivalently, but less painfully! -- repurposing all that
> infrastructure to the exclusive use of runtime PM.  Which I'd
> argue only really needs one cookie identifying the device state.
> Be it a string constant, or whatever.

That's why I wrote the patch in that form.  Since we need string constants 
for user interaction anyway, they may as well serve as the state 
indicator.

As part of the infrastructure changes, we could remove
dev->power.power_state, dev->power.prev_state, and maybe also
dev->power.saved_state (currently it is used only in a few platform
drivers for stuff that could easily be stored in a private data
structure).  Nothing would be needed but the name pointer, and that should
be under the bus/driver's control.

People have already pretty much agreed that ->resume needs to take a 
pm_message_t argument, and that will mean changing a lot of drivers.  We 
ought to settle on a decision about the rest of this too, so we won't have 
to go through and change them all yet again.

(BTW, a theoretical advantage of passing a pm_message_t to ->resume is
that a driver could use the _same_ subroutine as a combined suspend/resume
method.  :-))

> > The problem is made even worse by the way runtime_resume() and 
> > runtime_suspend() assign their own values to dev->power.power_state, 
> > overwriting whatever the driver may have put there.  I should have taken 
> > out those assignments; they really don't do any good.
> 
> Well, virtually no drivers set that value themselves.  I think
> that suspend.c::suspend_device() should probably set the value,
> at least for devices that don't change it themselves, ditto with
> the corresponding resume path.
> 
> (IMO it's clearly wrong to clobber a power_state value that the
> driver set ... it surely knows more about itself than the pm core.)

Drivers _should_ manage these values, especially if they will be used by 
sysfs.  On the other hand, if there are only two states then the PM core 
can assume that the first is ON and the second is SUSPEND, so it could 
handle the updates by itself.

> I suspect only driver code should ever care about device states.
> 
> The PM core should just tell drivers to become compatible with some
> new constraint (like ACPI S3, generally implying devices in PCI D2
> or D3; while S1 doesn't) ... and not worry about whether that involves
> a state change or not.
> 
> Maybe they're already _in_ that state for example.

The PM core shouldn't worry too much about redundant calls.  Drivers can
easily filter them out, and the core won't have enough information to
detect them in general.

As I see it, we can use the new .name field to distinguish between the 
different kinds of system state change calls.  The PM core could export 
strings like (for FREEZE):

	APM-standby, APM-suspend, snapshot, kexec;

or (for SUSPEND):

	RAM, S3, S4, S5, shutdown;

or (for RESUME):

	snapshot, RAM, kexec.

Something like this will provide drivers the means for telling which
target state to use, thus addressing one of your biggest complaints about
the current system.

Of course, this means that drivers will need code to map these 
higher-level names to the appropriate target state, although that code 
wouldn't need to be very sophisticated.  It would have to be smart enough 
to recognize that when .event is PM_EVENT_RUNTIME, the name indicates the 
requested power state directly.

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[Scott E. Preece]

[-- Attachment #1: Type: text/plain, Size: 3214 bytes --]



| From: "Alan Stern" <stern@rowland.harvard.edu>
| 
| IMO we could avoid the need for power_state by removing the guarantee
| about never calling ->suspend when a device is already in a low-power
| state.  The system sleep code could issue an optimistic ->suspend call,
| and if that fails then do ->resume followed by ->suspend.
---

That assumes it's "safe" to call suspend regardless of device power
state - that in the worst case, the device will just return failure.
That seems reasonable, but it's a constraint driver writers would want
to know about.

---
|
| People have already pretty much agreed that ->resume needs to take a 
| pm_message_t argument, and that will mean changing a lot of drivers.  We
| ought to settle on a decision about the rest of this too, so we won't
| have to go through and change them all yet again.
---

Yes, it seems reasonable to tell the device what state the system is
trying to get to, so it can determine what state it needs to be in to
support that.

---
| 
| (BTW, a theoretical advantage of passing a pm_message_t to ->resume is
| that a driver could use the _same_ subroutine as a combined
| suspend/resume
| method.  :-))
---

It would be nice to encourage code cohesion by NOT encouraging
multipurpose interfaces...

---
| 
| The PM core shouldn't worry too much about redundant calls.  Drivers can
| easily filter them out, and the core won't have enough information to
| detect them in general.
| 
| As I see it, we can use the new .name field to distinguish between the 
| different kinds of system state change calls.  The PM core could export 
| strings like (for FREEZE):
| 
| 	APM-standby, APM-suspend, snapshot, kexec;
| 
| or (for SUSPEND):
| 
| 	RAM, S3, S4, S5, shutdown;
| 
| or (for RESUME):
| 
| 	snapshot, RAM, kexec.
| 
| Something like this will provide drivers the means for telling which
| target state to use, thus addressing one of your biggest complaints
| about
| the current system.
| 
| Of course, this means that drivers will need code to map these 
| higher-level names to the appropriate target state, although that code 
| wouldn't need to be very sophisticated.  It would have to be smart
| enough 
| to recognize that when .event is PM_EVENT_RUNTIME, the name indicates
| the 
| requested power state directly.
---

It would be nicer if the system didn't need to know anything about the
internal state of the devices, but could just tell the device what
system state it wants to go to and let the device map that to whatever
internal state or sequence of sub-states the device needs. This would be
helped if the set of system states were either (a) a limited set of
well-known states or (b) described as a set of state-constraints that
describe what facilities are available to devices in a given state
(e.g., "Bus XYZ shut down") and what demands the system might expect
the device to provide while in that state (e.g., "provide wakeup
signal"). Or, maybe that's what you were describing and I just misread
it.

scott

-- 
scott preece
motorola mobile devices, il67, 1800 s. oak st., champaign, il  61820  
e-mail:	preece@motorola.com	fax:	+1-217-384-8550
phone:	+1-217-384-8589	cell: +1-217-433-6114	pager: 2174336114@vtext.com



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

Re: [RFC 3/3] Runtime PM support for named power states

[Adam Belay]

[-- Attachment #1: Type: text/plain, Size: 4148 bytes --]

On Wed, Oct 05, 2005 at 04:08:03PM -0400, Alan Stern wrote:
> On Tue, 4 Oct 2005, David Brownell wrote:
> 
> > Let's forget about the names for a moment (which I certainly prefer
> > in sysfs, especially with driver-controlled values).
> > 
> > The "confusion" is that there are two notions of state:
> > 
> >  - One of them is for system sleep transitions, and in practice it's
> >    nothing more than "I successfully called suspend_device()".
> > 
> >  - The other notion is for runtime power management, and it needs
> >    a richer notion.  Part of that is that the states are defined
> >    by the driver (or bus).
> 
> In the early stages of the PM development, the developers thought a device
> was either ON or SUSPENDed and imagined this would suffice for runtime 
> PM.  So there was originally just one notion of state, which has since 
> split into two.
> 
> > My theory is that the PM core doesn't need any notion that's more
> > complex than "moved device from <this> device list to <that> one".
> > Suspend and resume just move devices between lists.
> 
> Sounds reasonable.  There's one thing to consider, though.  System sleep
> transitions are supposed to work even when devices are already in a
> low-power state, which some drivers may not be able to handle.  That's the
> main reason why dev->power.power_state exists today -- so that the PM core
> can see if a device needs to be set back to full power before suspending
> it.
> 
> IMO we could avoid the need for power_state by removing the guarantee
> about never calling ->suspend when a device is already in a low-power
> state.  The system sleep code could issue an optimistic ->suspend call,
> and if that fails then do ->resume followed by ->suspend.

Another solution would be to make it entirely the driver's responsibility.
We could just unconditionally issue ->suspend and it could be a noop if
the device is already in a low power state.  If driver knows the device
needs to be powered back on and then off again to complete the suspend
transition then all of this can happen inside the ->suspend method.

I see ->suspend and ->resume as a way to notify drivers of userspace
requests and system state changes.  The driver can determine its own
actions for each of these requests.

> 
> > One nice consequence of that theory is that there's nothing to
> > prevent completly ripping out the other notions of power state.
> > Or -- equivalently, but less painfully! -- repurposing all that
> > infrastructure to the exclusive use of runtime PM.  Which I'd
> > argue only really needs one cookie identifying the device state.
> > Be it a string constant, or whatever.
> 
> That's why I wrote the patch in that form.  Since we need string constants 
> for user interaction anyway, they may as well serve as the state 
> indicator.
> 
> As part of the infrastructure changes, we could remove
> dev->power.power_state, dev->power.prev_state, and maybe also
> dev->power.saved_state (currently it is used only in a few platform
> drivers for stuff that could easily be stored in a private data
> structure).  Nothing would be needed but the name pointer, and that should
> be under the bus/driver's control.
> 
> People have already pretty much agreed that ->resume needs to take a 
> pm_message_t argument, and that will mean changing a lot of drivers.  We 
> ought to settle on a decision about the rest of this too, so we won't have 
> to go through and change them all yet again.

Right.

> 
> (BTW, a theoretical advantage of passing a pm_message_t to ->resume is
> that a driver could use the _same_ subroutine as a combined suspend/resume
> method.  :-))

Yeah, this occured to me also.  Should we go for it?  What would we call it?
(e.g. ->statectl()).

-->snip

> 
> > I suspect only driver code should ever care about device states.
> > 
> > The PM core should just tell drivers to become compatible with some
> > new constraint (like ACPI S3, generally implying devices in PCI D2
> > or D3; while S1 doesn't) ... and not worry about whether that involves
> > a state change or not.
> > 
> > Maybe they're already _in_ that state for example.

I agree.


-Adam

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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1564 bytes --]

On Wed, 5 Oct 2005, Adam Belay wrote:

> > IMO we could avoid the need for power_state by removing the guarantee
> > about never calling ->suspend when a device is already in a low-power
> > state.  The system sleep code could issue an optimistic ->suspend call,
> > and if that fails then do ->resume followed by ->suspend.
> 
> Another solution would be to make it entirely the driver's responsibility.
> We could just unconditionally issue ->suspend and it could be a noop if
> the device is already in a low power state.  If driver knows the device
> needs to be powered back on and then off again to complete the suspend
> transition then all of this can happen inside the ->suspend method.

Both are possible.  The core can call ->suspend, and if the driver needs 
to resume first and is able to do so, then it does.  Otherwise it returns 
an error and the core does the resume, then calls ->suspend again.

> I see ->suspend and ->resume as a way to notify drivers of userspace
> requests and system state changes.  The driver can determine its own
> actions for each of these requests.

That's the right idea.

> > (BTW, a theoretical advantage of passing a pm_message_t to ->resume is
> > that a driver could use the _same_ subroutine as a combined suspend/resume
> > method.  :-))
> 
> Yeah, this occured to me also.  Should we go for it?  What would we call it?
> (e.g. ->statectl()).

It's up to the individual driver.  We'll keep both method pointers in
struct device; the driver can set them to point at the same subroutine if
it wants.

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3472 bytes --]

On Wed, 5 Oct 2005, Scott E. Preece wrote:

> | From: "Alan Stern" <stern@rowland.harvard.edu>
> | 
> | IMO we could avoid the need for power_state by removing the guarantee
> | about never calling ->suspend when a device is already in a low-power
> | state.  The system sleep code could issue an optimistic ->suspend call,
> | and if that fails then do ->resume followed by ->suspend.
> ---
> 
> That assumes it's "safe" to call suspend regardless of device power
> state - that in the worst case, the device will just return failure.
> That seems reasonable, but it's a constraint driver writers would want
> to know about.

Yes.  This would be a change in the PM API, and driver authors would need 
to know about it.

> ---
> | 
> | (BTW, a theoretical advantage of passing a pm_message_t to ->resume is
> | that a driver could use the _same_ subroutine as a combined
> | suspend/resume
> | method.  :-))
> ---
> 
> It would be nice to encourage code cohesion by NOT encouraging
> multipurpose interfaces...

It's not so bad if you think of it as a simple "take care of this power 
change" routine.  That's just a single purpose.

In general, the core isn't supposed to know much about the internal states
of a device.  If the user requests a power change, the core doesn't really
know whether it's a suspend, a resume, or a move from one low-power state
to another.  From this point of view, it makes sense to have a single 
power-change method.

> ---
> | 
> | The PM core shouldn't worry too much about redundant calls.  Drivers can
> | easily filter them out, and the core won't have enough information to
> | detect them in general.
> | 
> | As I see it, we can use the new .name field to distinguish between the 
> | different kinds of system state change calls.  The PM core could export 
> | strings like (for FREEZE):
> | 
> | 	APM-standby, APM-suspend, snapshot, kexec;
> | 
> | or (for SUSPEND):
> | 
> | 	RAM, S3, S4, S5, shutdown;
> | 
> | or (for RESUME):
> | 
> | 	snapshot, RAM, kexec.
> | 
> | Something like this will provide drivers the means for telling which
> | target state to use, thus addressing one of your biggest complaints
> | about
> | the current system.
> | 
> | Of course, this means that drivers will need code to map these 
> | higher-level names to the appropriate target state, although that code 
> | wouldn't need to be very sophisticated.  It would have to be smart
> | enough 
> | to recognize that when .event is PM_EVENT_RUNTIME, the name indicates
> | the 
> | requested power state directly.
> ---
> 
> It would be nicer if the system didn't need to know anything about the
> internal state of the devices, but could just tell the device what
> system state it wants to go to and let the device map that to whatever
> internal state or sequence of sub-states the device needs. This would be
> helped if the set of system states were either (a) a limited set of
> well-known states or (b) described as a set of state-constraints that
> describe what facilities are available to devices in a given state
> (e.g., "Bus XYZ shut down") and what demands the system might expect
> the device to provide while in that state (e.g., "provide wakeup
> signal"). Or, maybe that's what you were describing and I just misread
> it.

I was trying to describe (a).  The names I listed were an attempt to 
define the limited set of well-known system states.

The wakeup signal stuff is already handled by a separate mechanism.

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]

On Wednesday 05 October 2005 1:08 pm, Alan Stern wrote:
> On Tue, 4 Oct 2005, David Brownell wrote:
> 
> > My theory is that the PM core doesn't need any notion that's more
> > complex than "moved device from <this> device list to <that> one".
> > Suspend and resume just move devices between lists.

(That is, for _system_ sleep state transitions.)


> Sounds reasonable.  There's one thing to consider, though.  System sleep
> transitions are supposed to work even when devices are already in a
> low-power state, which some drivers may not be able to handle.  That's the
> main reason why dev->power.power_state exists today -- so that the PM core
> can see if a device needs to be set back to full power before suspending
> it.

Then let the darn driver implement those rules.  Admittedly (as Scott
Preece implied) that's a change in semantics, and some drivers may well
expect suspend() is never followed by another suspend().  But my point
was that the states are device-specific ... so there's really not much
reasoning that least-common-denominator "pm core" code can do about
the states.  (IMO it's reasonable to define a "full-ON" state; but any
other states can't be very generic.)


> IMO we could avoid the need for power_state by removing the guarantee
> about never calling ->suspend when a device is already in a low-power
> state.  The system sleep code could issue an optimistic ->suspend call,
> and if that fails then do ->resume followed by ->suspend.

I'm sure I said part of that.  :)


> As part of the infrastructure changes, we could remove
> dev->power.power_state, dev->power.prev_state, and maybe also
> dev->power.saved_state (currently it is used only in a few platform
> drivers for stuff that could easily be stored in a private data
> structure).  Nothing would be needed but the name pointer, and that should
> be under the bus/driver's control.

Exactly.


> People have already pretty much agreed that ->resume needs to take a 
> pm_message_t argument, and that will mean changing a lot of drivers.

I certainly didn't agree on that, though I expect you remembered that.



> > (IMO it's clearly wrong to clobber a power_state value that the
> > driver set ... it surely knows more about itself than the pm core.)
> 
> Drivers _should_ manage these values, especially if they will be used by 
> sysfs.  On the other hand, if there are only two states then the PM core 
> can assume that the first is ON and the second is SUSPEND, so it could 
> handle the updates by itself.

That's a big "if".  Notice by the way there's no "OFF" ... what's with
that?  If there's "core" code knowing about states, "ON" and "OFF" would
make sense pretty much always.  Then there could be various driver
specific states that can morph back to ON -- preserving state, which
may be something USB needs more than most other frameworks -- with varying
degrees of speed.

- Dave


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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4513 bytes --]

On Wed, 5 Oct 2005, David Brownell wrote:

> On Wednesday 05 October 2005 1:08 pm, Alan Stern wrote:
> > On Tue, 4 Oct 2005, David Brownell wrote:
> > 
> > > My theory is that the PM core doesn't need any notion that's more
> > > complex than "moved device from <this> device list to <that> one".
> > > Suspend and resume just move devices between lists.
> 
> (That is, for _system_ sleep state transitions.)
> 
> 
> > Sounds reasonable.  There's one thing to consider, though.  System sleep
> > transitions are supposed to work even when devices are already in a
> > low-power state, which some drivers may not be able to handle.  That's the
> > main reason why dev->power.power_state exists today -- so that the PM core
> > can see if a device needs to be set back to full power before suspending
> > it.
> 
> Then let the darn driver implement those rules.  Admittedly (as Scott
> Preece implied) that's a change in semantics, and some drivers may well
> expect suspend() is never followed by another suspend().  But my point
> was that the states are device-specific ... so there's really not much
> reasoning that least-common-denominator "pm core" code can do about
> the states.  (IMO it's reasonable to define a "full-ON" state; but any
> other states can't be very generic.)
> 
> 
> > IMO we could avoid the need for power_state by removing the guarantee
> > about never calling ->suspend when a device is already in a low-power
> > state.  The system sleep code could issue an optimistic ->suspend call,
> > and if that fails then do ->resume followed by ->suspend.
> 
> I'm sure I said part of that.  :)

Ah, but which one of us said it first?  :-)

> > As part of the infrastructure changes, we could remove
> > dev->power.power_state, dev->power.prev_state, and maybe also
> > dev->power.saved_state (currently it is used only in a few platform
> > drivers for stuff that could easily be stored in a private data
> > structure).  Nothing would be needed but the name pointer, and that should
> > be under the bus/driver's control.
> 
> Exactly.

Then you agree with everything up to here?  Good.

FREEZE is a rather special case.  It doesn't correspond to any particular 
power state.  But maybe that doesn't matter -- when devices are in FREEZE, 
then almost by definition no user processes are awake to query their 
states.

> > People have already pretty much agreed that ->resume needs to take a 
> > pm_message_t argument, and that will mean changing a lot of drivers.
> 
> I certainly didn't agree on that, though I expect you remembered that.

I didn't remember you either agreeing or objecting to it.  But we do have
this issue about whether or not devices might be in an unknown state when
a RESUME occurs.  Passing an extra argument would enable drivers to _know_
when to expect an "unexpected" state change.

> > > (IMO it's clearly wrong to clobber a power_state value that the
> > > driver set ... it surely knows more about itself than the pm core.)
> > 
> > Drivers _should_ manage these values, especially if they will be used by 
> > sysfs.  On the other hand, if there are only two states then the PM core 
> > can assume that the first is ON and the second is SUSPEND, so it could 
> > handle the updates by itself.
> 
> That's a big "if".

But it's true for many devices.  All USB devices, for instance.  And the 
PM core could easily tell when it was true, by exporting a generic 
two-entry state array and checking whether a particular device was using 
that generic array.

>  Notice by the way there's no "OFF" ... what's with
> that?  If there's "core" code knowing about states, "ON" and "OFF" would
> make sense pretty much always.  Then there could be various driver
> specific states that can morph back to ON -- preserving state, which
> may be something USB needs more than most other frameworks -- with varying
> degrees of speed.

Is there a significant difference in meaning between OFF and SUSPEND?  
Seems to me that OFF should be a device's state only after a hot-unplug --
i.e., once the struct device has been unregistered.  So an OFF state would
never appear in the system.

Anyway, my point was that the simplest (two-state) case could be handled
directly by the PM core, while anything more complicated would have to be
handled by drivers.  This provides a relatively straightforward upgrade
path, since essentially every driver currently interacts with the core in
terms of a two-state model (or three if you count FREEZE as a separate
state).

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

Hi!

> As part of the infrastructure changes, we could remove
> dev->power.power_state, dev->power.prev_state, and maybe also
> dev->power.saved_state (currently it is used only in a few platform
> drivers for stuff that could easily be stored in a private data
> structure).  Nothing would be needed but the name pointer, and that should
> be under the bus/driver's control.
> 
> People have already pretty much agreed that ->resume needs to take a 
> pm_message_t argument, and that will mean changing a lot of drivers.  We 
> ought to settle on a decision about the rest of this too, so we won't have 
> to go through and change them all yet again.

I think they need to go in one-after-one, anyway... Adding parameter
to resume() should definitely be separate patch, because it needs to
change all drivers *at once*.  Removing power_state usage can be
done incrementally...
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         


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

Re: [RFC 3/3] Runtime PM support for named power states

[Alan Stern]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2026 bytes --]

On Sat, 8 Oct 2005, Pavel Machek wrote:

> > People have already pretty much agreed that ->resume needs to take a 
> > pm_message_t argument, and that will mean changing a lot of drivers.  We 
> > ought to settle on a decision about the rest of this too, so we won't have 
> > to go through and change them all yet again.
> 
> I think they need to go in one-after-one, anyway... Adding parameter
> to resume() should definitely be separate patch, because it needs to
> change all drivers *at once*.  Removing power_state usage can be
> done incrementally...

I suppose it could be done like that.  It would mean keeping the
power_state member in the structure until all the drivers are updated,
even though the core wouldn't use it any more.

There's a few other things I wanted to ask.  What do you think of the way
my patch used the new suspend_name member?  The idea was to allow users to
simply write a generic

	echo "suspend" >/sys/device/.../power/state

and have the driver do the appropriate thing, no matter what the actual
power states were.  A driver could even change the value of suspend_name
dynamically.  For instance, a PCI driver could initially make "suspend" an
alias for "D1", but when the device was put in D1 it could make "suspend"
an alias for "D2", and so on.  That's one way to implement multiple levels
of power saving.

Another question: Initially most drivers won't set up an array of named
states.  We could have the core treat such devices as though they were
using a generic two-entry array: "on" and "suspend".  That would provide
by default essentially the same functionality we have now, and it would
allow many drivers to use the new runtime PM scheme with (almost) no
changes.  Do you see anything wrong with that?

Third question: Is there a usable PCI core routine for choosing power
states?  If not, should a PCI ->suspend method usually use the
lowest-power available state?  I have a feeling this should involve ACPI,
but it looks like the code hasn't been written yet.

Alan Stern


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

Re: [RFC 3/3] Runtime PM support for named power states

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

Hi!

> > I think they need to go in one-after-one, anyway... Adding parameter
> > to resume() should definitely be separate patch, because it needs to
> > change all drivers *at once*.  Removing power_state usage can be
> > done incrementally...
> 
> I suppose it could be done like that.  It would mean keeping the
> power_state member in the structure until all the drivers are updated,
> even though the core wouldn't use it any more.

Yes... small price for being able to fix stuff incrementally.

> Another question: Initially most drivers won't set up an array of named
> states.  We could have the core treat such devices as though they were
> using a generic two-entry array: "on" and "suspend".  That would provide
> by default essentially the same functionality we have now, and it would
> allow many drivers to use the new runtime PM scheme with (almost) no
> changes.  Do you see anything wrong with that?

It will be broken for most existing drivers, anyway... I'd only enable
it if driver provided states.

> Third question: Is there a usable PCI core routine for choosing power
> states?  If not, should a PCI ->suspend method usually use the
> lowest-power available state?  I have a feeling this should involve ACPI,
> but it looks like the code hasn't been written yet.

pci_choose_state()? It even has some acpi hooks.
				Pavel
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         


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