[RFC][PATCH 0/2] PM: More suspend core modifications

[RFC][PATCH 0/2] PM: More suspend core modifications

[Rafael J. Wysocki]

Hi,

It seems that the core suspend code can be simplified even further.  Namely,
it's possible to reduce code duplication in drivers/base/power/suspend.c with
the help of an auxiliary inline finction and it's possible to remove the
suspend and resume support from struct device_type.

Comments welcome.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Reduce code duplication in drivers/base/suspend.c by introducing an inline
function for printing diagnostic messages.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/suspend.c |   49 +++++++++++++++----------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

Index: linux-2.6.22-rc4/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/suspend.c	2007-06-10 13:30:45.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c	2007-06-10 15:00:40.000000000 +0200
@@ -40,6 +40,14 @@ static inline char *suspend_verb(u32 eve
 }
 
 
+static inline void
+suspend_device_dbg(struct device * dev, pm_message_t state, char *info)
+{
+	dev_dbg(dev, "%s%s%s\n", info, suspend_verb(state.event),
+		((state.event == PM_EVENT_SUSPEND) && device_may_wakeup(dev)) ?
+		", may wakeup" : "");
+}
+
 /**
  *	suspend_device - Save state of one device.
  *	@dev:	Device.
@@ -66,37 +74,21 @@ int suspend_device(struct device * dev, 
 	dev->power.prev_state = dev->power.power_state;
 
 	if (dev->class && dev->class->suspend && !dev->power.power_state.event) {
-		dev_dbg(dev, "class %s%s\n",
-			suspend_verb(state.event),
-			((state.event == PM_EVENT_SUSPEND)
-					&& device_may_wakeup(dev))
-				? ", may wakeup"
-				: ""
-			);
+		suspend_device_dbg(dev, state, "class ");
 		error = dev->class->suspend(dev, state);
 		suspend_report_result(dev->class->suspend, error);
 	}
 
-	if (!error && dev->type && dev->type->suspend && !dev->power.power_state.event) {
-		dev_dbg(dev, "%s%s\n",
-			suspend_verb(state.event),
-			((state.event == PM_EVENT_SUSPEND)
-					&& device_may_wakeup(dev))
-				? ", may wakeup"
-				: ""
-			);
+	if (!error && dev->type && dev->type->suspend
+	    && !dev->power.power_state.event) {
+		suspend_device_dbg(dev, state, "type ");
 		error = dev->type->suspend(dev, state);
 		suspend_report_result(dev->type->suspend, error);
 	}
 
-	if (!error && dev->bus && dev->bus->suspend && !dev->power.power_state.event) {
-		dev_dbg(dev, "%s%s\n",
-			suspend_verb(state.event),
-			((state.event == PM_EVENT_SUSPEND)
-					&& device_may_wakeup(dev))
-				? ", may wakeup"
-				: ""
-			);
+	if (!error && dev->bus && dev->bus->suspend
+	    && !dev->power.power_state.event) {
+		suspend_device_dbg(dev, state, "");
 		error = dev->bus->suspend(dev, state);
 		suspend_report_result(dev->bus->suspend, error);
 	}
@@ -114,14 +106,9 @@ static int suspend_device_late(struct de
 {
 	int error = 0;
 
-	if (dev->bus && dev->bus->suspend_late && !dev->power.power_state.event) {
-		dev_dbg(dev, "LATE %s%s\n",
-			suspend_verb(state.event),
-			((state.event == PM_EVENT_SUSPEND)
-					&& device_may_wakeup(dev))
-				? ", may wakeup"
-				: ""
-			);
+	if (dev->bus && dev->bus->suspend_late
+	    && !dev->power.power_state.event) {
+		suspend_device_dbg(dev, state, "LATE ");
 		error = dev->bus->suspend_late(dev, state);
 		suspend_report_result(dev->bus->suspend_late, error);
 	}

[RFC][PATCH 2/2] PM: Remove suspend and resume support from struct device_type

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The suspend and resume support in struct device_type (include/linux/device.h)
is not used anywhere.  It is also undocumented, so it can be removed.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/resume.c  |    5 -----
 drivers/base/power/suspend.c |    7 -------
 include/linux/device.h       |    2 --
 3 files changed, 14 deletions(-)

Index: linux-2.6.22-rc4/drivers/base/power/resume.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/resume.c	2007-06-10 15:00:23.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/resume.c	2007-06-10 15:02:04.000000000 +0200
@@ -41,11 +41,6 @@ int resume_device(struct device * dev)
 		error = dev->bus->resume(dev);
 	}
 
-	if (!error && dev->type && dev->type->resume) {
-		dev_dbg(dev,"resuming\n");
-		error = dev->type->resume(dev);
-	}
-
 	if (!error && dev->class && dev->class->resume) {
 		dev_dbg(dev,"class resume\n");
 		error = dev->class->resume(dev);
Index: linux-2.6.22-rc4/drivers/base/power/suspend.c
===================================================================
--- linux-2.6.22-rc4.orig/drivers/base/power/suspend.c	2007-06-10 15:00:40.000000000 +0200
+++ linux-2.6.22-rc4/drivers/base/power/suspend.c	2007-06-10 15:02:04.000000000 +0200
@@ -79,13 +79,6 @@ int suspend_device(struct device * dev, 
 		suspend_report_result(dev->class->suspend, error);
 	}
 
-	if (!error && dev->type && dev->type->suspend
-	    && !dev->power.power_state.event) {
-		suspend_device_dbg(dev, state, "type ");
-		error = dev->type->suspend(dev, state);
-		suspend_report_result(dev->type->suspend, error);
-	}
-
 	if (!error && dev->bus && dev->bus->suspend
 	    && !dev->power.power_state.event) {
 		suspend_device_dbg(dev, state, "");
Index: linux-2.6.22-rc4/include/linux/device.h
===================================================================
--- linux-2.6.22-rc4.orig/include/linux/device.h	2007-06-10 15:00:23.000000000 +0200
+++ linux-2.6.22-rc4/include/linux/device.h	2007-06-10 15:02:04.000000000 +0200
@@ -343,8 +343,6 @@ struct device_type {
 	int (*uevent)(struct device *dev, char **envp, int num_envp,
 		      char *buffer, int buffer_size);
 	void (*release)(struct device *dev);
-	int (*suspend)(struct device * dev, pm_message_t state);
-	int (*resume)(struct device * dev);
 };
 
 /* interface for exporting device attributes */

