Message-ID: <1858154.Dd0R3NaGoS@vostro.rjw.lan>
Date:	Wed, 04 Dec 2013 00:41:34 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Ulf Hansson <ulf.hansson@...aro.org>
Cc:	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	linux-pm@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Kevin Hilman <khilman@...aro.org>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH 1/5] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

On Wednesday, December 04, 2013 12:15:13 AM Rafael J. Wysocki wrote:
> On Wednesday, November 27, 2013 04:34:56 PM Ulf Hansson wrote:
> > To put devices into low power state during sleep, it sometimes makes
> > sense at subsystem-level to re-use the runtime PM callbacks.
> > 
> > The PM core will at device_suspend_late disable runtime PM, after that
> > we can safely operate on these callbacks. At suspend_late the device
> > will be put into low power state by invoking the runtime_suspend
> > callbacks, unless the runtime status is already suspended. At
> > resume_early the state is restored by invoking the runtime_resume
> > callbacks. Soon after the PM core will re-enable runtime PM before
> > returning from device_resume_early.
> > 
> > The new pm_generic functions are named pm_generic_suspend_late_runtime
> > and pm_generic_resume_early_runtime, they are supposed to be used in
> > pairs.
> > 
> > Do note that these new generic callbacks will work smothly even with
> > and without CONFIG_PM_RUNTIME, as long as the runtime PM callbacks are
> > implemented inside CONFIG_PM instead of CONFIG_PM_RUNTIME.
> > 
> > A special thanks to Alan Stern who came up with this idea.
> > 
> > Cc: Kevin Hilman <khilman@...aro.org>
> > Cc: Alan Stern <stern@...land.harvard.edu>
> > Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> > ---
> >  drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm.h               |    2 +
> >  2 files changed, 88 insertions(+)
> > 
> > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > index 5ee030a..8e78ad1 100644
> > --- a/drivers/base/power/generic_ops.c
> > +++ b/drivers/base/power/generic_ops.c
> > @@ -93,6 +93,49 @@ int pm_generic_suspend_late(struct device *dev)
> >  EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
> 
> After a bit of reconsideration it appears to me that the only benefit of that
> would be to make it possible for drivers to work around incomplete PM domains
> implementations.  Which would be of questionable value.
> 
> >  /**
> > + * pm_generic_suspend_late_runtime - Generic suspend_late callback for drivers
> > + * @dev: Device to suspend.
> > + * Use to invoke runtime_suspend callbacks at suspend_late.
> > + */
> > +int pm_generic_suspend_late_runtime(struct device *dev)
> > +{
> > +	int (*callback)(struct device *);
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * PM core has disabled runtime PM in device_suspend_late, thus we can
> > +	 * safely check the device's runtime status and decide whether
> > +	 * additional actions are needed to put the device into low power state.
> > +	 * If so, we invoke the runtime_suspend callbacks.
> > +	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
> > +	 * returns false and therefore the runtime_suspend callback will be
> > +	 * invoked.
> > +	 */
> > +	if (pm_runtime_status_suspended(dev))
> > +		return 0;
> > +
> > +	if (dev->pm_domain)
> > +		callback = dev->pm_domain->ops.runtime_suspend;
> 
> First of all, for this to work you need to assume that the type/bus/class will
> not exercise hardware in any way by itself (or you can look at the PCI bus type
> for an example why it won't generally work otherwise), so you could simply skip
> the rest of this conditional.
> 
> For the bus types you are concerned about (platform and AMBA) that actually is
> the case as far as I can say.
> 
> > +	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;
> > +	else if (dev->bus && dev->bus->pm)
> > +		callback = dev->bus->pm->runtime_suspend;
> > +	else
> > +		callback = NULL;
> > +
> > +	if (!callback && dev->driver && dev->driver->pm)
> > +		callback = dev->driver->pm->runtime_suspend;
> > +
> > +	if (callback)
> > +		ret = callback(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_generic_suspend_late_runtime);
> 
> After that you're left with something like this:
> 
> int prototype_suspend_late(struct device *dev)
> {
> 	int (*callback)(struct device *);
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_suspend;
> 	else if (dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_suspend;
> 	else
> 		callback = NULL;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> Moreover, if a driver's .suspend_late is pointed to the above, it can be called
> in two ways.  The first way is via type/bus/class or the PM core itself which
> means that all PM handling at this stage is going to be done by
> prototype_suspend_late().  The other way is via a PM domain, in which case
> there may be some additional PM handling in the domain code in principle
> (before or after calling the driver's .suspend_late()).  However, in the
> latter case it generally wouldn't be OK to call PM domain's .runtime_suspend(),
> because that may interfere with the PM handling in its .suspend_late().  So
> again, this pretty much requires that the PM domain's .suspend_late pointer be
> NULL or point to a routine that simply executes the driver's callback and does
> noting in addition to that.
> 
> Now, I suspect that if the driver's runtime_suspend callback is
> driver_runtime_suspend() and the PM domain's runtime_suspend callback is
> pm_domain_runtime_suspend(), the code you actually want to be executed at the
> "late suspend" stage will be
> 
> 	if (!pm_runtime_status_suspended(dev))
> 		pm_domain_runtime_suspend()
> 			driver_runtime_suspend()
> 
> (where the assumption is that pm_domain_runtime_suspend() will call
> driver_runtime_suspend() for you).  Clearly, prototype_suspend_late() will lead
> to that effective result in the conditions in which it makes sense to use it.
> 
> So, instead of pointing the driver's .suspend_late() to prototype_suspend_late(),
> why don't you point the .suspend_late of the *PM* *domain* to the following
> slightly modified version of that function:
> 
> int pm_domain_simplified_suspend_late(struct device *dev)
> {
> 	int (*callback)(struct device *) = NULL;
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	/* Try to execute our own .runtime_suspend() callback. */
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_suspend;
> 
> 	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_suspend;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> This will cause exactly the code you need to be executed too (with
> a fallback to direct execution of the driver's callback in broken
> cases where the PM domain's own .runtime_suspend() is not implemented),
> but in a much cleaner way (no going back to the code layer that has just
> called you etc.).  And you *can* point the PM domain's .suspend_late
> to this, because it has to be either empty of trivial if the
> prototype_suspend_late() above is supposed to work as the driver's
> .suspend_late() callback.
> 
> So in my opinion, if you point the .suspend_late callback pointers of the
> PM domains in question to the pm_domain_simplified_suspend_late() above
> and their .resume_early callback pointers to the following function:
> 
> int pm_domain_simplified_resume_early(struct device *dev)
> {
> 	int (*callback)(struct device *) = NULL;
> 
> 	if (pm_runtime_status_suspended(dev))
> 		return 0;
> 
> 	if (dev->pm_domain)
> 		callback = dev->pm_domain->ops.runtime_resume;
> 
> 	if (WARN_ON(!callback) && dev->driver && dev->driver->pm)
> 		callback = dev->driver->pm->runtime_resume;
> 
> 	return callback ? callback(dev) : 0;
> }
> 
> it will solve your problems without that horrible jumping between code
> layers.

And in addition to that you can point the drivers' .suspend_late callbacks to
something like:

int pm_simplified_suspend_late(struct device *dev)
{ 
 	if (pm_runtime_status_suspended(dev))
 		return 0;

 	return dev->driver->pm->runtime_suspend ?
 		dev->driver->pm->runtime_suspend(dev) : 0;
}

(and analogously for the "early resume") which will cause their .runtime_suspend()
to be executed even if the PM domain doesn't have a .suspend_late (or there's
no PM domain at all).

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists