[v2] PM / Sleep: Add pm_generic functions to re-use runtime PM callbacks

To put devices into low power state during sleep, it sometimes makes
sense at subsystem-level to re-use device's 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 device's
runtime_suspend callback, unless the runtime status is already
suspended. At resume_early the state is restored by invoking the
device's runtime_resume callback. 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@linaro.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/generic_ops.c |   86 ++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h               |    2 +
 2 files changed, 88 insertions(+)

On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
> To put devices into low power state during sleep, it sometimes makes
> sense at subsystem-level to re-use device's 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 device's
> runtime_suspend callback, unless the runtime status is already
> suspended. At resume_early the state is restored by invoking the
> device's runtime_resume callback. 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.

What happens if the subsystem uses the new functions as its late suspend/
early resume callbacks, but some of its drivers implement .suspend_late()
and .resume_early()?

Also, these are system suspend/resume callbacks, so the "runtime" part of
the names is kind of confusing.  I have no idea for better naming at this
point, but still.

> 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@linaro.org>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.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..3132768 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);
>  
>  /**
> + * pm_generic_suspend_late_runtime - Generic suspend_late callback for
> + * subsystems that wants to use runtime_suspend callbacks at suspend_late.

This has to be one line.  You can add more description below the @dev line.

> + * @dev: Device to suspend.
> + */
> +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 device's runtime_suspend callback.
> +	 * 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;
> +	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);
> +
> +/**
>   * pm_generic_suspend - Generic suspend callback for subsystems.
>   * @dev: Device to suspend.
>   */
> @@ -237,6 +280,49 @@ int pm_generic_resume_early(struct device *dev)
>  EXPORT_SYMBOL_GPL(pm_generic_resume_early);
>  
>  /**
> + * pm_generic_resume_early_runtime - Generic resume_early callback for
> + * subsystems that wants to use runtime_resume callbacks at resume_early.

Same here.

> + * @dev: Device to resume.
> + */
> +int pm_generic_resume_early_runtime(struct device *dev)
> +{
> +	int (*callback)(struct device *);
> +	int ret = 0;
> +
> +	/*
> +	 * PM core has not yet enabled runtime PM in device_resume_early,
> +	 * thus we can safely check the device's runtime status and restore the
> +	 * previous state we had in device_suspend_late. If restore is needed
> +	 * we invoke the device's runtime_resume callback.
> +	 * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
> +	 * returns false and therefore the runtime_resume callback will be
> +	 * invoked.
> +	 */
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;
> +
> +	if (dev->pm_domain)
> +		callback = dev->pm_domain->ops.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;
> +	else if (dev->bus && dev->bus->pm)
> +		callback = dev->bus->pm->runtime_resume;
> +	else
> +		callback = NULL;
> +
> +	if (!callback && dev->driver && dev->driver->pm)
> +		callback = dev->driver->pm->runtime_resume;
> +
> +	if (callback)
> +		ret = callback(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(pm_generic_resume_early_runtime);
> +
> +/**
>   * pm_generic_resume - Generic resume callback for subsystems.
>   * @dev: Device to resume.
>   */
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index a224c7f..5bce0d4 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -656,9 +656,11 @@ extern void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *));
>  
>  extern int pm_generic_prepare(struct device *dev);
>  extern int pm_generic_suspend_late(struct device *dev);
> +extern int pm_generic_suspend_late_runtime(struct device *dev);
>  extern int pm_generic_suspend_noirq(struct device *dev);
>  extern int pm_generic_suspend(struct device *dev);
>  extern int pm_generic_resume_early(struct device *dev);
> +extern int pm_generic_resume_early_runtime(struct device *dev);
>  extern int pm_generic_resume_noirq(struct device *dev);
>  extern int pm_generic_resume(struct device *dev);
>  extern int pm_generic_freeze_noirq(struct device *dev);

Thanks!

On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
>> To put devices into low power state during sleep, it sometimes makes
>> sense at subsystem-level to re-use device's 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 device's
>> runtime_suspend callback, unless the runtime status is already
>> suspended. At resume_early the state is restored by invoking the
>> device's runtime_resume callback. 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.
>
> What happens if the subsystem uses the new functions as its late suspend/
> early resume callbacks, but some of its drivers implement .suspend_late()
> and .resume_early()?

Good point. Normally, I think the decision of using these callbacks
should be taken at the lowest level, in other words in the driver.
Otherwise the lowest layer of .suspend_late will be by-passed.

Do you think "subsystem-level" is too vague to indicate that? Should
we say "driver-level" in the functions comment field instead?

>
> Also, these are system suspend/resume callbacks, so the "runtime" part of
> the names is kind of confusing.  I have no idea for better naming at this
> point, but still.

Somehow I would like to indicate that it is actually the runtime
callbacks that will be indirectly invoked. Besides that, I would
happily change to whatever you propose. :-)

>
>> 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@linaro.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.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..3132768 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);
>>
>>  /**
>> + * pm_generic_suspend_late_runtime - Generic suspend_late callback for
>> + * subsystems that wants to use runtime_suspend callbacks at suspend_late.
>
> This has to be one line.  You can add more description below the @dev line.

OK

>
>> + * @dev: Device to suspend.
>> + */
>> +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 device's runtime_suspend callback.
>> +      * 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;
>> +     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);
>> +
>> +/**
>>   * pm_generic_suspend - Generic suspend callback for subsystems.
>>   * @dev: Device to suspend.
>>   */
>> @@ -237,6 +280,49 @@ int pm_generic_resume_early(struct device *dev)
>>  EXPORT_SYMBOL_GPL(pm_generic_resume_early);
>>
>>  /**
>> + * pm_generic_resume_early_runtime - Generic resume_early callback for
>> + * subsystems that wants to use runtime_resume callbacks at resume_early.
>
> Same here.

OK

>
>> + * @dev: Device to resume.
>> + */
>> +int pm_generic_resume_early_runtime(struct device *dev)
>> +{
>> +     int (*callback)(struct device *);
>> +     int ret = 0;
>> +
>> +     /*
>> +      * PM core has not yet enabled runtime PM in device_resume_early,
>> +      * thus we can safely check the device's runtime status and restore the
>> +      * previous state we had in device_suspend_late. If restore is needed
>> +      * we invoke the device's runtime_resume callback.
>> +      * For the !CONFIG_PM_RUNTIME case, pm_runtime_status_suspended() always
>> +      * returns false and therefore the runtime_resume callback will be
>> +      * invoked.
>> +      */
>> +     if (pm_runtime_status_suspended(dev))
>> +             return 0;
>> +
>> +     if (dev->pm_domain)
>> +             callback = dev->pm_domain->ops.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;
>> +     else if (dev->bus && dev->bus->pm)
>> +             callback = dev->bus->pm->runtime_resume;
>> +     else
>> +             callback = NULL;
>> +
>> +     if (!callback && dev->driver && dev->driver->pm)
>> +             callback = dev->driver->pm->runtime_resume;
>> +
>> +     if (callback)
>> +             ret = callback(dev);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(pm_generic_resume_early_runtime);
>> +
>> +/**
>>   * pm_generic_resume - Generic resume callback for subsystems.
>>   * @dev: Device to resume.
>>   */
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index a224c7f..5bce0d4 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -656,9 +656,11 @@ extern void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *));
>>
>>  extern int pm_generic_prepare(struct device *dev);
>>  extern int pm_generic_suspend_late(struct device *dev);
>> +extern int pm_generic_suspend_late_runtime(struct device *dev);
>>  extern int pm_generic_suspend_noirq(struct device *dev);
>>  extern int pm_generic_suspend(struct device *dev);
>>  extern int pm_generic_resume_early(struct device *dev);
>> +extern int pm_generic_resume_early_runtime(struct device *dev);
>>  extern int pm_generic_resume_noirq(struct device *dev);
>>  extern int pm_generic_resume(struct device *dev);
>>  extern int pm_generic_freeze_noirq(struct device *dev);
>

Thanks for the comments, I will submit a new version soon.

Kind regards
Ulf Hansson



> Thanks!
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 26, 2013 10:48:56 AM Ulf Hansson wrote:
> On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
> >> To put devices into low power state during sleep, it sometimes makes
> >> sense at subsystem-level to re-use device's 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 device's
> >> runtime_suspend callback, unless the runtime status is already
> >> suspended. At resume_early the state is restored by invoking the
> >> device's runtime_resume callback. 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.
> >
> > What happens if the subsystem uses the new functions as its late suspend/
> > early resume callbacks, but some of its drivers implement .suspend_late()
> > and .resume_early()?
> 
> Good point. Normally, I think the decision of using these callbacks
> should be taken at the lowest level, in other words in the driver.
> Otherwise the lowest layer of .suspend_late will be by-passed.

I'm not sure what's the point to add them, then.  If the driver has to decide
anyway, it may simply populate its .suspend_late and .resume_early pointers
I think.

Thanks!

On Tue, 26 Nov 2013, Rafael J. Wysocki wrote:

> On Tuesday, November 26, 2013 10:48:56 AM Ulf Hansson wrote:
> > On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
> > >> To put devices into low power state during sleep, it sometimes makes
> > >> sense at subsystem-level to re-use device's runtime PM callbacks.

Ulf, please be more careful about the distinction between "device" and 
"driver".  Devices don't have callbacks; drivers do.  You made this 
mistake a few times in the patch description.

> > >> 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 device's
> > >> runtime_suspend callback, unless the runtime status is already
> > >> suspended. At resume_early the state is restored by invoking the
> > >> device's runtime_resume callback. 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.
> > >
> > > What happens if the subsystem uses the new functions as its late suspend/
> > > early resume callbacks, but some of its drivers implement .suspend_late()
> > > and .resume_early()?

