lkml.org 
[lkml]   [2009]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 5)
    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



    \
     
     \ /
      Last update: 2009-06-24 23:33    [W:0.046 / U:149.512 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site