Date:	Wed, 24 Jun 2009 17:30:45 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
cc:	Linux-pm mailing list <linux-pm@...ts.linux-foundation.org>,
	Oliver Neukum <oliver@...kum.org>,
	Magnus Damm <magnus.damm@...il.com>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>, Greg KH <gregkh@...e.de>,
	Arjan van de Ven <arjan@...radead.org>
Subject: Re: [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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists