On Wed, 24 Jun 2009, Rafael J. Wysocki wrote:
> +config PM_RUNTIME
> + bool "Run-time PM core functionality"
> + depends on PM
> + ---help---
> + Enable functionality allowing I/O devices to be put into energy-saving
> + (low power) states at run time (or autosuspended) after a specified
> + period of inactivity and woken up in response to a hardware-generated
> + wake-up event or a driver's request.
> +
> + Hardware support is generally required for this functionality to work
> + and the bus type drivers of the buses the devices are on are
> + responsibile for the actual handling of the autosuspend requests and
s/ibile/ible/
> @@ -165,6 +168,28 @@ typedef struct pm_message {
> * It is allowed to unregister devices while the above callbacks are being
> * executed. However, it is not allowed to unregister a device from within any
> * of its own callbacks.
> + *
> + * There also are the following callbacks related to run-time power management
> + * of devices:
> + *
> + * @runtime_suspend: Prepare the device for a condition in which it won't be
> + * able to communicate with the CPU(s) and RAM due to power management.
> + * This need not mean that the device should be put into a low power state.
> + * For example, if the device is behind a link which is about to be turned
> + * off, the device may remain at full power. Still, if the device does go
s/Still, if/If/ -- the word "Still" seems a little odd in this context.
> + * to low power and if device_may_wakeup(dev) is true, remote wake-up
> + * (i.e. hardware mechanism allowing the device to request a change of its
s/i.e. /i.e., a /
> + * power state, such as PCI PME) should be enabled for it.
> + *
> + * @runtime_resume: Put the device into the fully active state in response to a
> + * wake-up event generated by hardware or at a request of software. If
s/at a request/at the request/
> + * necessary, put the device into the full power state and restore its
> + * registers, so that it is fully operational.
> + * RPM_ACTIVE Device is fully operational, no run-time PM requests are
> + * pending for it.
> + *
> + * RPM_IDLE It has been requested that the device be suspended.
> + * Suspend request has been put into the run-time PM
> + * workqueue and it's pending execution.
> + *
> + * RPM_SUSPENDING Device bus type's ->runtime_suspend() callback is being
> + * executed.
> + *
> + * RPM_SUSPENDED Device bus type's ->runtime_suspend() callback has
> + * completed successfully. The device is regarded as
> + * suspended.
> + *
> + * RPM_WAKE It has been requested that the device be woken up.
> + * Resume request has been put into the run-time PM
> + * workqueue and it's pending execution.
> + *
> + * RPM_RESUMING Device bus type's ->runtime_resume() callback is being
> + * executed.
Remember to add RPM_NOTIFY.
> +/**
> + * __pm_get_child - Increment the counter of unsuspended children of a device.
> + * @dev: Device to handle;
> + */
> +static void __pm_get_child(struct device *dev)
> +{
> + atomic_inc(&dev->power.child_count);
> +}
> +
> +/**
> + * __pm_put_child - Decrement the counter of unsuspended children of a device.
> + * @dev: Device to handle;
> + */
> +static void __pm_put_child(struct device *dev)
> +{
> + if (!atomic_add_unless(&dev->power.child_count, -1, 0))
> + dev_WARN(dev, "Unbalanced counter decrementation");
> +}
I think we don't need this dev_WARN. It should be straightforward to
verify that the increments and decrements balance correctly, and the
child_count field isn't manipulated by drivers.
In fact, these don't need to be separate routines at all. Just call
atomic_inc or atomic_dec directly.
> +
> +/**
> + * __pm_runtime_suspend - Run a device bus type's runtime_suspend() callback.
> + * @dev: Device to suspend.
> + * @sync: If unset, the funtion has been called via pm_wq.
> + *
> + * Check if the run-time PM status of the device is appropriate and run the
> + * ->runtime_suspend() callback provided by the device's bus type. Update the
> + * run-time PM flags in the device object to reflect the current status of the
> + * device.
> + */
> +int __pm_runtime_suspend(struct device *dev, bool sync)
> +{
> + struct device *parent = NULL;
> + unsigned long flags;
> + int error = -EINVAL;
Remove the initializer.
> +
> + might_sleep();
> +
> + spin_lock_irqsave(&dev->power.lock, flags);
> +
> + repeat:
> + if (dev->power.runtime_status == RPM_ERROR) {
Insert: error = -EINVAL;
> + goto out;
> + } else if (dev->power.runtime_status & RPM_SUSPENDED) {
...
> +void pm_runtime_put(struct device *dev)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&dev->power.lock, flags);
> +
> + if (!__pm_runtime_put(dev)) {
> + dev_WARN(dev, "Unbalanced counter decrementation");
"decrementation" isn't a word -- or if it is, it shouldn't be. :-)
Just use "decrement". Similarly in other places.
> +/**
> + * pm_runtime_add - Update run-time PM fields of a device while adding it.
> + * @dev: Device object being added to device hierarchy.
> + */
> +void pm_runtime_add(struct device *dev)
> +{
> + dev->power.runtime_notify = false;
> + INIT_DELAYED_WORK(&dev->power.suspend_work, pm_runtime_suspend_work);
Doesn't INIT_DELAYED_WORK belong in pm_runtime_init?
Do we want the bus subsystem to be responsible for doing:
dev->power.runtime_disabled = false;
pm_runtime_put(dev);
after calling device_add? Or should device_add do it?
> Index: linux-2.6/include/linux/pm_runtime.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6/include/linux/pm_runtime.h
> +static inline struct device *suspend_work_to_device(struct work_struct *work)
> +{
> + struct delayed_work *dw = to_delayed_work(work);
> + struct dev_pm_info *dpi;
> +
> + dpi = container_of(dw, struct dev_pm_info, suspend_work);
> + return container_of(dpi, struct device, power);
> +}
You don't need to iterate container_of like this. You can do:
return container_of(dw, struct device, power.suspend_work);
> +
> +static inline struct device *work_to_device(struct work_struct *work)
> +{
> + struct dev_pm_info *dpi;
> +
> + dpi = container_of(work, struct dev_pm_info, work);
> + return container_of(dpi, struct device, power);
> +}
Similarly here.
These two routines aren't used outside of runtime.c. They should be
moved into that file. The same goes for pm_children_suspended and
pm_suspend_possible.
> +
> +static inline void __pm_runtime_get(struct device *dev)
> +{
> + atomic_inc(&dev->power.resume_count);
> +}
Why introduce __pm_runtime_get? Just make this pm_runtime_get.
> +static inline void pm_runtime_remove(struct device *dev)
> +{
> + pm_runtime_disable(dev);
> +}
You forgot to decrement the parent's child_count if dev isn't
suspended (and then do a idle_notify on the parent). Because of this
additional complexity, don't inline the routine.
> Index: linux-2.6/drivers/base/dd.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/dd.c
> +++ linux-2.6/drivers/base/dd.c
> @@ -23,6 +23,7 @@
> #include <linux/kthread.h>
> #include <linux/wait.h>
> #include <linux/async.h>
> +#include <linux/pm_runtime.h>
>
> #include "base.h"
> #include "power/power.h"
> @@ -202,8 +203,12 @@ int driver_probe_device(struct device_dr
> pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
> drv->bus->name, __func__, dev_name(dev), drv->name);
>
> + pm_runtime_disable(dev);
> +
> ret = really_probe(dev, drv);
>
> + pm_runtime_enable(dev);
> +
Shouldn't we guarantee that a device isn't probed while it is in a
suspended state? So this should be
pm_runtime_get(dev);
ret = pm_runtime_resume(dev);
if (ret == 0)
ret = really_probe(dev, drv);
pm_runtime_put(dev);
It might be nice to have a simple combined pm_runtime_get_and_resume
for this sort of situation.
More comments to follow when I get time to review more of the code...
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
