Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Alan Stern]

On Thu, 17 Feb 2011, Rafael J. Wysocki wrote:

> > > Apart from this I think the order of checks introduced by the $subject patch
> > > should be:
> > > (1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
> > >     or otherwise
> > > (2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
> > >     or otherwise
> > > (3) use dev->bus (if present).
> > > as that would allow classes and device types to override bus type PM
> > > callbacks if they wish to.
> > 
> > I haven't heard of any device types being present on more than one kind
> > of bus, so it makes sense for device types to override bus types.
> 
> OK
> 
> > But I'm not so sure about the priority we should give to classes.  On the
> > other hand, if no classes define a dev_pm_ops then of course it doesn't
> > matter.
> 
> The change will also affect classes that provide "legacy" suspend-resume
> (if there are any, which I'm totally unsure of).
> 
> Anyway, I think we need to choose one ordering. :-)
> 
> What about type / bus / class , then?

I really don't know.  Somebody who has more experience with device 
class implementations should answer.

Greg, any ideas?

To recap: The issue is how to handle multiple PM callbacks.  Since the
bus type, device type, and device class may all have their own
callbacks, Rafael has decided the best approach is to prioritize them
and invoke only the highest-priority callback.  But what priority order
should we use?

Alan Stern

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Greg KH]

On Thu, Feb 17, 2011 at 09:55:46AM -0500, Alan Stern wrote:
> On Thu, 17 Feb 2011, Rafael J. Wysocki wrote:
> 
> > > > Apart from this I think the order of checks introduced by the $subject patch
> > > > should be:
> > > > (1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
> > > >     or otherwise
> > > > (2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
> > > >     or otherwise
> > > > (3) use dev->bus (if present).
> > > > as that would allow classes and device types to override bus type PM
> > > > callbacks if they wish to.
> > > 
> > > I haven't heard of any device types being present on more than one kind
> > > of bus, so it makes sense for device types to override bus types.
> > 
> > OK
> > 
> > > But I'm not so sure about the priority we should give to classes.  On the
> > > other hand, if no classes define a dev_pm_ops then of course it doesn't
> > > matter.
> > 
> > The change will also affect classes that provide "legacy" suspend-resume
> > (if there are any, which I'm totally unsure of).
> > 
> > Anyway, I think we need to choose one ordering. :-)
> > 
> > What about type / bus / class , then?
> 
> I really don't know.  Somebody who has more experience with device 
> class implementations should answer.
> 
> Greg, any ideas?
> 
> To recap: The issue is how to handle multiple PM callbacks.  Since the
> bus type, device type, and device class may all have their own
> callbacks, Rafael has decided the best approach is to prioritize them
> and invoke only the highest-priority callback.  But what priority order
> should we use?

I think we should do it in the following order:
	device type
	device class
	device bus

for the reasons that a device itself could override the default class
and bus information if it "knows" it is special.  After that, the class
of the device holds a lot of information about what is going on with the
logic involved (i.e. network stuff), and lastly, the bus knows some
default hardware information.

Sound reasonable?

I think that follows the default we have today, right?

thanks,

greg k-h

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

On Thursday, February 17, 2011, Greg KH wrote:
> On Thu, Feb 17, 2011 at 09:55:46AM -0500, Alan Stern wrote:
> > On Thu, 17 Feb 2011, Rafael J. Wysocki wrote:
> > 
> > > > > Apart from this I think the order of checks introduced by the $subject patch
> > > > > should be:
> > > > > (1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
> > > > >     or otherwise
> > > > > (2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
> > > > >     or otherwise
> > > > > (3) use dev->bus (if present).
> > > > > as that would allow classes and device types to override bus type PM
> > > > > callbacks if they wish to.
> > > > 
> > > > I haven't heard of any device types being present on more than one kind
> > > > of bus, so it makes sense for device types to override bus types.
> > > 
> > > OK
> > > 
> > > > But I'm not so sure about the priority we should give to classes.  On the
> > > > other hand, if no classes define a dev_pm_ops then of course it doesn't
> > > > matter.
> > > 
> > > The change will also affect classes that provide "legacy" suspend-resume
> > > (if there are any, which I'm totally unsure of).
> > > 
> > > Anyway, I think we need to choose one ordering. :-)
> > > 
> > > What about type / bus / class , then?
> > 
> > I really don't know.  Somebody who has more experience with device 
> > class implementations should answer.
> > 
> > Greg, any ideas?
> > 
> > To recap: The issue is how to handle multiple PM callbacks.  Since the
> > bus type, device type, and device class may all have their own
> > callbacks, Rafael has decided the best approach is to prioritize them
> > and invoke only the highest-priority callback.  But what priority order
> > should we use?
> 
> I think we should do it in the following order:
> 	device type
> 	device class
> 	device bus
> 
> for the reasons that a device itself could override the default class
> and bus information if it "knows" it is special.  After that, the class
> of the device holds a lot of information about what is going on with the
> logic involved (i.e. network stuff), and lastly, the bus knows some
> default hardware information.
> 
> Sound reasonable?
> 
> I think that follows the default we have today, right?

No, right now the class goes first during suspend and last during resume (and
of course they all can be called during system suspend/resume now).

That said I'm going to follow your suggestion. :-)

Thanks,
Rafael

[PATCH] PM: Make system-wide PM and runtime PM treat subsystems consistently

[R. J. Wysocki]

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

The code handling system-wide power transitions (eg. suspend-to-RAM)
can in theory execute callbacks provided by the device's bus type,
device type and class in each phase of the power transition.  In
turn, the runtime PM core code only calls one of those callbacks at
a time, preferring bus type callbacks to device type or class
callbacks and device type callbacks to class callbacks.

It seems reasonable to make them both behave in the same way in that
respect.  Moreover, even though a device may belong to two subsystems
(eg. bus type and device class) simultaneously, in practice power
management callbacks for system-wide power transitions are always
provided by only one of them (ie. if the bus type callbacks are
defined, the device class ones are not and vice versa).  Thus it is
possible to modify the code handling system-wide power transitions
so that it follows the core runtime PM code (ie. treats the
subsystem callbacks as mutually exclusive).

On the other hand, the core runtime PM code will choose to execute,
for example, a runtime suspend callback provided by the device type
even if the bus type's struct dev_pm_ops object exists, but the
runtime_suspend pointer in it happens to be NULL.  This is confusing,
because it may lead to the execution of callbacks from different
subsystems during different operations (eg. the bus type suspend
callback may be executed during runtime suspend of the device, while
the device type callback will be executed during system suspend).

Make all of the power management code treat subsystem callbacks in
a consistent way, such that:
(1) If the device's type is defined (eg. dev->type is not NULL)
    and its pm pointer is not NULL, the callbacks from dev->type->pm
    will be used.
(2) If dev->type is NULL or dev->type->pm is NULL, but the device's
    class is defined (eg. dev->class is not NULL) and its pm pointer
    is not NULL, the callbacks from dev->class->pm will be used.
(3) If dev->type is NULL or dev->type->pm is NULL and dev->class is
    NULL or dev->class->pm is NULL, the callbacks from dev->bus->pm
    will be used provided that both dev->bus and dev->bus->pm are
    not NULL.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Kevin Hilman <khilman@ti.com>
Reasoning-sounds-sane-to: Grant Likely <grant.likely@secretlab.ca>
---

Hi,

This patch is on top of suspend-2.6/linux-next.

I changed the ordering of callbacks following your advice, hopefully the
ACKs above still apply.

Thanks,
Rafael

---
Documentation/power/devices.txt |   28 +++----
 Documentation/power/devices.txt    |   29 ++-----
 Documentation/power/runtime_pm.txt |   13 +--
 drivers/base/power/main.c          |  150 +++++++++++++++----------------------
 drivers/base/power/runtime.c       |   18 ++--
 4 files changed, 92 insertions(+), 118 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -428,26 +428,17 @@ static int device_resume_noirq(struct de
 		pm_noirq_op(dev, &dev->pwr_domain->ops, state);
 	}
 
-	if (dev->bus && dev->bus->pm) {
-		pm_dev_dbg(dev, state, "EARLY ");
-		error = pm_noirq_op(dev, dev->bus->pm, state);
-		if (error)
-			goto End;
-	}
-
 	if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "EARLY type ");
 		error = pm_noirq_op(dev, dev->type->pm, state);
-		if (error)
-			goto End;
-	}
-
-	if (dev->class && dev->class->pm) {
+	} else if (dev->class && dev->class->pm) {
 		pm_dev_dbg(dev, state, "EARLY class ");
 		error = pm_noirq_op(dev, dev->class->pm, state);
+	} else if (dev->bus && dev->bus->pm) {
+		pm_dev_dbg(dev, state, "EARLY ");
+		error = pm_noirq_op(dev, dev->bus->pm, state);
 	}
 
-End:
 	TRACE_RESUME(error);
 	return error;
 }
@@ -528,36 +519,34 @@ static int device_resume(struct device *
 		pm_op(dev, &dev->pwr_domain->ops, state);
 	}
 
-	if (dev->bus) {
-		if (dev->bus->pm) {
-			pm_dev_dbg(dev, state, "");
-			error = pm_op(dev, dev->bus->pm, state);
-		} else if (dev->bus->resume) {
-			pm_dev_dbg(dev, state, "legacy ");
-			error = legacy_resume(dev, dev->bus->resume);
-		}
-		if (error)
-			goto End;
-	}
-
-	if (dev->type) {
-		if (dev->type->pm) {
-			pm_dev_dbg(dev, state, "type ");
-			error = pm_op(dev, dev->type->pm, state);
-		}
-		if (error)
-			goto End;
+	if (dev->type && dev->type->pm) {
+		pm_dev_dbg(dev, state, "type ");
+		error = pm_op(dev, dev->type->pm, state);
+		goto End;
 	}
 
 	if (dev->class) {
 		if (dev->class->pm) {
 			pm_dev_dbg(dev, state, "class ");
 			error = pm_op(dev, dev->class->pm, state);
+			goto End;
 		} else if (dev->class->resume) {
 			pm_dev_dbg(dev, state, "legacy class ");
 			error = legacy_resume(dev, dev->class->resume);
+			goto End;
 		}
 	}
+
+	if (dev->bus) {
+		if (dev->bus->pm) {
+			pm_dev_dbg(dev, state, "");
+			error = pm_op(dev, dev->bus->pm, state);
+		} else if (dev->bus->resume) {
+			pm_dev_dbg(dev, state, "legacy ");
+			error = legacy_resume(dev, dev->bus->resume);
+		}
+	}
+
  End:
 	device_unlock(dev);
 	complete_all(&dev->power.completion);
@@ -644,19 +633,18 @@ static void device_complete(struct devic
 		dev->pwr_domain->ops.complete(dev);
 	}
 
-	if (dev->class && dev->class->pm && dev->class->pm->complete) {
-		pm_dev_dbg(dev, state, "completing class ");
-		dev->class->pm->complete(dev);
-	}
-
-	if (dev->type && dev->type->pm && dev->type->pm->complete) {
+	if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "completing type ");
-		dev->type->pm->complete(dev);
-	}
-
-	if (dev->bus && dev->bus->pm && dev->bus->pm->complete) {
+		if (dev->type->pm->complete)
+			dev->type->pm->complete(dev);
+	} else if (dev->class && dev->class->pm) {
+		pm_dev_dbg(dev, state, "completing class ");
+		if (dev->class->pm->complete)
+			dev->class->pm->complete(dev);
+	} else if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "completing ");
-		dev->bus->pm->complete(dev);
+		if (dev->bus->pm->complete)
+			dev->bus->pm->complete(dev);
 	}
 
 	device_unlock(dev);
@@ -741,27 +729,23 @@ static pm_message_t resume_event(pm_mess
  */
 static int device_suspend_noirq(struct device *dev, pm_message_t state)
 {
-	int error = 0;
-
-	if (dev->class && dev->class->pm) {
-		pm_dev_dbg(dev, state, "LATE class ");
-		error = pm_noirq_op(dev, dev->class->pm, state);
-		if (error)
-			goto End;
-	}
+	int error;
 
 	if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "LATE type ");
 		error = pm_noirq_op(dev, dev->type->pm, state);
 		if (error)
-			goto End;
-	}
-
-	if (dev->bus && dev->bus->pm) {
+			return error;
+	} else if (dev->class && dev->class->pm) {
+		pm_dev_dbg(dev, state, "LATE class ");
+		error = pm_noirq_op(dev, dev->class->pm, state);
+		if (error)
+			return error;
+	} else if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "LATE ");
 		error = pm_noirq_op(dev, dev->bus->pm, state);
 		if (error)
-			goto End;
+			return error;
 	}
 
 	if (dev->pwr_domain) {
@@ -769,8 +753,7 @@ static int device_suspend_noirq(struct d
 		pm_noirq_op(dev, &dev->pwr_domain->ops, state);
 	}
 
-End:
-	return error;
+	return 0;
 }
 
 /**
@@ -857,25 +840,22 @@ static int __device_suspend(struct devic
 		goto End;
 	}
 
+	if (dev->type && dev->type->pm) {
+		pm_dev_dbg(dev, state, "type ");
+		error = pm_op(dev, dev->type->pm, state);
+		goto Domain;
+	}
+
 	if (dev->class) {
 		if (dev->class->pm) {
 			pm_dev_dbg(dev, state, "class ");
 			error = pm_op(dev, dev->class->pm, state);
+			goto Domain;
 		} else if (dev->class->suspend) {
 			pm_dev_dbg(dev, state, "legacy class ");
 			error = legacy_suspend(dev, state, dev->class->suspend);
+			goto Domain;
 		}
-		if (error)
-			goto End;
-	}
-
-	if (dev->type) {
-		if (dev->type->pm) {
-			pm_dev_dbg(dev, state, "type ");
-			error = pm_op(dev, dev->type->pm, state);
-		}
-		if (error)
-			goto End;
 	}
 
 	if (dev->bus) {
@@ -886,11 +866,10 @@ static int __device_suspend(struct devic
 			pm_dev_dbg(dev, state, "legacy ");
 			error = legacy_suspend(dev, state, dev->bus->suspend);
 		}
-		if (error)
-			goto End;
 	}
 
-	if (dev->pwr_domain) {
+ Domain:
+	if (!error && dev->pwr_domain) {
 		pm_dev_dbg(dev, state, "power domain ");
 		pm_op(dev, &dev->pwr_domain->ops, state);
 	}
@@ -985,28 +964,27 @@ static int device_prepare(struct device
 
 	device_lock(dev);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->prepare) {
-		pm_dev_dbg(dev, state, "preparing ");
-		error = dev->bus->pm->prepare(dev);
-		suspend_report_result(dev->bus->pm->prepare, error);
-		if (error)
-			goto End;
-	}
-
-	if (dev->type && dev->type->pm && dev->type->pm->prepare) {
+	if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "preparing type ");
-		error = dev->type->pm->prepare(dev);
+		if (dev->type->pm->prepare)
+			error = dev->type->pm->prepare(dev);
 		suspend_report_result(dev->type->pm->prepare, error);
 		if (error)
 			goto End;
-	}
-
-	if (dev->class && dev->class->pm && dev->class->pm->prepare) {
+	} else if (dev->class && dev->class->pm) {
 		pm_dev_dbg(dev, state, "preparing class ");
-		error = dev->class->pm->prepare(dev);
+		if (dev->class->pm->prepare)
+			error = dev->class->pm->prepare(dev);
 		suspend_report_result(dev->class->pm->prepare, error);
 		if (error)
 			goto End;
+	} else if (dev->bus && dev->bus->pm) {
+		pm_dev_dbg(dev, state, "preparing ");
+		if (dev->bus->pm->prepare)
+			error = dev->bus->pm->prepare(dev);
+		suspend_report_result(dev->bus->pm->prepare, error);
+		if (error)
+			goto End;
 	}
 
 	if (dev->pwr_domain && dev->pwr_domain->ops.prepare) {
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -214,12 +214,12 @@ static int rpm_idle(struct device *dev,
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
-		callback = dev->bus->pm->runtime_idle;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
+	if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_idle;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_idle;
+	else if (dev->bus && dev->bus->pm)
+		callback = dev->bus->pm->runtime_idle;
 	else
 		callback = NULL;
 
@@ -382,12 +382,12 @@ static int rpm_suspend(struct device *de
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
-		callback = dev->bus->pm->runtime_suspend;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_suspend)
+	if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_suspend;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_suspend;
+	else if (dev->bus && dev->bus->pm)
+		callback = dev->bus->pm->runtime_suspend;
 	else
 		callback = NULL;
 
@@ -584,12 +584,12 @@ static int rpm_resume(struct device *dev
 	if (dev->pwr_domain)
 		rpm_callback(dev->pwr_domain->ops.runtime_resume, dev);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
-		callback = dev->bus->pm->runtime_resume;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_resume)
+	if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_resume;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_resume;
+	else if (dev->bus && dev->bus->pm)
+		callback = dev->bus->pm->runtime_resume;
 	else
 		callback = NULL;
 
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -249,23 +249,18 @@ various phases always run after tasks ha
 unfrozen.  Furthermore, the *_noirq phases run at a time when IRQ handlers have
 been disabled (except for those marked with the IRQ_WAKEUP flag).
 
-Most phases use bus, type, and class callbacks (that is, methods defined in
-dev->bus->pm, dev->type->pm, and dev->class->pm).  The prepare and complete
-phases are exceptions; they use only bus callbacks.  When multiple callbacks
-are used in a phase, they are invoked in the order: <class, type, bus> during
-power-down transitions and in the opposite order during power-up transitions.
-For example, during the suspend phase the PM core invokes
-
-	dev->class->pm.suspend(dev);
-	dev->type->pm.suspend(dev);
-	dev->bus->pm.suspend(dev);
-
-before moving on to the next device, whereas during the resume phase the core
-invokes
-
-	dev->bus->pm.resume(dev);
-	dev->type->pm.resume(dev);
-	dev->class->pm.resume(dev);
+All phases use bus, type, or class callbacks (that is, methods defined in
+dev->bus->pm, dev->type->pm, or dev->class->pm).  These callbacks are mutually
+exclusive, so if the device type provides a struct dev_pm_ops object pointed to
+by its pm field (i.e. both dev->type and dev->type->pm are defined), the
+callbacks included in that object (i.e. dev->type->pm) will be used.  Otherwise,
+if the class provides a struct dev_pm_ops object pointed to by its pm field
+(i.e. both dev->class and dev->class->pm are defined), the PM core will use the
+callbacks from that object (i.e. dev->class->pm).  Finally, if the pm fields of
+both the device type and class objects are NULL (or those objects do not exist),
+the callbacks provided by the bus (that is, the callbacks from dev->bus->pm)
+will be used (this allows device types to override callbacks provided by bus
+types or classes if necessary).
 
 These callbacks may in turn invoke device- or driver-specific methods stored in
 dev->driver->pm, but they don't have to.
Index: linux-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- linux-2.6.orig/Documentation/power/runtime_pm.txt
+++ linux-2.6/Documentation/power/runtime_pm.txt
@@ -1,6 +1,6 @@
 Run-time Power Management Framework for I/O Devices
 
-(C) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
+(C) 2009-2011 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
 (C) 2010 Alan Stern <stern@rowland.harvard.edu>
 
 1. Introduction
@@ -44,11 +44,12 @@ struct dev_pm_ops {
 };
 
 The ->runtime_suspend(), ->runtime_resume() and ->runtime_idle() callbacks are
-executed by the PM core for either the bus type, or device type (if the bus
-type's callback is not defined), or device class (if the bus type's and device
-type's callbacks are not defined) of given device.  The bus type, device type
-and device class callbacks are referred to as subsystem-level callbacks in what
-follows.
+executed by the PM core for either the device type, or the class (if the device
+type's struct dev_pm_ops object does not exist), or the bus type (if the
+device type's and class' struct dev_pm_ops objects do not exist) of the given
+device (this allows device types to override callbacks provided by bus types or
+classes if necessary).  The bus type, device type and class callbacks are
+referred to as subsystem-level callbacks in what follows.
 
 By default, the callbacks are always invoked in process context with interrupts
 enabled.  However, subsystems can use the pm_runtime_irq_safe() helper function

Re: [PATCH] PM: Make system-wide PM and runtime PM treat subsystems consistently

[Greg KH]

On Fri, Feb 18, 2011 at 12:54:25AM +0100, R. J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The code handling system-wide power transitions (eg. suspend-to-RAM)
> can in theory execute callbacks provided by the device's bus type,
> device type and class in each phase of the power transition.  In
> turn, the runtime PM core code only calls one of those callbacks at
> a time, preferring bus type callbacks to device type or class
> callbacks and device type callbacks to class callbacks.
> 
> It seems reasonable to make them both behave in the same way in that
> respect.  Moreover, even though a device may belong to two subsystems
> (eg. bus type and device class) simultaneously, in practice power
> management callbacks for system-wide power transitions are always
> provided by only one of them (ie. if the bus type callbacks are
> defined, the device class ones are not and vice versa).  Thus it is
> possible to modify the code handling system-wide power transitions
> so that it follows the core runtime PM code (ie. treats the
> subsystem callbacks as mutually exclusive).
> 
> On the other hand, the core runtime PM code will choose to execute,
> for example, a runtime suspend callback provided by the device type
> even if the bus type's struct dev_pm_ops object exists, but the
> runtime_suspend pointer in it happens to be NULL.  This is confusing,
> because it may lead to the execution of callbacks from different
> subsystems during different operations (eg. the bus type suspend
> callback may be executed during runtime suspend of the device, while
> the device type callback will be executed during system suspend).
> 
> Make all of the power management code treat subsystem callbacks in
> a consistent way, such that:
> (1) If the device's type is defined (eg. dev->type is not NULL)
>     and its pm pointer is not NULL, the callbacks from dev->type->pm
>     will be used.
> (2) If dev->type is NULL or dev->type->pm is NULL, but the device's
>     class is defined (eg. dev->class is not NULL) and its pm pointer
>     is not NULL, the callbacks from dev->class->pm will be used.
> (3) If dev->type is NULL or dev->type->pm is NULL and dev->class is
>     NULL or dev->class->pm is NULL, the callbacks from dev->bus->pm
>     will be used provided that both dev->bus and dev->bus->pm are
>     not NULL.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Acked-by: Kevin Hilman <khilman@ti.com>
> Reasoning-sounds-sane-to: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

You are going to take this through your tree, right?

thanks,

greg k-h

Re: [PATCH] PM: Make system-wide PM and runtime PM treat subsystems consistently

[Rafael J. Wysocki]

On Friday, February 18, 2011, Greg KH wrote:
> On Fri, Feb 18, 2011 at 12:54:25AM +0100, R. J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The code handling system-wide power transitions (eg. suspend-to-RAM)
> > can in theory execute callbacks provided by the device's bus type,
> > device type and class in each phase of the power transition.  In
> > turn, the runtime PM core code only calls one of those callbacks at
> > a time, preferring bus type callbacks to device type or class
> > callbacks and device type callbacks to class callbacks.
> > 
> > It seems reasonable to make them both behave in the same way in that
> > respect.  Moreover, even though a device may belong to two subsystems
> > (eg. bus type and device class) simultaneously, in practice power
> > management callbacks for system-wide power transitions are always
> > provided by only one of them (ie. if the bus type callbacks are
> > defined, the device class ones are not and vice versa).  Thus it is
> > possible to modify the code handling system-wide power transitions
> > so that it follows the core runtime PM code (ie. treats the
> > subsystem callbacks as mutually exclusive).
> > 
> > On the other hand, the core runtime PM code will choose to execute,
> > for example, a runtime suspend callback provided by the device type
> > even if the bus type's struct dev_pm_ops object exists, but the
> > runtime_suspend pointer in it happens to be NULL.  This is confusing,
> > because it may lead to the execution of callbacks from different
> > subsystems during different operations (eg. the bus type suspend
> > callback may be executed during runtime suspend of the device, while
> > the device type callback will be executed during system suspend).
> > 
> > Make all of the power management code treat subsystem callbacks in
> > a consistent way, such that:
> > (1) If the device's type is defined (eg. dev->type is not NULL)
> >     and its pm pointer is not NULL, the callbacks from dev->type->pm
> >     will be used.
> > (2) If dev->type is NULL or dev->type->pm is NULL, but the device's
> >     class is defined (eg. dev->class is not NULL) and its pm pointer
> >     is not NULL, the callbacks from dev->class->pm will be used.
> > (3) If dev->type is NULL or dev->type->pm is NULL and dev->class is
> >     NULL or dev->class->pm is NULL, the callbacks from dev->bus->pm
> >     will be used provided that both dev->bus and dev->bus->pm are
> >     not NULL.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > Acked-by: Kevin Hilman <khilman@ti.com>
> > Reasoning-sounds-sane-to: Grant Likely <grant.likely@secretlab.ca>
> 
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> 
> You are going to take this through your tree, right?

Yes, I am.  Thanks!

Rafael

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

On Wednesday, February 16, 2011, Alan Stern wrote:
> On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:
> 
> > On Wednesday, February 16, 2011, Alan Stern wrote:
> > > On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:
> > > 
> > > > Unfortunately, it doesn't work on my Acer Ferrari One.  The problem is that
> > > > hcd_pci_suspend() fails for the EHCI controller, apparently because the root
> > > > hub is not suspended.  Do root hubs need both class suspend and bus type
> > > > suspend to work at the same time?
> > > 
> > > No, only the bus type suspend method is needed.
> > 
> > Bus type or device type?  It appears to be the latter from reading the code
> > (although admittedly not too thorough).
> 
> You're right.  I forgot about how the PM methods were split up.  :-(
> 
> > > Can you provide the dmesg log using a kernel built with CONFIG_USB_DEBUG?
> > 
> > Well, I know what the problem is.
> > 
> > USB defines usb_device_type pointing to usb_device_pm_ops that provides
> > system-wide PM callbacks only and usb_bus_type pointing to
> > usb_bus_pm_ops that provides runtime PM callbacks only.  So it looks like
> > we should move either the system-wide PM callbacks to usb_bus_pm_ops,
> > or the runtime PM callbacks to usb_device_pm_ops.
> 
> Yes.  IIRC, I did it that way so that the runtime PM routines could be 
> static.  Making them non-static isn't a big deal, though.
> 
> > FWIW, the appended patch helps on my test machine, but I'm not sure if it
> > is the right thing to do.
> 
> It is.  Except that the inline stubs aren't needed for anything; they 
> don't have to be added to usb.h.

OK, I'll remove them, add a changelog and repost.

> > Apart from this I think the order of checks introduced by the $subject patch
> > should be:
> > (1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
> >     or otherwise
> > (2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
> >     or otherwise
> > (3) use dev->bus (if present).
> > as that would allow classes and device types to override bus type PM
> > callbacks if they wish to.
> 
> I haven't heard of any device types being present on more than one kind
> of bus, so it makes sense for device types to override bus types.

OK

> But I'm not so sure about the priority we should give to classes.  On the
> other hand, if no classes define a dev_pm_ops then of course it doesn't
> matter.

The change will also affect classes that provide "legacy" suspend-resume
(if there are any, which I'm totally unsure of).

Anyway, I think we need to choose one ordering. :-)

What about type / bus / class , then?

Rafael

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Alan Stern]

On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:

> On Wednesday, February 16, 2011, Alan Stern wrote:
> > On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:
> > 
> > > Unfortunately, it doesn't work on my Acer Ferrari One.  The problem is that
> > > hcd_pci_suspend() fails for the EHCI controller, apparently because the root
> > > hub is not suspended.  Do root hubs need both class suspend and bus type
> > > suspend to work at the same time?
> > 
> > No, only the bus type suspend method is needed.
> 
> Bus type or device type?  It appears to be the latter from reading the code
> (although admittedly not too thorough).

You're right.  I forgot about how the PM methods were split up.  :-(

> > Can you provide the dmesg log using a kernel built with CONFIG_USB_DEBUG?
> 
> Well, I know what the problem is.
> 
> USB defines usb_device_type pointing to usb_device_pm_ops that provides
> system-wide PM callbacks only and usb_bus_type pointing to
> usb_bus_pm_ops that provides runtime PM callbacks only.  So it looks like
> we should move either the system-wide PM callbacks to usb_bus_pm_ops,
> or the runtime PM callbacks to usb_device_pm_ops.

Yes.  IIRC, I did it that way so that the runtime PM routines could be 
static.  Making them non-static isn't a big deal, though.

> FWIW, the appended patch helps on my test machine, but I'm not sure if it
> is the right thing to do.

It is.  Except that the inline stubs aren't needed for anything; they 
don't have to be added to usb.h.

> Apart from this I think the order of checks introduced by the $subject patch
> should be:
> (1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
>     or otherwise
> (2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
>     or otherwise
> (3) use dev->bus (if present).
> as that would allow classes and device types to override bus type PM
> callbacks if they wish to.

I haven't heard of any device types being present on more than one kind
of bus, so it makes sense for device types to override bus types.  But
I'm not so sure about the priority we should give to classes.  On the
other hand, if no classes define a dev_pm_ops then of course it doesn't
matter.

Alan Stern

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

On Wednesday, February 16, 2011, Alan Stern wrote:
> On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:
> 
> > Unfortunately, it doesn't work on my Acer Ferrari One.  The problem is that
> > hcd_pci_suspend() fails for the EHCI controller, apparently because the root
> > hub is not suspended.  Do root hubs need both class suspend and bus type
> > suspend to work at the same time?
> 
> No, only the bus type suspend method is needed.

Bus type or device type?  It appears to be the latter from reading the code
(although admittedly not too thorough).

> Can you provide the dmesg log using a kernel built with CONFIG_USB_DEBUG?

Well, I know what the problem is.

USB defines usb_device_type pointing to usb_device_pm_ops that provides
system-wide PM callbacks only and usb_bus_type pointing to
usb_bus_pm_ops that provides runtime PM callbacks only.  So it looks like
we should move either the system-wide PM callbacks to usb_bus_pm_ops,
or the runtime PM callbacks to usb_device_pm_ops.

FWIW, the appended patch helps on my test machine, but I'm not sure if it
is the right thing to do.

Apart from this I think the order of checks introduced by the $subject patch
should be:
(1) If dev->class != NULL and dev->class->pm != NULL, use dev->class,
    or otherwise
(2) if dev->type != NULL and dev->type->pm != NULL, use dev->type,
    or otherwise
(3) use dev->bus (if present).
as that would allow classes and device types to override bus type PM
callbacks if they wish to.

Thanks,
Rafael

---
 drivers/usb/core/driver.c |   15 +++------------
 drivers/usb/core/usb.c    |    5 +++++
 drivers/usb/core/usb.h    |   18 ++++++++++++++++++
 3 files changed, 26 insertions(+), 12 deletions(-)

Index: linux-2.6/drivers/usb/core/driver.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/driver.c
+++ linux-2.6/drivers/usb/core/driver.c
@@ -1646,7 +1646,7 @@ static int autosuspend_check(struct usb_
 	return 0;
 }
 
-static int usb_runtime_suspend(struct device *dev)
+int usb_runtime_suspend(struct device *dev)
 {
 	struct usb_device	*udev = to_usb_device(dev);
 	int			status;
@@ -1662,7 +1662,7 @@ static int usb_runtime_suspend(struct de
 	return status;
 }
 
-static int usb_runtime_resume(struct device *dev)
+int usb_runtime_resume(struct device *dev)
 {
 	struct usb_device	*udev = to_usb_device(dev);
 	int			status;
@@ -1674,7 +1674,7 @@ static int usb_runtime_resume(struct dev
 	return status;
 }
 
-static int usb_runtime_idle(struct device *dev)
+int usb_runtime_idle(struct device *dev)
 {
 	struct usb_device	*udev = to_usb_device(dev);
 
@@ -1686,19 +1686,10 @@ static int usb_runtime_idle(struct devic
 	return 0;
 }
 
-static const struct dev_pm_ops usb_bus_pm_ops = {
-	.runtime_suspend =	usb_runtime_suspend,
-	.runtime_resume =	usb_runtime_resume,
-	.runtime_idle =		usb_runtime_idle,
-};
-
 #endif /* CONFIG_USB_SUSPEND */
 
 struct bus_type usb_bus_type = {
 	.name =		"usb",
 	.match =	usb_device_match,
 	.uevent =	usb_uevent,
-#ifdef CONFIG_USB_SUSPEND
-	.pm =		&usb_bus_pm_ops,
-#endif
 };
Index: linux-2.6/drivers/usb/core/usb.h
===================================================================
--- linux-2.6.orig/drivers/usb/core/usb.h
+++ linux-2.6/drivers/usb/core/usb.h
@@ -77,6 +77,9 @@ static inline int usb_port_resume(struct
 extern void usb_autosuspend_device(struct usb_device *udev);
 extern int usb_autoresume_device(struct usb_device *udev);
 extern int usb_remote_wakeup(struct usb_device *dev);
+extern int usb_runtime_suspend(struct device *dev);
+extern int usb_runtime_resume(struct device *dev);
+extern int usb_runtime_idle(struct device *dev);
 
 #else
 
@@ -90,6 +93,21 @@ static inline int usb_remote_wakeup(stru
 {
 	return 0;
 }
+
+static inline int usb_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static inline int usb_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static inline int usb_runtime_idle(struct device *dev)
+{
+	return 0;
+}
 
 #endif
 
Index: linux-2.6/drivers/usb/core/usb.c
===================================================================
--- linux-2.6.orig/drivers/usb/core/usb.c
+++ linux-2.6/drivers/usb/core/usb.c
@@ -315,6 +315,11 @@ static const struct dev_pm_ops usb_devic
 	.thaw =		usb_dev_thaw,
 	.poweroff =	usb_dev_poweroff,
 	.restore =	usb_dev_restore,
+#ifdef CONFIG_USB_SUSPEND
+	.runtime_suspend =	usb_runtime_suspend,
+	.runtime_resume =	usb_runtime_resume,
+	.runtime_idle =		usb_runtime_idle,
+#endif
 };
 
 #endif	/* CONFIG_PM */

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Alan Stern]

