PM: Introduce core framework for run-time PM of I/O devices (rev. 11)

Hi,

OK, if this is to go into 2.6.32, the last moment for putting it into linux-next is now. If you have any objections, remarks, etc. please let me know or I'm going to put this one into the linux-next branch of the suspend-2.6 tree in the next couple of days.

+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the funtion has been called via pm_wq.

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+{

drivers/base/power/runtime.c new file +6 -0

+static int __pm_request_suspend(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		return -EINVAL;

+int __pm_request_resume(struct device *dev)

drivers/base/power/runtime.c new file +34 -0

+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	if (!dev->power.runtime_failure && !dev->power.disable_depth)
+		goto out;
+
+	if (dev->power.runtime_status == status)
+		goto out_clear;
+
+	if (status == RPM_SUSPENDED) {
+		/* It always is possible to set the status to 'suspended'. */
+		if (parent)
+			atomic_add_unless(&parent->power.child_count, -1, 0);
+		dev->power.runtime_status = status;
+		goto out_clear;
+	}
+
+	if (parent) {
+		spin_lock_irq(&parent->power.lock);
+
+		/*
+		 * It is invalid to put an active child under a parent that is
+		 * not active, has run-time PM enabled and the
+		 * 'power.ignore_children' flag unset.
+		 */
+		if (!parent->power.disable_depth
+		    && !parent->power.ignore_children
+		    && parent->power.runtime_status != RPM_ACTIVE) {
+			error = -EBUSY;
+		} else {
+			if (dev->power.runtime_status == RPM_SUSPENDED)
+				atomic_inc(&parent->power.child_count);
+			dev->power.runtime_status = status;
+		}
+

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

drivers/base/power/runtime.c new file +33 -0

+	}
+
+	if (dev->power.runtime_status == RPM_SUSPENDING
+	    || dev->power.runtime_status == RPM_RESUMING) {
+		DEFINE_WAIT(wait);
+
+		/* Suspend or wake-up in progress. */
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (dev->power.runtime_status != RPM_SUSPENDING
+			    && dev->power.runtime_status != RPM_RESUMING)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+
+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}
+
+	if (dev->power.runtime_failure)
+		goto out;
+
+	if (dev->power.idle_notification) {
+		DEFINE_WAIT(wait);
+
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (!dev->power.idle_notification)

drivers/base/power/runtime.c new file +4 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+

drivers/base/power/main.c +10 -0

@@ -49,6 +50,16 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
+ * device_pm_init - Initialize the PM-related part of a device object
+ * @dev: Device object to initialize.
+ */
+void device_pm_init(struct device *dev)
+{
+	dev->power.status = DPM_ON;
+	pm_runtime_init(dev);
+}
+
+/**
  *	device_pm_lock - lock the list of active devices used by the PM core
  */
 void device_pm_lock(void)

Hi,

OK, if this is to go into 2.6.32, the last moment for putting it into linux-next is now. If you have any objections, remarks, etc. please let me know or I'm going to put this one into the linux-next branch of the suspend-2.6 tree in the next couple of days.

+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the funtion has been called via pm_wq.

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+{

drivers/base/power/runtime.c new file +6 -0

+static int __pm_request_suspend(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		return -EINVAL;

+int __pm_request_resume(struct device *dev)

drivers/base/power/runtime.c new file +34 -0

+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	if (!dev->power.runtime_failure && !dev->power.disable_depth)
+		goto out;
+
+	if (dev->power.runtime_status == status)
+		goto out_clear;
+
+	if (status == RPM_SUSPENDED) {
+		/* It always is possible to set the status to 'suspended'. */
+		if (parent)
+			atomic_add_unless(&parent->power.child_count, -1, 0);
+		dev->power.runtime_status = status;
+		goto out_clear;
+	}
+
+	if (parent) {
+		spin_lock_irq(&parent->power.lock);
+
+		/*
+		 * It is invalid to put an active child under a parent that is
+		 * not active, has run-time PM enabled and the
+		 * 'power.ignore_children' flag unset.
+		 */
+		if (!parent->power.disable_depth
+		    && !parent->power.ignore_children
+		    && parent->power.runtime_status != RPM_ACTIVE) {
+			error = -EBUSY;
+		} else {
+			if (dev->power.runtime_status == RPM_SUSPENDED)
+				atomic_inc(&parent->power.child_count);
+			dev->power.runtime_status = status;
+		}
+

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

drivers/base/power/runtime.c new file +33 -0

+	}
+
+	if (dev->power.runtime_status == RPM_SUSPENDING
+	    || dev->power.runtime_status == RPM_RESUMING) {
+		DEFINE_WAIT(wait);
+
+		/* Suspend or wake-up in progress. */
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (dev->power.runtime_status != RPM_SUSPENDING
+			    && dev->power.runtime_status != RPM_RESUMING)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+
+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}
+
+	if (dev->power.runtime_failure)
+		goto out;
+
+	if (dev->power.idle_notification) {
+		DEFINE_WAIT(wait);
+
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (!dev->power.idle_notification)

drivers/base/power/runtime.c new file +4 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+

drivers/base/power/main.c +10 -0

@@ -49,6 +50,16 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
+ * device_pm_init - Initialize the PM-related part of a device object
+ * @dev: Device object to initialize.
+ */
+void device_pm_init(struct device *dev)
+{
+	dev->power.status = DPM_ON;
+	pm_runtime_init(dev);
+}
+
+/**
  *	device_pm_lock - lock the list of active devices used by the PM core
  */
 void device_pm_lock(void)

Quoted 6 lines · Rafael J. Wysocki

Hi,

OK, if this is to go into 2.6.32, the last moment for putting it into linux-next is now. If you have any objections, remarks, etc. please let me know or I'm going to put this one into the linux-next branch of the suspend-2.6 tree in the next couple of days.

I'm sorry I haven't been keeping on top of all your work on this. Lots of other stuff has been going on in the meantime...

One the whole this all looks very good. It's basically ready to be merged. There are a couple of minor issues remaining plus a bunch of unimportant implementation details.

pm_runtime_disable() gets used for several different purposes. For the usage in pm_runtime_remove(), it's silly to carry out a pending resume request. Should we add an argument saying whether or not to do so?

In the documentation, it would be nice to have a section listing the default runtime PM settings and explaining what a driver should do to activate runtime PM on a newly-registered device.

drivers/base/power/runtime.c line 39

Quoted 9 lines

drivers/base/power/runtime.c new file +9 -0

+static void pm_runtime_cancel_pending(struct device *dev)
+{
+	pm_runtime_deactivate_timer(dev);
+	/*
+	 * If there's a request pending, make sure its work function will return
+	 * without doing anything.
+	 */
+	if (dev->power.request_pending)
+		dev->power.request = RPM_REQ_NONE;

No need for the "if"; you can always do the assignment.

drivers/base/power/runtime.c line 53

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+static int __pm_runtime_idle(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		retval = -EINVAL;

Instead of assigning to retval, you could simply return these values.

drivers/base/power/runtime.c line 61

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+	else if (dev->power.idle_notification)
+		retval = -EINPROGRESS;
+	else if (atomic_read(&dev->power.usage_count) > 0
+	    || dev->power.disable_depth > 0
+	    || dev->power.timer_expires > 0
+	    || dev->power.runtime_status == RPM_SUSPENDED
+	    || dev->power.runtime_status == RPM_SUSPENDING)
+		retval = -EAGAIN;

Do we also want to rule out RPM_RESUMING? That is,

	    || dev->power.runtime_status != RPM_ACTIVE

drivers/base/power/runtime.c line 85

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		dev->bus->pm->runtime_idle(dev);
+
+	spin_lock_irq(&dev->power.lock);

Small optimization: Put the spin_{un}lock_irq stuff inside the "if" statement, so it doesn't happen if the test fails.

The same thing can be done in other places.

Quoted 3 lines

+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the funtion has been called via pm_wq.

Fix spelling of "function". Likewise in __pm_runtime_resume.

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+{

...

drivers/base/power/runtime.c line 218

Quoted 7 lines

drivers/base/power/runtime.c new file +7 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (parent && !parent->power.ignore_children)
+		pm_request_idle(parent);
+
+	if (notify)
+		pm_runtime_idle(dev);

Move this up before the spin_unlock_irq and call __pm_runtime_idle instead. The same sort of thing can be done in __pm_runtime_resume.

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+static int __pm_request_suspend(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		return -EINVAL;

drivers/base/power/runtime.c line 480

Quoted 10 lines

drivers/base/power/runtime.c new file +10 -0

+
+	if (dev->power.runtime_status == RPM_SUSPENDED)
+		retval = 1;
+	else if (atomic_read(&dev->power.usage_count) > 0
+	    || dev->power.disable_depth > 0)
+		retval = -EAGAIN;
+	else if (dev->power.runtime_status == RPM_SUSPENDING)
+		retval = -EINPROGRESS;
+	else if (!pm_children_suspended(dev))
+		retval = -EBUSY;

Insert:

	if (retval)
		return retval;

Or else change the assignments to "return" statements. Yes, we agreed that a suspend request should override an existing idle-notify request. But if the new request fails then it shouldn't override anything. (Of course, if it fails for any of the reasons here then there can't be a pending idle-notify request anyway.)

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	pm_runtime_deactivate_timer(dev);
+
+	if (dev->power.request_pending) {
+		/*
+		 * Pending resume requests take precedence over us, but we can
+		 * overtake any other pending request.

drivers/base/power/runtime.c line 493

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 */
+		if (dev->power.request == RPM_REQ_RESUME)
+			retval = -EAGAIN;
+		else if (dev->power.request != RPM_REQ_SUSPEND)
+			dev->power.request = retval ?
+						RPM_REQ_NONE : RPM_REQ_SUSPEND;

Now there's no need to check retval.

drivers/base/power/runtime.c line 496

Quoted 3 lines

drivers/base/power/runtime.c new file +3 -0

+
+		if (dev->power.request == RPM_REQ_SUSPEND)
+			return 0;

Just simply:

		return retval;

Some of these cases can't happen. For instance, if we reach here then the status can't be SUSPENDED or SUSPENDING, so there can't be a pending resume request.

drivers/base/power/runtime.c line 65

Quoted 4 lines

drivers/base/power/runtime.c new file +4 -0

+	}
+
+	if (retval)
+		return retval;

Now this isn't needed. Similar code rearrangements can be made in __pm_request_resume.

Quoted 1 line

+int __pm_request_resume(struct device *dev)

Should be static.

drivers/base/power/runtime.c line 710

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;
+	int error = 0;
+
+	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+		return -EINVAL;

This should go inside the spinlocked area.

Quoted 38 lines

drivers/base/power/runtime.c new file +38 -0

+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	if (!dev->power.runtime_failure && !dev->power.disable_depth)
+		goto out;
+
+	if (dev->power.runtime_status == status)
+		goto out_clear;
+
+	if (status == RPM_SUSPENDED) {
+		/* It always is possible to set the status to 'suspended'. */
+		if (parent)
+			atomic_add_unless(&parent->power.child_count, -1, 0);
+		dev->power.runtime_status = status;
+		goto out_clear;
+	}
+
+	if (parent) {
+		spin_lock_irq(&parent->power.lock);
+
+		/*
+		 * It is invalid to put an active child under a parent that is
+		 * not active, has run-time PM enabled and the
+		 * 'power.ignore_children' flag unset.
+		 */
+		if (!parent->power.disable_depth
+		    && !parent->power.ignore_children
+		    && parent->power.runtime_status != RPM_ACTIVE) {
+			error = -EBUSY;
+		} else {
+			if (dev->power.runtime_status == RPM_SUSPENDED)
+				atomic_inc(&parent->power.child_count);
+			dev->power.runtime_status = status;
+		}
+
+		spin_unlock_irq(&parent->power.lock);
+
+		if (error)
+			goto out;

drivers/base/power/runtime.c line 755

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	} else {
+		dev->power.runtime_status = status;
+	}
+
+ out_clear:
+	dev->power.runtime_failure = false;

Move all those assignments to dev->power.runtime_status down here.

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

...

drivers/base/power/runtime.c line 829

Quoted 5 lines

drivers/base/power/runtime.c new file +5 -0

+	if (dev->power.disable_depth++ > 0)
+		goto out;
+
+	if (dev->power.runtime_failure)
+		goto out;

I don't see why this is needed.

Quoted 7 lines

drivers/base/power/runtime.c new file +7 -0

+
+	pm_runtime_deactivate_timer(dev);
+
+	if (dev->power.request_pending) {
+		dev->power.request = RPM_REQ_NONE;
+
+		spin_unlock_irq(&dev->power.lock);

drivers/base/power/runtime.c line 842

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+
+		cancel_work_sync(&dev->power.work);
+
+		spin_lock_irq(&dev->power.lock);
+
+		dev->power.request_pending = false;

Remove excessive whitespace.

Quoted 37 lines

drivers/base/power/runtime.c new file +37 -0

+	}
+
+	if (dev->power.runtime_status == RPM_SUSPENDING
+	    || dev->power.runtime_status == RPM_RESUMING) {
+		DEFINE_WAIT(wait);
+
+		/* Suspend or wake-up in progress. */
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (dev->power.runtime_status != RPM_SUSPENDING
+			    && dev->power.runtime_status != RPM_RESUMING)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+
+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}
+
+	if (dev->power.runtime_failure)
+		goto out;
+
+	if (dev->power.idle_notification) {
+		DEFINE_WAIT(wait);
+
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (!dev->power.idle_notification)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+

drivers/base/power/runtime.c line 174

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}

This wait loop should be merged with the previous loop.

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+void pm_runtime_init(struct device *dev)
+{
+	spin_lock_init(&dev->power.lock);
+
+	dev->power.runtime_status = RPM_SUSPENDED;
+	dev->power.idle_notification = false;

drivers/base/power/runtime.c line 909

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+
+	dev->power.disable_depth = 1;
+	atomic_set(&dev->power.usage_count, 0);
+
+	dev->power.runtime_failure = false;
+	dev->power.last_error = 0;

You don't have to set values to 0; they are initialized by kzalloc.

drivers/base/power/runtime.c line 922

Quoted 3 lines

drivers/base/power/runtime.c new file +3 -0

+	dev->power.suspend_timer.expires = jiffies;
+	dev->power.suspend_timer.data = (unsigned long)dev;
+	dev->power.suspend_timer.function = pm_suspend_timer_fn;

Use setup_timer() instead of assigning these fields directly.

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+
+	if (dev->power.runtime_status == RPM_ACTIVE) {
+		struct device *parent = dev->parent;
+
+		/*

drivers/base/power/runtime.c line 944

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 * Change the status back to 'suspended' to match the initial
+		 * status.
+		 */
+		pm_runtime_set_suspended(dev);
+		if (parent && !parent->power.ignore_children)
+			pm_request_idle(parent);

Shouldn't these last two lines be part of __pm_runtime_set_status()?

include/linux/pm_runtime.h line 20

Quoted 5 lines

include/linux/pm_runtime.h new file +2 -0

 
+extern void pm_runtime_init(struct device *dev);
+extern void pm_runtime_remove(struct device *dev);

I don't like seeing these two functions included in the public header file. It's enough to put them in drivers/base/power/power.h.

Quoted 23 lines

drivers/base/power/main.c +10 -0

@@ -49,6 +50,16 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
+ * device_pm_init - Initialize the PM-related part of a device object
+ * @dev: Device object to initialize.
+ */
+void device_pm_init(struct device *dev)
+{
+	dev->power.status = DPM_ON;
+	pm_runtime_init(dev);
+}
+
+/**
  *	device_pm_lock - lock the list of active devices used by the PM core
  */
 void device_pm_lock(void)

@@ -105,6 +116,8 @@ void device_pm_remove(struct device *dev
 	mutex_lock(&dpm_list_mtx);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);

drivers/base/power/main.c void device_pm_remove(struct device *dev · line 121

Quoted 3 lines

drivers/base/power/main.c +2 -0

+
+	pm_runtime_remove(dev);
 }

Calling pm_runtime_init() from device_pm_init() and pm_runtime_remove() from device_pm_remove() isn't good. If CONFIG_PM_SLEEP isn't enabled then the calls won't be compiled, even if CONFIG_PM_RUNTIME is set.

drivers/base/power/main.c static int dpm_prepare(pm_message_t stat · line 775

Quoted 7 lines

drivers/base/power/main.c +2 -1

@@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
 		dev->power.status = DPM_PREPARING;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_prepare(dev, state);
+		if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+			error = -EBUSY;

What's the reason for the -EBUSY error?

Quoted 6 lines

drivers/base/dd.c +0 -0

@@ -202,7 +203,9 @@ 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);
 

drivers/base/dd.c int driver_probe_device(struct device_dr · line 208

Quoted 3 lines

drivers/base/dd.c +2 -0

+	pm_runtime_get_noresume(dev);
 	ret = really_probe(dev, drv);
+	pm_runtime_put_noidle(dev);

This is bad because it won't wait if there's a runtime-PM call in progress. Also, we shouldn't use put_noidle because it might subvert the driver's attempt to autosuspend.

Instead we should do something like this:

	/* Wait for runtime PM calls to finish and prevent new calls
	 * until the probe is done.
	 */
	pm_runtime_disable(dev);
	pm_runtime_get_noresume(dev);
	pm_runtime_enable(dev):
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

Documentation/power/runtime_pm.txt

Quoted 7 lines

Documentation/power/runtime_pm.txt new file +3 -0

 
+2. Device Run-time PM Callbacks
 
+In particular, it is recommended that ->runtime_suspend() return -EBUSY if
+device_may_wakeup() returns 'false' for the device.

What's the point of this? I don't understand -- we don't want to discourage people from suspending devices with wakeup enabled.

Documentation/power/runtime_pm.txt line 148

Quoted 6 lines

Documentation/power/runtime_pm.txt new file +6 -0

+Additionally, the helper functions provided by the PM core obey the following
+rules:
+
+  * If ->runtime_suspend() is about to be executed or the execution of it is
+    scheduled or there's a pending request to execute it, ->runtime_idle() will
+    not be executed for the same device.

Shouldn't we allow runtime_idle when a suspend is scheduled? The idle handler might decide to suspend right away instead of waiting for the timer to expire.

Documentation/power/runtime_pm.txt line 266

Quoted 9 lines

+4. Run-time PM Device Helper Functions

Documentation/power/runtime_pm.txt new file +7 -0

+
+The following run-time PM helper functions are defined in
+drivers/base/power/runtime.c and include/linux/pm_runtime.h:
 
+  int pm_schedule_suspend(struct device *dev, unsigned int delay);
+    - schedule the execution of ->runtime_suspend() for the device's bus type
+      in future, where 'delay' is the time to wait before queuing up a suspend
+      work item in pm_wq, in miliseconds (if 'delay' is zero, the work item is

Fix spelling of "milli".

Explain that the new delay will override the
old one if a suspend was already scheduled and not yet expired.

Documentation/power/runtime_pm.txt line 285

Quoted 2 lines

Documentation/power/runtime_pm.txt new file +2 -0

+  int pm_runtime_get_sync(struct device *dev);
+    - increment the device's usage counter, run pm_rutime_resume(dev) and return

Fix spelling of "runtime". Same under pm_runtime_put_sync.

Quoted 6 lines · Rafael J. Wysocki

Hi,

OK, if this is to go into 2.6.32, the last moment for putting it into linux-next is now. If you have any objections, remarks, etc. please let me know or I'm going to put this one into the linux-next branch of the suspend-2.6 tree in the next couple of days.

I'm sorry I haven't been keeping on top of all your work on this. Lots of other stuff has been going on in the meantime...

One the whole this all looks very good. It's basically ready to be merged. There are a couple of minor issues remaining plus a bunch of unimportant implementation details.

pm_runtime_disable() gets used for several different purposes. For the usage in pm_runtime_remove(), it's silly to carry out a pending resume request. Should we add an argument saying whether or not to do so?

In the documentation, it would be nice to have a section listing the default runtime PM settings and explaining what a driver should do to activate runtime PM on a newly-registered device.

drivers/base/power/runtime.c line 39

Quoted 9 lines

drivers/base/power/runtime.c new file +9 -0

+static void pm_runtime_cancel_pending(struct device *dev)
+{
+	pm_runtime_deactivate_timer(dev);
+	/*
+	 * If there's a request pending, make sure its work function will return
+	 * without doing anything.
+	 */
+	if (dev->power.request_pending)
+		dev->power.request = RPM_REQ_NONE;

No need for the "if"; you can always do the assignment.

drivers/base/power/runtime.c line 53

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+static int __pm_runtime_idle(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		retval = -EINVAL;

Instead of assigning to retval, you could simply return these values.

drivers/base/power/runtime.c line 61

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+	else if (dev->power.idle_notification)
+		retval = -EINPROGRESS;
+	else if (atomic_read(&dev->power.usage_count) > 0
+	    || dev->power.disable_depth > 0
+	    || dev->power.timer_expires > 0
+	    || dev->power.runtime_status == RPM_SUSPENDED
+	    || dev->power.runtime_status == RPM_SUSPENDING)
+		retval = -EAGAIN;

Do we also want to rule out RPM_RESUMING? That is,

	    || dev->power.runtime_status != RPM_ACTIVE

drivers/base/power/runtime.c line 85

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		dev->bus->pm->runtime_idle(dev);
+
+	spin_lock_irq(&dev->power.lock);

Small optimization: Put the spin_{un}lock_irq stuff inside the "if" statement, so it doesn't happen if the test fails.

The same thing can be done in other places.

Quoted 3 lines

+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the funtion has been called via pm_wq.

Fix spelling of "function". Likewise in __pm_runtime_resume.

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+{

...

drivers/base/power/runtime.c line 218

Quoted 7 lines

drivers/base/power/runtime.c new file +7 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (parent && !parent->power.ignore_children)
+		pm_request_idle(parent);
+
+	if (notify)
+		pm_runtime_idle(dev);

Move this up before the spin_unlock_irq and call __pm_runtime_idle instead. The same sort of thing can be done in __pm_runtime_resume.

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+static int __pm_request_suspend(struct device *dev)
+{
+	int retval = 0;
+
+	if (dev->power.runtime_failure)
+		return -EINVAL;

drivers/base/power/runtime.c line 480

Quoted 10 lines

drivers/base/power/runtime.c new file +10 -0

+
+	if (dev->power.runtime_status == RPM_SUSPENDED)
+		retval = 1;
+	else if (atomic_read(&dev->power.usage_count) > 0
+	    || dev->power.disable_depth > 0)
+		retval = -EAGAIN;
+	else if (dev->power.runtime_status == RPM_SUSPENDING)
+		retval = -EINPROGRESS;
+	else if (!pm_children_suspended(dev))
+		retval = -EBUSY;

Insert:

	if (retval)
		return retval;

Or else change the assignments to "return" statements. Yes, we agreed that a suspend request should override an existing idle-notify request. But if the new request fails then it shouldn't override anything. (Of course, if it fails for any of the reasons here then there can't be a pending idle-notify request anyway.)

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	pm_runtime_deactivate_timer(dev);
+
+	if (dev->power.request_pending) {
+		/*
+		 * Pending resume requests take precedence over us, but we can
+		 * overtake any other pending request.

drivers/base/power/runtime.c line 493

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 */
+		if (dev->power.request == RPM_REQ_RESUME)
+			retval = -EAGAIN;
+		else if (dev->power.request != RPM_REQ_SUSPEND)
+			dev->power.request = retval ?
+						RPM_REQ_NONE : RPM_REQ_SUSPEND;

Now there's no need to check retval.

drivers/base/power/runtime.c line 496

Quoted 3 lines

drivers/base/power/runtime.c new file +3 -0

+
+		if (dev->power.request == RPM_REQ_SUSPEND)
+			return 0;

Just simply:

		return retval;

Some of these cases can't happen. For instance, if we reach here then the status can't be SUSPENDED or SUSPENDING, so there can't be a pending resume request.

drivers/base/power/runtime.c line 65

Quoted 4 lines

drivers/base/power/runtime.c new file +4 -0

+	}
+
+	if (retval)
+		return retval;

Now this isn't needed. Similar code rearrangements can be made in __pm_request_resume.

Quoted 1 line

+int __pm_request_resume(struct device *dev)

Should be static.

drivers/base/power/runtime.c line 710

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;
+	int error = 0;
+
+	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+		return -EINVAL;

This should go inside the spinlocked area.

Quoted 38 lines

drivers/base/power/runtime.c new file +38 -0

+	spin_lock_irqsave(&dev->power.lock, flags);
+
+	if (!dev->power.runtime_failure && !dev->power.disable_depth)
+		goto out;
+
+	if (dev->power.runtime_status == status)
+		goto out_clear;
+
+	if (status == RPM_SUSPENDED) {
+		/* It always is possible to set the status to 'suspended'. */
+		if (parent)
+			atomic_add_unless(&parent->power.child_count, -1, 0);
+		dev->power.runtime_status = status;
+		goto out_clear;
+	}
+
+	if (parent) {
+		spin_lock_irq(&parent->power.lock);
+
+		/*
+		 * It is invalid to put an active child under a parent that is
+		 * not active, has run-time PM enabled and the
+		 * 'power.ignore_children' flag unset.
+		 */
+		if (!parent->power.disable_depth
+		    && !parent->power.ignore_children
+		    && parent->power.runtime_status != RPM_ACTIVE) {
+			error = -EBUSY;
+		} else {
+			if (dev->power.runtime_status == RPM_SUSPENDED)
+				atomic_inc(&parent->power.child_count);
+			dev->power.runtime_status = status;
+		}
+
+		spin_unlock_irq(&parent->power.lock);
+
+		if (error)
+			goto out;

drivers/base/power/runtime.c line 755

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	} else {
+		dev->power.runtime_status = status;
+	}
+
+ out_clear:
+	dev->power.runtime_failure = false;

Move all those assignments to dev->power.runtime_status down here.

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

...

drivers/base/power/runtime.c line 829

Quoted 5 lines

drivers/base/power/runtime.c new file +5 -0

+	if (dev->power.disable_depth++ > 0)
+		goto out;
+
+	if (dev->power.runtime_failure)
+		goto out;

I don't see why this is needed.

Quoted 7 lines

drivers/base/power/runtime.c new file +7 -0

+
+	pm_runtime_deactivate_timer(dev);
+
+	if (dev->power.request_pending) {
+		dev->power.request = RPM_REQ_NONE;
+
+		spin_unlock_irq(&dev->power.lock);

drivers/base/power/runtime.c line 842

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+
+		cancel_work_sync(&dev->power.work);
+
+		spin_lock_irq(&dev->power.lock);
+
+		dev->power.request_pending = false;

Remove excessive whitespace.

Quoted 37 lines

drivers/base/power/runtime.c new file +37 -0

+	}
+
+	if (dev->power.runtime_status == RPM_SUSPENDING
+	    || dev->power.runtime_status == RPM_RESUMING) {
+		DEFINE_WAIT(wait);
+
+		/* Suspend or wake-up in progress. */
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (dev->power.runtime_status != RPM_SUSPENDING
+			    && dev->power.runtime_status != RPM_RESUMING)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+
+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}
+
+	if (dev->power.runtime_failure)
+		goto out;
+
+	if (dev->power.idle_notification) {
+		DEFINE_WAIT(wait);
+
+		for (;;) {
+			prepare_to_wait(&dev->power.wait_queue, &wait,
+					TASK_UNINTERRUPTIBLE);
+			if (!dev->power.idle_notification)
+				break;
+
+			spin_unlock_irq(&dev->power.lock);
+

drivers/base/power/runtime.c line 174

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+			schedule();
+
+			spin_lock_irq(&dev->power.lock);
+		}
+		finish_wait(&dev->power.wait_queue, &wait);
+	}

This wait loop should be merged with the previous loop.

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+void pm_runtime_init(struct device *dev)
+{
+	spin_lock_init(&dev->power.lock);
+
+	dev->power.runtime_status = RPM_SUSPENDED;
+	dev->power.idle_notification = false;

drivers/base/power/runtime.c line 909

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+
+	dev->power.disable_depth = 1;
+	atomic_set(&dev->power.usage_count, 0);
+
+	dev->power.runtime_failure = false;
+	dev->power.last_error = 0;

You don't have to set values to 0; they are initialized by kzalloc.

drivers/base/power/runtime.c line 922

Quoted 3 lines

drivers/base/power/runtime.c new file +3 -0

+	dev->power.suspend_timer.expires = jiffies;
+	dev->power.suspend_timer.data = (unsigned long)dev;
+	dev->power.suspend_timer.function = pm_suspend_timer_fn;

Use setup_timer() instead of assigning these fields directly.

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+
+	if (dev->power.runtime_status == RPM_ACTIVE) {
+		struct device *parent = dev->parent;
+
+		/*

drivers/base/power/runtime.c line 944

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 * Change the status back to 'suspended' to match the initial
+		 * status.
+		 */
+		pm_runtime_set_suspended(dev);
+		if (parent && !parent->power.ignore_children)
+			pm_request_idle(parent);

Shouldn't these last two lines be part of __pm_runtime_set_status()?

include/linux/pm_runtime.h line 20

Quoted 5 lines

include/linux/pm_runtime.h new file +2 -0

 
+extern void pm_runtime_init(struct device *dev);
+extern void pm_runtime_remove(struct device *dev);

I don't like seeing these two functions included in the public header file. It's enough to put them in drivers/base/power/power.h.

Quoted 23 lines

drivers/base/power/main.c +10 -0

@@ -49,6 +50,16 @@ static DEFINE_MUTEX(dpm_list_mtx);
 static bool transition_started;
 
 /**
+ * device_pm_init - Initialize the PM-related part of a device object
+ * @dev: Device object to initialize.
+ */
+void device_pm_init(struct device *dev)
+{
+	dev->power.status = DPM_ON;
+	pm_runtime_init(dev);
+}
+
+/**
  *	device_pm_lock - lock the list of active devices used by the PM core
  */
 void device_pm_lock(void)

@@ -105,6 +116,8 @@ void device_pm_remove(struct device *dev
 	mutex_lock(&dpm_list_mtx);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);

drivers/base/power/main.c void device_pm_remove(struct device *dev · line 121

Quoted 3 lines

drivers/base/power/main.c +2 -0

+
+	pm_runtime_remove(dev);
 }

Calling pm_runtime_init() from device_pm_init() and pm_runtime_remove() from device_pm_remove() isn't good. If CONFIG_PM_SLEEP isn't enabled then the calls won't be compiled, even if CONFIG_PM_RUNTIME is set.

drivers/base/power/main.c static int dpm_prepare(pm_message_t stat · line 775

Quoted 7 lines

drivers/base/power/main.c +2 -1

@@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
 		dev->power.status = DPM_PREPARING;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_prepare(dev, state);
+		if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+			error = -EBUSY;

What's the reason for the -EBUSY error?

Quoted 6 lines

drivers/base/dd.c +0 -0

@@ -202,7 +203,9 @@ 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);
 

drivers/base/dd.c int driver_probe_device(struct device_dr · line 208

Quoted 3 lines

drivers/base/dd.c +2 -0

+	pm_runtime_get_noresume(dev);
 	ret = really_probe(dev, drv);
+	pm_runtime_put_noidle(dev);

This is bad because it won't wait if there's a runtime-PM call in progress. Also, we shouldn't use put_noidle because it might subvert the driver's attempt to autosuspend.

Instead we should do something like this:

	/* Wait for runtime PM calls to finish and prevent new calls
	 * until the probe is done.
	 */
	pm_runtime_disable(dev);
	pm_runtime_get_noresume(dev);
	pm_runtime_enable(dev):
	ret = really_probe(dev, drv);
	pm_runtime_put_sync(dev);

Documentation/power/runtime_pm.txt

Quoted 7 lines

Documentation/power/runtime_pm.txt new file +3 -0

 
+2. Device Run-time PM Callbacks
 
+In particular, it is recommended that ->runtime_suspend() return -EBUSY if
+device_may_wakeup() returns 'false' for the device.

What's the point of this? I don't understand -- we don't want to discourage people from suspending devices with wakeup enabled.

Documentation/power/runtime_pm.txt line 148

Quoted 6 lines

Documentation/power/runtime_pm.txt new file +6 -0

+Additionally, the helper functions provided by the PM core obey the following
+rules:
+
+  * If ->runtime_suspend() is about to be executed or the execution of it is
+    scheduled or there's a pending request to execute it, ->runtime_idle() will
+    not be executed for the same device.

Shouldn't we allow runtime_idle when a suspend is scheduled? The idle handler might decide to suspend right away instead of waiting for the timer to expire.

Documentation/power/runtime_pm.txt line 266

Quoted 9 lines

+4. Run-time PM Device Helper Functions

Documentation/power/runtime_pm.txt new file +7 -0

+
+The following run-time PM helper functions are defined in
+drivers/base/power/runtime.c and include/linux/pm_runtime.h:
 
+  int pm_schedule_suspend(struct device *dev, unsigned int delay);
+    - schedule the execution of ->runtime_suspend() for the device's bus type
+      in future, where 'delay' is the time to wait before queuing up a suspend
+      work item in pm_wq, in miliseconds (if 'delay' is zero, the work item is

Fix spelling of "milli".

Explain that the new delay will override the
old one if a suspend was already scheduled and not yet expired.

Documentation/power/runtime_pm.txt line 285

Quoted 2 lines

Documentation/power/runtime_pm.txt new file +2 -0

+  int pm_runtime_get_sync(struct device *dev);
+    - increment the device's usage counter, run pm_rutime_resume(dev) and return

Fix spelling of "runtime". Same under pm_runtime_put_sync.

Quoted 9 lines

drivers/base/power/runtime.c line 85

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		dev->bus->pm->runtime_idle(dev);
+
+	spin_lock_irq(&dev->power.lock);

Small optimization: Put the spin_{un}lock_irq stuff inside the "if" statement, so it doesn't happen if the test fails.

Well, I don't think so. We need to take the lock here unconditionally, because the caller is going to unlock it.

Quoted 1 line

The same thing can be done in other places.

I'm not really sure it can.

Quoted 10 lines

drivers/base/power/runtime.c line 710

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;
+	int error = 0;
+
+	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+		return -EINVAL;

This should go inside the spinlocked area.

Why? 'status' is a function argument, it doesn't need to be protected from concurrent modification.

Quoted 10 lines

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

...

drivers/base/power/runtime.c line 829

Quoted 5 lines

drivers/base/power/runtime.c new file +5 -0

+	if (dev->power.disable_depth++ > 0)
+		goto out;
+
+	if (dev->power.runtime_failure)
+		goto out;

I don't see why this is needed.

If dev->power.runtime_failure, there's no need to do anything more.

Quoted 8 lines

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+
+	if (dev->power.runtime_status == RPM_ACTIVE) {
+		struct device *parent = dev->parent;
+
+		/*

Quoted 8 lines

drivers/base/power/runtime.c line 944

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 * Change the status back to 'suspended' to match the initial
+		 * status.
+		 */
+		pm_runtime_set_suspended(dev);
+		if (parent && !parent->power.ignore_children)
+			pm_request_idle(parent);

Shouldn't these last two lines be part of __pm_runtime_set_status()?

No. It is valid to call __pm_runtime_set_status() when runtime PM is disabled for the device and I don't think we should kick the parent in such cases.

Quoted 9 lines

drivers/base/power/main.c static int dpm_prepare(pm_message_t stat · line 775

Quoted 7 lines

drivers/base/power/main.c +2 -1

@@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
 		dev->power.status = DPM_PREPARING;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_prepare(dev, state);
+		if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+			error = -EBUSY;

What's the reason for the -EBUSY error?

If this is a wake-up device and pm_runtime_disable(dev) returned 1 (it can only return 1 or 0), which means there was a resume request pending for the device, suspend fails with -EBUSY (wake-up event during suspend).

Quoted 11 lines

drivers/base/dd.c int driver_probe_device(struct device_dr · line 208

Quoted 7 lines

drivers/base/dd.c +2 -0

@@ -202,7 +203,9 @@ 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_get_noresume(dev);
 	ret = really_probe(dev, drv);
+	pm_runtime_put_noidle(dev);

This is bad because it won't wait if there's a runtime-PM call in progress. Also, we shouldn't use put_noidle because it might subvert the driver's attempt to autosuspend.

I'm not sure how that's possible, but whatever.

Quoted 7 lines

Documentation/power/runtime_pm.txt

Quoted 4 lines

Documentation/power/runtime_pm.txt new file +3 -0

+2. Device Run-time PM Callbacks
 
+In particular, it is recommended that ->runtime_suspend() return -EBUSY if
+device_may_wakeup() returns 'false' for the device.

What's the point of this? I don't understand -- we don't want to discourage people from suspending devices with wakeup enabled.

device_may_wakeup(dev) == false means wake-up is disabled for dev, so suspending it might not be a good idea.

Quoted 9 lines

drivers/base/power/runtime.c line 85

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		dev->bus->pm->runtime_idle(dev);
+
+	spin_lock_irq(&dev->power.lock);

Small optimization: Put the spin_{un}lock_irq stuff inside the "if" statement, so it doesn't happen if the test fails.

Well, I don't think so. We need to take the lock here unconditionally, because the caller is going to unlock it.

Quoted 1 line

The same thing can be done in other places.

I'm not really sure it can.

Quoted 10 lines

drivers/base/power/runtime.c line 710

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;
+	int error = 0;
+
+	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+		return -EINVAL;

This should go inside the spinlocked area.

Why? 'status' is a function argument, it doesn't need to be protected from concurrent modification.

Quoted 10 lines

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

...

drivers/base/power/runtime.c line 829

Quoted 5 lines

drivers/base/power/runtime.c new file +5 -0

+	if (dev->power.disable_depth++ > 0)
+		goto out;
+
+	if (dev->power.runtime_failure)
+		goto out;

I don't see why this is needed.

If dev->power.runtime_failure, there's no need to do anything more.

Quoted 8 lines

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+
+	if (dev->power.runtime_status == RPM_ACTIVE) {
+		struct device *parent = dev->parent;
+
+		/*

Quoted 8 lines

drivers/base/power/runtime.c line 944

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 * Change the status back to 'suspended' to match the initial
+		 * status.
+		 */
+		pm_runtime_set_suspended(dev);
+		if (parent && !parent->power.ignore_children)
+			pm_request_idle(parent);

Shouldn't these last two lines be part of __pm_runtime_set_status()?

No. It is valid to call __pm_runtime_set_status() when runtime PM is disabled for the device and I don't think we should kick the parent in such cases.

Quoted 9 lines

drivers/base/power/main.c static int dpm_prepare(pm_message_t stat · line 775

Quoted 7 lines

drivers/base/power/main.c +2 -1

@@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
 		dev->power.status = DPM_PREPARING;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_prepare(dev, state);
+		if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+			error = -EBUSY;

What's the reason for the -EBUSY error?

If this is a wake-up device and pm_runtime_disable(dev) returned 1 (it can only return 1 or 0), which means there was a resume request pending for the device, suspend fails with -EBUSY (wake-up event during suspend).

Quoted 11 lines

drivers/base/dd.c int driver_probe_device(struct device_dr · line 208

Quoted 7 lines

drivers/base/dd.c +2 -0

@@ -202,7 +203,9 @@ 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_get_noresume(dev);
 	ret = really_probe(dev, drv);
+	pm_runtime_put_noidle(dev);

This is bad because it won't wait if there's a runtime-PM call in progress. Also, we shouldn't use put_noidle because it might subvert the driver's attempt to autosuspend.

I'm not sure how that's possible, but whatever.

Quoted 7 lines

Documentation/power/runtime_pm.txt

Quoted 4 lines

Documentation/power/runtime_pm.txt new file +3 -0

+2. Device Run-time PM Callbacks
 
+In particular, it is recommended that ->runtime_suspend() return -EBUSY if
+device_may_wakeup() returns 'false' for the device.

What's the point of this? I don't understand -- we don't want to discourage people from suspending devices with wakeup enabled.

device_may_wakeup(dev) == false means wake-up is disabled for dev, so suspending it might not be a good idea.

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines

drivers/base/power/runtime.c line 85

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+	spin_unlock_irq(&dev->power.lock);
+
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		dev->bus->pm->runtime_idle(dev);
+
+	spin_lock_irq(&dev->power.lock);

Small optimization: Put the spin_{un}lock_irq stuff inside the "if" statement, so it doesn't happen if the test fails.

Quoted 2 lines

Well, I don't think so. We need to take the lock here unconditionally, because the caller is going to unlock it.

No, I meant do this:

	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
		spin_unlock_irq(&dev->power.lock);
		dev->bus->pm->runtime_idle(dev);
		spin_lock_irq(&dev->power.lock);
	}

By the way, I don't know if anyone still pays attention to sparse-type annotations. If you do,

you should add "__releases(&dev->power.lock)"
and "__acquires(&dev->power.lock)" annotations to functions like this, which release the lock without first acquiring it, and then acquire the lock without releasing before returning.

Quoted 3 lines

Quoted 1 line

The same thing can be done in other places.

I'm not really sure it can.

The other places aren't quite the same as this. I'll leave it to your discretion. :-)

Quoted 11 lines

Quoted 10 lines

drivers/base/power/runtime.c line 710

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;
+	int error = 0;
+
+	if (status != RPM_ACTIVE && status != RPM_SUSPENDED)
+		return -EINVAL;

This should go inside the spinlocked area.

Quoted 2 lines

Why? 'status' is a function argument, it doesn't need to be protected from concurrent modification.

Oops, my mistake. Never mind...

Quoted 11 lines

Quoted 10 lines

Quoted 2 lines

drivers/base/power/runtime.c new file +2 -0

+int pm_runtime_disable(struct device *dev)
+{

...

drivers/base/power/runtime.c line 829

Quoted 5 lines

drivers/base/power/runtime.c new file +5 -0

+	if (dev->power.disable_depth++ > 0)
+		goto out;
+
+	if (dev->power.runtime_failure)
+		goto out;

I don't see why this is needed.

Quoted 1 line

If dev->power.runtime_failure, there's no need to do anything more.

Don't you still want to deactivate the timer and kill any pending requests? True, they won't be able to do anything until the failure state is cleared, but even so...

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

drivers/base/power/runtime.c new file +8 -0

+void pm_runtime_remove(struct device *dev)
+{
+	pm_runtime_disable(dev);
+
+	if (dev->power.runtime_status == RPM_ACTIVE) {
+		struct device *parent = dev->parent;
+
+		/*

Quoted 8 lines

drivers/base/power/runtime.c line 944

Quoted 6 lines

drivers/base/power/runtime.c new file +6 -0

+		 * Change the status back to 'suspended' to match the initial
+		 * status.
+		 */
+		pm_runtime_set_suspended(dev);
+		if (parent && !parent->power.ignore_children)
+			pm_request_idle(parent);

Shouldn't these last two lines be part of __pm_runtime_set_status()?

Quoted 2 lines

No. It is valid to call __pm_runtime_set_status() when runtime PM is disabled for the device and I don't think we should kick the parent in such cases.

Ah, an interesting point. Suppose the device is in RPM_ACTIVE, and then someone calls pm_runtime_disable followed by pm_runtime_set_suspended. Then the device's status would change to RPM_SUSPENDED and the parent's count of active children would be decremented, perhaps to 0. If the count does go to 0, why wouldn't you want to send out an idle notification for the parent?

Quoted 10 lines

Quoted 9 lines

drivers/base/power/main.c static int dpm_prepare(pm_message_t stat · line 775

Quoted 7 lines

drivers/base/power/main.c +2 -1

@@ -757,11 +771,15 @@ static int dpm_prepare(pm_message_t stat
 		dev->power.status = DPM_PREPARING;
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_prepare(dev, state);
+		if (pm_runtime_disable(dev) && device_may_wakeup(dev))
+			error = -EBUSY;

What's the reason for the -EBUSY error?

Quoted 3 lines

If this is a wake-up device and pm_runtime_disable(dev) returned 1 (it can only return 1 or 0), which means there was a resume request pending for the device, suspend fails with -EBUSY (wake-up event during suspend).

I see. That's a little obscure; a comment would help. Even something as simple as:

error = -EBUSY; /* wake-up during suspend */

Quoted 12 lines

Quoted 11 lines

drivers/base/dd.c int driver_probe_device(struct device_dr · line 208

Quoted 7 lines

drivers/base/dd.c +2 -0

@@ -202,7 +203,9 @@ 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_get_noresume(dev);
 	ret = really_probe(dev, drv);
+	pm_runtime_put_noidle(dev);

This is bad because it won't wait if there's a runtime-PM call in progress. Also, we shouldn't use put_noidle because it might subvert the driver's attempt to autosuspend.

Quoted 1 line

I'm not sure how that's possible, but whatever.

Suppose the probe routine does:

	pm_runtime_get_sync(dev);
	/* do some work */
	pm_runtime_put_sync(dev);

There is a clear expectation that an idle notification will eventually be sent. But if the probe is surrounded by

	pm_runtime_get_noresume(dev);
	... probe ...
	pm_runtime_put_noidle(dev);

then there won't be any idle notifications.

Quoted 10 lines

Quoted 7 lines

Documentation/power/runtime_pm.txt

Quoted 4 lines

Documentation/power/runtime_pm.txt new file +3 -0

+2. Device Run-time PM Callbacks
 
+In particular, it is recommended that ->runtime_suspend() return -EBUSY if
+device_may_wakeup() returns 'false' for the device.

What's the point of this? I don't understand -- we don't want to discourage people from suspending devices with wakeup enabled.

device_may_wakeup(dev) == false means wake-up is disabled for dev, so suspending it might not be a good idea.

Okay. This needs to be rephrased. For example,

In particular, if the driver requires remote wakeup capability for proper functioning and device_may_wakeup() returns 'false' for the device, then ->runtime_suspend() should return -EBUSY.

The point is that plenty of drivers can work perfectly well without remote wakeup, so they have no reason to check device_may_wakeup().

Hi,

The patch below should address all of your recent comments.

Additionally I changed a few bits that I thought could turn out to be problematic at one point.

+/**
+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be suspended and run the ->runtime_suspend() callback
+ * provided by its bus type. If another suspend has been started earlier, wait
+ * for it to finish. If there's an idle notification pending, cancel it. If
+ * there's a suspend request scheduled while this function is running and @sync
+ * is 'true', cancel that request.

+ *
+ * This function must be called under dev->power.lock with interrupts disabled.
+ */

drivers/base/power/runtime.c new file +3 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+	__releases(&dev->power.lock) __acquires(&dev->power.lock)
+{

+/**
+ * __pm_runtime_resume - Carry out run-time resume of given device.
+ * @dev: Device to resume.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be woken up and run the ->runtime_resume() callback
+ * provided by its bus type. If another resume has been started earlier, wait
+ * for it to finish. If there's a suspend running in parallel with this
+ * function, wait for it to finish and resume the device. If there's a suspend
+ * request or idle notification pending, cancel it. If there's a resume request
+ * scheduled while this function is running, cancel that request.

drivers/base/power/runtime.c new file +4 -0

+	if (!parent && dev->parent) {
+		/*
+		 * Increment the parent's resume counter and resume it if
+		 * necessary.

drivers/base/power/runtime.c new file +4 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;

Documentation/power/runtime_pm.txt new file +1 -0

+5. Run-time PM Initialization

+If the defaul initial run-time PM status of the device (i.e. 'suspended')

+is successful.

Hi,

The patch below should address all of your recent comments.

Additionally I changed a few bits that I thought could turn out to be problematic at one point.

+/**
+ * __pm_runtime_suspend - Carry out run-time suspend of given device.
+ * @dev: Device to suspend.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be suspended and run the ->runtime_suspend() callback
+ * provided by its bus type. If another suspend has been started earlier, wait
+ * for it to finish. If there's an idle notification pending, cancel it. If
+ * there's a suspend request scheduled while this function is running and @sync
+ * is 'true', cancel that request.

+ *
+ * This function must be called under dev->power.lock with interrupts disabled.
+ */

drivers/base/power/runtime.c new file +3 -0

+int __pm_runtime_suspend(struct device *dev, bool from_wq)
+	__releases(&dev->power.lock) __acquires(&dev->power.lock)
+{

+/**
+ * __pm_runtime_resume - Carry out run-time resume of given device.
+ * @dev: Device to resume.
+ * @from_wq: If set, the function has been called via pm_wq.
+ *
+ * Check if the device can be woken up and run the ->runtime_resume() callback
+ * provided by its bus type. If another resume has been started earlier, wait
+ * for it to finish. If there's a suspend running in parallel with this
+ * function, wait for it to finish and resume the device. If there's a suspend
+ * request or idle notification pending, cancel it. If there's a resume request
+ * scheduled while this function is running, cancel that request.

drivers/base/power/runtime.c new file +4 -0

+	if (!parent && dev->parent) {
+		/*
+		 * Increment the parent's resume counter and resume it if
+		 * necessary.

drivers/base/power/runtime.c new file +4 -0

+int __pm_runtime_set_status(struct device *dev, unsigned int status)
+{
+	struct device *parent = dev->parent;
+	unsigned long flags;