Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755652AbZFXVay (ORCPT ); Wed, 24 Jun 2009 17:30:54 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752106AbZFXVap (ORCPT ); Wed, 24 Jun 2009 17:30:45 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:50852 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751679AbZFXVao (ORCPT ); Wed, 24 Jun 2009 17:30:44 -0400 Date: Wed, 24 Jun 2009 17:30:45 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: "Rafael J. Wysocki" cc: Linux-pm mailing list , Oliver Neukum , Magnus Damm , ACPI Devel Maling List , Ingo Molnar , LKML , Greg KH , Arjan van de Ven Subject: Re: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 5) In-Reply-To: <200906242124.05511.rjw@sisk.pl> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8240 Lines: 259 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 > #include > #include > +#include > > #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-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/