On Wed, 16 Feb 2011, Rafael J. Wysocki wrote:

> Unfortunately, it doesn't work on my Acer Ferrari One.  The problem is that
> hcd_pci_suspend() fails for the EHCI controller, apparently because the root
> hub is not suspended.  Do root hubs need both class suspend and bus type
> suspend to work at the same time?

No, only the bus type suspend method is needed.  Can you provide the
dmesg log using a kernel built with CONFIG_USB_DEBUG?

Alan Stern

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

On Monday, February 14, 2011, Alan Stern wrote:
> On Sat, 12 Feb 2011, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The code handling system-wide power transitions (eg. suspend-to-RAM)
> > can in theory execute callbacks provided by the device's bus type,
> > device type and class in each phase of the power transition.  In
> > turn, the runtime PM core code only calls one of those callbacks at
> > a time, preferring bus type callbacks to device type or class
> > callbacks and device type callbacks to class callbacks.
> > 
> > It seems reasonable to make them both behave in the same way in that
> > respect.  Moreover, even though a device may belong to two subsystems
> > (eg. bus type and device class) simultaneously, in practice power
> > management callbacks for system-wide power transitions are always
> > provided by only one of them (ie. if the bus type callbacks are
> > defined, the device class ones are not and vice versa).  Thus it is
> > possible to modify the code handling system-wide power transitions
> > so that it follows the core runtime PM code (ie. treats the
> > subsystem callbacks as mutually exclusive).
> > 
> > On the other hand, the core runtime PM code will choose to execute,
> > for example, a runtime suspend callback provided by the device type
> > even if the bus type's struct dev_pm_ops object exists, but the
> > runtime_suspend pointer in it happens to be NULL.  This is confusing,
> > because it may lead to the execution of callbacks from different
> > subsystems during different operations (eg. the bus type suspend
> > callback may be executed during runtime suspend, while the device
> > type callback will be executed during runtime resume).
> > 
> > Make all of the power management code treat subsystem callbacks in
> > a consistent way, such that:
> > (1) If the device's bus type is defined (eg. dev->bus is not NULL)
> >     and its pm pointer is not NULL, the callbacks from dev->bus->pm
> >     will be used.
> > (2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
> >     device type is defined (eg. dev->type is not NULL) and its pm
> >     pointer is not NULL, the callbacks from dev->type->pm will be
> >     used.
> > (3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
> >     NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
> >     will be used provided that both dev->class and dev->class->pm
> >     are not NULL.
> 
> It looks okay, but I haven't tested it.  Just one minor change needed 
> in the documentation:
> 
> > +All phases use bus, type, or class callbacks (that is, methods defined in
> > +dev->bus->pm, dev->type->pm, or dev->class->pm).  These callbacks are mutually
> > +exclusive, so if the bus provides a struct dev_pm_ops object pointed to by its
> > +pm field (i.e. both dev->bus and dev->bus->pm are defined), the callbacks
> > +included in that object (i.e. dev->bus->pm) will be used.  In turn, if the
> 
> s/In turn/Otherwise/

OK

> > +device type provides a struct dev_pm_ops object pointed to by its pm field
> > +(i.e. both dev->type and dev->type->pm are defined), the PM core will used the
> > +callbacks from that object (i.e. dev->type->pm).  Finally, if the pm fields of
> > +both the bus and device type objects are NULL (or those objects do not exist),
> > +the callbacks provided by the class (that is, the callbacks from dev->class->pm)
> > +will be used.
> >  
> >  These callbacks may in turn invoke device- or driver-specific methods stored in
> >  dev->driver->pm, but they don't have to.

Thanks,
Rafael

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

On Monday, February 14, 2011, Alan Stern wrote:
> On Sat, 12 Feb 2011, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > The code handling system-wide power transitions (eg. suspend-to-RAM)
> > can in theory execute callbacks provided by the device's bus type,
> > device type and class in each phase of the power transition.  In
> > turn, the runtime PM core code only calls one of those callbacks at
> > a time, preferring bus type callbacks to device type or class
> > callbacks and device type callbacks to class callbacks.
> > 
> > It seems reasonable to make them both behave in the same way in that
> > respect.  Moreover, even though a device may belong to two subsystems
> > (eg. bus type and device class) simultaneously, in practice power
> > management callbacks for system-wide power transitions are always
> > provided by only one of them (ie. if the bus type callbacks are
> > defined, the device class ones are not and vice versa).  Thus it is
> > possible to modify the code handling system-wide power transitions
> > so that it follows the core runtime PM code (ie. treats the
> > subsystem callbacks as mutually exclusive).
> > 
> > On the other hand, the core runtime PM code will choose to execute,
> > for example, a runtime suspend callback provided by the device type
> > even if the bus type's struct dev_pm_ops object exists, but the
> > runtime_suspend pointer in it happens to be NULL.  This is confusing,
> > because it may lead to the execution of callbacks from different
> > subsystems during different operations (eg. the bus type suspend
> > callback may be executed during runtime suspend, while the device
> > type callback will be executed during runtime resume).
> > 
> > Make all of the power management code treat subsystem callbacks in
> > a consistent way, such that:
> > (1) If the device's bus type is defined (eg. dev->bus is not NULL)
> >     and its pm pointer is not NULL, the callbacks from dev->bus->pm
> >     will be used.
> > (2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
> >     device type is defined (eg. dev->type is not NULL) and its pm
> >     pointer is not NULL, the callbacks from dev->type->pm will be
> >     used.
> > (3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
> >     NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
> >     will be used provided that both dev->class and dev->class->pm
> >     are not NULL.
> 
> It looks okay, but I haven't tested it.

Unfortunately, it doesn't work on my Acer Ferrari One.  The problem is that
hcd_pci_suspend() fails for the EHCI controller, apparently because the root
hub is not suspended.  Do root hubs need both class suspend and bus type
suspend to work at the same time?

Rafael

[RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Rafael J. Wysocki]

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

The code handling system-wide power transitions (eg. suspend-to-RAM)
can in theory execute callbacks provided by the device's bus type,
device type and class in each phase of the power transition.  In
turn, the runtime PM core code only calls one of those callbacks at
a time, preferring bus type callbacks to device type or class
callbacks and device type callbacks to class callbacks.

It seems reasonable to make them both behave in the same way in that
respect.  Moreover, even though a device may belong to two subsystems
(eg. bus type and device class) simultaneously, in practice power
management callbacks for system-wide power transitions are always
provided by only one of them (ie. if the bus type callbacks are
defined, the device class ones are not and vice versa).  Thus it is
possible to modify the code handling system-wide power transitions
so that it follows the core runtime PM code (ie. treats the
subsystem callbacks as mutually exclusive).

On the other hand, the core runtime PM code will choose to execute,
for example, a runtime suspend callback provided by the device type
even if the bus type's struct dev_pm_ops object exists, but the
runtime_suspend pointer in it happens to be NULL.  This is confusing,
because it may lead to the execution of callbacks from different
subsystems during different operations (eg. the bus type suspend
callback may be executed during runtime suspend, while the device
type callback will be executed during runtime resume).

Make all of the power management code treat subsystem callbacks in
a consistent way, such that:
(1) If the device's bus type is defined (eg. dev->bus is not NULL)
    and its pm pointer is not NULL, the callbacks from dev->bus->pm
    will be used.
(2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
    device type is defined (eg. dev->type is not NULL) and its pm
    pointer is not NULL, the callbacks from dev->type->pm will be
    used.
(3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
    NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
    will be used provided that both dev->class and dev->class->pm
    are not NULL.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/devices.txt |   28 +++----
 drivers/base/power/main.c       |  141 +++++++++++++++++-----------------------
 drivers/base/power/runtime.c    |   12 +--
 3 files changed, 79 insertions(+), 102 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -433,21 +433,17 @@ static int device_resume_noirq(struct de
 		error = pm_noirq_op(dev, dev->bus->pm, state);
 		if (error)
 			goto End;
-	}
-
-	if (dev->type && dev->type->pm) {
+	} else if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "EARLY type ");
 		error = pm_noirq_op(dev, dev->type->pm, state);
 		if (error)
 			goto End;
-	}
-
-	if (dev->class && dev->class->pm) {
+	} else if (dev->class && dev->class->pm) {
 		pm_dev_dbg(dev, state, "EARLY class ");
 		error = pm_noirq_op(dev, dev->class->pm, state);
 	}
 
-End:
+ End:
 	TRACE_RESUME(error);
 	return error;
 }
@@ -532,21 +528,18 @@ static int device_resume(struct device *
 		if (dev->bus->pm) {
 			pm_dev_dbg(dev, state, "");
 			error = pm_op(dev, dev->bus->pm, state);
+			goto End;
 		} else if (dev->bus->resume) {
 			pm_dev_dbg(dev, state, "legacy ");
 			error = legacy_resume(dev, dev->bus->resume);
-		}
-		if (error)
 			goto End;
+		}
 	}
 
-	if (dev->type) {
-		if (dev->type->pm) {
-			pm_dev_dbg(dev, state, "type ");
-			error = pm_op(dev, dev->type->pm, state);
-		}
-		if (error)
-			goto End;
+	if (dev->type && dev->type->pm) {
+		pm_dev_dbg(dev, state, "type ");
+		error = pm_op(dev, dev->type->pm, state);
+		goto End;
 	}
 
 	if (dev->class) {
@@ -558,6 +551,7 @@ static int device_resume(struct device *
 			error = legacy_resume(dev, dev->class->resume);
 		}
 	}
+
  End:
 	device_unlock(dev);
 	complete_all(&dev->power.completion);
@@ -644,19 +638,18 @@ static void device_complete(struct devic
 		dev->pwr_domain->ops.complete(dev);
 	}
 
-	if (dev->class && dev->class->pm && dev->class->pm->complete) {
-		pm_dev_dbg(dev, state, "completing class ");
-		dev->class->pm->complete(dev);
-	}
-
-	if (dev->type && dev->type->pm && dev->type->pm->complete) {
-		pm_dev_dbg(dev, state, "completing type ");
-		dev->type->pm->complete(dev);
-	}
-
-	if (dev->bus && dev->bus->pm && dev->bus->pm->complete) {
+	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "completing ");
-		dev->bus->pm->complete(dev);
+		if (dev->bus->pm->complete)
+			dev->bus->pm->complete(dev);
+	} else if (dev->type && dev->type->pm) {
+		pm_dev_dbg(dev, state, "completing type ");
+		if (dev->type->pm->complete)
+			dev->type->pm->complete(dev);
+	} else if (dev->class && dev->class->pm) {
+		pm_dev_dbg(dev, state, "completing class ");
+		if (dev->class->pm->complete)
+			dev->class->pm->complete(dev);
 	}
 
 	device_unlock(dev);
@@ -741,27 +734,23 @@ static pm_message_t resume_event(pm_mess
  */
 static int device_suspend_noirq(struct device *dev, pm_message_t state)
 {
-	int error = 0;
+	int error;
 
-	if (dev->class && dev->class->pm) {
-		pm_dev_dbg(dev, state, "LATE class ");
-		error = pm_noirq_op(dev, dev->class->pm, state);
+	if (dev->bus && dev->bus->pm) {
+		pm_dev_dbg(dev, state, "LATE ");
+		error = pm_noirq_op(dev, dev->bus->pm, state);
 		if (error)
-			goto End;
-	}
-
-	if (dev->type && dev->type->pm) {
+			return error;
+	} else if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "LATE type ");
 		error = pm_noirq_op(dev, dev->type->pm, state);
 		if (error)
-			goto End;
-	}
-
-	if (dev->bus && dev->bus->pm) {
-		pm_dev_dbg(dev, state, "LATE ");
-		error = pm_noirq_op(dev, dev->bus->pm, state);
+			return error;
+	} else if (dev->class && dev->class->pm) {
+		pm_dev_dbg(dev, state, "LATE class ");
+		error = pm_noirq_op(dev, dev->class->pm, state);
 		if (error)
-			goto End;
+			return error;
 	}
 
 	if (dev->pwr_domain) {
@@ -769,8 +758,7 @@ static int device_suspend_noirq(struct d
 		pm_noirq_op(dev, &dev->pwr_domain->ops, state);
 	}
 
-End:
-	return error;
+	return 0;
 }
 
 /**
@@ -857,40 +845,36 @@ static int __device_suspend(struct devic
 		goto End;
 	}
 
-	if (dev->class) {
-		if (dev->class->pm) {
-			pm_dev_dbg(dev, state, "class ");
-			error = pm_op(dev, dev->class->pm, state);
-		} else if (dev->class->suspend) {
-			pm_dev_dbg(dev, state, "legacy class ");
-			error = legacy_suspend(dev, state, dev->class->suspend);
-		}
-		if (error)
-			goto End;
-	}
-
-	if (dev->type) {
-		if (dev->type->pm) {
-			pm_dev_dbg(dev, state, "type ");
-			error = pm_op(dev, dev->type->pm, state);
-		}
-		if (error)
-			goto End;
-	}
-
 	if (dev->bus) {
 		if (dev->bus->pm) {
 			pm_dev_dbg(dev, state, "");
 			error = pm_op(dev, dev->bus->pm, state);
+			goto Domain;
 		} else if (dev->bus->suspend) {
 			pm_dev_dbg(dev, state, "legacy ");
 			error = legacy_suspend(dev, state, dev->bus->suspend);
+			goto Domain;
 		}
-		if (error)
-			goto End;
 	}
 
-	if (dev->pwr_domain) {
+	if (dev->type && dev->type->pm) {
+		pm_dev_dbg(dev, state, "type ");
+		error = pm_op(dev, dev->type->pm, state);
+		goto Domain;
+	}
+
+	if (dev->class) {
+		if (dev->class->pm) {
+			pm_dev_dbg(dev, state, "class ");
+			error = pm_op(dev, dev->class->pm, state);
+		} else if (dev->class->suspend) {
+			pm_dev_dbg(dev, state, "legacy class ");
+			error = legacy_suspend(dev, state, dev->class->suspend);
+		}
+	}
+
+ Domain:
+	if (!error && dev->pwr_domain) {
 		pm_dev_dbg(dev, state, "power domain ");
 		pm_op(dev, &dev->pwr_domain->ops, state);
 	}
@@ -985,25 +969,24 @@ static int device_prepare(struct device
 
 	device_lock(dev);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->prepare) {
+	if (dev->bus && dev->bus->pm) {
 		pm_dev_dbg(dev, state, "preparing ");
-		error = dev->bus->pm->prepare(dev);
+		if (dev->bus->pm->prepare)
+			error = dev->bus->pm->prepare(dev);
 		suspend_report_result(dev->bus->pm->prepare, error);
 		if (error)
 			goto End;
-	}
-
-	if (dev->type && dev->type->pm && dev->type->pm->prepare) {
+	} else if (dev->type && dev->type->pm) {
 		pm_dev_dbg(dev, state, "preparing type ");
-		error = dev->type->pm->prepare(dev);
+		if (dev->type->pm->prepare)
+			error = dev->type->pm->prepare(dev);
 		suspend_report_result(dev->type->pm->prepare, error);
 		if (error)
 			goto End;
-	}
-
-	if (dev->class && dev->class->pm && dev->class->pm->prepare) {
+	} else if (dev->class && dev->class->pm) {
 		pm_dev_dbg(dev, state, "preparing class ");
-		error = dev->class->pm->prepare(dev);
+		if (dev->class->pm->prepare)
+			error = dev->class->pm->prepare(dev);
 		suspend_report_result(dev->class->pm->prepare, error);
 		if (error)
 			goto End;
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -214,9 +214,9 @@ static int rpm_idle(struct device *dev,
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+	if (dev->bus && dev->bus->pm)
 		callback = dev->bus->pm->runtime_idle;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
+	else if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_idle;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_idle;
@@ -382,9 +382,9 @@ static int rpm_suspend(struct device *de
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
+	if (dev->bus && dev->bus->pm)
 		callback = dev->bus->pm->runtime_suspend;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_suspend)
+	else if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_suspend;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_suspend;
@@ -584,9 +584,9 @@ static int rpm_resume(struct device *dev
 	if (dev->pwr_domain)
 		rpm_callback(dev->pwr_domain->ops.runtime_resume, dev);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
+	if (dev->bus && dev->bus->pm)
 		callback = dev->bus->pm->runtime_resume;
-	else if (dev->type && dev->type->pm && dev->type->pm->runtime_resume)
+	else if (dev->type && dev->type->pm)
 		callback = dev->type->pm->runtime_resume;
 	else if (dev->class && dev->class->pm)
 		callback = dev->class->pm->runtime_resume;
Index: linux-2.6/Documentation/power/devices.txt
===================================================================
--- linux-2.6.orig/Documentation/power/devices.txt
+++ linux-2.6/Documentation/power/devices.txt
@@ -249,23 +249,17 @@ various phases always run after tasks ha
 unfrozen.  Furthermore, the *_noirq phases run at a time when IRQ handlers have
 been disabled (except for those marked with the IRQ_WAKEUP flag).
 
-Most phases use bus, type, and class callbacks (that is, methods defined in
-dev->bus->pm, dev->type->pm, and dev->class->pm).  The prepare and complete
-phases are exceptions; they use only bus callbacks.  When multiple callbacks
-are used in a phase, they are invoked in the order: <class, type, bus> during
-power-down transitions and in the opposite order during power-up transitions.
-For example, during the suspend phase the PM core invokes
-
-	dev->class->pm.suspend(dev);
-	dev->type->pm.suspend(dev);
-	dev->bus->pm.suspend(dev);
-
-before moving on to the next device, whereas during the resume phase the core
-invokes
-
-	dev->bus->pm.resume(dev);
-	dev->type->pm.resume(dev);
-	dev->class->pm.resume(dev);
+All phases use bus, type, or class callbacks (that is, methods defined in
+dev->bus->pm, dev->type->pm, or dev->class->pm).  These callbacks are mutually
+exclusive, so if the bus provides a struct dev_pm_ops object pointed to by its
+pm field (i.e. both dev->bus and dev->bus->pm are defined), the callbacks
+included in that object (i.e. dev->bus->pm) will be used.  In turn, if the
+device type provides a struct dev_pm_ops object pointed to by its pm field
+(i.e. both dev->type and dev->type->pm are defined), the PM core will used the
+callbacks from that object (i.e. dev->type->pm).  Finally, if the pm fields of
+both the bus and device type objects are NULL (or those objects do not exist),
+the callbacks provided by the class (that is, the callbacks from dev->class->pm)
+will be used.
 
 These callbacks may in turn invoke device- or driver-specific methods stored in
 dev->driver->pm, but they don't have to.

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Alan Stern]

On Sat, 12 Feb 2011, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The code handling system-wide power transitions (eg. suspend-to-RAM)
> can in theory execute callbacks provided by the device's bus type,
> device type and class in each phase of the power transition.  In
> turn, the runtime PM core code only calls one of those callbacks at
> a time, preferring bus type callbacks to device type or class
> callbacks and device type callbacks to class callbacks.
> 
> It seems reasonable to make them both behave in the same way in that
> respect.  Moreover, even though a device may belong to two subsystems
> (eg. bus type and device class) simultaneously, in practice power
> management callbacks for system-wide power transitions are always
> provided by only one of them (ie. if the bus type callbacks are
> defined, the device class ones are not and vice versa).  Thus it is
> possible to modify the code handling system-wide power transitions
> so that it follows the core runtime PM code (ie. treats the
> subsystem callbacks as mutually exclusive).
> 
> On the other hand, the core runtime PM code will choose to execute,
> for example, a runtime suspend callback provided by the device type
> even if the bus type's struct dev_pm_ops object exists, but the
> runtime_suspend pointer in it happens to be NULL.  This is confusing,
> because it may lead to the execution of callbacks from different
> subsystems during different operations (eg. the bus type suspend
> callback may be executed during runtime suspend, while the device
> type callback will be executed during runtime resume).
> 
> Make all of the power management code treat subsystem callbacks in
> a consistent way, such that:
> (1) If the device's bus type is defined (eg. dev->bus is not NULL)
>     and its pm pointer is not NULL, the callbacks from dev->bus->pm
>     will be used.
> (2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
>     device type is defined (eg. dev->type is not NULL) and its pm
>     pointer is not NULL, the callbacks from dev->type->pm will be
>     used.
> (3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
>     NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
>     will be used provided that both dev->class and dev->class->pm
>     are not NULL.

It looks okay, but I haven't tested it.  Just one minor change needed 
in the documentation:

> +All phases use bus, type, or class callbacks (that is, methods defined in
> +dev->bus->pm, dev->type->pm, or dev->class->pm).  These callbacks are mutually
> +exclusive, so if the bus provides a struct dev_pm_ops object pointed to by its
> +pm field (i.e. both dev->bus and dev->bus->pm are defined), the callbacks
> +included in that object (i.e. dev->bus->pm) will be used.  In turn, if the

s/In turn/Otherwise/

> +device type provides a struct dev_pm_ops object pointed to by its pm field
> +(i.e. both dev->type and dev->type->pm are defined), the PM core will used the
> +callbacks from that object (i.e. dev->type->pm).  Finally, if the pm fields of
> +both the bus and device type objects are NULL (or those objects do not exist),
> +the callbacks provided by the class (that is, the callbacks from dev->class->pm)
> +will be used.
>  
>  These callbacks may in turn invoke device- or driver-specific methods stored in
>  dev->driver->pm, but they don't have to.

Alan Stern

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> The code handling system-wide power transitions (eg. suspend-to-RAM)
> can in theory execute callbacks provided by the device's bus type,
> device type and class in each phase of the power transition.  In
> turn, the runtime PM core code only calls one of those callbacks at
> a time, preferring bus type callbacks to device type or class
> callbacks and device type callbacks to class callbacks.
>
> It seems reasonable to make them both behave in the same way in that
> respect.  Moreover, even though a device may belong to two subsystems
> (eg. bus type and device class) simultaneously, in practice power
> management callbacks for system-wide power transitions are always
> provided by only one of them (ie. if the bus type callbacks are
> defined, the device class ones are not and vice versa).  Thus it is
> possible to modify the code handling system-wide power transitions
> so that it follows the core runtime PM code (ie. treats the
> subsystem callbacks as mutually exclusive).
>
> On the other hand, the core runtime PM code will choose to execute,
> for example, a runtime suspend callback provided by the device type
> even if the bus type's struct dev_pm_ops object exists, but the
> runtime_suspend pointer in it happens to be NULL.  This is confusing,
> because it may lead to the execution of callbacks from different
> subsystems during different operations (eg. the bus type suspend
> callback may be executed during runtime suspend, while the device
> type callback will be executed during runtime resume).
>
> Make all of the power management code treat subsystem callbacks in
> a consistent way, such that:
> (1) If the device's bus type is defined (eg. dev->bus is not NULL)
>     and its pm pointer is not NULL, the callbacks from dev->bus->pm
>     will be used.
> (2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
>     device type is defined (eg. dev->type is not NULL) and its pm
>     pointer is not NULL, the callbacks from dev->type->pm will be
>     used.
> (3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
>     NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
>     will be used provided that both dev->class and dev->class->pm
>     are not NULL.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

This looks good,  consistency between system and runtime PM is a great
help to readability.

Acked-by: Kevin Hilman <khilman@ti.com>


> ---
>  Documentation/power/devices.txt |   28 +++----
>  drivers/base/power/main.c       |  141 +++++++++++++++++-----------------------
>  drivers/base/power/runtime.c    |   12 +--
>  3 files changed, 79 insertions(+), 102 deletions(-)
>
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -433,21 +433,17 @@ static int device_resume_noirq(struct de
>  		error = pm_noirq_op(dev, dev->bus->pm, state);
>  		if (error)
>  			goto End;
> -	}
> -
> -	if (dev->type && dev->type->pm) {
> +	} else if (dev->type && dev->type->pm) {
>  		pm_dev_dbg(dev, state, "EARLY type ");
>  		error = pm_noirq_op(dev, dev->type->pm, state);
>  		if (error)
>  			goto End;
> -	}
> -
> -	if (dev->class && dev->class->pm) {
> +	} else if (dev->class && dev->class->pm) {
>  		pm_dev_dbg(dev, state, "EARLY class ");
>  		error = pm_noirq_op(dev, dev->class->pm, state);
>  	}
>  
> -End:
> + End:
>  	TRACE_RESUME(error);
>  	return error;
>  }
> @@ -532,21 +528,18 @@ static int device_resume(struct device *
>  		if (dev->bus->pm) {
>  			pm_dev_dbg(dev, state, "");
>  			error = pm_op(dev, dev->bus->pm, state);
> +			goto End;
>  		} else if (dev->bus->resume) {
>  			pm_dev_dbg(dev, state, "legacy ");
>  			error = legacy_resume(dev, dev->bus->resume);
> -		}
> -		if (error)
>  			goto End;
> +		}
>  	}
>  
> -	if (dev->type) {
> -		if (dev->type->pm) {
> -			pm_dev_dbg(dev, state, "type ");
> -			error = pm_op(dev, dev->type->pm, state);
> -		}
> -		if (error)
> -			goto End;
> +	if (dev->type && dev->type->pm) {
> +		pm_dev_dbg(dev, state, "type ");
> +		error = pm_op(dev, dev->type->pm, state);
> +		goto End;
>  	}
>  
>  	if (dev->class) {
> @@ -558,6 +551,7 @@ static int device_resume(struct device *
>  			error = legacy_resume(dev, dev->class->resume);
>  		}
>  	}
> +
>   End:
>  	device_unlock(dev);
>  	complete_all(&dev->power.completion);
> @@ -644,19 +638,18 @@ static void device_complete(struct devic
>  		dev->pwr_domain->ops.complete(dev);
>  	}
>  
> -	if (dev->class && dev->class->pm && dev->class->pm->complete) {
> -		pm_dev_dbg(dev, state, "completing class ");
> -		dev->class->pm->complete(dev);
> -	}
> -
> -	if (dev->type && dev->type->pm && dev->type->pm->complete) {
> -		pm_dev_dbg(dev, state, "completing type ");
> -		dev->type->pm->complete(dev);
> -	}
> -
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->complete) {
> +	if (dev->bus && dev->bus->pm) {
>  		pm_dev_dbg(dev, state, "completing ");
> -		dev->bus->pm->complete(dev);
> +		if (dev->bus->pm->complete)
> +			dev->bus->pm->complete(dev);
> +	} else if (dev->type && dev->type->pm) {
> +		pm_dev_dbg(dev, state, "completing type ");
> +		if (dev->type->pm->complete)
> +			dev->type->pm->complete(dev);
> +	} else if (dev->class && dev->class->pm) {
> +		pm_dev_dbg(dev, state, "completing class ");
> +		if (dev->class->pm->complete)
> +			dev->class->pm->complete(dev);
>  	}
>  
>  	device_unlock(dev);
> @@ -741,27 +734,23 @@ static pm_message_t resume_event(pm_mess
>   */
>  static int device_suspend_noirq(struct device *dev, pm_message_t state)
>  {
> -	int error = 0;
> +	int error;
>  
> -	if (dev->class && dev->class->pm) {
> -		pm_dev_dbg(dev, state, "LATE class ");
> -		error = pm_noirq_op(dev, dev->class->pm, state);
> +	if (dev->bus && dev->bus->pm) {
> +		pm_dev_dbg(dev, state, "LATE ");
> +		error = pm_noirq_op(dev, dev->bus->pm, state);
>  		if (error)
> -			goto End;
> -	}
> -
> -	if (dev->type && dev->type->pm) {
> +			return error;
> +	} else if (dev->type && dev->type->pm) {
>  		pm_dev_dbg(dev, state, "LATE type ");
>  		error = pm_noirq_op(dev, dev->type->pm, state);
>  		if (error)
> -			goto End;
> -	}
> -
> -	if (dev->bus && dev->bus->pm) {
> -		pm_dev_dbg(dev, state, "LATE ");
> -		error = pm_noirq_op(dev, dev->bus->pm, state);
> +			return error;
> +	} else if (dev->class && dev->class->pm) {
> +		pm_dev_dbg(dev, state, "LATE class ");
> +		error = pm_noirq_op(dev, dev->class->pm, state);
>  		if (error)
> -			goto End;
> +			return error;
>  	}
>  
>  	if (dev->pwr_domain) {
> @@ -769,8 +758,7 @@ static int device_suspend_noirq(struct d
>  		pm_noirq_op(dev, &dev->pwr_domain->ops, state);
>  	}
>  
> -End:
> -	return error;
> +	return 0;
>  }
>  
>  /**
> @@ -857,40 +845,36 @@ static int __device_suspend(struct devic
>  		goto End;
>  	}
>  
> -	if (dev->class) {
> -		if (dev->class->pm) {
> -			pm_dev_dbg(dev, state, "class ");
> -			error = pm_op(dev, dev->class->pm, state);
> -		} else if (dev->class->suspend) {
> -			pm_dev_dbg(dev, state, "legacy class ");
> -			error = legacy_suspend(dev, state, dev->class->suspend);
> -		}
> -		if (error)
> -			goto End;
> -	}
> -
> -	if (dev->type) {
> -		if (dev->type->pm) {
> -			pm_dev_dbg(dev, state, "type ");
> -			error = pm_op(dev, dev->type->pm, state);
> -		}
> -		if (error)
> -			goto End;
> -	}
> -
>  	if (dev->bus) {
>  		if (dev->bus->pm) {
>  			pm_dev_dbg(dev, state, "");
>  			error = pm_op(dev, dev->bus->pm, state);
> +			goto Domain;
>  		} else if (dev->bus->suspend) {
>  			pm_dev_dbg(dev, state, "legacy ");
>  			error = legacy_suspend(dev, state, dev->bus->suspend);
> +			goto Domain;
>  		}
> -		if (error)
> -			goto End;
>  	}
>  
> -	if (dev->pwr_domain) {
> +	if (dev->type && dev->type->pm) {
> +		pm_dev_dbg(dev, state, "type ");
> +		error = pm_op(dev, dev->type->pm, state);
> +		goto Domain;
> +	}
> +
> +	if (dev->class) {
> +		if (dev->class->pm) {
> +			pm_dev_dbg(dev, state, "class ");
> +			error = pm_op(dev, dev->class->pm, state);
> +		} else if (dev->class->suspend) {
> +			pm_dev_dbg(dev, state, "legacy class ");
> +			error = legacy_suspend(dev, state, dev->class->suspend);
> +		}
> +	}
> +
> + Domain:
> +	if (!error && dev->pwr_domain) {
>  		pm_dev_dbg(dev, state, "power domain ");
>  		pm_op(dev, &dev->pwr_domain->ops, state);
>  	}
> @@ -985,25 +969,24 @@ static int device_prepare(struct device
>  
>  	device_lock(dev);
>  
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->prepare) {
> +	if (dev->bus && dev->bus->pm) {
>  		pm_dev_dbg(dev, state, "preparing ");
> -		error = dev->bus->pm->prepare(dev);
> +		if (dev->bus->pm->prepare)
> +			error = dev->bus->pm->prepare(dev);
>  		suspend_report_result(dev->bus->pm->prepare, error);
>  		if (error)
>  			goto End;
> -	}
> -
> -	if (dev->type && dev->type->pm && dev->type->pm->prepare) {
> +	} else if (dev->type && dev->type->pm) {
>  		pm_dev_dbg(dev, state, "preparing type ");
> -		error = dev->type->pm->prepare(dev);
> +		if (dev->type->pm->prepare)
> +			error = dev->type->pm->prepare(dev);
>  		suspend_report_result(dev->type->pm->prepare, error);
>  		if (error)
>  			goto End;
> -	}
> -
> -	if (dev->class && dev->class->pm && dev->class->pm->prepare) {
> +	} else if (dev->class && dev->class->pm) {
>  		pm_dev_dbg(dev, state, "preparing class ");
> -		error = dev->class->pm->prepare(dev);
> +		if (dev->class->pm->prepare)
> +			error = dev->class->pm->prepare(dev);
>  		suspend_report_result(dev->class->pm->prepare, error);
>  		if (error)
>  			goto End;
> Index: linux-2.6/drivers/base/power/runtime.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/runtime.c
> +++ linux-2.6/drivers/base/power/runtime.c
> @@ -214,9 +214,9 @@ static int rpm_idle(struct device *dev,
>  
>  	dev->power.idle_notification = true;
>  
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
> +	if (dev->bus && dev->bus->pm)
>  		callback = dev->bus->pm->runtime_idle;
> -	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
> +	else if (dev->type && dev->type->pm)
>  		callback = dev->type->pm->runtime_idle;
>  	else if (dev->class && dev->class->pm)
>  		callback = dev->class->pm->runtime_idle;
> @@ -382,9 +382,9 @@ static int rpm_suspend(struct device *de
>  
>  	__update_runtime_status(dev, RPM_SUSPENDING);
>  
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
> +	if (dev->bus && dev->bus->pm)
>  		callback = dev->bus->pm->runtime_suspend;
> -	else if (dev->type && dev->type->pm && dev->type->pm->runtime_suspend)
> +	else if (dev->type && dev->type->pm)
>  		callback = dev->type->pm->runtime_suspend;
>  	else if (dev->class && dev->class->pm)
>  		callback = dev->class->pm->runtime_suspend;
> @@ -584,9 +584,9 @@ static int rpm_resume(struct device *dev
>  	if (dev->pwr_domain)
>  		rpm_callback(dev->pwr_domain->ops.runtime_resume, dev);
>  
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
> +	if (dev->bus && dev->bus->pm)
>  		callback = dev->bus->pm->runtime_resume;
> -	else if (dev->type && dev->type->pm && dev->type->pm->runtime_resume)
> +	else if (dev->type && dev->type->pm)
>  		callback = dev->type->pm->runtime_resume;
>  	else if (dev->class && dev->class->pm)
>  		callback = dev->class->pm->runtime_resume;
> Index: linux-2.6/Documentation/power/devices.txt
> ===================================================================
> --- linux-2.6.orig/Documentation/power/devices.txt
> +++ linux-2.6/Documentation/power/devices.txt
> @@ -249,23 +249,17 @@ various phases always run after tasks ha
>  unfrozen.  Furthermore, the *_noirq phases run at a time when IRQ handlers have
>  been disabled (except for those marked with the IRQ_WAKEUP flag).
>  
> -Most phases use bus, type, and class callbacks (that is, methods defined in
> -dev->bus->pm, dev->type->pm, and dev->class->pm).  The prepare and complete
> -phases are exceptions; they use only bus callbacks.  When multiple callbacks
> -are used in a phase, they are invoked in the order: <class, type, bus> during
> -power-down transitions and in the opposite order during power-up transitions.
> -For example, during the suspend phase the PM core invokes
> -
> -	dev->class->pm.suspend(dev);
> -	dev->type->pm.suspend(dev);
> -	dev->bus->pm.suspend(dev);
> -
> -before moving on to the next device, whereas during the resume phase the core
> -invokes
> -
> -	dev->bus->pm.resume(dev);
> -	dev->type->pm.resume(dev);
> -	dev->class->pm.resume(dev);
> +All phases use bus, type, or class callbacks (that is, methods defined in
> +dev->bus->pm, dev->type->pm, or dev->class->pm).  These callbacks are mutually
> +exclusive, so if the bus provides a struct dev_pm_ops object pointed to by its
> +pm field (i.e. both dev->bus and dev->bus->pm are defined), the callbacks
> +included in that object (i.e. dev->bus->pm) will be used.  In turn, if the
> +device type provides a struct dev_pm_ops object pointed to by its pm field
> +(i.e. both dev->type and dev->type->pm are defined), the PM core will used the
> +callbacks from that object (i.e. dev->type->pm).  Finally, if the pm fields of
> +both the bus and device type objects are NULL (or those objects do not exist),
> +the callbacks provided by the class (that is, the callbacks from dev->class->pm)
> +will be used.
>  
>  These callbacks may in turn invoke device- or driver-specific methods stored in
>  dev->driver->pm, but they don't have to.

Re: [RFC][PATCH 2/2] PM: Make system-wide PM and runtime PM handle subsystems consistently

[Grant Likely]

On Tue, Feb 15, 2011 at 10:10:06AM -0800, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > The code handling system-wide power transitions (eg. suspend-to-RAM)
> > can in theory execute callbacks provided by the device's bus type,
> > device type and class in each phase of the power transition.  In
> > turn, the runtime PM core code only calls one of those callbacks at
> > a time, preferring bus type callbacks to device type or class
> > callbacks and device type callbacks to class callbacks.
> >
> > It seems reasonable to make them both behave in the same way in that
> > respect.  Moreover, even though a device may belong to two subsystems
> > (eg. bus type and device class) simultaneously, in practice power
> > management callbacks for system-wide power transitions are always
> > provided by only one of them (ie. if the bus type callbacks are
> > defined, the device class ones are not and vice versa).  Thus it is
> > possible to modify the code handling system-wide power transitions
> > so that it follows the core runtime PM code (ie. treats the
> > subsystem callbacks as mutually exclusive).
> >
> > On the other hand, the core runtime PM code will choose to execute,
> > for example, a runtime suspend callback provided by the device type
> > even if the bus type's struct dev_pm_ops object exists, but the
> > runtime_suspend pointer in it happens to be NULL.  This is confusing,
> > because it may lead to the execution of callbacks from different
> > subsystems during different operations (eg. the bus type suspend
> > callback may be executed during runtime suspend, while the device
> > type callback will be executed during runtime resume).
> >
> > Make all of the power management code treat subsystem callbacks in
> > a consistent way, such that:
> > (1) If the device's bus type is defined (eg. dev->bus is not NULL)
> >     and its pm pointer is not NULL, the callbacks from dev->bus->pm
> >     will be used.
> > (2) If dev->bus is NULL or dev->bus->pm is NULL, but the device's
> >     device type is defined (eg. dev->type is not NULL) and its pm
> >     pointer is not NULL, the callbacks from dev->type->pm will be
> >     used.
> > (3) If dev->bus is NULL or dev->bus->pm is NULL and dev->type is
> >     NULL or dev->type->pm is NULL, the callbacks from dev->class->pm
> >     will be used provided that both dev->class and dev->class->pm
> >     are not NULL.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> This looks good,  consistency between system and runtime PM is a great
> help to readability.
> 
> Acked-by: Kevin Hilman <khilman@ti.com>

Reasoning-sounds-sane-to: Grant Likely <grant.likely@secretlab.ca>