Messages in this thread | | | Date | Mon, 9 Aug 2004 09:02:30 -0700 (PDT) | From | Patrick Mochel <> | Subject | Re: [RFC] Fix Device Power Management States |
| |
On Mon, 9 Aug 2004, Pavel Machek wrote:
> > - ->dev_stop() and ->dev_start() to struct class > > This provides the framework to shutdown a device from a functional > > level, rather than at a hardware level, as well as the entry points > > to stop/start ALL devices in the system. > > Hmm, that will need lots of new code to call these, right? Like > device_suspend() will now get device_stop() equivalent, etc....
Uh, not really. During suspend-to-disk, you would call
device_stop(); device_save(); <snapshot system> class_device_start(suspend_device); <write snapshot> class_device_stop(suspend_device); device_power_down(state);
> > This is implemented by iterating through the list of struct classes, > > then through each of their struct class_device's. The class_device is > > the only argument to those functions. > > Aha, so you are saying these do not need to be done in hardware order?
AFAICT, no.
> Semantics of dev_stop is "may not do DMA and interrupts when stopped", > right?
To be more precise, "device is not processing any transactions and will not be used to submit more to". It's up to the class to remove it from any queues, etc, so DMA never has a chance to begin.
> I do not think we want to see "u32 state" any more. This really needs > to be specified as "enum something" otherwise everyone will get it > wrong... again.
That's not the point, at least at this stage. Eventually, the u32 could be replaced with an enum. But, remember why there is confusion - the value of the system suspend level was also passed to the drivers as the device suspend level. If you read the email again and look at the code, you will see that drivers always get passed a 'struct pm_state', and the 'u32' are only used between the suspend imeplementations and the driver core (which is the re-mapper). I think compile-time checks are good, but we're in bigger trouble if we can't get a half-dozen calls using the right value..
> > pointers, that is in dev->power.pm_system. The pointers in that array > > point to entries in dev->power.pm_supports, which is an array of all the > > device power states that device supports. Please see include/linux/pm.h in > > the patch below. These arrays should be initialized by the bus, though > > they can be fixed up by the individual drivers, should they have a > > different set of states they support, or given user policy. > > I'd simply pass system state down to the driver, and let them map as > they see fit... This seems to be way too complex. OTOH you are solving > 'runtime suspend', too...
It's not too complex; it's simply that the driver core is the one responsible between mapping a system suspend state to the device suspend state, based on values that the driver knows a priori. If you push that responsibility down to the drivers, you require all of them to implement the same thing, causing code duplication, which means more copy-n-pasting, and more of a chance to get it wrong. If we choose to do it once and right in the driver core, the resulting drivers become simpler, since they only have to respond to something that says "enter this power state, damnit"
On the other hand, if it's too complicated for you, I encourage you to modify the patch or create a new one that solves all of the problems in a simpler manner.
> > +enum { > > + pm_sys_ON = 1, > > + pm_sys_SHUTDOWN, > > + pm_sys_RESET, > > + pm_sys_STANDBY, > > + pm_sys_SUSPEND, > > + pm_sys_HIBERNATE, > > + pm_sys_NUM, > > +}; > > I believe different state is needed for "quiesce for atomic copy" and > for "we are really going down to S4 now".
There is nothing fundamentally different at the functional level - you don't want any devices fulfilling any request. Besides, by the time the system is actually ready to be placed in S4, the devices have long-since been stopped, and the class devices do not need another notification beyond "stop"
Thanks,
Pat
- 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/
| |