[PATCH 0/2] Run callback of device_prepare/complete consistently

[PATCH 0/2] Run callback of device_prepare/complete consistently

[shuox.liu]

dpm_run_callback is used in other stages of power states changing.
It provides debug info message and time measurement when call these
callback. We also want to benefit ->prepare and ->complete.

[PATCH 1/2] PM: use dpm_run_callback in device_prepare
[PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete

[PATCH 1/2] PM: use dpm_run_callback in device_prepare

[shuox.liu]

From: ShuoX Liu <shuox.liu@intel.com>

dpm_run_callback could show more debug info around prepare stage.

Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
---
 drivers/base/power/main.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 5a9b656..cb89323 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1223,7 +1223,7 @@ int dpm_suspend(pm_message_t state)
  */
 static int device_prepare(struct device *dev, pm_message_t state)
 {
-	int (*callback)(struct device *) = NULL;
+	pm_callback_t callback = NULL;
 	char *info = NULL;
 	int error = 0;
 
@@ -1261,10 +1261,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 		callback = dev->driver->pm->prepare;
 	}
 
-	if (callback) {
-		error = callback(dev);
-		suspend_report_result(callback, error);
-	}
+	error = dpm_run_callback(callback, dev, state, info);
 
 	device_unlock(dev);
 
@@ -1280,6 +1277,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 int dpm_prepare(pm_message_t state)
 {
 	int error = 0;
+	ktime_t starttime = ktime_get();
 
 	might_sleep();
 
@@ -1311,6 +1309,7 @@ int dpm_prepare(pm_message_t state)
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
+	dpm_show_time(starttime, state, "prepare");
 	return error;
 }
 
-- 
1.7.1

[PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete

[shuox.liu]

From: ShuoX Liu <shuox.liu@intel.com>

dpm_run_callback_void could show more debug info around complete stage

Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
Signed-off-by: Liu ShuoX <shuox.liu@intel.com>
---
 drivers/base/power/main.c |   27 ++++++++++++++++++++++-----
 1 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index cb89323..7021491 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -33,6 +33,7 @@
 #include "power.h"
 
 typedef int (*pm_callback_t)(struct device *);
+typedef void (*pm_callback_void_t)(struct device *);
 
 /*
  * The entries in the dpm_list list are in a depth first order, simply
@@ -384,6 +385,23 @@ static int dpm_run_callback(pm_callback_t cb, struct device *dev,
 	return error;
 }
 
+static void dpm_run_callback_void(pm_callback_void_t cb, struct device *dev,
+			    pm_message_t state, char *info)
+{
+	ktime_t calltime;
+	int error;
+
+	if (!cb)
+		return;
+
+	calltime = initcall_debug_start(dev);
+
+	pm_dev_dbg(dev, state, info);
+	cb(dev);
+
+	initcall_debug_report(dev, calltime, error);
+}
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -722,7 +740,7 @@ void dpm_resume(pm_message_t state)
  */
 static void device_complete(struct device *dev, pm_message_t state)
 {
-	void (*callback)(struct device *) = NULL;
+	pm_callback_void_t callback = NULL;
 	char *info = NULL;
 
 	if (dev->power.syscore)
@@ -749,10 +767,7 @@ static void device_complete(struct device *dev, pm_message_t state)
 		callback = dev->driver->pm->complete;
 	}
 
-	if (callback) {
-		pm_dev_dbg(dev, state, info);
-		callback(dev);
-	}
+	dpm_run_callback_void(callback, dev, state, info);
 
 	device_unlock(dev);
 
@@ -769,6 +784,7 @@ static void device_complete(struct device *dev, pm_message_t state)
 void dpm_complete(pm_message_t state)
 {
 	struct list_head list;
+	ktime_t starttime = ktime_get();
 
 	might_sleep();
 
@@ -789,6 +805,7 @@ void dpm_complete(pm_message_t state)
 	}
 	list_splice(&list, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
+	dpm_show_time(starttime, state, "complete");
 }
 
 /**
-- 
1.7.1

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> dpm_run_callback is used in other stages of power states changing.
> It provides debug info message and time measurement when call these
> callback. We also want to benefit ->prepare and ->complete.
> 
> [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete

Is this an "Oh, why don't we do that?" series, or is it useful for anything
in practice?  I'm asking, because we haven't added that stuff to start with
since we didn't see why it would be useful to anyone.

And while patch [1/2] reduces the code size (by 1 line), so I can see some
(tiny) benefit from applying it, patch [2/2] adds more code and is there any
paractical reason?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] PM: use dpm_run_callback in device_prepare

[Greg KH]

On Fri, Jun 07, 2013 at 04:20:31PM +0800, shuox.liu@intel.com wrote:
> From: ShuoX Liu <shuox.liu@intel.com>
> 
> dpm_run_callback could show more debug info around prepare stage.

Why?  Who needs this?  What problem does it solve?

Without answers to this, why would you expect us to accept such a
change?

greg k-h

Re: [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete

[Greg KH]

On Fri, Jun 07, 2013 at 04:20:32PM +0800, shuox.liu@intel.com wrote:
> From: ShuoX Liu <shuox.liu@intel.com>
> 
> dpm_run_callback_void could show more debug info around complete stage

Again, why?  What does this benifit us?

greg k-h

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Yanmin Zhang]

On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > dpm_run_callback is used in other stages of power states changing.
> > It provides debug info message and time measurement when call these
> > callback. We also want to benefit ->prepare and ->complete.
> > 
> > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> 
> Is this an "Oh, why don't we do that?" series, or is it useful for anything
> in practice?  I'm asking, because we haven't added that stuff to start with
> since we didn't see why it would be useful to anyone.
> 
> And while patch [1/2] reduces the code size (by 1 line), so I can see some
> (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> paractical reason?
Sometimes, suspend-to-ram path spends too much time (either suspend slowly
or wakeup slowly) and we need optimize it.
With the 2 patches, we could collect initcall_debug printk info and manually
check what prepare/complete callbacks consume too much time.

Thanks,
Yanmin

Re: [PATCH 1/2] PM: use dpm_run_callback in device_prepare

[Yanmin Zhang]

On Fri, 2013-06-07 at 10:37 -0700, Greg KH wrote:
> On Fri, Jun 07, 2013 at 04:20:31PM +0800, shuox.liu@intel.com wrote:
> > From: ShuoX Liu <shuox.liu@intel.com>
> > 
> > dpm_run_callback could show more debug info around prepare stage.
> 
> Why?  Who needs this?  What problem does it solve?
> 
> Without answers to this, why would you expect us to accept such a
> change?
It's to provide more debug info and developers can quickly find what
prepare/complete callback consumes too much time.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Saturday, June 08, 2013 08:42:12 AM Yanmin Zhang wrote:
> On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > dpm_run_callback is used in other stages of power states changing.
> > > It provides debug info message and time measurement when call these
> > > callback. We also want to benefit ->prepare and ->complete.
> > > 
> > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > 
> > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > in practice?  I'm asking, because we haven't added that stuff to start with
> > since we didn't see why it would be useful to anyone.
> > 
> > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > paractical reason?
> Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> or wakeup slowly) and we need optimize it.
> With the 2 patches, we could collect initcall_debug printk info and manually
> check what prepare/complete callbacks consume too much time.

Well, can you point me to a single driver where prepare/complete causes this
type of problems to happen?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/2] PM: use dpm_run_callback in device_prepare

[Greg KH]

On Sat, Jun 08, 2013 at 08:43:55AM +0800, Yanmin Zhang wrote:
> On Fri, 2013-06-07 at 10:37 -0700, Greg KH wrote:
> > On Fri, Jun 07, 2013 at 04:20:31PM +0800, shuox.liu@intel.com wrote:
> > > From: ShuoX Liu <shuox.liu@intel.com>
> > > 
> > > dpm_run_callback could show more debug info around prepare stage.
> > 
> > Why?  Who needs this?  What problem does it solve?
> > 
> > Without answers to this, why would you expect us to accept such a
> > change?
> It's to provide more debug info and developers can quickly find what
> prepare/complete callback consumes too much time.

What exact debug info is this providing, and why wouldn't you say all of
this in the original changelog information?

{hint}

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Greg KH]

On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > dpm_run_callback is used in other stages of power states changing.
> > > It provides debug info message and time measurement when call these
> > > callback. We also want to benefit ->prepare and ->complete.
> > > 
> > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > 
> > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > in practice?  I'm asking, because we haven't added that stuff to start with
> > since we didn't see why it would be useful to anyone.
> > 
> > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > paractical reason?
> Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> or wakeup slowly) and we need optimize it.
> With the 2 patches, we could collect initcall_debug printk info and manually
> check what prepare/complete callbacks consume too much time.

But initcall information is for initialization stuff, not suspend/resume
things, right?  Doesn't the existing tools for parsing this choke if it
sees the information at suspend/resume time?

greg k-h

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Yanmin Zhang]

On Sat, 2013-06-08 at 02:54 +0200, Rafael J. Wysocki wrote:
> On Saturday, June 08, 2013 08:42:12 AM Yanmin Zhang wrote:
> > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > dpm_run_callback is used in other stages of power states changing.
> > > > It provides debug info message and time measurement when call these
> > > > callback. We also want to benefit ->prepare and ->complete.
> > > > 
> > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > 
> > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > since we didn't see why it would be useful to anyone.
> > > 
> > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > paractical reason?
> > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > or wakeup slowly) and we need optimize it.
> > With the 2 patches, we could collect initcall_debug printk info and manually
> > check what prepare/complete callbacks consume too much time.
> 
> Well, can you point me to a single driver where prepare/complete causes this
> type of problems to happen?

That's a good question. We are enabling Android mobiles and need optimize
suspend-to-ram. In the current upstream, we don't find a driver's prepare/complete
callback spends too much time.

Re: [PATCH 1/2] PM: use dpm_run_callback in device_prepare

[Yanmin Zhang]

On Fri, 2013-06-07 at 18:15 -0700, Greg KH wrote:
> On Sat, Jun 08, 2013 at 08:43:55AM +0800, Yanmin Zhang wrote:
> > On Fri, 2013-06-07 at 10:37 -0700, Greg KH wrote:
> > > On Fri, Jun 07, 2013 at 04:20:31PM +0800, shuox.liu@intel.com wrote:
> > > > From: ShuoX Liu <shuox.liu@intel.com>
> > > > 
> > > > dpm_run_callback could show more debug info around prepare stage.
> > > 
> > > Why?  Who needs this?  What problem does it solve?
> > > 
> > > Without answers to this, why would you expect us to accept such a
> > > change?
> > It's to provide more debug info and developers can quickly find what
> > prepare/complete callback consumes too much time.
> 
> What exact debug info is this providing, and why wouldn't you say all of
> this in the original changelog information?
Thanks for the reminder.

Below are examples, after applying the patches. We can see clearly
how much time every device prepare callback consumes.

[  600.384779] sr 1:0:0:0: preparing bus suspend
[  600.384780] call 1:0:0:0+ returned 0 after 2 usecs
[  600.384785] calling  2-1+ @ 2556, parent: usb2
[  600.384788] usb 2-1: preparing type suspend
[  600.384789] call 2-1+ returned 0 after 2 usecs
[  600.384794] calling  1-1.1+ @ 2556, parent: 1-1
[  600.384797] usb 1-1.1: preparing type suspend
[  600.384799] call 1-1.1+ returned 0 after 2 usecs
[  600.384802] calling  1-1.3+ @ 2556, parent: 1-1
[  600.384804] usb 1-1.3: preparing type suspend
[  600.384805] call 1-1.3+ returned 0 after 2 usecs
[  600.384809] calling  1-1.4+ @ 2556, parent: 1-1
[  600.384811] usb 1-1.4: preparing type suspend
[  600.384813] call 1-1.4+ returned 0 after 2 usecs
[  600.384816] calling  1-1.5+ @ 2556, parent: 1-1
[  600.384819] usb 1-1.5: preparing type suspend, may wakeup
[  600.384820] call 1-1.5+ returned 0 after 2 usecs
[  600.384823] calling  1-1.4.1+ @ 2556, parent: 1-1.4
[  600.384825] usb 1-1.4.1: preparing type suspend
[  600.384827] call 1-1.4.1+ returned 0 after 2 usecs
[  600.384829] calling  1-1.4.2+ @ 2556, parent: 1-1.4
[  600.384831] usb 1-1.4.2: preparing type suspend
[  600.384833] call 1-1.4.2+ returned 0 after 2 usecs
[  600.384865] PM: prepare suspend of devices complete after 0.471 msecs

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Saturday, June 08, 2013 09:17:13 AM Yanmin Zhang wrote:
> On Sat, 2013-06-08 at 02:54 +0200, Rafael J. Wysocki wrote:
> > On Saturday, June 08, 2013 08:42:12 AM Yanmin Zhang wrote:
> > > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > > dpm_run_callback is used in other stages of power states changing.
> > > > > It provides debug info message and time measurement when call these
> > > > > callback. We also want to benefit ->prepare and ->complete.
> > > > > 
> > > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > > 
> > > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > > since we didn't see why it would be useful to anyone.
> > > > 
> > > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > > paractical reason?
> > > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > > or wakeup slowly) and we need optimize it.
> > > With the 2 patches, we could collect initcall_debug printk info and manually
> > > check what prepare/complete callbacks consume too much time.
> > 
> > Well, can you point me to a single driver where prepare/complete causes this
> > type of problems to happen?
> 
> That's a good question. We are enabling Android mobiles and need optimize
> suspend-to-ram. In the current upstream, we don't find a driver's prepare/complete
> callback spends too much time.

I understand your needs, but I don't think prepare/complete are likely to take
too much time on Android either.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
> On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > dpm_run_callback is used in other stages of power states changing.
> > > > It provides debug info message and time measurement when call these
> > > > callback. We also want to benefit ->prepare and ->complete.
> > > > 
> > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > 
> > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > since we didn't see why it would be useful to anyone.
> > > 
> > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > paractical reason?
> > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > or wakeup slowly) and we need optimize it.
> > With the 2 patches, we could collect initcall_debug printk info and manually
> > check what prepare/complete callbacks consume too much time.
> 
> But initcall information is for initialization stuff, not suspend/resume
> things, right?  Doesn't the existing tools for parsing this choke if it
> sees the information at suspend/resume time?

We've been using that for suspend/resume for quite some time too, but not
for the prepare/complete phases (because we still believe that's not really
useful for them). 

Well, I'll be handling patches changing code under drivers/base/power,
I promise. :-)

I've been doing that for quite a few years now ...

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Yanmin Zhang]

On Fri, 2013-06-07 at 18:16 -0700, Greg KH wrote:
> On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > dpm_run_callback is used in other stages of power states changing.
> > > > It provides debug info message and time measurement when call these
> > > > callback. We also want to benefit ->prepare and ->complete.
> > > > 
> > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > 
> > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > since we didn't see why it would be useful to anyone.
> > > 
> > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > paractical reason?
> > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > or wakeup slowly) and we need optimize it.
> > With the 2 patches, we could collect initcall_debug printk info and manually
> > check what prepare/complete callbacks consume too much time.
> 
> But initcall information is for initialization stuff, not suspend/resume
> things, right? 
initcall_debug also controls shutdown callbacks, and suspend/resume callbacks.
See __device_suspend=>dpm_run_callback. It's very useful when we want to optimize
suspend-to-ram, or debug some hard issues which happen when async suspend is enabled.

>  Doesn't the existing tools for parsing this choke if it
> sees the information at suspend/resume time?
Current kernel doesn't print out info around prepare/complete.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Yanmin Zhang]

On Sat, 2013-06-08 at 03:30 +0200, Rafael J. Wysocki wrote:
> On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
> > On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > > dpm_run_callback is used in other stages of power states changing.
> > > > > It provides debug info message and time measurement when call these
> > > > > callback. We also want to benefit ->prepare and ->complete.
> > > > > 
> > > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > > 
> > > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > > since we didn't see why it would be useful to anyone.
> > > > 
> > > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > > paractical reason?
> > > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > > or wakeup slowly) and we need optimize it.
> > > With the 2 patches, we could collect initcall_debug printk info and manually
> > > check what prepare/complete callbacks consume too much time.
> > 
> > But initcall information is for initialization stuff, not suspend/resume
> > things, right?  Doesn't the existing tools for parsing this choke if it
> > sees the information at suspend/resume time?
> 
> We've been using that for suspend/resume for quite some time too, but not
> for the prepare/complete phases (because we still believe that's not really
> useful for them). 
> 
> Well, I'll be handling patches changing code under drivers/base/power,
> I promise. :-)
> 
> I've been doing that for quite a few years now ...
Yes, indeed. Power is one of the most important features on embedded devices.
Lots of smart phones don't really go through the full cycles of suspend-to-ram.
We are following the full steps of the suspend.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Saturday, June 08, 2013 09:36:03 AM Yanmin Zhang wrote:
> On Sat, 2013-06-08 at 03:30 +0200, Rafael J. Wysocki wrote:
> > On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
> > > On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > > > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > > > dpm_run_callback is used in other stages of power states changing.
> > > > > > It provides debug info message and time measurement when call these
> > > > > > callback. We also want to benefit ->prepare and ->complete.
> > > > > > 
> > > > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > > > 
> > > > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > > > since we didn't see why it would be useful to anyone.
> > > > > 
> > > > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > > > paractical reason?
> > > > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > > > or wakeup slowly) and we need optimize it.
> > > > With the 2 patches, we could collect initcall_debug printk info and manually
> > > > check what prepare/complete callbacks consume too much time.
> > > 
> > > But initcall information is for initialization stuff, not suspend/resume
> > > things, right?  Doesn't the existing tools for parsing this choke if it
> > > sees the information at suspend/resume time?
> > 
> > We've been using that for suspend/resume for quite some time too, but not
> > for the prepare/complete phases (because we still believe that's not really
> > useful for them). 
> > 
> > Well, I'll be handling patches changing code under drivers/base/power,
> > I promise. :-)
> > 
> > I've been doing that for quite a few years now ...
> Yes, indeed. Power is one of the most important features on embedded devices.
> Lots of smart phones don't really go through the full cycles of suspend-to-ram.
> We are following the full steps of the suspend.

But if you go through the code, you'll see that alomost no drivers actually
implemet .prepare() and .complete().  Some subsystems do, but they really don't
take too much time to execute.  Which means that your patches with
initcall_debug will add quite a pile of useless garbage to the kernel log and
I'm not sure how that helps?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Yanmin Zhang]

On Sat, 2013-06-08 at 03:52 +0200, Rafael J. Wysocki wrote:
> On Saturday, June 08, 2013 09:36:03 AM Yanmin Zhang wrote:
> > On Sat, 2013-06-08 at 03:30 +0200, Rafael J. Wysocki wrote:
> > > On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
> > > > On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > > > > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > > > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > > > > dpm_run_callback is used in other stages of power states changing.
> > > > > > > It provides debug info message and time measurement when call these
> > > > > > > callback. We also want to benefit ->prepare and ->complete.
> > > > > > > 
> > > > > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > > > > 
> > > > > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > > > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > > > > since we didn't see why it would be useful to anyone.
> > > > > > 
> > > > > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > > > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > > > > paractical reason?
> > > > > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > > > > or wakeup slowly) and we need optimize it.
> > > > > With the 2 patches, we could collect initcall_debug printk info and manually
> > > > > check what prepare/complete callbacks consume too much time.
> > > > 
> > > > But initcall information is for initialization stuff, not suspend/resume
> > > > things, right?  Doesn't the existing tools for parsing this choke if it
> > > > sees the information at suspend/resume time?
> > > 
> > > We've been using that for suspend/resume for quite some time too, but not
> > > for the prepare/complete phases (because we still believe that's not really
> > > useful for them). 
> > > 
> > > Well, I'll be handling patches changing code under drivers/base/power,
> > > I promise. :-)
> > > 
> > > I've been doing that for quite a few years now ...
> > Yes, indeed. Power is one of the most important features on embedded devices.
> > Lots of smart phones don't really go through the full cycles of suspend-to-ram.
> > We are following the full steps of the suspend.
> 
> But if you go through the code, you'll see that alomost no drivers actually
> implemet .prepare() and .complete().  Some subsystems do, but they really don't
> take too much time to execute.  Which means that your patches with
> initcall_debug will add quite a pile of useless garbage to the kernel log
Does that mean we need add more log levels around such info instead of just having or
not having?

Yanmin

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Rafael J. Wysocki]

On Saturday, June 08, 2013 10:37:18 AM Yanmin Zhang wrote:
> On Sat, 2013-06-08 at 03:52 +0200, Rafael J. Wysocki wrote:
> > On Saturday, June 08, 2013 09:36:03 AM Yanmin Zhang wrote:
> > > On Sat, 2013-06-08 at 03:30 +0200, Rafael J. Wysocki wrote:
> > > > On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
> > > > > On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
> > > > > > On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
> > > > > > > On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > > > > > > > dpm_run_callback is used in other stages of power states changing.
> > > > > > > > It provides debug info message and time measurement when call these
> > > > > > > > callback. We also want to benefit ->prepare and ->complete.
> > > > > > > > 
> > > > > > > > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > > > > > > > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> > > > > > > 
> > > > > > > Is this an "Oh, why don't we do that?" series, or is it useful for anything
> > > > > > > in practice?  I'm asking, because we haven't added that stuff to start with
> > > > > > > since we didn't see why it would be useful to anyone.
> > > > > > > 
> > > > > > > And while patch [1/2] reduces the code size (by 1 line), so I can see some
> > > > > > > (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> > > > > > > paractical reason?
> > > > > > Sometimes, suspend-to-ram path spends too much time (either suspend slowly
> > > > > > or wakeup slowly) and we need optimize it.
> > > > > > With the 2 patches, we could collect initcall_debug printk info and manually
> > > > > > check what prepare/complete callbacks consume too much time.
> > > > > 
> > > > > But initcall information is for initialization stuff, not suspend/resume
> > > > > things, right?  Doesn't the existing tools for parsing this choke if it
> > > > > sees the information at suspend/resume time?
> > > > 
> > > > We've been using that for suspend/resume for quite some time too, but not
> > > > for the prepare/complete phases (because we still believe that's not really
> > > > useful for them). 
> > > > 
> > > > Well, I'll be handling patches changing code under drivers/base/power,
> > > > I promise. :-)
> > > > 
> > > > I've been doing that for quite a few years now ...
> > > Yes, indeed. Power is one of the most important features on embedded devices.
> > > Lots of smart phones don't really go through the full cycles of suspend-to-ram.
> > > We are following the full steps of the suspend.
> > 
> > But if you go through the code, you'll see that alomost no drivers actually
> > implemet .prepare() and .complete().  Some subsystems do, but they really don't
> > take too much time to execute.  Which means that your patches with
> > initcall_debug will add quite a pile of useless garbage to the kernel log
> Does that mean we need add more log levels around such info instead of just having or
> not having?

Since we don't have any code in the tree that causes problems those patches are
supposed to catch, I don't see why we need them in the tree.  Would it be
viable to keep them out of the tree for the time being and re-submit once
there is real need?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[ShuoX Liu]

On 2013-06-08 18:54, Rafael J. Wysocki wrote:

> On Saturday, June 08, 2013 10:37:18 AM Yanmin Zhang wrote:
>> On Sat, 2013-06-08 at 03:52 +0200, Rafael J. Wysocki wrote:
>>> On Saturday, June 08, 2013 09:36:03 AM Yanmin Zhang wrote:
>>>> On Sat, 2013-06-08 at 03:30 +0200, Rafael J. Wysocki wrote:
>>>>> On Friday, June 07, 2013 06:16:25 PM Greg KH wrote:
>>>>>> On Sat, Jun 08, 2013 at 08:42:12AM +0800, Yanmin Zhang wrote:
>>>>>>> On Fri, 2013-06-07 at 12:36 +0200, Rafael J. Wysocki wrote:
>>>>>>>> On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
>>>>>>>>> dpm_run_callback is used in other stages of power states changing.
>>>>>>>>> It provides debug info message and time measurement when call these
>>>>>>>>> callback. We also want to benefit ->prepare and ->complete.
>>>>>>>>>
>>>>>>>>> [PATCH 1/2] PM: use dpm_run_callback in device_prepare
>>>>>>>>> [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
>>>>>>>>
>>>>>>>> Is this an "Oh, why don't we do that?" series, or is it useful for anything
>>>>>>>> in practice?  I'm asking, because we haven't added that stuff to start with
>>>>>>>> since we didn't see why it would be useful to anyone.
>>>>>>>>
>>>>>>>> And while patch [1/2] reduces the code size (by 1 line), so I can see some
>>>>>>>> (tiny) benefit from applying it, patch [2/2] adds more code and is there any
>>>>>>>> paractical reason?
>>>>>>> Sometimes, suspend-to-ram path spends too much time (either suspend slowly
>>>>>>> or wakeup slowly) and we need optimize it.
>>>>>>> With the 2 patches, we could collect initcall_debug printk info and manually
>>>>>>> check what prepare/complete callbacks consume too much time.
>>>>>>
>>>>>> But initcall information is for initialization stuff, not suspend/resume
>>>>>> things, right?  Doesn't the existing tools for parsing this choke if it
>>>>>> sees the information at suspend/resume time?
>>>>>
>>>>> We've been using that for suspend/resume for quite some time too, but not
>>>>> for the prepare/complete phases (because we still believe that's not really
>>>>> useful for them). 
>>>>>
>>>>> Well, I'll be handling patches changing code under drivers/base/power,
>>>>> I promise. :-)
>>>>>
>>>>> I've been doing that for quite a few years now ...
>>>> Yes, indeed. Power is one of the most important features on embedded devices.
>>>> Lots of smart phones don't really go through the full cycles of suspend-to-ram.
>>>> We are following the full steps of the suspend.
>>>
>>> But if you go through the code, you'll see that alomost no drivers actually
>>> implemet .prepare() and .complete().  Some subsystems do, but they really don't
>>> take too much time to execute.  Which means that your patches with
>>> initcall_debug will add quite a pile of useless garbage to the kernel log
>> Does that mean we need add more log levels around such info instead of just having or
>> not having?
> 
> Since we don't have any code in the tree that causes problems those patches are
> supposed to catch, I don't see why we need them in the tree.  Would it be
> viable to keep them out of the tree for the time being and re-submit once
> there is real need?

It's OK with me. I will keep them in my debug tree.
Thanks all.

Shuo

Re: [PATCH 0/2] Run callback of device_prepare/complete consistently

[Pavel Machek]

On Fri 2013-06-07 12:36:12, Rafael J. Wysocki wrote:
> On Friday, June 07, 2013 04:20:30 PM shuox.liu@intel.com wrote:
> > dpm_run_callback is used in other stages of power states changing.
> > It provides debug info message and time measurement when call these
> > callback. We also want to benefit ->prepare and ->complete.
> > 
> > [PATCH 1/2] PM: use dpm_run_callback in device_prepare
> > [PATCH 2/2] PM: add dpm_run_callback_void and use it in device_complete
> 
> Is this an "Oh, why don't we do that?" series, or is it useful for anything
> in practice?  I'm asking, because we haven't added that stuff to start with
> since we didn't see why it would be useful to anyone.
> 
> And while patch [1/2] reduces the code size (by 1 line), so I can see some
> (tiny) benefit from applying it, patch [2/2] adds more code and is there any
> paractical reason?

Well, we have android people periodically trying to push suspend
instrumentation; usually the patches are much uglier than this. (Like,
essentially reinventing printk).

So... there is some demand for this.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html