The same thing that happens whenever any PM-related callback is defined
by both the subsystem and the driver: The PM core uses the subsystem's
callback.  (Or the pm_domain's callback, or the type's callback, or the 
class's callback.)

Besides, Ulf specifically assumed at the top of this message that
re-using the runtime-suspend callback makes sense at the subsystem
level.  This must mean it is appropriate for all drivers in the
subsystem.  So why would a driver want to override the subsystem's 
behavior?

> > Good point. Normally, I think the decision of using these callbacks
> > should be taken at the lowest level, in other words in the driver.
> > Otherwise the lowest layer of .suspend_late will be by-passed.
> 
> I'm not sure what's the point to add them, then.  If the driver has to decide
> anyway, it may simply populate its .suspend_late and .resume_early pointers
> I think.

Another possibility is to have the pm_generic_suspend_late_runtime
routine check to see if the driver has defined a suspend_late callback.  
If the driver has its own callback, we could invoke it instead of
invoking the runtime_suspend callback.

However, I'm not sure that doing this would be worthwhile.  And it
would hang in an infinite loop if the driver set its suspend_late
pointer to pm_generic_suspend_late_runtime!

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 26 November 2013 22:09, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 26 Nov 2013, Rafael J. Wysocki wrote:
>
>> On Tuesday, November 26, 2013 10:48:56 AM Ulf Hansson wrote:
>> > On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > > On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
>> > >> To put devices into low power state during sleep, it sometimes makes
>> > >> sense at subsystem-level to re-use device's runtime PM callbacks.
>
> Ulf, please be more careful about the distinction between "device" and
> "driver".  Devices don't have callbacks; drivers do.  You made this
> mistake a few times in the patch description.

Sorry, my bad.

>
>> > >> 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 device's
>> > >> runtime_suspend callback, unless the runtime status is already
>> > >> suspended. At resume_early the state is restored by invoking the
>> > >> device's runtime_resume callback. 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.
>> > >
>> > > What happens if the subsystem uses the new functions as its late suspend/
>> > > early resume callbacks, but some of its drivers implement .suspend_late()
>> > > and .resume_early()?
>
> The same thing that happens whenever any PM-related callback is defined
> by both the subsystem and the driver: The PM core uses the subsystem's
> callback.  (Or the pm_domain's callback, or the type's callback, or the
> class's callback.)
>
> Besides, Ulf specifically assumed at the top of this message that
> re-using the runtime-suspend callback makes sense at the subsystem
> level.  This must mean it is appropriate for all drivers in the
> subsystem.  So why would a driver want to override the subsystem's
> behavior?
>
>> > Good point. Normally, I think the decision of using these callbacks
>> > should be taken at the lowest level, in other words in the driver.
>> > Otherwise the lowest layer of .suspend_late will be by-passed.
>>
>> I'm not sure what's the point to add them, then.  If the driver has to decide
>> anyway, it may simply populate its .suspend_late and .resume_early pointers
>> I think.
>
> Another possibility is to have the pm_generic_suspend_late_runtime
> routine check to see if the driver has defined a suspend_late callback.
> If the driver has its own callback, we could invoke it instead of
> invoking the runtime_suspend callback.

There are already a generic callbacks that does this. Normally the
generic callbacks is used from buses and power domains.

My new callbacks are intended to be used from the driver, so those are
kind of different from the others in that sense.

>
> However, I'm not sure that doing this would be worthwhile.  And it
> would hang in an infinite loop if the driver set its suspend_late
> pointer to pm_generic_suspend_late_runtime!

:-)

>
> Alan Stern
>

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 26, 2013 04:09:44 PM Alan Stern wrote:
> On Tue, 26 Nov 2013, Rafael J. Wysocki wrote:
> 
> > On Tuesday, November 26, 2013 10:48:56 AM Ulf Hansson wrote:
> > > On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
> > > >> To put devices into low power state during sleep, it sometimes makes
> > > >> sense at subsystem-level to re-use device's runtime PM callbacks.
> 
> Ulf, please be more careful about the distinction between "device" and 
> "driver".  Devices don't have callbacks; drivers do.  You made this 
> mistake a few times in the patch description.
> 
> > > >> 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 device's
> > > >> runtime_suspend callback, unless the runtime status is already
> > > >> suspended. At resume_early the state is restored by invoking the
> > > >> device's runtime_resume callback. 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.
> > > >
> > > > What happens if the subsystem uses the new functions as its late suspend/
> > > > early resume callbacks, but some of its drivers implement .suspend_late()
> > > > and .resume_early()?
> 
> The same thing that happens whenever any PM-related callback is defined
> by both the subsystem and the driver: The PM core uses the subsystem's
> callback.  (Or the pm_domain's callback, or the type's callback, or the 
> class's callback.)

Precisely.  But my question was more about how we are going to indicate it
to the driver writers (who may not be familiar with that subsystem to start
with and may have experience with some other subsystems that don't use
these new functions) that they are not supposed to populate .suspend_late
and .resume_early.

> Besides, Ulf specifically assumed at the top of this message that
> re-using the runtime-suspend callback makes sense at the subsystem
> level.  This must mean it is appropriate for all drivers in the
> subsystem.  So why would a driver want to override the subsystem's 
> behavior?

By mistake?

> > > Good point. Normally, I think the decision of using these callbacks
> > > should be taken at the lowest level, in other words in the driver.
> > > Otherwise the lowest layer of .suspend_late will be by-passed.
> > 
> > I'm not sure what's the point to add them, then.  If the driver has to decide
> > anyway, it may simply populate its .suspend_late and .resume_early pointers
> > I think.
> 
> Another possibility is to have the pm_generic_suspend_late_runtime
> routine check to see if the driver has defined a suspend_late callback.  
> If the driver has its own callback, we could invoke it instead of
> invoking the runtime_suspend callback.
> 
> However, I'm not sure that doing this would be worthwhile.  And it
> would hang in an infinite loop if the driver set its suspend_late
> pointer to pm_generic_suspend_late_runtime!

Well, that's supposed to be a subsystem-level callback.

Anyway, my personal opinion is that we seem to be adding confusion instead of
reducing it here ...

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday, November 27, 2013 12:44:35 AM Ulf Hansson wrote:
> On 26 November 2013 22:09, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 26 Nov 2013, Rafael J. Wysocki wrote:
> >
> >> On Tuesday, November 26, 2013 10:48:56 AM Ulf Hansson wrote:
> >> > On 26 November 2013 00:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > > On Monday, November 25, 2013 01:40:21 PM Ulf Hansson wrote:
> >> > >> To put devices into low power state during sleep, it sometimes makes
> >> > >> sense at subsystem-level to re-use device's runtime PM callbacks.
> >
> > Ulf, please be more careful about the distinction between "device" and
> > "driver".  Devices don't have callbacks; drivers do.  You made this
> > mistake a few times in the patch description.
> 
> Sorry, my bad.
> 
> >
> >> > >> 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 device's
> >> > >> runtime_suspend callback, unless the runtime status is already
> >> > >> suspended. At resume_early the state is restored by invoking the
> >> > >> device's runtime_resume callback. 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.
> >> > >
> >> > > What happens if the subsystem uses the new functions as its late suspend/
> >> > > early resume callbacks, but some of its drivers implement .suspend_late()
> >> > > and .resume_early()?
> >
> > The same thing that happens whenever any PM-related callback is defined
> > by both the subsystem and the driver: The PM core uses the subsystem's
> > callback.  (Or the pm_domain's callback, or the type's callback, or the
> > class's callback.)
> >
> > Besides, Ulf specifically assumed at the top of this message that
> > re-using the runtime-suspend callback makes sense at the subsystem
> > level.  This must mean it is appropriate for all drivers in the
> > subsystem.  So why would a driver want to override the subsystem's
> > behavior?
> >
> >> > Good point. Normally, I think the decision of using these callbacks
> >> > should be taken at the lowest level, in other words in the driver.
> >> > Otherwise the lowest layer of .suspend_late will be by-passed.
> >>
> >> I'm not sure what's the point to add them, then.  If the driver has to decide
> >> anyway, it may simply populate its .suspend_late and .resume_early pointers
> >> I think.
> >
> > Another possibility is to have the pm_generic_suspend_late_runtime
> > routine check to see if the driver has defined a suspend_late callback.
> > If the driver has its own callback, we could invoke it instead of
> > invoking the runtime_suspend callback.
> 
> There are already a generic callbacks that does this. Normally the
> generic callbacks is used from buses and power domains.
> 
> My new callbacks are intended to be used from the driver, so those are
> kind of different from the others in that sense.

Hmm.  I see.

For that to work, the subsystem's .suspend() and .resume() callbacks will
have to be implemented in a special way, because of the unknown runtime PM
status of devices while those callbacks are being executed.

Presumably you think about subsystems that don't implement those callbacks
at all?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

>>
>> There are already a generic callbacks that does this. Normally the
>> generic callbacks is used from buses and power domains.
>>
>> My new callbacks are intended to be used from the driver, so those are
>> kind of different from the others in that sense.
>
> Hmm.  I see.
>
> For that to work, the subsystem's .suspend() and .resume() callbacks will
> have to be implemented in a special way, because of the unknown runtime PM
> status of devices while those callbacks are being executed.

Not sure I understand why you think the runtime PM status is unknown?

In .suspend the runtime status can still be changed. Since the PM core
disables runtime PM before invoking .suspend_late and since the PM
core will keep runtime PM disabled until after the .resume_early has
been invoked, it will be safe to operate on the runtime PM callbacks
during this period - if the driver/bus are adopted for it. While the
.resume callback gets invoked, the runtime status are restored to it's
previous state, which the bus/driver would expect.

Do you want me to send a patch for a driver that uses these callbacks?
I suppose it would clarify how I am thinking...

>
> Presumably you think about subsystems that don't implement those callbacks
> at all?

If you mean that "drivers" is not a part of the term "subsystem" here,
then I somewhat agree.

My view is that it will be mostly drivers that make use of these callbacks.

Though, I don't think we need to prevent subsystems or power domains
from using them, they will only have to know what the consequences
are. :-)

Maybe there could even be some cases were it actually makes good sense
of using them for subsystems, even if I yet not have come a cross such
a case.

Kind regards
Ulf Hansson

>
> Rafael
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday, November 27, 2013 10:59:57 AM Ulf Hansson wrote:
> >>
> >> There are already a generic callbacks that does this. Normally the
> >> generic callbacks is used from buses and power domains.
> >>
> >> My new callbacks are intended to be used from the driver, so those are
> >> kind of different from the others in that sense.
> >
> > Hmm.  I see.
> >
> > For that to work, the subsystem's .suspend() and .resume() callbacks will
> > have to be implemented in a special way, because of the unknown runtime PM
> > status of devices while those callbacks are being executed.
> 
> Not sure I understand why you think the runtime PM status is unknown?
> 
> In .suspend the runtime status can still be changed. Since the PM core
> disables runtime PM before invoking .suspend_late and since the PM
> core will keep runtime PM disabled until after the .resume_early has
> been invoked, it will be safe to operate on the runtime PM callbacks
> during this period - if the driver/bus are adopted for it. While the
> .resume callback gets invoked, the runtime status are restored to it's
> previous state, which the bus/driver would expect.
> 
> Do you want me to send a patch for a driver that uses these callbacks?
> I suppose it would clarify how I am thinking...
> 
> > Presumably you think about subsystems that don't implement those callbacks
> > at all?
> 
> If you mean that "drivers" is not a part of the term "subsystem" here,
> then I somewhat agree.
> 
> My view is that it will be mostly drivers that make use of these callbacks.
> 
> Though, I don't think we need to prevent subsystems or power domains
> from using them, they will only have to know what the consequences
> are. :-)
> 
> Maybe there could even be some cases were it actually makes good sense
> of using them for subsystems, even if I yet not have come a cross such
> a case.

We first need to clarify the terminology as that seems to be the source of
the confusion to some extent at least.

When I (or Alan presumably too) say "a subsystem", I mean a bus type, a device
type or a device class as represented by struct bus_type, struct device_type
and struct class, respectively.  So "a subsystem callback" means the specific
callback defined in the given bus type, device type or device class object.
You can think about that as of "subsystem level" if that's easier.  So that's
what we mean when we talk about subsystems, not the whole collection of drivers
and the code above them.

Now, say you have a bus type.  Usually, the bus type's PM callbacks will run
device drivers' PM callbacks.  Quite often they will do things to hardware
in addition to what the drivers do.  But if your driver's .suspend_late()
depends on the runtime PM status of the device, then the bus type's .suspend()
needs to preserve that status.  That is, it cannot do anything to the hardware
that may cause the runtime PM status of the device to become stale.  That makes
the .suspend() somewhat special, doesn't it?

Now I *guess* that your goal is something like "if that device has been
runtime suspended, don't touch it", which is a reasonable goal to have.  I'd
like to implement something like that too, but I think that it needs to be done
differently.

Actually, my idea is to allow the subsystem-level .prepare() callback to let
the core know if the device needs to be handled during the given suspend/resume
cycle.  It may be arranged, for example, so that if the subsystem-level (e.g.
bus type) .prepare() returns a positive number, the core will put the device
into a special list and it won't take it into consideration at all during the
whole cycle.

Why this way?  Because subsystems have much better ways to determine whether
or not it is necessary to access the device during the upcoming system
suspend/resume cycle than the core does.

Thanks!

>
> We first need to clarify the terminology as that seems to be the source of
> the confusion to some extent at least.
>
> When I (or Alan presumably too) say "a subsystem", I mean a bus type, a device
> type or a device class as represented by struct bus_type, struct device_type
> and struct class, respectively.  So "a subsystem callback" means the specific
> callback defined in the given bus type, device type or device class object.
> You can think about that as of "subsystem level" if that's easier.  So that's
> what we mean when we talk about subsystems, not the whole collection of drivers
> and the code above them.

Thanks, this makes it perfectly clear now. I will try to improve while
communicating.

So we have subsystems, power domains and drivers to consider.

>
> Now, say you have a bus type.  Usually, the bus type's PM callbacks will run
> device drivers' PM callbacks.  Quite often they will do things to hardware
> in addition to what the drivers do.  But if your driver's .suspend_late()
> depends on the runtime PM status of the device, then the bus type's .suspend()
> needs to preserve that status.  That is, it cannot do anything to the hardware
> that may cause the runtime PM status of the device to become stale.  That makes
> the .suspend() somewhat special, doesn't it?

I agree, but I can see why this should be a problem for each and every
driver/bus.

Do note that idea here is only to provide the option of allowing
runtime PM callbacks to be executed as a part of the suspend_late ->
resume_early sequence. It is not a rule, it will have to be decided
for each subsystem/driver it they can benefit from this set up.

>
> Now I *guess* that your goal is something like "if that device has been
> runtime suspended, don't touch it", which is a reasonable goal to have.  I'd
> like to implement something like that too, but I think that it needs to be done
> differently.

That is just a minor important part, but nice to have. Please have
look at the recently submitted patch set, "[PATCH 0/5] PM: Enable
option of re-use runtime PM callbacks at system suspend", I hope the
"cover letter" will describe my intention better.

>
> Actually, my idea is to allow the subsystem-level .prepare() callback to let
> the core know if the device needs to be handled during the given suspend/resume
> cycle.  It may be arranged, for example, so that if the subsystem-level (e.g.
> bus type) .prepare() returns a positive number, the core will put the device
> into a special list and it won't take it into consideration at all during the
> whole cycle.
>
> Why this way?  Because subsystems have much better ways to determine whether
> or not it is necessary to access the device during the upcoming system
> suspend/resume cycle than the core does.

I am not sure I understand how this will solve my issues (except the
minor "if that device has been runtime suspended, don't touch it" -
thing).

I will still have to have a complex power domain and still
drivers/buses need wrapper functions around their the runtime PM
callbacks to invoke them during system suspend, if they need them.

In my view, I don't think your idea is conflict or can replace what I
suggest. Both can improve the situation, but maybe for different
scenarios.

Kind regards
Ulf Hansson

>
> Thanks!
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 27 November 2013 17:05, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> We first need to clarify the terminology as that seems to be the source of
>> the confusion to some extent at least.
>>
>> When I (or Alan presumably too) say "a subsystem", I mean a bus type, a device
>> type or a device class as represented by struct bus_type, struct device_type
>> and struct class, respectively.  So "a subsystem callback" means the specific
>> callback defined in the given bus type, device type or device class object.
>> You can think about that as of "subsystem level" if that's easier.  So that's
>> what we mean when we talk about subsystems, not the whole collection of drivers
>> and the code above them.
>
> Thanks, this makes it perfectly clear now. I will try to improve while
> communicating.
>
> So we have subsystems, power domains and drivers to consider.
>
>>
>> Now, say you have a bus type.  Usually, the bus type's PM callbacks will run
>> device drivers' PM callbacks.  Quite often they will do things to hardware
>> in addition to what the drivers do.  But if your driver's .suspend_late()
>> depends on the runtime PM status of the device, then the bus type's .suspend()
>> needs to preserve that status.  That is, it cannot do anything to the hardware
>> that may cause the runtime PM status of the device to become stale.  That makes
>> the .suspend() somewhat special, doesn't it?
>

can -> can't

> I agree, but I can see why this should be a problem for each and every
> driver/bus.
>
> Do note that idea here is only to provide the option of allowing
> runtime PM callbacks to be executed as a part of the suspend_late ->
> resume_early sequence. It is not a rule, it will have to be decided
> for each subsystem/driver it they can benefit from this set up.
>
>>
>> Now I *guess* that your goal is something like "if that device has been
>> runtime suspended, don't touch it", which is a reasonable goal to have.  I'd
>> like to implement something like that too, but I think that it needs to be done
>> differently.
>
> That is just a minor important part, but nice to have. Please have
> look at the recently submitted patch set, "[PATCH 0/5] PM: Enable
> option of re-use runtime PM callbacks at system suspend", I hope the
> "cover letter" will describe my intention better.
>
>>
>> Actually, my idea is to allow the subsystem-level .prepare() callback to let
>> the core know if the device needs to be handled during the given suspend/resume
>> cycle.  It may be arranged, for example, so that if the subsystem-level (e.g.
>> bus type) .prepare() returns a positive number, the core will put the device
>> into a special list and it won't take it into consideration at all during the
>> whole cycle.
>>
>> Why this way?  Because subsystems have much better ways to determine whether
>> or not it is necessary to access the device during the upcoming system
>> suspend/resume cycle than the core does.
>
> I am not sure I understand how this will solve my issues (except the
> minor "if that device has been runtime suspended, don't touch it" -
> thing).
>
> I will still have to have a complex power domain and still
> drivers/buses need wrapper functions around their the runtime PM
> callbacks to invoke them during system suspend, if they need them.
>
> In my view, I don't think your idea is conflict or can replace what I
> suggest. Both can improve the situation, but maybe for different
> scenarios.
>
> Kind regards
> Ulf Hansson
>
>>
>> Thanks!
>>
>> --
>> I speak only for myself.
>> Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html