Re: [RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Alan Stern]

On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Reduce code duplication in drivers/base/suspend.c by introducing an inline
> function for printing diagnostic messages.

Could you save object-code space by making the function non-inline?  
It looks complicated enough.

Or is the compiler supposed to figure such things out for itself?

Alan Stern

Re: [RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Rafael J. Wysocki]

On Sunday, 10 June 2007 16:55, Alan Stern wrote:
> On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Reduce code duplication in drivers/base/suspend.c by introducing an inline
> > function for printing diagnostic messages.
> 
> Could you save object-code space by making the function non-inline?  
> It looks complicated enough.

I thought I would do that, but then I thought I'd want it to be compiled out if
dev_dbg() were a noop (like when the debugging is off).

> Or is the compiler supposed to figure such things out for itself?

I think it is.  I'll remove this 'inline'.

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Pavel Machek]

Hi!

> Reduce code duplication in drivers/base/suspend.c by introducing an inline
> function for printing diagnostic messages.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Looks ok to me.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 2/2] PM: Remove suspend and resume support from struct device_type

[Pavel Machek]

Hi!

> The suspend and resume support in struct device_type (include/linux/device.h)
> is not used anywhere.  It is also undocumented, so it can be removed.

I guess you should cc gregkh on this one.. and the previous series
that touches driver model. And perhaps cc lkml is good idea.

							Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Alan Stern]

On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:

> On Sunday, 10 June 2007 16:55, Alan Stern wrote:
> > On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Reduce code duplication in drivers/base/suspend.c by introducing an inline
> > > function for printing diagnostic messages.
> > 
> > Could you save object-code space by making the function non-inline?  
> > It looks complicated enough.
> 
> I thought I would do that, but then I thought I'd want it to be compiled out if
> dev_dbg() were a noop (like when the debugging is off).

You can always do something like this:

#ifdef DEBUG

static void print_msg(...)
{
...
}

#else

static inline void print_msg(...)
{ }
#endif

Alan Stern

Re: [RFC][PATCH 1/2] PM: Reduce code duplication in the core suspend code

[Rafael J. Wysocki]

On Sunday, 10 June 2007 22:37, Alan Stern wrote:
> On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:
> 
> > On Sunday, 10 June 2007 16:55, Alan Stern wrote:
> > > On Sun, 10 Jun 2007, Rafael J. Wysocki wrote:
> > > 
> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Reduce code duplication in drivers/base/suspend.c by introducing an inline
> > > > function for printing diagnostic messages.
> > > 
> > > Could you save object-code space by making the function non-inline?  
> > > It looks complicated enough.
> > 
> > I thought I would do that, but then I thought I'd want it to be compiled out if
> > dev_dbg() were a noop (like when the debugging is off).
> 
> You can always do something like this:
> 
> #ifdef DEBUG
> 
> static void print_msg(...)
> {
> ...
> }
> 
> #else
> 
> static inline void print_msg(...)
> { }
> #endif

Well, IMHO, that doesn't look very nice. ;-)

Greetings,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth