On Wednesday 24 June 2009, Alan Stern wrote:
> 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.
Just to clarify, the last version of the patch I sent didn't address the
comments from this, but this was not because I didn't agree with them, but
I was just focusing on simplifying drivers/base/power/resume.c .
I'll address them in the next version.
Best,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
