Re: [patch update] PM: Introduce core framework for run-time PM of I/O devices (rev. 5)

["Rafael J. Wysocki" <rjw@sisk.pl>]

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