[PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Magnus Damm]

PM: Runtime PM v13 for Platform Devices 20090807

[PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers
[PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed
[PATCH 03/05] PM: Runtime PM v13 - add debug printouts
[PATCH 04/05] PM: Runtime PM v13 - CONFIG_PM_SLEEP=n support
[PATCH 05/05] PM: Runtime PM v13 - platform device bus support

These patches contain architecture independent changes for Runtime PM
and are meant to be applied on top of: 
"PM: Introduce core framework for run-time PM of I/O devices (rev. 13)"
by Rafael J. Wysocki.

Last week patches for v11 were posted, and since then the main change is
the updated and included platform device bus patch.

I would like to see at least part of this series rolled in to a future
Runtime PM release (v14?) or if not then some feedback on how to 
integrate the generic platform bus code with the Runtime PM code in
a better way. My Runtime PM implementation for SuperH Mobile builds on
top of this, so at least [01/05], [02/05] and [05/05] are needed.

Please see each individual patch for more information. Thank you!

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 drivers/base/core.c          |    3 +
 drivers/base/platform.c      |   48 +++++++++++++++++++++++++------
 drivers/base/power/main.c    |    2 -
 drivers/base/power/runtime.c |   65 ++++++++++++++++++++++--------------------
 include/linux/pm_runtime.h   |   19 ++++++++++++
 5 files changed, 96 insertions(+), 41 deletions(-)

[PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch breaks out the dev_pm_ops handling code
for Runtime PM into separate functions. This change
makes is possible for the main code and separate
bus implementations to deal with callbacks and error
codes in a consistent fashion.

The broken out functions are used by the Runtime PM
platform device bus implementation for SuperH.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Changes for v11->v13:
 - solved conflicts introduced by v11->v13 update

 drivers/base/power/runtime.c |   29 ++++++++++-------------------
 include/linux/pm_runtime.h   |   19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 19 deletions(-)

--- 0007/drivers/base/power/runtime.c
+++ work/drivers/base/power/runtime.c	2009-08-07 13:42:57.000000000 +0900
@@ -77,13 +77,12 @@ static int __pm_runtime_idle(struct devi
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irq(&dev->power.lock);
 
-		dev->bus->pm->runtime_idle(dev);
+	if (dev->bus)
+		pm_runtime_ops_idle(dev, dev->bus->pm);
 
-		spin_lock_irq(&dev->power.lock);
-	}
+	spin_lock_irq(&dev->power.lock);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
@@ -173,15 +172,11 @@ int __pm_runtime_suspend(struct device *
 
 	dev->power.runtime_status = RPM_SUSPENDING;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irq(&dev->power.lock);
 
-		retval = dev->bus->pm->runtime_suspend(dev);
+	retval = dev->bus ? pm_runtime_ops_suspend(dev, dev->bus->pm) : -ENOSYS;
 
-		spin_lock_irq(&dev->power.lock);
-	} else {
-		retval = -ENOSYS;
-	}
+	spin_lock_irq(&dev->power.lock);
 
 	if (retval) {
 		dev->power.runtime_status = RPM_ACTIVE;
@@ -332,15 +327,11 @@ int __pm_runtime_resume(struct device *d
 
 	dev->power.runtime_status = RPM_RESUMING;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irq(&dev->power.lock);
 
-		retval = dev->bus->pm->runtime_resume(dev);
+	retval = dev->bus ? pm_runtime_ops_resume(dev, dev->bus->pm) : -ENOSYS;
 
-		spin_lock_irq(&dev->power.lock);
-	} else {
-		retval = -ENOSYS;
-	}
+	spin_lock_irq(&dev->power.lock);
 
 	if (retval) {
 		dev->power.runtime_status = RPM_SUSPENDED;
--- 0007/include/linux/pm_runtime.h
+++ work/include/linux/pm_runtime.h	2009-08-07 13:37:27.000000000 +0900
@@ -112,4 +112,23 @@ static inline int pm_runtime_disable(str
 	return __pm_runtime_disable(dev, true);
 }
 
+static inline int pm_runtime_ops_suspend(struct device *dev,
+					 struct dev_pm_ops *p)
+{
+	return p && p->runtime_suspend ? p->runtime_suspend(dev) : -ENOSYS;
+}
+
+static inline int pm_runtime_ops_resume(struct device *dev,
+					struct dev_pm_ops *p)
+{
+	return p && p->runtime_resume ? p->runtime_resume(dev) : -ENOSYS;
+}
+
+static inline void pm_runtime_ops_idle(struct device *dev,
+				       struct dev_pm_ops *p)
+{
+	if (p && p->runtime_idle)
+		p->runtime_idle(dev);
+}
+
 #endif

[PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch allows bus-less devices to pass as being
suspended even though there is no dev_pm_ops structure
to use to get the Runtime PM callbacks.

Workarounds the fact that "struct device platform_bus"
becomes the parent of otherwise parent-less platform
devices. Runtime suspend and resume of devices on the
platform bus is impossible without this change. It may
however be better to modify the actual struct device
than working around things in the Runtime PM core code.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Please let me know if this Runtime PM core change is acceptable.

 Changes for v11->v13:
 - none

 drivers/base/power/runtime.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- 0008/drivers/base/power/runtime.c
+++ work/drivers/base/power/runtime.c	2009-08-07 13:45:22.000000000 +0900
@@ -174,7 +174,7 @@ int __pm_runtime_suspend(struct device *
 
 	spin_unlock_irq(&dev->power.lock);
 
-	retval = dev->bus ? pm_runtime_ops_suspend(dev, dev->bus->pm) : -ENOSYS;
+	retval = dev->bus ? pm_runtime_ops_suspend(dev, dev->bus->pm) : 0;
 
 	spin_lock_irq(&dev->power.lock);
 
@@ -329,7 +329,7 @@ int __pm_runtime_resume(struct device *d
 
 	spin_unlock_irq(&dev->power.lock);
 
-	retval = dev->bus ? pm_runtime_ops_resume(dev, dev->bus->pm) : -ENOSYS;
+	retval = dev->bus ? pm_runtime_ops_resume(dev, dev->bus->pm) : 0;
 
 	spin_lock_irq(&dev->power.lock);
 

[PATCH 03/05] PM: Runtime PM v13 - add debug printouts

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch adds dev_dbg() printouts to the Runtime PM
code. Just add a #define DEBUG at the top of the file
to get suspend and resume printouts. Also add a missing
newline to the pm_runtime_enable() printout while at it.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Can this patch be rolled into the main Runtime PM patch?

 Changes for v11->v13:
 - solved conflicts introduced by v11->v13 update

 drivers/base/power/runtime.c |   32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

--- 0009/drivers/base/power/runtime.c
+++ work/drivers/base/power/runtime.c	2009-08-07 13:55:20.000000000 +0900
@@ -125,13 +125,19 @@ int __pm_runtime_suspend(struct device *
 	bool notify = false;
 	int retval = 0;
 
+	dev_dbg(dev, "__pm_runtime_suspend() %d!\n", from_wq);
  repeat:
-	if (dev->power.runtime_error)
-		return -EINVAL;
+	if (dev->power.runtime_error) {
+		retval = -EINVAL;
+		goto out;
+	}
 
 	/* Pending resume requests take precedence over us. */
-	if (dev->power.request_pending && dev->power.request == RPM_REQ_RESUME)
-			return -EAGAIN;
+	if (dev->power.request_pending
+	    && dev->power.request == RPM_REQ_RESUME) {
+		retval = -EAGAIN;
+		goto out;
+	}
 
 	/* Other scheduled or pending requests need to be canceled. */
 	pm_runtime_cancel_pending(dev);
@@ -145,13 +151,15 @@ int __pm_runtime_suspend(struct device *
 	else if (!pm_children_suspended(dev))
 		retval = -EBUSY;
 	if (retval)
-		return retval;
+		goto out;
 
 	if (dev->power.runtime_status == RPM_SUSPENDING) {
 		DEFINE_WAIT(wait);
 
-		if (from_wq)
-			return -EINPROGRESS;
+		if (from_wq) {
+			retval = -EINPROGRESS;
+			goto out;
+		}
 
 		/* Wait for the other suspend running in parallel with us. */
 		for (;;) {
@@ -200,7 +208,8 @@ int __pm_runtime_suspend(struct device *
 	if (dev->power.deferred_resume) {
 		dev->power.deferred_resume = false;
 		__pm_runtime_resume(dev, false);
-		return -EAGAIN;
+		retval = -EAGAIN;
+		goto out;
 	}
 
 	if (notify)
@@ -213,6 +222,8 @@ int __pm_runtime_suspend(struct device *
 
 		spin_lock_irq(&dev->power.lock);
 	}
+ out:
+	dev_dbg(dev, "__pm_runtime_suspend() returns %d!\n", retval);
 
 	return retval;
 }
@@ -252,6 +263,7 @@ int __pm_runtime_resume(struct device *d
 	struct device *parent = NULL;
 	int retval = 0;
 
+	dev_dbg(dev, "__pm_runtime_resume() %d!\n", from_wq);
  repeat:
 	if (dev->power.runtime_error) {
 		retval = -EINVAL;
@@ -358,6 +370,8 @@ int __pm_runtime_resume(struct device *d
 		spin_lock_irq(&dev->power.lock);
 	}
 
+	dev_dbg(dev, "__pm_runtime_resume() returns %d!\n", retval);
+
 	return retval;
 }
 
@@ -795,7 +809,7 @@ void pm_runtime_enable(struct device *de
 	if (dev->power.disable_depth > 0)
 		dev->power.disable_depth--;
 	else
-		dev_warn(dev, "Unbalanced %s!", __func__);
+		dev_warn(dev, "Unbalanced %s!\n", __func__);
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 }

[PATCH 04/05] PM: Runtime PM v13 - CONFIG_PM_SLEEP=n support

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch contains a few fixes to allow Runtime PM
to be used even though suspend and hibernation are disabled
in the kconfig. Without this patch the Runtime PM code
compiles just fine with CONFIG_PM_SLEEP=n but the functions
pm_runtime_init() and pm_runtime_remove() never gets called
during runtime.

Not sure if CONFIG_PM_RUNTIME=y and CONFIG_PM_SLEEP=n really
is a valid combination, but I can easily imagine use cases
for Runtime PM without system suspend/hibernation support.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Can this patch be rolled into the main Runtime PM patch?

 Changes for v11->v13:
 - solved conflicts introduced by v11->v13 update

 drivers/base/core.c       |    3 +++
 drivers/base/power/main.c |    2 --
 2 files changed, 3 insertions(+), 2 deletions(-)

--- 0001/drivers/base/core.c
+++ work/drivers/base/core.c	2009-08-07 13:56:54.000000000 +0900
@@ -23,6 +23,7 @@
 #include <linux/semaphore.h>
 #include <linux/mutex.h>
 #include <linux/async.h>
+#include <linux/pm_runtime.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -555,6 +556,7 @@ void device_initialize(struct device *de
 	INIT_LIST_HEAD(&dev->devres_head);
 	device_init_wakeup(dev, 0);
 	device_pm_init(dev);
+	pm_runtime_init(dev);
 	set_dev_node(dev, -1);
 }
 
@@ -1063,6 +1065,7 @@ void device_del(struct device *dev)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
 	device_pm_remove(dev);
+	pm_runtime_remove(dev);
 	dpm_sysfs_remove(dev);
 	if (parent)
 		klist_del(&dev->p->knode_parent);
--- 0007/drivers/base/power/main.c
+++ work/drivers/base/power/main.c	2009-08-07 13:57:25.000000000 +0900
@@ -56,7 +56,6 @@ static bool transition_started;
 void device_pm_init(struct device *dev)
 {
 	dev->power.status = DPM_ON;
-	pm_runtime_init(dev);
 }
 
 /**
@@ -116,7 +115,6 @@ void device_pm_remove(struct device *dev
 	mutex_lock(&dpm_list_mtx);
 	list_del_init(&dev->power.entry);
 	mutex_unlock(&dpm_list_mtx);
-	pm_runtime_remove(dev);
 }
 
 /**

[PATCH 05/05] PM: Runtime PM v13 - platform device bus support

[Magnus Damm]

From: Magnus Damm <damm@igel.co.jp>

This patch adds default Runtime PM callbacks to the dev_pm_ops
belonging to the platform bus. The callbacks are weak symbols
that architecture specific code may override. Also, call
pm_runtime_enable() for the platform bus struct device.

Allows Runtime PM even though CONFIG_PM_SLEEP=n.

Signed-off-by: Magnus Damm <damm@igel.co.jp>
---

 Please let me know if this platform device core change is acceptable.

 Changes:
 - Based on "Driver Core: Runtime PM callbacks for the Platform Bus V2"
 - Reworked to handle CONFIG_PM_SLEEP=n case

 drivers/base/platform.c |   48 ++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

--- 0001/drivers/base/platform.c
+++ work/drivers/base/platform.c	2009-08-07 15:25:14.000000000 +0900
@@ -17,6 +17,7 @@
 #include <linux/bootmem.h>
 #include <linux/err.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 
 #include "base.h"
 
@@ -683,6 +684,13 @@ static void platform_pm_complete(struct 
 		drv->pm->complete(dev);
 }
 
+#else /* !CONFIG_PM_SLEEP */
+
+#define platform_pm_prepare		NULL
+#define platform_pm_complete		NULL
+
+#endif /* !CONFIG_PM_SLEEP */
+
 #ifdef CONFIG_SUSPEND
 
 static int platform_pm_suspend(struct device *dev)
@@ -925,6 +933,30 @@ static int platform_pm_restore_noirq(str
 
 #endif /* !CONFIG_HIBERNATION */
 
+#ifdef CONFIG_PM_RUNTIME
+
+int __weak platform_pm_runtime_suspend(struct device *dev)
+{
+	return -ENOSYS;
+};
+
+int __weak platform_pm_runtime_resume(struct device *dev)
+{
+	return -ENOSYS;
+};
+
+void __weak platform_pm_runtime_idle(struct device *dev)
+{
+};
+
+#else /* !CONFIG_PM_RUNTIME */
+
+#define platform_pm_runtime_suspend NULL
+#define platform_pm_runtime_resume NULL
+#define platform_pm_runtime_idle NULL
+
+#endif /* !CONFIG_PM_RUNTIME */
+
 static struct dev_pm_ops platform_dev_pm_ops = {
 	.prepare = platform_pm_prepare,
 	.complete = platform_pm_complete,
@@ -940,22 +972,17 @@ static struct dev_pm_ops platform_dev_pm
 	.thaw_noirq = platform_pm_thaw_noirq,
 	.poweroff_noirq = platform_pm_poweroff_noirq,
 	.restore_noirq = platform_pm_restore_noirq,
+	.runtime_suspend = platform_pm_runtime_suspend,
+	.runtime_resume = platform_pm_runtime_resume,
+	.runtime_idle = platform_pm_runtime_idle,
 };
 
-#define PLATFORM_PM_OPS_PTR	(&platform_dev_pm_ops)
-
-#else /* !CONFIG_PM_SLEEP */
-
-#define PLATFORM_PM_OPS_PTR	NULL
-
-#endif /* !CONFIG_PM_SLEEP */
-
 struct bus_type platform_bus_type = {
 	.name		= "platform",
 	.dev_attrs	= platform_dev_attrs,
 	.match		= platform_match,
 	.uevent		= platform_uevent,
-	.pm		= PLATFORM_PM_OPS_PTR,
+	.pm		= &platform_dev_pm_ops,
 };
 EXPORT_SYMBOL_GPL(platform_bus_type);
 
@@ -968,6 +995,9 @@ int __init platform_bus_init(void)
 	error = device_register(&platform_bus);
 	if (error)
 		return error;
+
+	pm_runtime_enable(&platform_bus);
+
 	error =  bus_register(&platform_bus_type);
 	if (error)
 		device_unregister(&platform_bus);

Re: [PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Alan Stern]

On Fri, 7 Aug 2009, Magnus Damm wrote:

> PM: Runtime PM v13 for Platform Devices 20090807
> 
> [PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers

This patch doesn't do anything much, besides reverting a change I asked 
Rafael to make.  I don't see how it helps platform-specific code do 
anything.

> [PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed

This could be added without 01/05.  But why do you want it?  Busless
devices don't have PM runtime callbacks, so whether the core thinks the
callbacks succeed or not doesn't make any difference.

You say that "Runtime suspend and resume of devices on the platform bus
is impossible without this change", but you don't explain why -- or why
the patch makes runtime suspend and resume of these devices possible.

> [PATCH 03/05] PM: Runtime PM v13 - add debug printouts

This looks good.

> [PATCH 04/05] PM: Runtime PM v13 - CONFIG_PM_SLEEP=n support

This isn't needed at all; equivalent functionality is already present 
in v13 of Runtime PM.

> [PATCH 05/05] PM: Runtime PM v13 - platform device bus support

This doesn't affect the Runtime PM patch, so it shouldn't be rolled in 
with it.  They should remain separate.

Alan Stern

Re: [PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Magnus Damm]

On Fri, Aug 7, 2009 at 11:32 PM, Alan Stern<stern@rowland.harvard.edu> wrote:
> On Fri, 7 Aug 2009, Magnus Damm wrote:
>
>> PM: Runtime PM v13 for Platform Devices 20090807
>>
>> [PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers
>
> This patch doesn't do anything much, besides reverting a change I asked
> Rafael to make.  I don't see how it helps platform-specific code do
> anything.

It's helping different bus types to implement the runtime part of the
dev_pm_ops in a consistent way. I suggest that all bus types should
return -ENOSYS if the callback is missing. And they can do so by using
the helper functions. The change is not platform specific, but my
latest SuperH platform Runtime PM prototype makes use of it.

The latest SuperH specific Runtime PM implementation require
dev_pm_ops even though there is no work to be done for the driver. The
code works in a sort of opt-in way at this point, so callbacks are
explicitly required. I'd like us to standardize on this behaviour if
possible, so runtime pm enabled platform drivers can be shared between
different platform bus implementations. So my LCDC platform driver
will work fine on both SuperH SoCs and ARM SoCs.

Sorry for reverting your change, but I couldn't see any clear benefit
with the v11->v13 lock-drop-inside-the-if-case change. Isn't it just
avoiding dropping the lock in the uncommon error case? Maybe I'm
misunderstanding.

>> [PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed
>
> This could be added without 01/05.  But why do you want it?  Busless
> devices don't have PM runtime callbacks, so whether the core thinks the
> callbacks succeed or not doesn't make any difference.
>
> You say that "Runtime suspend and resume of devices on the platform bus
> is impossible without this change", but you don't explain why -- or why
> the patch makes runtime suspend and resume of these devices possible.

Right now, in the standard upstream kernel all platform devices get
assigned a parent device unless one exists are registration time. The
shared parent device is parent-less. Since the Runtime PM code resumes
the parent before the child, the resume operation will fail because
there is no dev_pm_ops for the shared parent.

So I wonder which way that is the best to allow resuming platform
devices. Patch [02/05] is one way, but maybe there are more elegant
ways to handle it? Should the platform code be modified instead? If
so, how? I suppose root hubs for USB may have a similar issue, no?

>> [PATCH 03/05] PM: Runtime PM v13 - add debug printouts
>
> This looks good.

Thanks. Maybe it's a good plan to add similar printouts to other
functions as well?

>> [PATCH 04/05] PM: Runtime PM v13 - CONFIG_PM_SLEEP=n support
>
> This isn't needed at all; equivalent functionality is already present
> in v13 of Runtime PM.

Yep, I totally missed that one. Will drop.

>> [PATCH 05/05] PM: Runtime PM v13 - platform device bus support
>
> This doesn't affect the Runtime PM patch, so it shouldn't be rolled in
> with it.  They should remain separate.

Yeah, it should be kept separate. I still would like to hear an
ACK/NACK from someone if possible.

Thanks for your feedback!

/ magnus

Re: [PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Alan Stern]

On Sat, 8 Aug 2009, Magnus Damm wrote:

> On Fri, Aug 7, 2009 at 11:32 PM, Alan Stern<stern@rowland.harvard.edu> wrote:
> > On Fri, 7 Aug 2009, Magnus Damm wrote:
> >
> >> PM: Runtime PM v13 for Platform Devices 20090807
> >>
> >> [PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers
> >
> > This patch doesn't do anything much, besides reverting a change I asked
> > Rafael to make.  I don't see how it helps platform-specific code do
> > anything.
> 
> It's helping different bus types to implement the runtime part of the
> dev_pm_ops in a consistent way. I suggest that all bus types should
> return -ENOSYS if the callback is missing.

Doesn't Rafael's code already do this?

>  And they can do so by using
> the helper functions. The change is not platform specific, but my
> latest SuperH platform Runtime PM prototype makes use of it.
> 
> The latest SuperH specific Runtime PM implementation require
> dev_pm_ops even though there is no work to be done for the driver. The
> code works in a sort of opt-in way at this point, so callbacks are
> explicitly required. I'd like us to standardize on this behaviour if
> possible, so runtime pm enabled platform drivers can be shared between
> different platform bus implementations. So my LCDC platform driver
> will work fine on both SuperH SoCs and ARM SoCs.

I still don't see the connection.  Why are helper functions useful?

> Sorry for reverting your change, but I couldn't see any clear benefit
> with the v11->v13 lock-drop-inside-the-if-case change. Isn't it just
> avoiding dropping the lock in the uncommon error case? Maybe I'm
> misunderstanding.

Basically you are right, except for one thing: The error case might not
be so uncommon.  That's the benefit.

Rafael, along these lines, I suspect we might not want to go into an
error state if a runtime suspend fails because there is no callback
function.  Returning -ENOSYS to the caller is fine, but leave 
runtime_error set to 0.  Maybe do the same for runtime resume.


> >> [PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed
> >
> > This could be added without 01/05.  But why do you want it?  Busless
> > devices don't have PM runtime callbacks, so whether the core thinks the
> > callbacks succeed or not doesn't make any difference.
> >
> > You say that "Runtime suspend and resume of devices on the platform bus
> > is impossible without this change", but you don't explain why -- or why
> > the patch makes runtime suspend and resume of these devices possible.
> 
> Right now, in the standard upstream kernel all platform devices get
> assigned a parent device unless one exists are registration time. The
> shared parent device is parent-less. Since the Runtime PM code resumes
> the parent before the child, the resume operation will fail because
> there is no dev_pm_ops for the shared parent.

Not if the parent is disabled for runtime PM, which it is in this case, 
right?

> So I wonder which way that is the best to allow resuming platform
> devices. Patch [02/05] is one way, but maybe there are more elegant
> ways to handle it? Should the platform code be modified instead? If
> so, how? I suppose root hubs for USB may have a similar issue, no?

If necessary, I would suggest adding appropriate dummy runtime PM 
routines for that catch-all parent device.  But it may not be 
necessary.


> >> [PATCH 03/05] PM: Runtime PM v13 - add debug printouts
> >
> > This looks good.
> 
> Thanks. Maybe it's a good plan to add similar printouts to other
> functions as well?

Perhaps so.  When writing similar code for USB I found that lots of 
debugging printouts were needed all over the place, to get everything 
working right.  Once it was working, they were a nuisance.

Alan Stern

Re: [PATCH 03/05] PM: Runtime PM v13 - add debug printouts

[Rafael J. Wysocki]

On Friday 07 August 2009, Magnus Damm wrote:
> From: Magnus Damm <damm@igel.co.jp>
> 
> This patch adds dev_dbg() printouts to the Runtime PM
> code. Just add a #define DEBUG at the top of the file
> to get suspend and resume printouts. Also add a missing
> newline to the pm_runtime_enable() printout while at it.
> 
> Signed-off-by: Magnus Damm <damm@igel.co.jp>
> ---
> 
>  Can this patch be rolled into the main Runtime PM patch?

Yes, I've just folded it into the runtime PM patch, with some minor modifications.

Thanks,
Rafael


>  Changes for v11->v13:
>  - solved conflicts introduced by v11->v13 update
> 
>  drivers/base/power/runtime.c |   32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> --- 0009/drivers/base/power/runtime.c
> +++ work/drivers/base/power/runtime.c	2009-08-07 13:55:20.000000000 +0900
> @@ -125,13 +125,19 @@ int __pm_runtime_suspend(struct device *
>  	bool notify = false;
>  	int retval = 0;
>  
> +	dev_dbg(dev, "__pm_runtime_suspend() %d!\n", from_wq);
>   repeat:
> -	if (dev->power.runtime_error)
> -		return -EINVAL;
> +	if (dev->power.runtime_error) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
>  
>  	/* Pending resume requests take precedence over us. */
> -	if (dev->power.request_pending && dev->power.request == RPM_REQ_RESUME)
> -			return -EAGAIN;
> +	if (dev->power.request_pending
> +	    && dev->power.request == RPM_REQ_RESUME) {
> +		retval = -EAGAIN;
> +		goto out;
> +	}
>  
>  	/* Other scheduled or pending requests need to be canceled. */
>  	pm_runtime_cancel_pending(dev);
> @@ -145,13 +151,15 @@ int __pm_runtime_suspend(struct device *
>  	else if (!pm_children_suspended(dev))
>  		retval = -EBUSY;
>  	if (retval)
> -		return retval;
> +		goto out;
>  
>  	if (dev->power.runtime_status == RPM_SUSPENDING) {
>  		DEFINE_WAIT(wait);
>  
> -		if (from_wq)
> -			return -EINPROGRESS;
> +		if (from_wq) {
> +			retval = -EINPROGRESS;
> +			goto out;
> +		}
>  
>  		/* Wait for the other suspend running in parallel with us. */
>  		for (;;) {
> @@ -200,7 +208,8 @@ int __pm_runtime_suspend(struct device *
>  	if (dev->power.deferred_resume) {
>  		dev->power.deferred_resume = false;
>  		__pm_runtime_resume(dev, false);
> -		return -EAGAIN;
> +		retval = -EAGAIN;
> +		goto out;
>  	}
>  
>  	if (notify)
> @@ -213,6 +222,8 @@ int __pm_runtime_suspend(struct device *
>  
>  		spin_lock_irq(&dev->power.lock);
>  	}
> + out:
> +	dev_dbg(dev, "__pm_runtime_suspend() returns %d!\n", retval);
>  
>  	return retval;
>  }
> @@ -252,6 +263,7 @@ int __pm_runtime_resume(struct device *d
>  	struct device *parent = NULL;
>  	int retval = 0;
>  
> +	dev_dbg(dev, "__pm_runtime_resume() %d!\n", from_wq);
>   repeat:
>  	if (dev->power.runtime_error) {
>  		retval = -EINVAL;
> @@ -358,6 +370,8 @@ int __pm_runtime_resume(struct device *d
>  		spin_lock_irq(&dev->power.lock);
>  	}
>  
> +	dev_dbg(dev, "__pm_runtime_resume() returns %d!\n", retval);
> +
>  	return retval;
>  }
>  
> @@ -795,7 +809,7 @@ void pm_runtime_enable(struct device *de
>  	if (dev->power.disable_depth > 0)
>  		dev->power.disable_depth--;
>  	else
> -		dev_warn(dev, "Unbalanced %s!", __func__);
> +		dev_warn(dev, "Unbalanced %s!\n", __func__);
>  
>  	spin_unlock_irqrestore(&dev->power.lock, flags);
>  }
> 

Re: [PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Rafael J. Wysocki]

On Friday 07 August 2009, Alan Stern wrote:
> On Sat, 8 Aug 2009, Magnus Damm wrote:
> 
> > On Fri, Aug 7, 2009 at 11:32 PM, Alan Stern<stern@rowland.harvard.edu> wrote:
> > > On Fri, 7 Aug 2009, Magnus Damm wrote:
> > >
> > >> PM: Runtime PM v13 for Platform Devices 20090807
> > >>
> > >> [PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers
> > >
> > > This patch doesn't do anything much, besides reverting a change I asked
> > > Rafael to make.  I don't see how it helps platform-specific code do
> > > anything.
> > 
> > It's helping different bus types to implement the runtime part of the
> > dev_pm_ops in a consistent way. I suggest that all bus types should
> > return -ENOSYS if the callback is missing.
> 
> Doesn't Rafael's code already do this?
> 
> >  And they can do so by using
> > the helper functions. The change is not platform specific, but my
> > latest SuperH platform Runtime PM prototype makes use of it.
> > 
> > The latest SuperH specific Runtime PM implementation require
> > dev_pm_ops even though there is no work to be done for the driver. The
> > code works in a sort of opt-in way at this point, so callbacks are
> > explicitly required. I'd like us to standardize on this behaviour if
> > possible, so runtime pm enabled platform drivers can be shared between
> > different platform bus implementations. So my LCDC platform driver
> > will work fine on both SuperH SoCs and ARM SoCs.
> 
> I still don't see the connection.  Why are helper functions useful?
> 
> > Sorry for reverting your change, but I couldn't see any clear benefit
> > with the v11->v13 lock-drop-inside-the-if-case change. Isn't it just
> > avoiding dropping the lock in the uncommon error case? Maybe I'm
> > misunderstanding.
> 
> Basically you are right, except for one thing: The error case might not
> be so uncommon.  That's the benefit.
> 
> Rafael, along these lines, I suspect we might not want to go into an
> error state if a runtime suspend fails because there is no callback
> function.  Returning -ENOSYS to the caller is fine, but leave 
> runtime_error set to 0.  Maybe do the same for runtime resume.

Yes, I've just made this change.

> > >> [PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed
> > >
> > > This could be added without 01/05.  But why do you want it?  Busless
> > > devices don't have PM runtime callbacks, so whether the core thinks the
> > > callbacks succeed or not doesn't make any difference.
> > >
> > > You say that "Runtime suspend and resume of devices on the platform bus
> > > is impossible without this change", but you don't explain why -- or why
> > > the patch makes runtime suspend and resume of these devices possible.
> > 
> > Right now, in the standard upstream kernel all platform devices get
> > assigned a parent device unless one exists are registration time. The
> > shared parent device is parent-less. Since the Runtime PM code resumes
> > the parent before the child, the resume operation will fail because
> > there is no dev_pm_ops for the shared parent.
> 
> Not if the parent is disabled for runtime PM, which it is in this case, 
> right?
> 
> > So I wonder which way that is the best to allow resuming platform
> > devices. Patch [02/05] is one way, but maybe there are more elegant
> > ways to handle it? Should the platform code be modified instead? If
> > so, how? I suppose root hubs for USB may have a similar issue, no?
> 
> If necessary, I would suggest adding appropriate dummy runtime PM 
> routines for that catch-all parent device.  But it may not be 
> necessary.
> 
> 
> > >> [PATCH 03/05] PM: Runtime PM v13 - add debug printouts
> > >
> > > This looks good.
> > 
> > Thanks. Maybe it's a good plan to add similar printouts to other
> > functions as well?
> 
> Perhaps so.  When writing similar code for USB I found that lots of 
> debugging printouts were needed all over the place, to get everything 
> working right.  Once it was working, they were a nuisance.

I've added such messages to __pm_runtime_idle(), for consistency.

Thanks,
Rafael

Re: [PATCH 00/05] PM: Runtime PM v13 for Platform Devices 20090807

[Magnus Damm]

On Sat, Aug 8, 2009 at 1:17 AM, Alan Stern<stern@rowland.harvard.edu> wrote:
> On Sat, 8 Aug 2009, Magnus Damm wrote:
>
>> On Fri, Aug 7, 2009 at 11:32 PM, Alan Stern<stern@rowland.harvard.edu> wrote:
>> > On Fri, 7 Aug 2009, Magnus Damm wrote:
>> >
>> >> PM: Runtime PM v13 for Platform Devices 20090807
>> >>
>> >> [PATCH 01/05] PM: Runtime PM v13 - add dev_pm_ops helpers
>> >
>> > This patch doesn't do anything much, besides reverting a change I asked
>> > Rafael to make.  I don't see how it helps platform-specific code do
>> > anything.
>>
>> It's helping different bus types to implement the runtime part of the
>> dev_pm_ops in a consistent way. I suggest that all bus types should
>> return -ENOSYS if the callback is missing.
>
> Doesn't Rafael's code already do this?

Yes, but I wanted people writing bus level code to share this code. To
make sure that the code performing dev->driver->pm->runtime_xxx() is
somewhat consistent.

>>  And they can do so by using
>> the helper functions. The change is not platform specific, but my
>> latest SuperH platform Runtime PM prototype makes use of it.
>>
>> The latest SuperH specific Runtime PM implementation require
>> dev_pm_ops even though there is no work to be done for the driver. The
>> code works in a sort of opt-in way at this point, so callbacks are
>> explicitly required. I'd like us to standardize on this behaviour if
>> possible, so runtime pm enabled platform drivers can be shared between
>> different platform bus implementations. So my LCDC platform driver
>> will work fine on both SuperH SoCs and ARM SoCs.
>
> I still don't see the connection.  Why are helper functions useful?

To clearly show that -ENOSYS will be returned if
dev->bus->pm->runtime_xxx() is missing _or_ if
dev->driver->pm_runtime_xxx() is missing. This regardless of the bus
implementation.

It's not rocket science though, so I think it's just easier if I drop
this part as well.

>> >> [PATCH 02/05] PM: Runtime PM v13 - let bus-less devices succeed
>> >
>> > This could be added without 01/05.  But why do you want it?  Busless
>> > devices don't have PM runtime callbacks, so whether the core thinks the
>> > callbacks succeed or not doesn't make any difference.
>> >
>> > You say that "Runtime suspend and resume of devices on the platform bus
>> > is impossible without this change", but you don't explain why -- or why
>> > the patch makes runtime suspend and resume of these devices possible.
>>
>> Right now, in the standard upstream kernel all platform devices get
>> assigned a parent device unless one exists are registration time. The
>> shared parent device is parent-less. Since the Runtime PM code resumes
>> the parent before the child, the resume operation will fail because
>> there is no dev_pm_ops for the shared parent.
>
> Not if the parent is disabled for runtime PM, which it is in this case,
> right?

Correct. The enabling code was incorrectly left there since quite a
few revisions ago.

>> So I wonder which way that is the best to allow resuming platform
>> devices. Patch [02/05] is one way, but maybe there are more elegant
>> ways to handle it? Should the platform code be modified instead? If
>> so, how? I suppose root hubs for USB may have a similar issue, no?
>
> If necessary, I would suggest adding appropriate dummy runtime PM
> routines for that catch-all parent device.  But it may not be
> necessary.

I does not seem necessary.

Thanks for your help!

/ magnus