Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761653AbZFZVts (ORCPT ); Fri, 26 Jun 2009 17:49:48 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755132AbZFZVtj (ORCPT ); Fri, 26 Jun 2009 17:49:39 -0400 Received: from ogre.sisk.pl ([217.79.144.158]:60000 "EHLO ogre.sisk.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751287AbZFZVti (ORCPT ); Fri, 26 Jun 2009 17:49:38 -0400 From: "Rafael J. Wysocki" To: Alan Stern Subject: Re: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 5) Date: Fri, 26 Jun 2009 23:49:41 +0200 User-Agent: KMail/1.11.2 (Linux/2.6.30-rjw; KDE/4.2.4; x86_64; ; ) Cc: "Linux-pm mailing list" , Oliver Neukum , Magnus Damm , ACPI Devel Maling List , Ingo Molnar , LKML , Greg KH , Arjan van de Ven References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200906262349.42851.rjw@sisk.pl> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8964 Lines: 263 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 > > #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. 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 -- 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/