[RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[Rafael J. Wysocki]

Hi All,

We've discussed that at length here:

http://marc.info/?t=139950469000003&r=1&w=4

but I'm starting a new thread to refresh things a bit.

This is about adding a mechanism allowing us to avoid runtime-suspended
devices during system suspend.  The reason why it has to touch the PM core
is because that needs to be coordinated across the device hierarchy.

The idea is to add a new device PM flag and to modify the PM core as follows.

 - If ->prepare() returns a positive number for a device, that means "this
   device is runtime-suspended and you can leave it like that if you do the
   same for all of its descendants".

 - If that happens, the PM core sets the new flag for the device in
   question *if* the device is indeed runtime-suspended *and* *if*
   the transition is a suspend (and not hibernation, for example).
   Otherwise, it clears the flag for the device.  All of that happens in
   device_prepare().

 - In __device_suspend() the PM core clears the new flag for the device's
   parent if it is clear for the device to ensure that the flag will only
   be set for a device if it is also set for all of its descendants.

 - PM core skips ->suspend/late/noirq and ->resume/early/noirq for all devices
   having the flag set - so the flag can be called "direct_complete" as it
   causes the PM core to go directy for the ->complete() callback when set.

 - The ->complete() callback has to check direct_complete if ->prepare()
   returned a positive number previously and is responsible for further
   handling of the device.

That is introduced by patch [2/3].

To simplify things slightly it is helpful to move the invocation of
pm_runtime_barrier() from __device_suspend() to device_prepare(), but still
under pm_runtime_get_noresume() beforehand (patch [1/3]).

Patch [3/3] shows how this can be used by adding support for it to the ACPI
PM comain.

Thanks!

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

[RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Move the invocation of the runtime PM barrier during system suspend
(or hibernation) from __device_suspend() to device_prepare() to make
all runtime PM transitions in progress complete before executing
->prepare() callbacks for devices.

That will allow those callbacks to check if devices are runtime
suspended in a non-racy way.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
 
 	dpm_wait_for_children(dev, async);
 
-	if (async_error)
-		goto Complete;
-
-	/*
-	 * If a device configured to wake up the system from sleep states
-	 * has been suspended at run time and there's a resume request pending
-	 * for it, this is equivalent to the device signaling wakeup, so the
-	 * system suspend operation should be aborted.
-	 */
-	if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
-		pm_wakeup_event(dev, 0);
-
-	if (pm_wakeup_pending()) {
-		async_error = -EBUSY;
-		goto Complete;
-	}
-
-	if (dev->power.syscore)
+	if (async_error || dev->power.syscore)
 		goto Complete;
 
 	dpm_watchdog_set(&wd, dev);
@@ -1500,6 +1483,18 @@ static int device_prepare(struct device
 	 */
 	pm_runtime_get_noresume(dev);
 
+	/*
+	 * If a device configured to wake up the system from sleep states
+	 * has been suspended at run time and there's a resume request pending
+	 * for it, this is equivalent to the device signaling wakeup, so the
+	 * system suspend operation should be aborted.
+	 */
+	if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
+		pm_wakeup_event(dev, 0);
+
+	if (pm_wakeup_pending())
+		return -EBUSY;
+
 	device_lock(dev);
 
 	dev->power.wakeup_path = device_may_wakeup(dev);

[RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
resume all runtime-suspended devices during system suspend, mostly
because those devices may need to be reprogrammed due to different
wakeup settings for system sleep and for runtime PM.

For some devices, though, it's OK to remain in runtime suspend 
throughout a complete system suspend/resume cycle (if the device was in
runtime suspend at the start of the cycle).  We would like to do this
whenever possible, to avoid the overhead of extra power-up and power-down
events.

However, problems may arise because the device's descendants may require
it to be at full power at various points during the cycle.  Therefore the
most straightforward way to do this safely is if the device and all its
descendants can remain runtime suspended until the complete stage of
system resume.

To this end, introduce a new device PM flag, power.direct_complete
and modify the PM core to use that flag as follows.

If the ->prepare() callback of a device returns a positive number,
the PM core will regard that as an indication that it may leave the
device runtime-suspended.  It will then check if the system power
transition in progress is a suspend (and not hibernation in particular)
and if the device is, indeed, runtime-suspended.  In that case, the PM
core will set the device's power.direct_complete flag.  Otherwise it
will clear power.direct_complete for the device and it also will later
clear it for the device's parent (if there's one).

Next, the PM core will not invoke the ->suspend() ->suspend_late(),
->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
callbacks for all devices having power.direct_complete set.  It
will invoke their ->complete() callbacks, however, and those
callbacks are then responsible for resuming the devices as
appropriate, if necessary.

Changelog partly based on an Alan Stern's description of the idea
(http://marc.info/?l=linux-pm&m=139940466625569&w=2).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   45 ++++++++++++++++++++++++++++-----------------
 include/linux/pm.h        |    1 +
 2 files changed, 29 insertions(+), 17 deletions(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -546,6 +546,7 @@ struct dev_pm_info {
 	bool			is_late_suspended:1;
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
+	bool			direct_complete:1;	/* Owned by the PM core */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_noirq_suspended)
@@ -605,7 +605,7 @@ static int device_resume_early(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_late_suspended)
@@ -732,7 +732,7 @@ static int device_resume(struct device *
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait(dev->parent, async);
@@ -1007,7 +1007,7 @@ static int __device_suspend_noirq(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1146,7 +1146,7 @@ static int __device_suspend_late(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1312,7 +1312,7 @@ static int __device_suspend(struct devic
 
 	dpm_wait_for_children(dev, async);
 
-	if (async_error || dev->power.syscore)
+	if (async_error || dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_watchdog_set(&wd, dev);
@@ -1365,10 +1365,19 @@ static int __device_suspend(struct devic
 
  End:
 	if (!error) {
+		struct device *parent = dev->parent;
+
 		dev->power.is_suspended = true;
-		if (dev->power.wakeup_path
-		    && dev->parent && !dev->parent->power.ignore_children)
-			dev->parent->power.wakeup_path = true;
+		if (parent) {
+			spin_lock_irq(&parent->power.lock);
+
+			dev->parent->power.direct_complete = false;
+			if (dev->power.wakeup_path
+			    && !dev->parent->power.ignore_children)
+				dev->parent->power.wakeup_path = true;
+
+			spin_unlock_irq(&parent->power.lock);
+		}
 	}
 
 	device_unlock(dev);
@@ -1470,7 +1479,7 @@ static int device_prepare(struct device
 {
 	int (*callback)(struct device *) = NULL;
 	char *info = NULL;
-	int error = 0;
+	int ret = 0;
 
 	if (dev->power.syscore)
 		return 0;
@@ -1518,17 +1527,19 @@ static int device_prepare(struct device
 		callback = dev->driver->pm->prepare;
 	}
 
-	if (callback) {
-		error = callback(dev);
-		suspend_report_result(callback, error);
-	}
+	if (callback)
+		ret = callback(dev);
 
 	device_unlock(dev);
 
-	if (error)
+	if (ret < 0) {
+		suspend_report_result(callback, ret);
 		pm_runtime_put(dev);
-
-	return error;
+		return ret;
+	}
+	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
+					&& pm_runtime_suspended(dev);
+	return 0;
 }
 
 /**

[RFC][PATCH 3/3] ACPI / PM: Avoid resuming devices in ACPI PM domain during system suspend

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rework the ACPI PM domain's PM callbacks to avoid resuming devices
during system suspend (in order to modify their wakeup settings etc.)
if that isn't necessary.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -261,7 +261,8 @@ struct acpi_device_power_flags {
 	u32 inrush_current:1;	/* Serialize Dx->D0 */
 	u32 power_removed:1;	/* Optimize Dx->D0 */
 	u32 ignore_parent:1;	/* Power is independent of parent power state */
-	u32 reserved:27;
+	u32 dsw_present:1;	/* _DSW present? */
+	u32 reserved:26;
 };
 
 struct acpi_device_power_state {
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1551,9 +1551,13 @@ static void acpi_bus_get_power_flags(str
 	 */
 	if (acpi_has_method(device->handle, "_PSC"))
 		device->power.flags.explicit_get = 1;
+
 	if (acpi_has_method(device->handle, "_IRC"))
 		device->power.flags.inrush_current = 1;
 
+	if (acpi_has_method(device->handle, "_DSW"))
+		device->power.flags.dsw_present = 1;
+
 	/*
 	 * Enumerate supported power management states
 	 */
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -900,18 +900,46 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume_early)
  */
 int acpi_subsys_prepare(struct device *dev)
 {
-	/*
-	 * Devices having power.ignore_children set may still be necessary for
-	 * suspending their children in the next phase of device suspend.
-	 */
-	if (dev->power.ignore_children)
-		pm_runtime_resume(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	u32 sys_target;
+	int ret, state;
+
+	ret = pm_generic_prepare(dev);
+	if (ret < 0)
+		return ret;
+
+	if (!adev || !pm_runtime_suspended(dev)
+	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		return 0;
+
+	sys_target = acpi_target_system_state();
+	if (sys_target == ACPI_STATE_S0)
+		return 1;
+
+	if (adev->power.flags.dsw_present)
+		return 0;
 
-	return pm_generic_prepare(dev);
+	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+	return !ret && state == adev->power.state;
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
+ * acpi_subsys_complete - Finalize device's resume during system resume.
+ * @dev: Device to handle.
+ */
+void acpi_subsys_complete(struct device *dev)
+{
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete)
+		pm_request_resume(dev);
+}
+
+/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -979,6 +1007,7 @@ static struct dev_pm_domain acpi_general
 #endif
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,

Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Ulf Hansson]

On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move the invocation of the runtime PM barrier during system suspend
> (or hibernation) from __device_suspend() to device_prepare() to make
> all runtime PM transitions in progress complete before executing
> ->prepare() callbacks for devices.
>
> That will allow those callbacks to check if devices are runtime
> suspended in a non-racy way.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
>
>         dpm_wait_for_children(dev, async);
>
> -       if (async_error)
> -               goto Complete;
> -
> -       /*
> -        * If a device configured to wake up the system from sleep states
> -        * has been suspended at run time and there's a resume request pending
> -        * for it, this is equivalent to the device signaling wakeup, so the
> -        * system suspend operation should be aborted.
> -        */
> -       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> -               pm_wakeup_event(dev, 0);
> -
> -       if (pm_wakeup_pending()) {
> -               async_error = -EBUSY;
> -               goto Complete;
> -       }

I suppose you went a bit too far here!?

We can still have wakeup pending at this point, and thus we should
bail out, right?

> -
> -       if (dev->power.syscore)
> +       if (async_error || dev->power.syscore)
>                 goto Complete;
>
>         dpm_watchdog_set(&wd, dev);
> @@ -1500,6 +1483,18 @@ static int device_prepare(struct device
>          */
>         pm_runtime_get_noresume(dev);
>
> +       /*
> +        * If a device configured to wake up the system from sleep states
> +        * has been suspended at run time and there's a resume request pending
> +        * for it, this is equivalent to the device signaling wakeup, so the
> +        * system suspend operation should be aborted.
> +        */
> +       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> +               pm_wakeup_event(dev, 0);
> +
> +       if (pm_wakeup_pending())
> +               return -EBUSY;
> +
>         device_lock(dev);
>
>         dev->power.wakeup_path = device_may_wakeup(dev);
>

Kind regards
Ulf Hansson

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Ulf Hansson]

On 13 May 2014 03:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> resume all runtime-suspended devices during system suspend, mostly
> because those devices may need to be reprogrammed due to different
> wakeup settings for system sleep and for runtime PM.
>
> For some devices, though, it's OK to remain in runtime suspend
> throughout a complete system suspend/resume cycle (if the device was in
> runtime suspend at the start of the cycle).  We would like to do this
> whenever possible, to avoid the overhead of extra power-up and power-down
> events.
>
> However, problems may arise because the device's descendants may require
> it to be at full power at various points during the cycle.  Therefore the
> most straightforward way to do this safely is if the device and all its
> descendants can remain runtime suspended until the complete stage of
> system resume.
>
> To this end, introduce a new device PM flag, power.direct_complete
> and modify the PM core to use that flag as follows.
>
> If the ->prepare() callback of a device returns a positive number,
> the PM core will regard that as an indication that it may leave the
> device runtime-suspended.  It will then check if the system power
> transition in progress is a suspend (and not hibernation in particular)
> and if the device is, indeed, runtime-suspended.  In that case, the PM
> core will set the device's power.direct_complete flag.  Otherwise it
> will clear power.direct_complete for the device and it also will later
> clear it for the device's parent (if there's one).
>
> Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> callbacks for all devices having power.direct_complete set.  It
> will invoke their ->complete() callbacks, however, and those
> callbacks are then responsible for resuming the devices as
> appropriate, if necessary.
>
> Changelog partly based on an Alan Stern's description of the idea
> (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I like this idea! And we don't need to add new functions to the runtime PM API.

[snip]

> -
> -       return error;
> +               return ret;
> +       }
> +       dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
> +                                       && pm_runtime_suspended(dev);

This might deserve a comment!?

> +       return 0;
>  }
>
>  /**
>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson

Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote:
> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Move the invocation of the runtime PM barrier during system suspend
> > (or hibernation) from __device_suspend() to device_prepare() to make
> > all runtime PM transitions in progress complete before executing
> > ->prepare() callbacks for devices.
> >
> > That will allow those callbacks to check if devices are runtime
> > suspended in a non-racy way.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c |   31 +++++++++++++------------------
> >  1 file changed, 13 insertions(+), 18 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/main.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/main.c
> > +++ linux-pm/drivers/base/power/main.c
> > @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
> >
> >         dpm_wait_for_children(dev, async);
> >
> > -       if (async_error)
> > -               goto Complete;
> > -
> > -       /*
> > -        * If a device configured to wake up the system from sleep states
> > -        * has been suspended at run time and there's a resume request pending
> > -        * for it, this is equivalent to the device signaling wakeup, so the
> > -        * system suspend operation should be aborted.
> > -        */
> > -       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> > -               pm_wakeup_event(dev, 0);
> > -
> > -       if (pm_wakeup_pending()) {
> > -               async_error = -EBUSY;
> > -               goto Complete;
> > -       }
> 
> I suppose you went a bit too far here!?
> 
> We can still have wakeup pending at this point, and thus we should
> bail out, right?

That pm_wakeup_pending() is part of the barrier handling, so ->

> > -
> > -       if (dev->power.syscore)
> > +       if (async_error || dev->power.syscore)
> >                 goto Complete;
> >
> >         dpm_watchdog_set(&wd, dev);
> > @@ -1500,6 +1483,18 @@ static int device_prepare(struct device
> >          */
> >         pm_runtime_get_noresume(dev);
> >
> > +       /*
> > +        * If a device configured to wake up the system from sleep states
> > +        * has been suspended at run time and there's a resume request pending
> > +        * for it, this is equivalent to the device signaling wakeup, so the
> > +        * system suspend operation should be aborted.
> > +        */
> > +       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> > +               pm_wakeup_event(dev, 0);
> > +
> > +       if (pm_wakeup_pending())
> > +               return -EBUSY;
> > +

-> it is done here now.

I don't see why it would be still necessary in __device_suspend().

> >         device_lock(dev);
> >
> >         dev->power.wakeup_path = device_may_wakeup(dev);
> >

Thanks!

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

Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Ulf Hansson]

On 13 May 2014 12:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote:
>> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > Move the invocation of the runtime PM barrier during system suspend
>> > (or hibernation) from __device_suspend() to device_prepare() to make
>> > all runtime PM transitions in progress complete before executing
>> > ->prepare() callbacks for devices.
>> >
>> > That will allow those callbacks to check if devices are runtime
>> > suspended in a non-racy way.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > ---
>> >  drivers/base/power/main.c |   31 +++++++++++++------------------
>> >  1 file changed, 13 insertions(+), 18 deletions(-)
>> >
>> > Index: linux-pm/drivers/base/power/main.c
>> > ===================================================================
>> > --- linux-pm.orig/drivers/base/power/main.c
>> > +++ linux-pm/drivers/base/power/main.c
>> > @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
>> >
>> >         dpm_wait_for_children(dev, async);
>> >
>> > -       if (async_error)
>> > -               goto Complete;
>> > -
>> > -       /*
>> > -        * If a device configured to wake up the system from sleep states
>> > -        * has been suspended at run time and there's a resume request pending
>> > -        * for it, this is equivalent to the device signaling wakeup, so the
>> > -        * system suspend operation should be aborted.
>> > -        */
>> > -       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
>> > -               pm_wakeup_event(dev, 0);
>> > -
>> > -       if (pm_wakeup_pending()) {
>> > -               async_error = -EBUSY;
>> > -               goto Complete;
>> > -       }
>>
>> I suppose you went a bit too far here!?
>>
>> We can still have wakeup pending at this point, and thus we should
>> bail out, right?
>
> That pm_wakeup_pending() is part of the barrier handling, so ->
>
>> > -
>> > -       if (dev->power.syscore)
>> > +       if (async_error || dev->power.syscore)
>> >                 goto Complete;
>> >
>> >         dpm_watchdog_set(&wd, dev);
>> > @@ -1500,6 +1483,18 @@ static int device_prepare(struct device
>> >          */
>> >         pm_runtime_get_noresume(dev);
>> >
>> > +       /*
>> > +        * If a device configured to wake up the system from sleep states
>> > +        * has been suspended at run time and there's a resume request pending
>> > +        * for it, this is equivalent to the device signaling wakeup, so the
>> > +        * system suspend operation should be aborted.
>> > +        */
>> > +       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
>> > +               pm_wakeup_event(dev, 0);
>> > +
>> > +       if (pm_wakeup_pending())
>> > +               return -EBUSY;
>> > +
>
> -> it is done here now.
>
> I don't see why it would be still necessary in __device_suspend().

Can't we have wakeup configured for !CONFIG_PM_RUNTIME case?
pm_runtime_barrier() won't handle those scenarios, right?

Similar check for pm_wakeup_pending() is done at
__device_suspend_noirq, __device_suspend_late - I assumed it was
because of the same reasons.

Kind regards
Ulf Hansson

>
>> >         device_lock(dev);
>> >
>> >         dev->power.wakeup_path = device_may_wakeup(dev);
>> >
>
> Thanks!
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> Hi All,
> 
> We've discussed that at length here:
> 
> http://marc.info/?t=139950469000003&r=1&w=4
> 
> but I'm starting a new thread to refresh things a bit.
> 
> This is about adding a mechanism allowing us to avoid runtime-suspended
> devices during system suspend.  The reason why it has to touch the PM core
> is because that needs to be coordinated across the device hierarchy.
> 
> The idea is to add a new device PM flag and to modify the PM core as follows.
> 
>  - If ->prepare() returns a positive number for a device, that means "this
>    device is runtime-suspended and you can leave it like that if you do the
>    same for all of its descendants".
> 
>  - If that happens, the PM core sets the new flag for the device in
>    question *if* the device is indeed runtime-suspended *and* *if*
>    the transition is a suspend (and not hibernation, for example).
>    Otherwise, it clears the flag for the device.  All of that happens in
>    device_prepare().
> 
>  - In __device_suspend() the PM core clears the new flag for the device's
>    parent if it is clear for the device to ensure that the flag will only
>    be set for a device if it is also set for all of its descendants.

There's nothing to prevent a runtime-suspended device from being 
resumed in between the ->prepare() and ->suspend() callbacks.  (Ulf 
mentioned this too.)

Therefore it makes little sense to check the device's runtime status in 
device_prepare().  The check should be done in __device_suspend().

>  - PM core skips ->suspend/late/noirq and ->resume/early/noirq for all devices
>    having the flag set - so the flag can be called "direct_complete" as it
>    causes the PM core to go directy for the ->complete() callback when set.
> 
>  - The ->complete() callback has to check direct_complete if ->prepare()
>    returned a positive number previously and is responsible for further
>    handling of the device.
> 
> That is introduced by patch [2/3].
> 
> To simplify things slightly it is helpful to move the invocation of
> pm_runtime_barrier() from __device_suspend() to device_prepare(), but still
> under pm_runtime_get_noresume() beforehand (patch [1/3]).

If the check is moved to __device_suspend() then the barrier can remain 
where it is now.

> Patch [3/3] shows how this can be used by adding support for it to the ACPI
> PM comain.
> 
> Thanks!

Aside from this one matter, everything seems pretty good.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> resume all runtime-suspended devices during system suspend, mostly
> because those devices may need to be reprogrammed due to different
> wakeup settings for system sleep and for runtime PM.
> 
> For some devices, though, it's OK to remain in runtime suspend 
> throughout a complete system suspend/resume cycle (if the device was in
> runtime suspend at the start of the cycle).  We would like to do this
> whenever possible, to avoid the overhead of extra power-up and power-down
> events.
> 
> However, problems may arise because the device's descendants may require
> it to be at full power at various points during the cycle.  Therefore the
> most straightforward way to do this safely is if the device and all its
> descendants can remain runtime suspended until the complete stage of
> system resume.
> 
> To this end, introduce a new device PM flag, power.direct_complete
> and modify the PM core to use that flag as follows.
> 
> If the ->prepare() callback of a device returns a positive number,
> the PM core will regard that as an indication that it may leave the
> device runtime-suspended.  It will then check if the system power
> transition in progress is a suspend (and not hibernation in particular)
> and if the device is, indeed, runtime-suspended.  In that case, the PM
> core will set the device's power.direct_complete flag.  Otherwise it
> will clear power.direct_complete for the device and it also will later
> clear it for the device's parent (if there's one).
> 
> Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> callbacks for all devices having power.direct_complete set.  It
> will invoke their ->complete() callbacks, however, and those
> callbacks are then responsible for resuming the devices as
> appropriate, if necessary.

Perhaps you should mention here (and maybe even as a comment in the 
code) that ->complete() callbacks may want to call pm_request_resume() 
if dev->power.direct_resume is set, but they shouldn't call 
pm_runtime_resume().

> Changelog partly based on an Alan Stern's description of the idea
> (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

...

> @@ -1518,17 +1527,19 @@ static int device_prepare(struct device
>  		callback = dev->driver->pm->prepare;
>  	}
>  
> -	if (callback) {
> -		error = callback(dev);
> -		suspend_report_result(callback, error);
> -	}
> +	if (callback)
> +		ret = callback(dev);
>  
>  	device_unlock(dev);
>  
> -	if (error)
> +	if (ret < 0) {
> +		suspend_report_result(callback, ret);
>  		pm_runtime_put(dev);
> -
> -	return error;
> +		return ret;
> +	}
> +	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
> +					&& pm_runtime_suspended(dev);

Shouldn't the flag be set under the spinlock?

Alan Stern

Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 12:59:43 PM Ulf Hansson wrote:
> On 13 May 2014 12:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote:
> >> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >
> >> > Move the invocation of the runtime PM barrier during system suspend
> >> > (or hibernation) from __device_suspend() to device_prepare() to make
> >> > all runtime PM transitions in progress complete before executing
> >> > ->prepare() callbacks for devices.
> >> >
> >> > That will allow those callbacks to check if devices are runtime
> >> > suspended in a non-racy way.
> >> >
> >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> > ---
> >> >  drivers/base/power/main.c |   31 +++++++++++++------------------
> >> >  1 file changed, 13 insertions(+), 18 deletions(-)
> >> >
> >> > Index: linux-pm/drivers/base/power/main.c
> >> > ===================================================================
> >> > --- linux-pm.orig/drivers/base/power/main.c
> >> > +++ linux-pm/drivers/base/power/main.c
> >> > @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
> >> >
> >> >         dpm_wait_for_children(dev, async);
> >> >
> >> > -       if (async_error)
> >> > -               goto Complete;
> >> > -
> >> > -       /*
> >> > -        * If a device configured to wake up the system from sleep states
> >> > -        * has been suspended at run time and there's a resume request pending
> >> > -        * for it, this is equivalent to the device signaling wakeup, so the
> >> > -        * system suspend operation should be aborted.
> >> > -        */
> >> > -       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> >> > -               pm_wakeup_event(dev, 0);
> >> > -
> >> > -       if (pm_wakeup_pending()) {
> >> > -               async_error = -EBUSY;
> >> > -               goto Complete;
> >> > -       }
> >>
> >> I suppose you went a bit too far here!?
> >>
> >> We can still have wakeup pending at this point, and thus we should
> >> bail out, right?
> >
> > That pm_wakeup_pending() is part of the barrier handling, so ->
> >
> >> > -
> >> > -       if (dev->power.syscore)
> >> > +       if (async_error || dev->power.syscore)
> >> >                 goto Complete;
> >> >
> >> >         dpm_watchdog_set(&wd, dev);
> >> > @@ -1500,6 +1483,18 @@ static int device_prepare(struct device
> >> >          */
> >> >         pm_runtime_get_noresume(dev);
> >> >
> >> > +       /*
> >> > +        * If a device configured to wake up the system from sleep states
> >> > +        * has been suspended at run time and there's a resume request pending
> >> > +        * for it, this is equivalent to the device signaling wakeup, so the
> >> > +        * system suspend operation should be aborted.
> >> > +        */
> >> > +       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> >> > +               pm_wakeup_event(dev, 0);
> >> > +
> >> > +       if (pm_wakeup_pending())
> >> > +               return -EBUSY;
> >> > +
> >
> > -> it is done here now.
> >
> > I don't see why it would be still necessary in __device_suspend().
> 
> Can't we have wakeup configured for !CONFIG_PM_RUNTIME case?
> pm_runtime_barrier() won't handle those scenarios, right?

The pm_wakeup_pending() is in effect for CONFIG_PM_RUNTIME unset too.

> Similar check for pm_wakeup_pending() is done at
> __device_suspend_noirq, __device_suspend_late - I assumed it was
> because of the same reasons.

Hmm, OK.  I'll leave it in __device_suspend() too, then.

Thanks!

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

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> > > +	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
> > > +					&& pm_runtime_suspended(dev);
> > 
> > Shouldn't the flag be set under the spinlock?
> 
> I guess you're worried about runtime PM flags being modified in parallel to
> this?  But we've just done the barrier a while ago, so is that still a concern
> here?

A wakeup request from the hardware could cause a runtime resume to 
occur at this time.  The barrier wouldn't prevent that.

It's unlikely, I agree, but not impossible.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 10:49:32 AM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> > resume all runtime-suspended devices during system suspend, mostly
> > because those devices may need to be reprogrammed due to different
> > wakeup settings for system sleep and for runtime PM.
> > 
> > For some devices, though, it's OK to remain in runtime suspend 
> > throughout a complete system suspend/resume cycle (if the device was in
> > runtime suspend at the start of the cycle).  We would like to do this
> > whenever possible, to avoid the overhead of extra power-up and power-down
> > events.
> > 
> > However, problems may arise because the device's descendants may require
> > it to be at full power at various points during the cycle.  Therefore the
> > most straightforward way to do this safely is if the device and all its
> > descendants can remain runtime suspended until the complete stage of
> > system resume.
> > 
> > To this end, introduce a new device PM flag, power.direct_complete
> > and modify the PM core to use that flag as follows.
> > 
> > If the ->prepare() callback of a device returns a positive number,
> > the PM core will regard that as an indication that it may leave the
> > device runtime-suspended.  It will then check if the system power
> > transition in progress is a suspend (and not hibernation in particular)
> > and if the device is, indeed, runtime-suspended.  In that case, the PM
> > core will set the device's power.direct_complete flag.  Otherwise it
> > will clear power.direct_complete for the device and it also will later
> > clear it for the device's parent (if there's one).
> > 
> > Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> > ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> > callbacks for all devices having power.direct_complete set.  It
> > will invoke their ->complete() callbacks, however, and those
> > callbacks are then responsible for resuming the devices as
> > appropriate, if necessary.
> 
> Perhaps you should mention here (and maybe even as a comment in the 
> code) that ->complete() callbacks may want to call pm_request_resume() 
> if dev->power.direct_resume is set, but they shouldn't call 
> pm_runtime_resume().

OK

> > Changelog partly based on an Alan Stern's description of the idea
> > (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ...
> 
> > @@ -1518,17 +1527,19 @@ static int device_prepare(struct device
> >  		callback = dev->driver->pm->prepare;
> >  	}
> >  
> > -	if (callback) {
> > -		error = callback(dev);
> > -		suspend_report_result(callback, error);
> > -	}
> > +	if (callback)
> > +		ret = callback(dev);
> >  
> >  	device_unlock(dev);
> >  
> > -	if (error)
> > +	if (ret < 0) {
> > +		suspend_report_result(callback, ret);
> >  		pm_runtime_put(dev);
> > -
> > -	return error;
> > +		return ret;
> > +	}
> > +	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
> > +					&& pm_runtime_suspended(dev);
> 
> Shouldn't the flag be set under the spinlock?

I guess you're worried about runtime PM flags being modified in parallel to
this?  But we've just done the barrier a while ago, so is that still a concern
here?

This won't run in parallel with device_prepare() for any other devices, because
the "complete" phase is sequential.

Rafael

Re: [RFC][PATCH 1/3] PM / sleep: Move runtime PM barrier invocation to device_prepare()

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 05:07:12 PM Rafael J. Wysocki wrote:
> On Tuesday, May 13, 2014 12:59:43 PM Ulf Hansson wrote:
> > On 13 May 2014 12:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > On Tuesday, May 13, 2014 11:16:34 AM Ulf Hansson wrote:
> > >> On 13 May 2014 03:03, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> >
> > >> > Move the invocation of the runtime PM barrier during system suspend
> > >> > (or hibernation) from __device_suspend() to device_prepare() to make
> > >> > all runtime PM transitions in progress complete before executing
> > >> > ->prepare() callbacks for devices.
> > >> >
> > >> > That will allow those callbacks to check if devices are runtime
> > >> > suspended in a non-racy way.
> > >> >
> > >> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> > ---
> > >> >  drivers/base/power/main.c |   31 +++++++++++++------------------
> > >> >  1 file changed, 13 insertions(+), 18 deletions(-)
> > >> >
> > >> > Index: linux-pm/drivers/base/power/main.c
> > >> > ===================================================================
> > >> > --- linux-pm.orig/drivers/base/power/main.c
> > >> > +++ linux-pm/drivers/base/power/main.c
> > >> > @@ -1312,24 +1312,7 @@ static int __device_suspend(struct devic
> > >> >
> > >> >         dpm_wait_for_children(dev, async);
> > >> >
> > >> > -       if (async_error)
> > >> > -               goto Complete;
> > >> > -
> > >> > -       /*
> > >> > -        * If a device configured to wake up the system from sleep states
> > >> > -        * has been suspended at run time and there's a resume request pending
> > >> > -        * for it, this is equivalent to the device signaling wakeup, so the
> > >> > -        * system suspend operation should be aborted.
> > >> > -        */
> > >> > -       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> > >> > -               pm_wakeup_event(dev, 0);
> > >> > -
> > >> > -       if (pm_wakeup_pending()) {
> > >> > -               async_error = -EBUSY;
> > >> > -               goto Complete;
> > >> > -       }
> > >>
> > >> I suppose you went a bit too far here!?
> > >>
> > >> We can still have wakeup pending at this point, and thus we should
> > >> bail out, right?
> > >
> > > That pm_wakeup_pending() is part of the barrier handling, so ->
> > >
> > >> > -
> > >> > -       if (dev->power.syscore)
> > >> > +       if (async_error || dev->power.syscore)
> > >> >                 goto Complete;
> > >> >
> > >> >         dpm_watchdog_set(&wd, dev);
> > >> > @@ -1500,6 +1483,18 @@ static int device_prepare(struct device
> > >> >          */
> > >> >         pm_runtime_get_noresume(dev);
> > >> >
> > >> > +       /*
> > >> > +        * If a device configured to wake up the system from sleep states
> > >> > +        * has been suspended at run time and there's a resume request pending
> > >> > +        * for it, this is equivalent to the device signaling wakeup, so the
> > >> > +        * system suspend operation should be aborted.
> > >> > +        */
> > >> > +       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> > >> > +               pm_wakeup_event(dev, 0);
> > >> > +
> > >> > +       if (pm_wakeup_pending())
> > >> > +               return -EBUSY;
> > >> > +
> > >
> > > -> it is done here now.
> > >
> > > I don't see why it would be still necessary in __device_suspend().
> > 
> > Can't we have wakeup configured for !CONFIG_PM_RUNTIME case?
> > pm_runtime_barrier() won't handle those scenarios, right?
> 
> The pm_wakeup_pending() is in effect for CONFIG_PM_RUNTIME unset too.
> 
> > Similar check for pm_wakeup_pending() is done at
> > __device_suspend_noirq, __device_suspend_late - I assumed it was
> > because of the same reasons.
> 
> Hmm, OK.  I'll leave it in __device_suspend() too, then.

Well, actually, that wouldn't make much sense in my opinion.

Why would the device status change between device_prepare() and
__device_suspend() if we do the barrier in device_prepare()?


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

Re: [RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[Alan Stern]

Crossing emails again...

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> > There's nothing to prevent a runtime-suspended device from being 
> > resumed in between the ->prepare() and ->suspend() callbacks.
> 
> I'm moving the barrier from __device_suspend() to device_prepare(), so there
> shouldn't be surprise resumes in that time frame.

A wakeup request from the hardware can cause a runtime resume, even 
if most threads are in the freezer:

	Not all kernel threads get frozen.  One of the unfrozen threads 
	could respond to the wakeup request by calling 
	pm_runtime_resume().

	Some runtime PM callbacks are marked as IRQ-safe and can run
	directly within an interrupt handler.

> > Therefore it makes little sense to check the device's runtime status in 
> > device_prepare().  The check should be done in __device_suspend().
> 
> If we do the barrier in device_prepare(), then I'm not sure what mechanism
> would cause the device to resume.

See above.  A wakeup request can arrive after the barrier has finished.

> If there is one, the whole approach is in danger, because ->prepare() has to
> check if devices are runtime-suspended and has to be sure that their status
> won't change after it has returned 1.

->prepare() cannot guarantee in all cases that a device will remain in 
runtime suspend.  Fortunately, it doesn't need to.  In fact (as I 
mentioned sometime before), it doesn't even need to check whether the 
device currently is runtime suspended -- it suffices to know that _if_ 
the device is runtime suspended _then_ it has the proper settings for 
system suspend.

Regardless, status changes cannot cause a problem.  If the device does
get runtime-resumed after ->prepare(), it will remain that way when
__device_suspend() runs.  The device can't be runtime-suspended again,
because device_prepare() does pm_get_noresume().

Therefore, if the device is still runtime-suspended when
__device_suspend() runs, we can be sure that its status and state are
still the same as when ->prepare() ran.

Alan Stern

Re: [RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 10:45:38 AM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > Hi All,
> > 
> > We've discussed that at length here:
> > 
> > http://marc.info/?t=139950469000003&r=1&w=4
> > 
> > but I'm starting a new thread to refresh things a bit.
> > 
> > This is about adding a mechanism allowing us to avoid runtime-suspended
> > devices during system suspend.  The reason why it has to touch the PM core
> > is because that needs to be coordinated across the device hierarchy.
> > 
> > The idea is to add a new device PM flag and to modify the PM core as follows.
> > 
> >  - If ->prepare() returns a positive number for a device, that means "this
> >    device is runtime-suspended and you can leave it like that if you do the
> >    same for all of its descendants".
> > 
> >  - If that happens, the PM core sets the new flag for the device in
> >    question *if* the device is indeed runtime-suspended *and* *if*
> >    the transition is a suspend (and not hibernation, for example).
> >    Otherwise, it clears the flag for the device.  All of that happens in
> >    device_prepare().
> > 
> >  - In __device_suspend() the PM core clears the new flag for the device's
> >    parent if it is clear for the device to ensure that the flag will only
> >    be set for a device if it is also set for all of its descendants.
> 
> There's nothing to prevent a runtime-suspended device from being 
> resumed in between the ->prepare() and ->suspend() callbacks.

I'm moving the barrier from __device_suspend() to device_prepare(), so there
shouldn't be surprise resumes in that time frame.

> (Ulf mentioned this too.)

Ulf was talking about pm_wakeup_pending(), which is tangentially related.

> Therefore it makes little sense to check the device's runtime status in 
> device_prepare().  The check should be done in __device_suspend().

If we do the barrier in device_prepare(), then I'm not sure what mechanism
would cause the device to resume.

If there is one, the whole approach is in danger, because ->prepare() has to
check if devices are runtime-suspended and has to be sure that their status
won't change after it has returned 1.

> >  - PM core skips ->suspend/late/noirq and ->resume/early/noirq for all devices
> >    having the flag set - so the flag can be called "direct_complete" as it
> >    causes the PM core to go directy for the ->complete() callback when set.
> > 
> >  - The ->complete() callback has to check direct_complete if ->prepare()
> >    returned a positive number previously and is responsible for further
> >    handling of the device.
> > 
> > That is introduced by patch [2/3].
> > 
> > To simplify things slightly it is helpful to move the invocation of
> > pm_runtime_barrier() from __device_suspend() to device_prepare(), but still
> > under pm_runtime_get_noresume() beforehand (patch [1/3]).
> 
> If the check is moved to __device_suspend() then the barrier can remain 
> where it is now.

The check also needs to be done in ->prepare().

> > Patch [3/3] shows how this can be used by adding support for it to the ACPI
> > PM comain.
> > 
> > Thanks!
> 
> Aside from this one matter, everything seems pretty good.

Well, that's a quite a big issue.

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 11:12:28 AM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > > > +	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND
> > > > +					&& pm_runtime_suspended(dev);
> > > 
> > > Shouldn't the flag be set under the spinlock?
> > 
> > I guess you're worried about runtime PM flags being modified in parallel to
> > this?  But we've just done the barrier a while ago, so is that still a concern
> > here?
> 
> A wakeup request from the hardware could cause a runtime resume to 
> occur at this time.  The barrier wouldn't prevent that.
> 
> It's unlikely, I agree, but not impossible.

Yeah, I didn't think about that.

But that also can occur in __device_suspend(), after we've checked the flag
and decided not to invoke the ->suspend() callback, right?  So moving the
check in there doesn't help much I'd say.  It closes the race window, but
that's it.

That means that the whole approach based on ->prepare() is problematic
unless we somehow mix it with disabling runtime PM.

Rafael

Re: [RFC][PATCH 0/3] PM / sleep: Avoid resuming runtime-suspended devices during system suspend

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 11:25:07 AM Alan Stern wrote:
> Crossing emails again...
> 
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > > There's nothing to prevent a runtime-suspended device from being 
> > > resumed in between the ->prepare() and ->suspend() callbacks.
> > 
> > I'm moving the barrier from __device_suspend() to device_prepare(), so there
> > shouldn't be surprise resumes in that time frame.
> 
> A wakeup request from the hardware can cause a runtime resume, even 
> if most threads are in the freezer:
> 
> 	Not all kernel threads get frozen.  One of the unfrozen threads 
> 	could respond to the wakeup request by calling 
> 	pm_runtime_resume().
> 
> 	Some runtime PM callbacks are marked as IRQ-safe and can run
> 	directly within an interrupt handler.
> 
> > > Therefore it makes little sense to check the device's runtime status in 
> > > device_prepare().  The check should be done in __device_suspend().
> > 
> > If we do the barrier in device_prepare(), then I'm not sure what mechanism
> > would cause the device to resume.
> 
> See above.  A wakeup request can arrive after the barrier has finished.
> 
> > If there is one, the whole approach is in danger, because ->prepare() has to
> > check if devices are runtime-suspended and has to be sure that their status
> > won't change after it has returned 1.
> 
> ->prepare() cannot guarantee in all cases that a device will remain in 
> runtime suspend.  Fortunately, it doesn't need to.  In fact (as I 
> mentioned sometime before), it doesn't even need to check whether the 
> device currently is runtime suspended -- it suffices to know that _if_ 
> the device is runtime suspended _then_ it has the proper settings for 
> system suspend.
> 
> Regardless, status changes cannot cause a problem.  If the device does
> get runtime-resumed after ->prepare(), it will remain that way when
> __device_suspend() runs.  The device can't be runtime-suspended again,
> because device_prepare() does pm_get_noresume().
> 
> Therefore, if the device is still runtime-suspended when
> __device_suspend() runs, we can be sure that its status and state are
> still the same as when ->prepare() ran.

But if the device is runtime-suspended, we cannot know if it's going to
resume a while later.  That's the problem.

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> > A wakeup request from the hardware could cause a runtime resume to 
> > occur at this time.  The barrier wouldn't prevent that.
> > 
> > It's unlikely, I agree, but not impossible.
> 
> Yeah, I didn't think about that.

Come to think of it, if the hardware sends a wakeup request then it
must have been enabled for remote wakeup.  And if the hardware settings
are appropriate for system suspend then it must be enabled for system
wakeup.  Consequently a wakeup from the hardware ought to abort the
system suspend in any case.  So maybe we don't care about this 
scenario.

On the other hand, there may be other mechanisms that could cause a 
runtime resume at this inconvenient time.  A timer routine, for 
instance.

> But that also can occur in __device_suspend(), after we've checked the flag
> and decided not to invoke the ->suspend() callback, right?  So moving the
> check in there doesn't help much I'd say.  It closes the race window, but
> that's it.
> 
> That means that the whole approach based on ->prepare() is problematic
> unless we somehow mix it with disabling runtime PM.

Maybe the call to __pm_runtime_disable() should be moved from
__device_suspend_late() to __device_suspend(), after the callback has
been invoked (or skipped, as the case may be).  Then after runtime PM
has been disabled, you can check the device's status has changed and go
back to invoke the callback if necessary.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 11:46:55 AM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > > A wakeup request from the hardware could cause a runtime resume to 
> > > occur at this time.  The barrier wouldn't prevent that.
> > > 
> > > It's unlikely, I agree, but not impossible.
> > 
> > Yeah, I didn't think about that.
> 
> Come to think of it, if the hardware sends a wakeup request then it
> must have been enabled for remote wakeup.  And if the hardware settings
> are appropriate for system suspend then it must be enabled for system
> wakeup.  Consequently a wakeup from the hardware ought to abort the
> system suspend in any case.  So maybe we don't care about this 
> scenario.
> 
> On the other hand, there may be other mechanisms that could cause a 
> runtime resume at this inconvenient time.  A timer routine, for 
> instance.
> 
> > But that also can occur in __device_suspend(), after we've checked the flag
> > and decided not to invoke the ->suspend() callback, right?  So moving the
> > check in there doesn't help much I'd say.  It closes the race window, but
> > that's it.
> > 
> > That means that the whole approach based on ->prepare() is problematic
> > unless we somehow mix it with disabling runtime PM.
> 
> Maybe the call to __pm_runtime_disable() should be moved from
> __device_suspend_late() to __device_suspend(), after the callback has
> been invoked (or skipped, as the case may be).  Then after runtime PM
> has been disabled, you can check the device's status has changed and go
> back to invoke the callback if necessary.

We moved __pm_runtime_disable() to __device_suspend_late() to be able to
use pm_runtime_resume() in __device_suspend() (and we actually do that in
some places now).

But, in principle, we can do __pm_runtime_disable() temporarily in some place
between ->prepare() and ->suspend(), it doesn't matter if that's in
device_prepare() in __device_suspend() really.  Then, we can check the device's
runtime PM status (that'd need to be done carefully to take the disabling into
account) and
(1) if the device is runtime-suspended, set direct_complete for it without
    enabling runtime PM, or
(2) if the device is not runtime-suspended, clear direct_complete for it
    and re-enable runtime PM.
and in case of (1) we would re-enable runtime PM in device_complete().

That should work I suppose?

Of course, question is what ->prepare() is supposed to do then if it needs
to check the state of the device before deciding whether or not to return 1.
I guess it would need to disable runtime PM around that check too.

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> > Maybe the call to __pm_runtime_disable() should be moved from
> > __device_suspend_late() to __device_suspend(), after the callback has
> > been invoked (or skipped, as the case may be).  Then after runtime PM
> > has been disabled, you can check the device's status has changed and go
> > back to invoke the callback if necessary.
> 
> We moved __pm_runtime_disable() to __device_suspend_late() to be able to
> use pm_runtime_resume() in __device_suspend() (and we actually do that in
> some places now).
> 
> But, in principle, we can do __pm_runtime_disable() temporarily in some place
> between ->prepare() and ->suspend(), it doesn't matter if that's in
> device_prepare() in __device_suspend() really.

It should be as late as possible, to allow for detecting wakeup 
requests.

>  Then, we can check the device's
> runtime PM status (that'd need to be done carefully to take the disabling into
> account) and
> (1) if the device is runtime-suspended, set direct_complete for it without
>     enabling runtime PM, or
> (2) if the device is not runtime-suspended, clear direct_complete for it
>     and re-enable runtime PM.
> and in case of (1) we would re-enable runtime PM in device_complete().
> 
> That should work I suppose?

Yes; it's similar to what I proposed.  Note that this can be skipped if 
direct_complete is already clear.

> Of course, question is what ->prepare() is supposed to do then if it needs
> to check the state of the device before deciding whether or not to return 1.
> I guess it would need to disable runtime PM around that check too.

It would be surprising if ->prepare() needed to make any difficult
checks.  This would imply that the device could have multiple
runtime-suspend states, some of which are appropriate for system
suspend while others aren't.  Not impossible, but I wouldn't expect it
to come up often.

Besides, as I mentioned before, we never have to worry about status 
changes.  If one occurs while ->prepare() is running or afterward, it 
means the device is runtime-resumed and therefore the setting of 
direct_complete doesn't matter.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Tuesday, May 13, 2014 12:19:14 PM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > > Maybe the call to __pm_runtime_disable() should be moved from
> > > __device_suspend_late() to __device_suspend(), after the callback has
> > > been invoked (or skipped, as the case may be).  Then after runtime PM
> > > has been disabled, you can check the device's status has changed and go
> > > back to invoke the callback if necessary.
> > 
> > We moved __pm_runtime_disable() to __device_suspend_late() to be able to
> > use pm_runtime_resume() in __device_suspend() (and we actually do that in
> > some places now).
> > 
> > But, in principle, we can do __pm_runtime_disable() temporarily in some place
> > between ->prepare() and ->suspend(), it doesn't matter if that's in
> > device_prepare() in __device_suspend() really.
> 
> It should be as late as possible, to allow for detecting wakeup 
> requests.

I came to the same conclusion.  It has to be done in __device_suspend(), because
a child's ->suspend() may need to resume the parent, so runtime PM for the parent
cannot be disabled when the child's ->suspend() may be running.

> >  Then, we can check the device's
> > runtime PM status (that'd need to be done carefully to take the disabling into
> > account) and
> > (1) if the device is runtime-suspended, set direct_complete for it without
> >     enabling runtime PM, or
> > (2) if the device is not runtime-suspended, clear direct_complete for it
> >     and re-enable runtime PM.
> > and in case of (1) we would re-enable runtime PM in device_complete().
> > 
> > That should work I suppose?
> 
> Yes; it's similar to what I proposed.  Note that this can be skipped if 
> direct_complete is already clear.

Sure.

> > Of course, question is what ->prepare() is supposed to do then if it needs
> > to check the state of the device before deciding whether or not to return 1.
> > I guess it would need to disable runtime PM around that check too.
> 
> It would be surprising if ->prepare() needed to make any difficult
> checks.  This would imply that the device could have multiple
> runtime-suspend states, some of which are appropriate for system
> suspend while others aren't.  Not impossible, but I wouldn't expect it
> to come up often.

That is the case for every device with ACPI power management in principle. :-)

Please see patch [3/3] for details.

> Besides, as I mentioned before, we never have to worry about status 
> changes.  If one occurs while ->prepare() is running or afterward, it 
> means the device is runtime-resumed and therefore the setting of 
> direct_complete doesn't matter.

That's correct.

OK, I've updated the $subject patch in the meantime and the result is appended
Former patch [1/3] is not necessary any more now and patch [3/3] is still valid.

Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
resume all runtime-suspended devices during system suspend, mostly
because those devices may need to be reprogrammed due to different
wakeup settings for system sleep and for runtime PM.

For some devices, though, it's OK to remain in runtime suspend 
throughout a complete system suspend/resume cycle (if the device was in
runtime suspend at the start of the cycle).  We would like to do this
whenever possible, to avoid the overhead of extra power-up and power-down
events.

However, problems may arise because the device's descendants may require
it to be at full power at various points during the cycle.  Therefore the
most straightforward way to do this safely is if the device and all its
descendants can remain runtime suspended until the complete stage of
system resume.

To this end, introduce a new device PM flag, power.direct_complete
and modify the PM core to use that flag as follows.

If the ->prepare() callback of a device returns a positive number,
the PM core will regard that as an indication that it may leave the
device runtime-suspended.  It will then check if the system power
transition in progress is a suspend (and not hibernation in particular)
and if the device is, indeed, runtime-suspended.  In that case, the PM
core will set the device's power.direct_complete flag.  Otherwise it
will clear power.direct_complete for the device and it also will later
clear it for the device's parent (if there's one).

Next, the PM core will not invoke the ->suspend() ->suspend_late(),
->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
callbacks for all devices having power.direct_complete set.  It
will invoke their ->complete() callbacks, however, and those
callbacks are then responsible for resuming the devices as
appropriate, if necessary.  For example, in some cases they may
need to queue up runtime resume requests for the devices with the
help of pm_request_resume().

Changelog partly based on an Alan Stern's description of the idea
(http://marc.info/?l=linux-pm&m=139940466625569&w=2).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   65 +++++++++++++++++++++++++++++++++++-----------
 include/linux/pm.h        |    1 
 2 files changed, 51 insertions(+), 15 deletions(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -546,6 +546,7 @@ struct dev_pm_info {
 	bool			is_late_suspended:1;
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
+	bool			direct_complete:1;	/* Owned by the PM core */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_noirq_suspended)
@@ -605,7 +605,7 @@ static int device_resume_early(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_late_suspended)
@@ -735,6 +735,12 @@ static int device_resume(struct device *
 	if (dev->power.syscore)
 		goto Complete;
 
+	if (dev->power.direct_complete) {
+		/* Match the pm_runtime_disable() in __device_suspend(). */
+		pm_runtime_enable(dev);
+		goto Complete;
+	}
+
 	dpm_wait(dev->parent, async);
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
@@ -1007,7 +1013,7 @@ static int __device_suspend_noirq(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1146,7 +1152,7 @@ static int __device_suspend_late(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1332,6 +1338,16 @@ static int __device_suspend(struct devic
 	if (dev->power.syscore)
 		goto Complete;
 
+	if (dev->power.direct_complete) {
+		pm_runtime_disable(dev);
+		if (dev->power.disable_depth == 1
+		    && pm_runtime_status_suspended(dev))
+			goto Complete;
+
+		dev->power.direct_complete = false;
+		pm_runtime_enable(dev);
+	}
+
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
 
@@ -1382,10 +1398,19 @@ static int __device_suspend(struct devic
 
  End:
 	if (!error) {
+		struct device *parent = dev->parent;
+
 		dev->power.is_suspended = true;
-		if (dev->power.wakeup_path
-		    && dev->parent && !dev->parent->power.ignore_children)
-			dev->parent->power.wakeup_path = true;
+		if (parent) {
+			spin_lock_irq(&parent->power.lock);
+
+			dev->parent->power.direct_complete = false;
+			if (dev->power.wakeup_path
+			    && !dev->parent->power.ignore_children)
+				dev->parent->power.wakeup_path = true;
+
+			spin_unlock_irq(&parent->power.lock);
+		}
 	}
 
 	device_unlock(dev);
@@ -1487,7 +1512,7 @@ static int device_prepare(struct device
 {
 	int (*callback)(struct device *) = NULL;
 	char *info = NULL;
-	int error = 0;
+	int ret = 0;
 
 	if (dev->power.syscore)
 		return 0;
@@ -1523,17 +1548,27 @@ static int device_prepare(struct device
 		callback = dev->driver->pm->prepare;
 	}
 
-	if (callback) {
-		error = callback(dev);
-		suspend_report_result(callback, error);
-	}
+	if (callback)
+		ret = callback(dev);
 
 	device_unlock(dev);
 
-	if (error)
+	if (ret < 0) {
+		suspend_report_result(callback, ret);
 		pm_runtime_put(dev);
-
-	return error;
+		return ret;
+	}
+	/*
+	 * A positive return value from ->prepare() means "this device appears
+	 * to be runtime-suspended and its state is fine, so if it really is
+	 * runtime-suspended, you can leave it in that state provided that you
+	 * will do the same thing with all of its descendants".  This only
+	 * applies to suspend transitions, however.
+	 */
+	spin_lock_irq(&dev->power.lock);
+	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;
+	spin_unlock_irq(&dev->power.lock);
+	return 0;
 }
 
 /**

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Tue, 13 May 2014, Rafael J. Wysocki wrote:

> > It would be surprising if ->prepare() needed to make any difficult
> > checks.  This would imply that the device could have multiple
> > runtime-suspend states, some of which are appropriate for system
> > suspend while others aren't.  Not impossible, but I wouldn't expect it
> > to come up often.
> 
> That is the case for every device with ACPI power management in principle. :-)
> 
> Please see patch [3/3] for details.

I don't understand enough about the ACPI subsystem to follow the
details of that patch.

> OK, I've updated the $subject patch in the meantime and the result is appended
> Former patch [1/3] is not necessary any more now and patch [3/3] is still valid.
> 
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily
> 
> Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> resume all runtime-suspended devices during system suspend, mostly
> because those devices may need to be reprogrammed due to different
> wakeup settings for system sleep and for runtime PM.

...

> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This is looking quite good.  I have one suggestion for a small 
improvement...

> @@ -1332,6 +1338,16 @@ static int __device_suspend(struct devic
>  	if (dev->power.syscore)
>  		goto Complete;
>  
> +	if (dev->power.direct_complete) {
> +		pm_runtime_disable(dev);
> +		if (dev->power.disable_depth == 1
> +		    && pm_runtime_status_suspended(dev))
> +			goto Complete;
> +
> +		dev->power.direct_complete = false;
> +		pm_runtime_enable(dev);
> +	}

Do we want to allow ->prepare() to return > 0 if the device isn't
runtime suspended?  If we do then non-suspended devices may be a common
case.  We should then avoid the extra overhead of disable + enable.  
So I would write:

	if (dev->power.direct_complete) {
		if (pm_runtime_status_suspended(dev)) {
			pm_runtime_disable(dev);
			if (dev->power.disable_depth == 1
			    && pm_runtime_status_suspended(dev))
				goto Complete;
			pm_runtime_enable(dev);
		}
		dev->power.direct_complete = false;
	}

Also, now that we have finally settled on the appropriate API, there
needs to ba a patch updating the PM documentation.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Jacob Pan]

On Tue, 13 May 2014 03:10:19 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> resume all runtime-suspended devices during system suspend, mostly
> because those devices may need to be reprogrammed due to different
> wakeup settings for system sleep and for runtime PM.
> 
> For some devices, though, it's OK to remain in runtime suspend 
> throughout a complete system suspend/resume cycle (if the device was
> in runtime suspend at the start of the cycle).  We would like to do
> this whenever possible, to avoid the overhead of extra power-up and
> power-down events.
> 
> However, problems may arise because the device's descendants may
> require it to be at full power at various points during the cycle.
> Therefore the most straightforward way to do this safely is if the
> device and all its descendants can remain runtime suspended until the
> complete stage of system resume.
> 
> To this end, introduce a new device PM flag, power.direct_complete
> and modify the PM core to use that flag as follows.
> 
> If the ->prepare() callback of a device returns a positive number,
> the PM core will regard that as an indication that it may leave the
> device runtime-suspended.  It will then check if the system power
> transition in progress is a suspend (and not hibernation in
> particular) and if the device is, indeed, runtime-suspended.  In that
> case, the PM core will set the device's power.direct_complete flag.
> Otherwise it will clear power.direct_complete for the device and it
> also will later clear it for the device's parent (if there's one).
> 
> Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> callbacks for all devices having power.direct_complete set.  It
> will invoke their ->complete() callbacks, however, and those
> callbacks are then responsible for resuming the devices as
> appropriate, if necessary.
> 
> Changelog partly based on an Alan Stern's description of the idea
> (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   45
> ++++++++++++++++++++++++++++-----------------
> include/linux/pm.h        |    1 + 2 files changed, 29 insertions(+),
> 17 deletions(-)
> 
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -546,6 +546,7 @@ struct dev_pm_info {
>  	bool			is_late_suspended:1;
>  	bool			ignore_children:1;
>  	bool			early_init:1;	/* Owned by
> the PM core */
> +	bool			direct_complete:1;	/*
> Owned by the PM core */ spinlock_t		lock;
>  #ifdef CONFIG_PM_SLEEP
>  	struct list_head	entry;
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
>  	TRACE_DEVICE(dev);
>  	TRACE_RESUME(0);
>  
> -	if (dev->power.syscore)
> +	if (dev->power.syscore || dev->power.direct_complete)
>  		goto Out;
>  
>  	if (!dev->power.is_noirq_suspended)
> @@ -605,7 +605,7 @@ static int device_resume_early(struct de
>  	TRACE_DEVICE(dev);
>  	TRACE_RESUME(0);
>  
> -	if (dev->power.syscore)
> +	if (dev->power.syscore || dev->power.direct_complete)
>  		goto Out;
>  
>  	if (!dev->power.is_late_suspended)
> @@ -732,7 +732,7 @@ static int device_resume(struct device *
>  	TRACE_DEVICE(dev);
>  	TRACE_RESUME(0);
>  
> -	if (dev->power.syscore)
> +	if (dev->power.syscore || dev->power.direct_complete)
>  		goto Complete;
>  
>  	dpm_wait(dev->parent, async);
> @@ -1007,7 +1007,7 @@ static int __device_suspend_noirq(struct
>  		goto Complete;
>  	}
>  
> -	if (dev->power.syscore)
> +	if (dev->power.syscore || dev->power.direct_complete)
>  		goto Complete;
>  
>  	dpm_wait_for_children(dev, async);
> @@ -1146,7 +1146,7 @@ static int __device_suspend_late(struct
>  		goto Complete;
>  	}
>  
> -	if (dev->power.syscore)
> +	if (dev->power.syscore || dev->power.direct_complete)
>  		goto Complete;
>  
>  	dpm_wait_for_children(dev, async);
> @@ -1312,7 +1312,7 @@ static int __device_suspend(struct devic
>  
>  	dpm_wait_for_children(dev, async);
>  
> -	if (async_error || dev->power.syscore)
> +	if (async_error || dev->power.syscore ||
> dev->power.direct_complete) goto Complete;
>  
>  	dpm_watchdog_set(&wd, dev);
> @@ -1365,10 +1365,19 @@ static int __device_suspend(struct devic
>  
>   End:
>  	if (!error) {
> +		struct device *parent = dev->parent;
> +
>  		dev->power.is_suspended = true;
> -		if (dev->power.wakeup_path
> -		    && dev->parent
> && !dev->parent->power.ignore_children)
> -			dev->parent->power.wakeup_path = true;
> +		if (parent) {
> +			spin_lock_irq(&parent->power.lock);
> +
> +			dev->parent->power.direct_complete = false;
should we respect ignore_children flag here? not all parent devices
create children with proper .prepare() function. this allows parents
override children.
I am looking at USB, a USB device could have logical children such as
ep_xx, they don't go through the same subsystem .prepare().

> +			if (dev->power.wakeup_path
> +			    && !dev->parent->power.ignore_children)
> +				dev->parent->power.wakeup_path =
> true; +
> +			spin_unlock_irq(&parent->power.lock);
> +		}
>  	}
>  
>  	device_unlock(dev);
> @@ -1470,7 +1479,7 @@ static int device_prepare(struct device
>  {
>  	int (*callback)(struct device *) = NULL;
>  	char *info = NULL;
> -	int error = 0;
> +	int ret = 0;
>  
>  	if (dev->power.syscore)
>  		return 0;
> @@ -1518,17 +1527,19 @@ static int device_prepare(struct device
>  		callback = dev->driver->pm->prepare;
>  	}
>  
> -	if (callback) {
> -		error = callback(dev);
> -		suspend_report_result(callback, error);
> -	}
> +	if (callback)
> +		ret = callback(dev);
>  
>  	device_unlock(dev);
>  
> -	if (error)
> +	if (ret < 0) {
> +		suspend_report_result(callback, ret);
>  		pm_runtime_put(dev);
> -
> -	return error;
> +		return ret;
> +	}
> +	dev->power.direct_complete = ret > 0 && state.event ==
> PM_EVENT_SUSPEND
> +					&& pm_runtime_suspended(dev);
> +	return 0;
>  }
>  
>  /**
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Jacob Pan]

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Jacob Pan]

On Thu, 15 May 2014 10:29:42 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 15 May 2014, Jacob Pan wrote:
> 
> > > > should we respect ignore_children flag here? not all parent
> > > > devices create children with proper .prepare() function. this
> > > > allows parents override children.
> > > > I am looking at USB, a USB device could have logical children
> > > > such as ep_xx, they don't go through the same
> > > > subsystem .prepare().
> > > 
> > > Well, I'm not sure about that.  Let me consider that for a while.
> > OK. let me be more clear about the situation i see in USB. Correct
> > me if I am wrong, a USB device will always has at least one
> > endpoint/ep_00 as a kid for control pipe, it is a logical device.
> > So when device_prepare() is called, its call back is NULL which
> > makes .direct_complete = 0. Since children device suspend is called
> > before parents, the parents .direct_complete flag will always get
> > cleared.
> > 
> > What i am trying to achieve here is to see if we avoid resuming
> > built-in (hardwired connect_type) non-hub USB devices based on this
> > new patchset. E.g. we don't want to resume/suspend USB camera every
> > time in system suspend/resume cycle if they are already rpm
> > suspended. We can save ~100ms resume time for the devices we have
> > tested.
> 
> This is a good point, but I don't think it is at all related to 
> ignore_children.
> 
> Instead, it seems that the best way to solve it would be to add a 
> ->prepare() handler for usb_ep_device_type that would always turn 
> on direct_complete.
> 
yeah, that would solve the problem with EP device type. But what about
other subdevices. e.g. for USB camera, uvcvideo device? We can add
.prepare(return 1;) for each level but would it be better to have a
flag similar to ignore_children if not ignore_children itself.
Actually, I don't understand why this is not related to
ignore_children. Could you explain?

If the parent knows it can ignore children and already rpm suspended,
why do we still ask children?

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

[Jacob Pan]

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Wednesday, May 14, 2014 03:24:21 PM Jacob Pan wrote:
> On Tue, 13 May 2014 03:10:19 +0200
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> > resume all runtime-suspended devices during system suspend, mostly
> > because those devices may need to be reprogrammed due to different
> > wakeup settings for system sleep and for runtime PM.
> > 
> > For some devices, though, it's OK to remain in runtime suspend 
> > throughout a complete system suspend/resume cycle (if the device was
> > in runtime suspend at the start of the cycle).  We would like to do
> > this whenever possible, to avoid the overhead of extra power-up and
> > power-down events.
> > 
> > However, problems may arise because the device's descendants may
> > require it to be at full power at various points during the cycle.
> > Therefore the most straightforward way to do this safely is if the
> > device and all its descendants can remain runtime suspended until the
> > complete stage of system resume.
> > 
> > To this end, introduce a new device PM flag, power.direct_complete
> > and modify the PM core to use that flag as follows.
> > 
> > If the ->prepare() callback of a device returns a positive number,
> > the PM core will regard that as an indication that it may leave the
> > device runtime-suspended.  It will then check if the system power
> > transition in progress is a suspend (and not hibernation in
> > particular) and if the device is, indeed, runtime-suspended.  In that
> > case, the PM core will set the device's power.direct_complete flag.
> > Otherwise it will clear power.direct_complete for the device and it
> > also will later clear it for the device's parent (if there's one).
> > 
> > Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> > ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> > callbacks for all devices having power.direct_complete set.  It
> > will invoke their ->complete() callbacks, however, and those
> > callbacks are then responsible for resuming the devices as
> > appropriate, if necessary.
> > 
> > Changelog partly based on an Alan Stern's description of the idea
> > (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c |   45
> > ++++++++++++++++++++++++++++-----------------
> > include/linux/pm.h        |    1 + 2 files changed, 29 insertions(+),
> > 17 deletions(-)
> > 
> > Index: linux-pm/include/linux/pm.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm.h
> > +++ linux-pm/include/linux/pm.h
> > @@ -546,6 +546,7 @@ struct dev_pm_info {
> >  	bool			is_late_suspended:1;
> >  	bool			ignore_children:1;
> >  	bool			early_init:1;	/* Owned by
> > the PM core */
> > +	bool			direct_complete:1;	/*
> > Owned by the PM core */ spinlock_t		lock;
> >  #ifdef CONFIG_PM_SLEEP
> >  	struct list_head	entry;
> > Index: linux-pm/drivers/base/power/main.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/main.c
> > +++ linux-pm/drivers/base/power/main.c
> > @@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
> >  	TRACE_DEVICE(dev);
> >  	TRACE_RESUME(0);
> >  
> > -	if (dev->power.syscore)
> > +	if (dev->power.syscore || dev->power.direct_complete)
> >  		goto Out;
> >  
> >  	if (!dev->power.is_noirq_suspended)
> > @@ -605,7 +605,7 @@ static int device_resume_early(struct de
> >  	TRACE_DEVICE(dev);
> >  	TRACE_RESUME(0);
> >  
> > -	if (dev->power.syscore)
> > +	if (dev->power.syscore || dev->power.direct_complete)
> >  		goto Out;
> >  
> >  	if (!dev->power.is_late_suspended)
> > @@ -732,7 +732,7 @@ static int device_resume(struct device *
> >  	TRACE_DEVICE(dev);
> >  	TRACE_RESUME(0);
> >  
> > -	if (dev->power.syscore)
> > +	if (dev->power.syscore || dev->power.direct_complete)
> >  		goto Complete;
> >  
> >  	dpm_wait(dev->parent, async);
> > @@ -1007,7 +1007,7 @@ static int __device_suspend_noirq(struct
> >  		goto Complete;
> >  	}
> >  
> > -	if (dev->power.syscore)
> > +	if (dev->power.syscore || dev->power.direct_complete)
> >  		goto Complete;
> >  
> >  	dpm_wait_for_children(dev, async);
> > @@ -1146,7 +1146,7 @@ static int __device_suspend_late(struct
> >  		goto Complete;
> >  	}
> >  
> > -	if (dev->power.syscore)
> > +	if (dev->power.syscore || dev->power.direct_complete)
> >  		goto Complete;
> >  
> >  	dpm_wait_for_children(dev, async);
> > @@ -1312,7 +1312,7 @@ static int __device_suspend(struct devic
> >  
> >  	dpm_wait_for_children(dev, async);
> >  
> > -	if (async_error || dev->power.syscore)
> > +	if (async_error || dev->power.syscore ||
> > dev->power.direct_complete) goto Complete;
> >  
> >  	dpm_watchdog_set(&wd, dev);
> > @@ -1365,10 +1365,19 @@ static int __device_suspend(struct devic
> >  
> >   End:
> >  	if (!error) {
> > +		struct device *parent = dev->parent;
> > +
> >  		dev->power.is_suspended = true;
> > -		if (dev->power.wakeup_path
> > -		    && dev->parent
> > && !dev->parent->power.ignore_children)
> > -			dev->parent->power.wakeup_path = true;
> > +		if (parent) {
> > +			spin_lock_irq(&parent->power.lock);
> > +
> > +			dev->parent->power.direct_complete = false;
> should we respect ignore_children flag here? not all parent devices
> create children with proper .prepare() function. this allows parents
> override children.
> I am looking at USB, a USB device could have logical children such as
> ep_xx, they don't go through the same subsystem .prepare().

Well, I'm not sure about that.  Let me consider that for a while.

Alan, what do you think?

> 
> > +			if (dev->power.wakeup_path
> > +			    && !dev->parent->power.ignore_children)
> > +				dev->parent->power.wakeup_path =
> > true; +
> > +			spin_unlock_irq(&parent->power.lock);
> > +		}
> >  	}
> >  
> >  	device_unlock(dev);
> > @@ -1470,7 +1479,7 @@ static int device_prepare(struct device
> >  {
> >  	int (*callback)(struct device *) = NULL;
> >  	char *info = NULL;
> > -	int error = 0;
> > +	int ret = 0;
> >  
> >  	if (dev->power.syscore)
> >  		return 0;
> > @@ -1518,17 +1527,19 @@ static int device_prepare(struct device
> >  		callback = dev->driver->pm->prepare;
> >  	}
> >  
> > -	if (callback) {
> > -		error = callback(dev);
> > -		suspend_report_result(callback, error);
> > -	}
> > +	if (callback)
> > +		ret = callback(dev);
> >  
> >  	device_unlock(dev);
> >  
> > -	if (error)
> > +	if (ret < 0) {
> > +		suspend_report_result(callback, ret);
> >  		pm_runtime_put(dev);
> > -
> > -	return error;
> > +		return ret;
> > +	}
> > +	dev->power.direct_complete = ret > 0 && state.event ==
> > PM_EVENT_SUSPEND
> > +					&& pm_runtime_suspended(dev);
> > +	return 0;
> >  }
> >  
> >  /**
> > 
> > --

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Wednesday, May 14, 2014 10:53:16 AM Alan Stern wrote:
> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
> 
> > > It would be surprising if ->prepare() needed to make any difficult
> > > checks.  This would imply that the device could have multiple
> > > runtime-suspend states, some of which are appropriate for system
> > > suspend while others aren't.  Not impossible, but I wouldn't expect it
> > > to come up often.
> > 
> > That is the case for every device with ACPI power management in principle. :-)
> > 
> > Please see patch [3/3] for details.
> 
> I don't understand enough about the ACPI subsystem to follow the
> details of that patch.
> 
> > OK, I've updated the $subject patch in the meantime and the result is appended
> > Former patch [1/3] is not necessary any more now and patch [3/3] is still valid.
> > 
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily
> > 
> > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> > resume all runtime-suspended devices during system suspend, mostly
> > because those devices may need to be reprogrammed due to different
> > wakeup settings for system sleep and for runtime PM.
> 
> ...
> 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> This is looking quite good.  I have one suggestion for a small 
> improvement...
> 
> > @@ -1332,6 +1338,16 @@ static int __device_suspend(struct devic
> >  	if (dev->power.syscore)
> >  		goto Complete;
> >  
> > +	if (dev->power.direct_complete) {
> > +		pm_runtime_disable(dev);
> > +		if (dev->power.disable_depth == 1
> > +		    && pm_runtime_status_suspended(dev))
> > +			goto Complete;
> > +
> > +		dev->power.direct_complete = false;
> > +		pm_runtime_enable(dev);
> > +	}
> 
> Do we want to allow ->prepare() to return > 0 if the device isn't
> runtime suspended?  If we do then non-suspended devices may be a common
> case.  We should then avoid the extra overhead of disable + enable.  
> So I would write:
> 
> 	if (dev->power.direct_complete) {
> 		if (pm_runtime_status_suspended(dev)) {
> 			pm_runtime_disable(dev);
> 			if (dev->power.disable_depth == 1
> 			    && pm_runtime_status_suspended(dev))
> 				goto Complete;
> 			pm_runtime_enable(dev);
> 		}
> 		dev->power.direct_complete = false;
> 	}

That is a good idea, thanks!

> Also, now that we have finally settled on the appropriate API, there
> needs to ba a patch updating the PM documentation.

Absolutely.  I thought about updating the documentation in the same patch
(at least the comments in pm.h), but I guess a separate patch for files
under Documentation/ may be better.

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Ulf Hansson]

> Do we want to allow ->prepare() to return > 0 if the device isn't
> runtime suspended?  If we do then non-suspended devices may be a common
> case.  We should then avoid the extra overhead of disable + enable.
> So I would write:
>
>         if (dev->power.direct_complete) {
>                 if (pm_runtime_status_suspended(dev)) {
>                         pm_runtime_disable(dev);
>                         if (dev->power.disable_depth == 1
>                             && pm_runtime_status_suspended(dev))
>                                 goto Complete;
>                         pm_runtime_enable(dev);
>                 }
>                 dev->power.direct_complete = false;
>         }
>

I am wondering whether the above pm_runtime_disable|enable actually
belongs better in driver/subsystem in favour of the PM core?

Doesn't the driver/subsystem anyway needs to be on top of what goes
on? Typically, while runtime PM has been disabled, that might affect
it's wakeup handling? Or this case are already handled due to other
circumstances?

Kind regards
Ulf Hansson

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Thursday, May 15, 2014 02:06:59 PM Ulf Hansson wrote:
> > Do we want to allow ->prepare() to return > 0 if the device isn't
> > runtime suspended?  If we do then non-suspended devices may be a common
> > case.  We should then avoid the extra overhead of disable + enable.
> > So I would write:
> >
> >         if (dev->power.direct_complete) {
> >                 if (pm_runtime_status_suspended(dev)) {
> >                         pm_runtime_disable(dev);
> >                         if (dev->power.disable_depth == 1
> >                             && pm_runtime_status_suspended(dev))
> >                                 goto Complete;
> >                         pm_runtime_enable(dev);
> >                 }
> >                 dev->power.direct_complete = false;
> >         }
> >
> 
> I am wondering whether the above pm_runtime_disable|enable actually
> belongs better in driver/subsystem in favour of the PM core?

No, it doesn't.

> Doesn't the driver/subsystem anyway needs to be on top of what goes
> on? Typically, while runtime PM has been disabled, that might affect
> it's wakeup handling? Or this case are already handled due to other
> circumstances?

Yes, that's the case.

Thanks!

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

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Jacob Pan]

On Thu, 15 May 2014 13:11:15 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Wednesday, May 14, 2014 03:24:21 PM Jacob Pan wrote:
> > On Tue, 13 May 2014 03:10:19 +0200
> > "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> > 
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have
> > > to resume all runtime-suspended devices during system suspend,
> > > mostly because those devices may need to be reprogrammed due to
> > > different wakeup settings for system sleep and for runtime PM.
> > > 
> > > For some devices, though, it's OK to remain in runtime suspend 
> > > throughout a complete system suspend/resume cycle (if the device
> > > was in runtime suspend at the start of the cycle).  We would like
> > > to do this whenever possible, to avoid the overhead of extra
> > > power-up and power-down events.
> > > 
> > > However, problems may arise because the device's descendants may
> > > require it to be at full power at various points during the cycle.
> > > Therefore the most straightforward way to do this safely is if the
> > > device and all its descendants can remain runtime suspended until
> > > the complete stage of system resume.
> > > 
> > > To this end, introduce a new device PM flag, power.direct_complete
> > > and modify the PM core to use that flag as follows.
> > > 
> > > If the ->prepare() callback of a device returns a positive number,
> > > the PM core will regard that as an indication that it may leave
> > > the device runtime-suspended.  It will then check if the system
> > > power transition in progress is a suspend (and not hibernation in
> > > particular) and if the device is, indeed, runtime-suspended.  In
> > > that case, the PM core will set the device's
> > > power.direct_complete flag. Otherwise it will clear
> > > power.direct_complete for the device and it also will later clear
> > > it for the device's parent (if there's one).
> > > 
> > > Next, the PM core will not invoke the ->suspend()
> > > ->suspend_late(), ->suspend_irq(), ->resume_irq(),
> > > ->resume_early(), or ->resume() callbacks for all devices having
> > > power.direct_complete set.  It will invoke their ->complete()
> > > callbacks, however, and those callbacks are then responsible for
> > > resuming the devices as appropriate, if necessary.
> > > 
> > > Changelog partly based on an Alan Stern's description of the idea
> > > (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/base/power/main.c |   45
> > > ++++++++++++++++++++++++++++-----------------
> > > include/linux/pm.h        |    1 + 2 files changed, 29
> > > insertions(+), 17 deletions(-)
> > > 
> > > Index: linux-pm/include/linux/pm.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/pm.h
> > > +++ linux-pm/include/linux/pm.h
> > > @@ -546,6 +546,7 @@ struct dev_pm_info {
> > >  	bool			is_late_suspended:1;
> > >  	bool			ignore_children:1;
> > >  	bool			early_init:1;	/*
> > > Owned by the PM core */
> > > +	bool			direct_complete:1;	/*
> > > Owned by the PM core */ spinlock_t		lock;
> > >  #ifdef CONFIG_PM_SLEEP
> > >  	struct list_head	entry;
> > > Index: linux-pm/drivers/base/power/main.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/base/power/main.c
> > > +++ linux-pm/drivers/base/power/main.c
> > > @@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
> > >  	TRACE_DEVICE(dev);
> > >  	TRACE_RESUME(0);
> > >  
> > > -	if (dev->power.syscore)
> > > +	if (dev->power.syscore || dev->power.direct_complete)
> > >  		goto Out;
> > >  
> > >  	if (!dev->power.is_noirq_suspended)
> > > @@ -605,7 +605,7 @@ static int device_resume_early(struct de
> > >  	TRACE_DEVICE(dev);
> > >  	TRACE_RESUME(0);
> > >  
> > > -	if (dev->power.syscore)
> > > +	if (dev->power.syscore || dev->power.direct_complete)
> > >  		goto Out;
> > >  
> > >  	if (!dev->power.is_late_suspended)
> > > @@ -732,7 +732,7 @@ static int device_resume(struct device *
> > >  	TRACE_DEVICE(dev);
> > >  	TRACE_RESUME(0);
> > >  
> > > -	if (dev->power.syscore)
> > > +	if (dev->power.syscore || dev->power.direct_complete)
> > >  		goto Complete;
> > >  
> > >  	dpm_wait(dev->parent, async);
> > > @@ -1007,7 +1007,7 @@ static int __device_suspend_noirq(struct
> > >  		goto Complete;
> > >  	}
> > >  
> > > -	if (dev->power.syscore)
> > > +	if (dev->power.syscore || dev->power.direct_complete)
> > >  		goto Complete;
> > >  
> > >  	dpm_wait_for_children(dev, async);
> > > @@ -1146,7 +1146,7 @@ static int __device_suspend_late(struct
> > >  		goto Complete;
> > >  	}
> > >  
> > > -	if (dev->power.syscore)
> > > +	if (dev->power.syscore || dev->power.direct_complete)
> > >  		goto Complete;
> > >  
> > >  	dpm_wait_for_children(dev, async);
> > > @@ -1312,7 +1312,7 @@ static int __device_suspend(struct devic
> > >  
> > >  	dpm_wait_for_children(dev, async);
> > >  
> > > -	if (async_error || dev->power.syscore)
> > > +	if (async_error || dev->power.syscore ||
> > > dev->power.direct_complete) goto Complete;
> > >  
> > >  	dpm_watchdog_set(&wd, dev);
> > > @@ -1365,10 +1365,19 @@ static int __device_suspend(struct devic
> > >  
> > >   End:
> > >  	if (!error) {
> > > +		struct device *parent = dev->parent;
> > > +
> > >  		dev->power.is_suspended = true;
> > > -		if (dev->power.wakeup_path
> > > -		    && dev->parent
> > > && !dev->parent->power.ignore_children)
> > > -			dev->parent->power.wakeup_path = true;
> > > +		if (parent) {
> > > +			spin_lock_irq(&parent->power.lock);
> > > +
> > > +			dev->parent->power.direct_complete =
> > > false;
> > should we respect ignore_children flag here? not all parent devices
> > create children with proper .prepare() function. this allows parents
> > override children.
> > I am looking at USB, a USB device could have logical children such
> > as ep_xx, they don't go through the same subsystem .prepare().
> 
> Well, I'm not sure about that.  Let me consider that for a while.
OK. let me be more clear about the situation i see in USB. Correct me
if I am wrong, a USB device will always has at least one endpoint/ep_00
as a kid for control pipe, it is a logical device. So when
device_prepare() is called, its call back is NULL which
makes .direct_complete = 0. Since children device suspend is called
before parents, the parents .direct_complete flag will always get
cleared.

What i am trying to achieve here is to see if we avoid resuming
built-in (hardwired connect_type) non-hub USB devices based on this new
patchset. E.g. we don't want to resume/suspend USB camera every time in
system suspend/resume cycle if they are already rpm suspended. We can
save ~100ms resume time for the devices we have tested.

> 
> Alan, what do you think?
> 
> > 
> > > +			if (dev->power.wakeup_path
> > > +
> > > && !dev->parent->power.ignore_children)
> > > +				dev->parent->power.wakeup_path =
> > > true; +
> > > +			spin_unlock_irq(&parent->power.lock);
> > > +		}
> > >  	}
> > >  
> > >  	device_unlock(dev);
> > > @@ -1470,7 +1479,7 @@ static int device_prepare(struct device
> > >  {
> > >  	int (*callback)(struct device *) = NULL;
> > >  	char *info = NULL;
> > > -	int error = 0;
> > > +	int ret = 0;
> > >  
> > >  	if (dev->power.syscore)
> > >  		return 0;
> > > @@ -1518,17 +1527,19 @@ static int device_prepare(struct device
> > >  		callback = dev->driver->pm->prepare;
> > >  	}
> > >  
> > > -	if (callback) {
> > > -		error = callback(dev);
> > > -		suspend_report_result(callback, error);
> > > -	}
> > > +	if (callback)
> > > +		ret = callback(dev);
> > >  
> > >  	device_unlock(dev);
> > >  
> > > -	if (error)
> > > +	if (ret < 0) {
> > > +		suspend_report_result(callback, ret);
> > >  		pm_runtime_put(dev);
> > > -
> > > -	return error;
> > > +		return ret;
> > > +	}
> > > +	dev->power.direct_complete = ret > 0 && state.event ==
> > > PM_EVENT_SUSPEND
> > > +					&&
> > > pm_runtime_suspended(dev);
> > > +	return 0;
> > >  }
> > >  
> > >  /**
> > > 
> > > --
> 
> Rafael
> 

[Jacob Pan]

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Thu, 15 May 2014, Jacob Pan wrote:

> > > should we respect ignore_children flag here? not all parent devices
> > > create children with proper .prepare() function. this allows parents
> > > override children.
> > > I am looking at USB, a USB device could have logical children such
> > > as ep_xx, they don't go through the same subsystem .prepare().
> > 
> > Well, I'm not sure about that.  Let me consider that for a while.
> OK. let me be more clear about the situation i see in USB. Correct me
> if I am wrong, a USB device will always has at least one endpoint/ep_00
> as a kid for control pipe, it is a logical device. So when
> device_prepare() is called, its call back is NULL which
> makes .direct_complete = 0. Since children device suspend is called
> before parents, the parents .direct_complete flag will always get
> cleared.
> 
> What i am trying to achieve here is to see if we avoid resuming
> built-in (hardwired connect_type) non-hub USB devices based on this new
> patchset. E.g. we don't want to resume/suspend USB camera every time in
> system suspend/resume cycle if they are already rpm suspended. We can
> save ~100ms resume time for the devices we have tested.

This is a good point, but I don't think it is at all related to 
ignore_children.

Instead, it seems that the best way to solve it would be to add a 
->prepare() handler for usb_ep_device_type that would always turn 
on direct_complete.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Thu, 15 May 2014, Jacob Pan wrote:

> On Thu, 15 May 2014 10:29:42 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Thu, 15 May 2014, Jacob Pan wrote:
> > 
> > > > > should we respect ignore_children flag here? not all parent
> > > > > devices create children with proper .prepare() function. this
> > > > > allows parents override children.
> > > > > I am looking at USB, a USB device could have logical children
> > > > > such as ep_xx, they don't go through the same
> > > > > subsystem .prepare().
> > > > 
> > > > Well, I'm not sure about that.  Let me consider that for a while.
> > > OK. let me be more clear about the situation i see in USB. Correct
> > > me if I am wrong, a USB device will always has at least one
> > > endpoint/ep_00 as a kid for control pipe, it is a logical device.
> > > So when device_prepare() is called, its call back is NULL which
> > > makes .direct_complete = 0. Since children device suspend is called
> > > before parents, the parents .direct_complete flag will always get
> > > cleared.
> > > 
> > > What i am trying to achieve here is to see if we avoid resuming
> > > built-in (hardwired connect_type) non-hub USB devices based on this
> > > new patchset. E.g. we don't want to resume/suspend USB camera every
> > > time in system suspend/resume cycle if they are already rpm
> > > suspended. We can save ~100ms resume time for the devices we have
> > > tested.
> > 
> > This is a good point, but I don't think it is at all related to 
> > ignore_children.
> > 
> > Instead, it seems that the best way to solve it would be to add a 
> > ->prepare() handler for usb_ep_device_type that would always turn 
> > on direct_complete.
> > 
> yeah, that would solve the problem with EP device type. But what about
> other subdevices. e.g. for USB camera, uvcvideo device? We can add
> .prepare(return 1;) for each level but would it be better to have a
> flag similar to ignore_children if not ignore_children itself.

Something like that could always be added.

> Actually, I don't understand why this is not related to
> ignore_children. Could you explain?

It's hard to explain why two things are totally separate.  Much better 
for you to describe why you think they _are_ related, so that I can 
explain how you are wrong.

> If the parent knows it can ignore children and already rpm suspended,
> why do we still ask children?

The "ignore_children" flag doesn't mean that the parent can ignore its 
children.  It means that the PM core is allowed to do a runtime suspend 
of the parent while leaving the children at full power.

In particular, it doesn't mean that the children's ->suspend() callback 
will work correctly if it is called while the parent is runtime 
suspended.

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 13 May 2014, Rafael J. Wysocki wrote:
>
>> > A wakeup request from the hardware could cause a runtime resume to 
>> > occur at this time.  The barrier wouldn't prevent that.
>> > 
>> > It's unlikely, I agree, but not impossible.
>> 
>> Yeah, I didn't think about that.
>
> Come to think of it, if the hardware sends a wakeup request then it
> must have been enabled for remote wakeup.  And if the hardware settings
> are appropriate for system suspend then it must be enabled for system
> wakeup.  Consequently a wakeup from the hardware ought to abort the
> system suspend in any case.  So maybe we don't care about this 
> scenario.
>
> On the other hand, there may be other mechanisms that could cause a 
> runtime resume at this inconvenient time.  A timer routine, for 
> instance.

Another common case is when device X depends on device Y in it's
->prepare or ->suspend path (e.g. need to write to an I2C connected
GPIO/PMIC) in which case, device Y (and the I2C bus) would be runtime
resumed during device X's ->prepare or ->suspend path, and possibly
after device Y (or the I2C busses) ->prepare and ->suspend.

Kevin

[PATCH 0/3] (was: Re: PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily)

[Rafael J. Wysocki]

On Thursday, May 15, 2014 01:13:02 PM Rafael J. Wysocki wrote:
> On Wednesday, May 14, 2014 10:53:16 AM Alan Stern wrote:
> > On Tue, 13 May 2014, Rafael J. Wysocki wrote:

[cut]

> > 
> > 	if (dev->power.direct_complete) {
> > 		if (pm_runtime_status_suspended(dev)) {
> > 			pm_runtime_disable(dev);
> > 			if (dev->power.disable_depth == 1
> > 			    && pm_runtime_status_suspended(dev))
> > 				goto Complete;
> > 			pm_runtime_enable(dev);
> > 		}
> > 		dev->power.direct_complete = false;
> > 	}
> 
> That is a good idea, thanks!

New patches follow.

[1/3] is the core change with the above added.

> > Also, now that we have finally settled on the appropriate API, there
> > needs to ba a patch updating the PM documentation.
> 
> Absolutely.  I thought about updating the documentation in the same patch
> (at least the comments in pm.h), but I guess a separate patch for files
> under Documentation/ may be better.

[2/3] is the corresponding documentation update (I hope I haven't overlooked
anything important).

[3/3] is a resend of the ACPI PM patch on top of the core change.

Rafael

[PATCH 1/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
resume all runtime-suspended devices during system suspend, mostly
because those devices may need to be reprogrammed due to different
wakeup settings for system sleep and for runtime PM.

For some devices, though, it's OK to remain in runtime suspend 
throughout a complete system suspend/resume cycle (if the device was in
runtime suspend at the start of the cycle).  We would like to do this
whenever possible, to avoid the overhead of extra power-up and power-down
events.

However, problems may arise because the device's descendants may require
it to be at full power at various points during the cycle.  Therefore the
most straightforward way to do this safely is if the device and all its
descendants can remain runtime suspended until the complete stage of
system resume.

To this end, introduce a new device PM flag, power.direct_complete
and modify the PM core to use that flag as follows.

If the ->prepare() callback of a device returns a positive number,
the PM core will regard that as an indication that it may leave the
device runtime-suspended.  It will then check if the system power
transition in progress is a suspend (and not hibernation in particular)
and if the device is, indeed, runtime-suspended.  In that case, the PM
core will set the device's power.direct_complete flag.  Otherwise it
will clear power.direct_complete for the device and it also will later
clear it for the device's parent (if there's one).

Next, the PM core will not invoke the ->suspend() ->suspend_late(),
->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
callbacks for all devices having power.direct_complete set.  It
will invoke their ->complete() callbacks, however, and those
callbacks are then responsible for resuming the devices as
appropriate, if necessary.  For example, in some cases they may
need to queue up runtime resume requests for the devices using
pm_request_resume().

Changelog partly based on an Alan Stern's description of the idea
(http://marc.info/?l=linux-pm&m=139940466625569&w=2).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c  |   66 ++++++++++++++++++++++++++++++++++-----------
 include/linux/pm.h         |   36 +++++++++++++++++++-----
 include/linux/pm_runtime.h |    6 ++++
 3 files changed, 85 insertions(+), 23 deletions(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -93,13 +93,23 @@ typedef struct pm_message {
  *	been registered) to recover from the race condition.
  *	This method is executed for all kinds of suspend transitions and is
  *	followed by one of the suspend callbacks: @suspend(), @freeze(), or
- *	@poweroff().  The PM core executes subsystem-level @prepare() for all
- *	devices before starting to invoke suspend callbacks for any of them, so
- *	generally devices may be assumed to be functional or to respond to
- *	runtime resume requests while @prepare() is being executed.  However,
- *	device drivers may NOT assume anything about the availability of user
- *	space at that time and it is NOT valid to request firmware from within
- *	@prepare() (it's too late to do that).  It also is NOT valid to allocate
+ *	@poweroff().  If the transition is a suspend to memory or standby (that
+ *	is, not related to hibernation), the return value of @prepare() may be
+ *	used to indicate to the PM core to leave the device in runtime suspend
+ *	if applicable.  Namely, if @prepare() returns a positive number, the PM
+ *	core will understand that as a declaration that the device appears to be
+ *	runtime-suspended and it may be left in that state during the entire
+ *	transition and during the subsequent resume if all of its descendants
+ *	are left in runtime suspend too.  If that happens, @complete() will be
+ *	executed directly after @prepare() and it must ensure the proper
+ *	functioning of the device after the system resume.
+ *	The PM core executes subsystem-level @prepare() for all devices before
+ *	starting to invoke suspend callbacks for any of them, so generally
+ *	devices may be assumed to be functional or to respond to runtime resume
+ *	requests while @prepare() is being executed.  However, device drivers
+ *	may NOT assume anything about the availability of user space at that
+ *	time and it is NOT valid to request firmware from within @prepare()
+ *	(it's too late to do that).  It also is NOT valid to allocate
  *	substantial amounts of memory from @prepare() in the GFP_KERNEL mode.
  *	[To work around these limitations, drivers may register suspend and
  *	hibernation notifiers to be executed before the freezing of tasks.]
@@ -112,7 +122,16 @@ typedef struct pm_message {
  *	of the other devices that the PM core has unsuccessfully attempted to
  *	suspend earlier).
  *	The PM core executes subsystem-level @complete() after it has executed
- *	the appropriate resume callbacks for all devices.
+ *	the appropriate resume callbacks for all devices.  If the corresponding
+ *	@prepare() at the beginning of the suspend transition returned a
+ *	positive number and the device was left in runtime suspend (without
+ *	executing any suspend and resume callbacks for it), @complete() will be
+ *	the only callback executed for the device during resume.  In that case,
+ *	@complete() must be prepared to do whatever is necessary to ensure the
+ *	proper functioning of the device after the system resume.  To this end,
+ *	@complete() can check the power.direct_complete flag of the device to
+ *	learn whether (unset) or not (set) the previous suspend and resume
+ *	callbacks have been executed for it.
  *
  * @suspend: Executed before putting the system into a sleep state in which the
  *	contents of main memory are preserved.  The exact action to perform
@@ -546,6 +565,7 @@ struct dev_pm_info {
 	bool			is_late_suspended:1;
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
+	bool			direct_complete:1;	/* Owned by the PM core */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -479,7 +479,7 @@ static int device_resume_noirq(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_noirq_suspended)
@@ -605,7 +605,7 @@ static int device_resume_early(struct de
 	TRACE_DEVICE(dev);
 	TRACE_RESUME(0);
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Out;
 
 	if (!dev->power.is_late_suspended)
@@ -735,6 +735,12 @@ static int device_resume(struct device *
 	if (dev->power.syscore)
 		goto Complete;
 
+	if (dev->power.direct_complete) {
+		/* Match the pm_runtime_disable() in __device_suspend(). */
+		pm_runtime_enable(dev);
+		goto Complete;
+	}
+
 	dpm_wait(dev->parent, async);
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
@@ -1007,7 +1013,7 @@ static int __device_suspend_noirq(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1146,7 +1152,7 @@ static int __device_suspend_late(struct
 		goto Complete;
 	}
 
-	if (dev->power.syscore)
+	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
 	dpm_wait_for_children(dev, async);
@@ -1332,6 +1338,17 @@ static int __device_suspend(struct devic
 	if (dev->power.syscore)
 		goto Complete;
 
+	if (dev->power.direct_complete) {
+		if (pm_runtime_status_suspended(dev)) {
+			pm_runtime_disable(dev);
+			if (pm_runtime_suspended_if_enabled(dev))
+				goto Complete;
+
+			pm_runtime_enable(dev);
+		}
+		dev->power.direct_complete = false;
+	}
+
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
 
@@ -1382,10 +1399,19 @@ static int __device_suspend(struct devic
 
  End:
 	if (!error) {
+		struct device *parent = dev->parent;
+
 		dev->power.is_suspended = true;
-		if (dev->power.wakeup_path
-		    && dev->parent && !dev->parent->power.ignore_children)
-			dev->parent->power.wakeup_path = true;
+		if (parent) {
+			spin_lock_irq(&parent->power.lock);
+
+			dev->parent->power.direct_complete = false;
+			if (dev->power.wakeup_path
+			    && !dev->parent->power.ignore_children)
+				dev->parent->power.wakeup_path = true;
+
+			spin_unlock_irq(&parent->power.lock);
+		}
 	}
 
 	device_unlock(dev);
@@ -1487,7 +1513,7 @@ static int device_prepare(struct device
 {
 	int (*callback)(struct device *) = NULL;
 	char *info = NULL;
-	int error = 0;
+	int ret = 0;
 
 	if (dev->power.syscore)
 		return 0;
@@ -1523,17 +1549,27 @@ static int device_prepare(struct device
 		callback = dev->driver->pm->prepare;
 	}
 
-	if (callback) {
-		error = callback(dev);
-		suspend_report_result(callback, error);
-	}
+	if (callback)
+		ret = callback(dev);
 
 	device_unlock(dev);
 
-	if (error)
+	if (ret < 0) {
+		suspend_report_result(callback, ret);
 		pm_runtime_put(dev);
-
-	return error;
+		return ret;
+	}
+	/*
+	 * A positive return value from ->prepare() means "this device appears
+	 * to be runtime-suspended and its state is fine, so if it really is
+	 * runtime-suspended, you can leave it in that state provided that you
+	 * will do the same thing with all of its descendants".  This only
+	 * applies to suspend transitions, however.
+	 */
+	spin_lock_irq(&dev->power.lock);
+	dev->power.direct_complete = ret > 0 && state.event == PM_EVENT_SUSPEND;
+	spin_unlock_irq(&dev->power.lock);
+	return 0;
 }
 
 /**
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -101,6 +101,11 @@ static inline bool pm_runtime_status_sus
 	return dev->power.runtime_status == RPM_SUSPENDED;
 }
 
+static inline bool pm_runtime_suspended_if_enabled(struct device *dev)
+{
+	return pm_runtime_status_suspended(dev) && dev->power.disable_depth == 1;
+}
+
 static inline bool pm_runtime_enabled(struct device *dev)
 {
 	return !dev->power.disable_depth;
@@ -150,6 +155,7 @@ static inline void device_set_run_wake(s
 static inline bool pm_runtime_suspended(struct device *dev) { return false; }
 static inline bool pm_runtime_active(struct device *dev) { return true; }
 static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
+static inline bool pm_runtime_suspended_if_enabled(struct device *dev) { return false; }
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
 static inline void pm_runtime_no_callbacks(struct device *dev) {}

[PATCH 2/3] PM / sleep: Update device PM documentation to cover direct_complete

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Update the device PM documentation in devices.txt and runtime_pm.txt
to reflect the changes in the system suspend and resume handling
related to the introduction of the new power.direct_complete flag.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/devices.txt    |   34 ++++++++++++++++++++++++++++++----
 Documentation/power/runtime_pm.txt |   17 +++++++++++++++++
 2 files changed, 47 insertions(+), 4 deletions(-)

Index: linux-pm/Documentation/power/devices.txt
===================================================================
--- linux-pm.orig/Documentation/power/devices.txt
+++ linux-pm/Documentation/power/devices.txt
@@ -2,6 +2,7 @@ Device Power Management
 
 Copyright (c) 2010-2011 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
 Copyright (c) 2010 Alan Stern <stern@rowland.harvard.edu>
+Copyright (c) 2014 Intel Corp., Rafael J. Wysocki <rafael.j.wysocki@intel.com>
 
 
 Most of the code in Linux is device drivers, so most of the Linux power
@@ -326,6 +327,20 @@ the phases are:
 	driver in some way for the upcoming system power transition, but it
 	should not put the device into a low-power state.
 
+	For devices supporting runtime power management, the return value of the
+	prepare callback can be used to indicate to the PM core that it may
+	safely leave the device in runtime suspend (if runtime-suspended
+	already), provided that all of the device's descendants are also left in
+	runtime suspend.  Namely, if the prepare callback returns a positive
+	number and that happens for all of the descendants of the device too,
+	and all of them (including the device itself) are runtime-suspended, the
+	PM core will skip the suspend, suspend_late and	suspend_noirq suspend
+	phases as well as the resume_noirq, resume_early and resume phases of
+	the following system resume for all of these devices.	In that case,
+	the complete callback will be called directly after the prepare callback
+	and is entirely responsible for bringing the device back to the
+	functional state as appropriate.
+
     2.	The suspend methods should quiesce the device to stop it from performing
 	I/O.  They also may save the device registers and put it into the
 	appropriate low-power state, depending on the bus type the device is on,
@@ -400,12 +415,23 @@ When resuming from freeze, standby or me
 	the resume callbacks occur; it's not necessary to wait until the
 	complete phase.
 
+	Moreover, if the preceding prepare callback returned a positive number,
+	the device may have been left in runtime suspend throughout the whole
+	system suspend and resume (the suspend, suspend_late, suspend_noirq
+	phases of system suspend and the resume_noirq, resume_early, resume
+	phases of system resume may have been skipped for it).  In that case,
+	the complete callback is entirely responsible for bringing the device
+	back to the functional state after system suspend if necessary.  [For
+	example, it may need to queue up a runtime resume request for the device
+	for this purpose.]  To check if that is the case, the complete callback
+	can consult the device's power.direct_complete flag.  Namely, if that
+	flag is set when the complete callback is being run, it has been called
+	directly after the preceding prepare and special action may be required
+	to make the device work correctly afterward.
+
 At the end of these phases, drivers should be as functional as they were before
 suspending: I/O can be performed using DMA and IRQs, and the relevant clocks are
-gated on.  Even if the device was in a low-power state before the system sleep
-because of runtime power management, afterwards it should be back in its
-full-power state.  There are multiple reasons why it's best to do this; they are
-discussed in more detail in Documentation/power/runtime_pm.txt.
+gated on.
 
 However, the details here may again be platform-specific.  For example,
 some systems support multiple "run" states, and the mode in effect at
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -2,6 +2,7 @@ Runtime Power Management Framework for I
 
 (C) 2009-2011 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc.
 (C) 2010 Alan Stern <stern@rowland.harvard.edu>
+(C) 2014 Intel Corp., Rafael J. Wysocki <rafael.j.wysocki@intel.com>
 
 1. Introduction
 
@@ -444,6 +445,10 @@ drivers/base/power/runtime.c and include
   bool pm_runtime_status_suspended(struct device *dev);
     - return true if the device's runtime PM status is 'suspended'
 
+  bool pm_runtime_suspended_if_enabled(struct device *dev);
+    - return true if the device's runtime PM status is 'suspended' and its
+      'power.disable_depth' field is equal to 1
+
   void pm_runtime_allow(struct device *dev);
     - set the power.runtime_auto flag for the device and decrease its usage
       counter (used by the /sys/devices/.../power/control interface to
@@ -644,6 +649,18 @@ place (in particular, if the system is n
 be more efficient to leave the devices that had been suspended before the system
 suspend began in the suspended state.
 
+To this end, the PM core provides a mechanism allowing some coordination between
+different levels of device hierarchy.  Namely, if a system suspend .prepare()
+callback returns a positive number for a device, that indicates to the PM core
+that the device appears to be runtime-suspended and its state is fine, so it
+may be left in runtime suspend provided that all of its descendants are also
+left in runtime suspend.  If that happens, the PM core will not execute any
+system suspend and resume callbacks for all of those devices, except for the
+complete callback, which is then entirely responsible for handling the device
+as appropriate.  This only applies to system suspend transitions that are not
+related to hibernation (see Documentation/power/devices.txt for more
+information).
+
 The PM core does its best to reduce the probability of race conditions between
 the runtime PM and system suspend/resume (and hibernation) callbacks by carrying
 out the following operations:

[PATCH 3/3][Resend] ACPI / PM: Avoid resuming devices in ACPI PM domain during system suspend

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rework the ACPI PM domain's PM callbacks to avoid resuming devices
during system suspend (in order to modify their wakeup settings etc.)
if that isn't necessary.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/acpi/scan.c      |    4 ++++
 include/acpi/acpi_bus.h  |    3 ++-
 3 files changed, 48 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -907,20 +907,44 @@ int acpi_subsys_prepare(struct device *d
 	if (dev->power.ignore_children)
 		pm_runtime_resume(dev);
 
+	pm_set_direct_resume(dev, true);
 	return pm_generic_prepare(dev);
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
- * acpi_subsys_suspend - Run the device driver's suspend callback.
+ * acpi_subsys_suspend - Handle device suspend stage of system suspend.
  * @dev: Device to handle.
- *
- * Follow PCI and resume devices suspended at run time before running their
- * system suspend callbacks.
  */
 int acpi_subsys_suspend(struct device *dev)
 {
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	u32 sys_target;
+
+	if (!adev || !pm_runtime_enabled_and_suspended(dev)) {
+		pm_set_direct_resume(dev, false);
+		goto out;
+	}
+	if (!pm_direct_resume_is_set(dev)
+	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		goto resume;
+
+	sys_target = acpi_target_system_state();
+	if (sys_target != ACPI_STATE_S0) {
+		int ret, state;
+
+		if (adev->power.flags.dsw_present)
+			goto resume;
+
+		ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+		if (!ret && state == adev->power.state)
+			return 0;
+	}
+
+ resume:
 	pm_runtime_resume(dev);
+
+ out:
 	return pm_generic_suspend(dev);
 }
 
@@ -954,6 +978,19 @@ int acpi_subsys_resume_early(struct devi
 EXPORT_SYMBOL_GPL(acpi_subsys_resume_early);
 
 /**
+ * acpi_subsys_resume - Handle device resume stage of system resume.
+ * @dev: Device to handle.
+ */
+int acpi_subsys_resume(struct device *dev)
+{
+	if (pm_direct_resume_is_set(dev)) {
+		pm_runtime_resume(dev);
+		return 0;
+	}
+	return pm_generic_resume(dev);
+}
+
+/**
  * acpi_subsys_freeze - Run the device driver's freeze callback.
  * @dev: Device to handle.
  */
@@ -982,6 +1019,7 @@ static struct dev_pm_domain acpi_general
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,
+		.resume = acpi_subsys_resume,
 		.freeze = acpi_subsys_freeze,
 		.poweroff = acpi_subsys_suspend,
 		.poweroff_late = acpi_subsys_suspend_late,
Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -261,7 +261,8 @@ struct acpi_device_power_flags {
 	u32 inrush_current:1;	/* Serialize Dx->D0 */
 	u32 power_removed:1;	/* Optimize Dx->D0 */
 	u32 ignore_parent:1;	/* Power is independent of parent power state */
-	u32 reserved:27;
+	u32 dsw_present:1;	/* _DSW present? */
+	u32 reserved:26;
 };
 
 struct acpi_device_power_state {
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1551,9 +1551,13 @@ static void acpi_bus_get_power_flags(str
 	 */
 	if (acpi_has_method(device->handle, "_PSC"))
 		device->power.flags.explicit_get = 1;
+
 	if (acpi_has_method(device->handle, "_IRC"))
 		device->power.flags.inrush_current = 1;
 
+	if (acpi_has_method(device->handle, "_DSW"))
+		device->power.flags.dsw_present = 1;
+
 	/*
 	 * Enumerate supported power management states
 	 */

Re: [PATCH 1/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Fri, 16 May 2014, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> resume all runtime-suspended devices during system suspend, mostly
> because those devices may need to be reprogrammed due to different
> wakeup settings for system sleep and for runtime PM.
> 
> For some devices, though, it's OK to remain in runtime suspend 
> throughout a complete system suspend/resume cycle (if the device was in
> runtime suspend at the start of the cycle).  We would like to do this
> whenever possible, to avoid the overhead of extra power-up and power-down
> events.
> 
> However, problems may arise because the device's descendants may require
> it to be at full power at various points during the cycle.  Therefore the
> most straightforward way to do this safely is if the device and all its
> descendants can remain runtime suspended until the complete stage of
> system resume.
> 
> To this end, introduce a new device PM flag, power.direct_complete
> and modify the PM core to use that flag as follows.
> 
> If the ->prepare() callback of a device returns a positive number,
> the PM core will regard that as an indication that it may leave the
> device runtime-suspended.  It will then check if the system power
> transition in progress is a suspend (and not hibernation in particular)
> and if the device is, indeed, runtime-suspended.  In that case, the PM
> core will set the device's power.direct_complete flag.  Otherwise it
> will clear power.direct_complete for the device and it also will later
> clear it for the device's parent (if there's one).
> 
> Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> callbacks for all devices having power.direct_complete set.  It
> will invoke their ->complete() callbacks, however, and those
> callbacks are then responsible for resuming the devices as
> appropriate, if necessary.  For example, in some cases they may
> need to queue up runtime resume requests for the devices using
> pm_request_resume().
> 
> Changelog partly based on an Alan Stern's description of the idea
> (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>

And likewise for the documentation patches.

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Jacob Pan]

On Thu, 15 May 2014 11:58:55 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 15 May 2014, Jacob Pan wrote:
> 
> > On Thu, 15 May 2014 10:29:42 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > 
> > > > > > should we respect ignore_children flag here? not all parent
> > > > > > devices create children with proper .prepare() function.
> > > > > > this allows parents override children.
> > > > > > I am looking at USB, a USB device could have logical
> > > > > > children such as ep_xx, they don't go through the same
> > > > > > subsystem .prepare().
> > > > > 
> > > > > Well, I'm not sure about that.  Let me consider that for a
> > > > > while.
> > > > OK. let me be more clear about the situation i see in USB.
> > > > Correct me if I am wrong, a USB device will always has at least
> > > > one endpoint/ep_00 as a kid for control pipe, it is a logical
> > > > device. So when device_prepare() is called, its call back is
> > > > NULL which makes .direct_complete = 0. Since children device
> > > > suspend is called before parents, the parents .direct_complete
> > > > flag will always get cleared.
> > > > 
> > > > What i am trying to achieve here is to see if we avoid resuming
> > > > built-in (hardwired connect_type) non-hub USB devices based on
> > > > this new patchset. E.g. we don't want to resume/suspend USB
> > > > camera every time in system suspend/resume cycle if they are
> > > > already rpm suspended. We can save ~100ms resume time for the
> > > > devices we have tested.
> > > 
> > > This is a good point, but I don't think it is at all related to 
> > > ignore_children.
> > > 
> > > Instead, it seems that the best way to solve it would be to add a 
> > > ->prepare() handler for usb_ep_device_type that would always turn 
> > > on direct_complete.
> > > 
> > yeah, that would solve the problem with EP device type. But what
> > about other subdevices. e.g. for USB camera, uvcvideo device? We
> > can add .prepare(return 1;) for each level but would it be better
> > to have a flag similar to ignore_children if not ignore_children
> > itself.
> 
> Something like that could always be added.
or, how about if a device's .prepare() is NULL, we could
assume .direct_resume() should be set. i.e.


--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1539,7 +1539,7 @@ static int device_prepare(struct device *dev,
pm_message_t state) pm_runtime_put(dev);
                return ret;
        }
-       dev->power.direct_complete = ret > 0 && state.event ==
PM_EVENT_SUSPEND
+       dev->power.direct_complete = (!callback || ret > 0) &&
state.event == PM_EVENT_SUSPEND && pm_runtime_suspended(dev);
        dev_dbg(dev, "%s:direct_complete %d, info %s\n", __func__,
dev->power.direct_complete, info);

> 
> > Actually, I don't understand why this is not related to
> > ignore_children. Could you explain?
> 
> It's hard to explain why two things are totally separate.  Much
> better for you to describe why you think they _are_ related, so that
> I can explain how you are wrong.
> 
> > If the parent knows it can ignore children and already rpm
> > suspended, why do we still ask children?
> 
> The "ignore_children" flag doesn't mean that the parent can ignore
> its children.  It means that the PM core is allowed to do a runtime
> suspend of the parent while leaving the children at full power.
> 
> In particular, it doesn't mean that the children's ->suspend()
> callback will work correctly if it is called while the parent is
> runtime suspended.
that explains my question about ignore_chilren flag. thanks.
> 
> Alan Stern
> 

[Jacob Pan]

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Friday, May 16, 2014 08:20:55 AM Jacob Pan wrote:
> On Thu, 15 May 2014 11:58:55 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Thu, 15 May 2014, Jacob Pan wrote:
> > 
> > > On Thu, 15 May 2014 10:29:42 -0400 (EDT)
> > > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > 
> > > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > > 
> > > > > > > should we respect ignore_children flag here? not all parent
> > > > > > > devices create children with proper .prepare() function.
> > > > > > > this allows parents override children.
> > > > > > > I am looking at USB, a USB device could have logical
> > > > > > > children such as ep_xx, they don't go through the same
> > > > > > > subsystem .prepare().
> > > > > > 
> > > > > > Well, I'm not sure about that.  Let me consider that for a
> > > > > > while.
> > > > > OK. let me be more clear about the situation i see in USB.
> > > > > Correct me if I am wrong, a USB device will always has at least
> > > > > one endpoint/ep_00 as a kid for control pipe, it is a logical
> > > > > device. So when device_prepare() is called, its call back is
> > > > > NULL which makes .direct_complete = 0. Since children device
> > > > > suspend is called before parents, the parents .direct_complete
> > > > > flag will always get cleared.
> > > > > 
> > > > > What i am trying to achieve here is to see if we avoid resuming
> > > > > built-in (hardwired connect_type) non-hub USB devices based on
> > > > > this new patchset. E.g. we don't want to resume/suspend USB
> > > > > camera every time in system suspend/resume cycle if they are
> > > > > already rpm suspended. We can save ~100ms resume time for the
> > > > > devices we have tested.
> > > > 
> > > > This is a good point, but I don't think it is at all related to 
> > > > ignore_children.
> > > > 
> > > > Instead, it seems that the best way to solve it would be to add a 
> > > > ->prepare() handler for usb_ep_device_type that would always turn 
> > > > on direct_complete.
> > > > 
> > > yeah, that would solve the problem with EP device type. But what
> > > about other subdevices. e.g. for USB camera, uvcvideo device? We
> > > can add .prepare(return 1;) for each level but would it be better
> > > to have a flag similar to ignore_children if not ignore_children
> > > itself.
> > 
> > Something like that could always be added.
> or, how about if a device's .prepare() is NULL, we could
> assume .direct_resume() should be set. i.e.

You mean direct_complete (which is a flag, not a function), I suppose?

Wouldn't that go a bit too far?  It seems to be based on the assumption that
all devices having no ->prepare() callback can be safely left in runtime
suspend over a system suspend/resume cycle, but is that assumption actually
satisfied for all such devices?

> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1539,7 +1539,7 @@ static int device_prepare(struct device *dev,
> pm_message_t state) pm_runtime_put(dev);
>                 return ret;
>         }
> -       dev->power.direct_complete = ret > 0 && state.event ==
> PM_EVENT_SUSPEND
> +       dev->power.direct_complete = (!callback || ret > 0) &&
> state.event == PM_EVENT_SUSPEND && pm_runtime_suspended(dev);
>         dev_dbg(dev, "%s:direct_complete %d, info %s\n", __func__,
> dev->power.direct_complete, info);
> 
> > 
> > > Actually, I don't understand why this is not related to
> > > ignore_children. Could you explain?
> > 
> > It's hard to explain why two things are totally separate.  Much
> > better for you to describe why you think they _are_ related, so that
> > I can explain how you are wrong.
> > 
> > > If the parent knows it can ignore children and already rpm
> > > suspended, why do we still ask children?
> > 
> > The "ignore_children" flag doesn't mean that the parent can ignore
> > its children.  It means that the PM core is allowed to do a runtime
> > suspend of the parent while leaving the children at full power.
> > 
> > In particular, it doesn't mean that the children's ->suspend()
> > callback will work correctly if it is called while the parent is
> > runtime suspended.
> that explains my question about ignore_chilren flag. thanks.
> > 
> > Alan Stern
> > 
> 
> [Jacob Pan]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Friday, May 16, 2014 10:27:37 AM Alan Stern wrote:
> On Fri, 16 May 2014, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to
> > resume all runtime-suspended devices during system suspend, mostly
> > because those devices may need to be reprogrammed due to different
> > wakeup settings for system sleep and for runtime PM.
> > 
> > For some devices, though, it's OK to remain in runtime suspend 
> > throughout a complete system suspend/resume cycle (if the device was in
> > runtime suspend at the start of the cycle).  We would like to do this
> > whenever possible, to avoid the overhead of extra power-up and power-down
> > events.
> > 
> > However, problems may arise because the device's descendants may require
> > it to be at full power at various points during the cycle.  Therefore the
> > most straightforward way to do this safely is if the device and all its
> > descendants can remain runtime suspended until the complete stage of
> > system resume.
> > 
> > To this end, introduce a new device PM flag, power.direct_complete
> > and modify the PM core to use that flag as follows.
> > 
> > If the ->prepare() callback of a device returns a positive number,
> > the PM core will regard that as an indication that it may leave the
> > device runtime-suspended.  It will then check if the system power
> > transition in progress is a suspend (and not hibernation in particular)
> > and if the device is, indeed, runtime-suspended.  In that case, the PM
> > core will set the device's power.direct_complete flag.  Otherwise it
> > will clear power.direct_complete for the device and it also will later
> > clear it for the device's parent (if there's one).
> > 
> > Next, the PM core will not invoke the ->suspend() ->suspend_late(),
> > ->suspend_irq(), ->resume_irq(), ->resume_early(), or ->resume()
> > callbacks for all devices having power.direct_complete set.  It
> > will invoke their ->complete() callbacks, however, and those
> > callbacks are then responsible for resuming the devices as
> > appropriate, if necessary.  For example, in some cases they may
> > need to queue up runtime resume requests for the devices using
> > pm_request_resume().
> > 
> > Changelog partly based on an Alan Stern's description of the idea
> > (http://marc.info/?l=linux-pm&m=139940466625569&w=2).
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 
> And likewise for the documentation patches.

Thanks a lot!

[PATCH 3/3][update] ACPI / PM: Avoid resuming devices in ACPI PM domain during system suspend

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Avoid resuming devices in ACPI PM domain during system suspend

Rework the ACPI PM domain's PM callbacks to avoid resuming devices
during system suspend (in order to modify their wakeup settings etc.)
if that isn't necessary.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

I sent an outdated patch previously, sorry about that.  This is what was meant
to be sent.

Thanks!

---
 drivers/acpi/device_pm.c |   43 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

Index: linux-pm/include/acpi/acpi_bus.h
===================================================================
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -261,7 +261,8 @@ struct acpi_device_power_flags {
 	u32 inrush_current:1;	/* Serialize Dx->D0 */
 	u32 power_removed:1;	/* Optimize Dx->D0 */
 	u32 ignore_parent:1;	/* Power is independent of parent power state */
-	u32 reserved:27;
+	u32 dsw_present:1;	/* _DSW present? */
+	u32 reserved:26;
 };
 
 struct acpi_device_power_state {
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1551,9 +1551,13 @@ static void acpi_bus_get_power_flags(str
 	 */
 	if (acpi_has_method(device->handle, "_PSC"))
 		device->power.flags.explicit_get = 1;
+
 	if (acpi_has_method(device->handle, "_IRC"))
 		device->power.flags.inrush_current = 1;
 
+	if (acpi_has_method(device->handle, "_DSW"))
+		device->power.flags.dsw_present = 1;
+
 	/*
 	 * Enumerate supported power management states
 	 */
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -900,18 +900,46 @@ EXPORT_SYMBOL_GPL(acpi_dev_resume_early)
  */
 int acpi_subsys_prepare(struct device *dev)
 {
-	/*
-	 * Devices having power.ignore_children set may still be necessary for
-	 * suspending their children in the next phase of device suspend.
-	 */
-	if (dev->power.ignore_children)
-		pm_runtime_resume(dev);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	u32 sys_target;
+	int ret, state;
+
+	ret = pm_generic_prepare(dev);
+	if (ret < 0)
+		return ret;
+
+	if (!adev || !pm_runtime_suspended(dev)
+	    || device_may_wakeup(dev) != !!adev->wakeup.prepare_count)
+		return 0;
+
+	sys_target = acpi_target_system_state();
+	if (sys_target == ACPI_STATE_S0)
+		return 1;
+
+	if (adev->power.flags.dsw_present)
+		return 0;
 
-	return pm_generic_prepare(dev);
+	ret = acpi_dev_pm_get_state(dev, adev, sys_target, NULL, &state);
+	return !ret && state == adev->power.state;
 }
 EXPORT_SYMBOL_GPL(acpi_subsys_prepare);
 
 /**
+ * acpi_subsys_complete - Finalize device's resume during system resume.
+ * @dev: Device to handle.
+ */
+void acpi_subsys_complete(struct device *dev)
+{
+	/*
+	 * If the device had been runtime-suspended before the system went into
+	 * the sleep state it is going out of and it has never been resumed till
+	 * now, resume it in case the firmware powered it up.
+	 */
+	if (dev->power.direct_complete)
+		pm_request_resume(dev);
+}
+
+/**
  * acpi_subsys_suspend - Run the device driver's suspend callback.
  * @dev: Device to handle.
  *
@@ -979,6 +1007,7 @@ static struct dev_pm_domain acpi_general
 #endif
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
+		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
 		.suspend_late = acpi_subsys_suspend_late,
 		.resume_early = acpi_subsys_resume_early,

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Jacob Pan]

On Fri, 16 May 2014 23:08:01 +0200
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Friday, May 16, 2014 08:20:55 AM Jacob Pan wrote:
> > On Thu, 15 May 2014 11:58:55 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > 
> > > > On Thu, 15 May 2014 10:29:42 -0400 (EDT)
> > > > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > 
> > > > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > > > 
> > > > > > > > should we respect ignore_children flag here? not all
> > > > > > > > parent devices create children with proper .prepare()
> > > > > > > > function. this allows parents override children.
> > > > > > > > I am looking at USB, a USB device could have logical
> > > > > > > > children such as ep_xx, they don't go through the same
> > > > > > > > subsystem .prepare().
> > > > > > > 
> > > > > > > Well, I'm not sure about that.  Let me consider that for a
> > > > > > > while.
> > > > > > OK. let me be more clear about the situation i see in USB.
> > > > > > Correct me if I am wrong, a USB device will always has at
> > > > > > least one endpoint/ep_00 as a kid for control pipe, it is a
> > > > > > logical device. So when device_prepare() is called, its
> > > > > > call back is NULL which makes .direct_complete = 0. Since
> > > > > > children device suspend is called before parents, the
> > > > > > parents .direct_complete flag will always get cleared.
> > > > > > 
> > > > > > What i am trying to achieve here is to see if we avoid
> > > > > > resuming built-in (hardwired connect_type) non-hub USB
> > > > > > devices based on this new patchset. E.g. we don't want to
> > > > > > resume/suspend USB camera every time in system
> > > > > > suspend/resume cycle if they are already rpm suspended. We
> > > > > > can save ~100ms resume time for the devices we have tested.
> > > > > 
> > > > > This is a good point, but I don't think it is at all related
> > > > > to ignore_children.
> > > > > 
> > > > > Instead, it seems that the best way to solve it would be to
> > > > > add a ->prepare() handler for usb_ep_device_type that would
> > > > > always turn on direct_complete.
> > > > > 
> > > > yeah, that would solve the problem with EP device type. But what
> > > > about other subdevices. e.g. for USB camera, uvcvideo device? We
> > > > can add .prepare(return 1;) for each level but would it be
> > > > better to have a flag similar to ignore_children if not
> > > > ignore_children itself.
> > > 
> > > Something like that could always be added.
> > or, how about if a device's .prepare() is NULL, we could
> > assume .direct_resume() should be set. i.e.
> 
> You mean direct_complete (which is a flag, not a function), I suppose?
> 
yes.
> Wouldn't that go a bit too far?  It seems to be based on the
> assumption that all devices having no ->prepare() callback can be
> safely left in runtime suspend over a system suspend/resume cycle,
> but is that assumption actually satisfied for all such devices?
> 
yes, I agree it is risky though i don't see problems with my limited
testing. But on the other side, it is too strict.
I also tried adding .prepare( return 1;) to usb_ep_device_type pm ops,
that didn't work either. The reason is that ep devices don't support
runtime pm (disable_depth > 0). I think in this case ignore_children
flag should be the right indicator to ignore pm_runtime_suspended()?

> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1539,7 +1539,7 @@ static int device_prepare(struct device *dev,
> > pm_message_t state) pm_runtime_put(dev);
> >                 return ret;
> >         }
> > -       dev->power.direct_complete = ret > 0 && state.event ==
> > PM_EVENT_SUSPEND
> > +       dev->power.direct_complete = (!callback || ret > 0) &&
> > state.event == PM_EVENT_SUSPEND && pm_runtime_suspended(dev);
> >         dev_dbg(dev, "%s:direct_complete %d, info %s\n", __func__,
> > dev->power.direct_complete, info);
> > 
> > > 
> > > > Actually, I don't understand why this is not related to
> > > > ignore_children. Could you explain?
> > > 
> > > It's hard to explain why two things are totally separate.  Much
> > > better for you to describe why you think they _are_ related, so
> > > that I can explain how you are wrong.
> > > 
> > > > If the parent knows it can ignore children and already rpm
> > > > suspended, why do we still ask children?
> > > 
> > > The "ignore_children" flag doesn't mean that the parent can ignore
> > > its children.  It means that the PM core is allowed to do a
> > > runtime suspend of the parent while leaving the children at full
> > > power.
> > > 
> > > In particular, it doesn't mean that the children's ->suspend()
> > > callback will work correctly if it is called while the parent is
> > > runtime suspended.
> > that explains my question about ignore_chilren flag. thanks.
> > > 
> > > Alan Stern
> > > 
> > 
> > [Jacob Pan]
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-acpi" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[Jacob Pan]

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Alan Stern]

On Mon, 19 May 2014, Jacob Pan wrote:

> > Wouldn't that go a bit too far?  It seems to be based on the
> > assumption that all devices having no ->prepare() callback can be
> > safely left in runtime suspend over a system suspend/resume cycle,
> > but is that assumption actually satisfied for all such devices?
> > 
> yes, I agree it is risky though i don't see problems with my limited
> testing. But on the other side, it is too strict.
> I also tried adding .prepare( return 1;) to usb_ep_device_type pm ops,
> that didn't work either. The reason is that ep devices don't support
> runtime pm (disable_depth > 0). I think in this case ignore_children
> flag should be the right indicator to ignore pm_runtime_suspended()?

Maybe it would be better to add a new flag that means "This is a 
virtual device and the PM core can ignore it completely".

Alan Stern

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Monday, May 19, 2014 03:53:58 PM Alan Stern wrote:
> On Mon, 19 May 2014, Jacob Pan wrote:
> 
> > > Wouldn't that go a bit too far?  It seems to be based on the
> > > assumption that all devices having no ->prepare() callback can be
> > > safely left in runtime suspend over a system suspend/resume cycle,
> > > but is that assumption actually satisfied for all such devices?
> > > 
> > yes, I agree it is risky though i don't see problems with my limited
> > testing. But on the other side, it is too strict.
> > I also tried adding .prepare( return 1;) to usb_ep_device_type pm ops,
> > that didn't work either. The reason is that ep devices don't support
> > runtime pm (disable_depth > 0). I think in this case ignore_children
> > flag should be the right indicator to ignore pm_runtime_suspended()?
> 
> Maybe it would be better to add a new flag that means "This is a 
> virtual device and the PM core can ignore it completely".

I like that idea. :-)

Rafael

Re: [RFC][PATCH 2/3] PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

[Rafael J. Wysocki]

On Monday, May 19, 2014 02:18:31 AM Jacob Pan wrote:
> On Fri, 16 May 2014 23:08:01 +0200
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> 
> > On Friday, May 16, 2014 08:20:55 AM Jacob Pan wrote:
> > > On Thu, 15 May 2014 11:58:55 -0400 (EDT)
> > > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > 
> > > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > > 
> > > > > On Thu, 15 May 2014 10:29:42 -0400 (EDT)
> > > > > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > 
> > > > > > On Thu, 15 May 2014, Jacob Pan wrote:
> > > > > > 
> > > > > > > > > should we respect ignore_children flag here? not all
> > > > > > > > > parent devices create children with proper .prepare()
> > > > > > > > > function. this allows parents override children.
> > > > > > > > > I am looking at USB, a USB device could have logical
> > > > > > > > > children such as ep_xx, they don't go through the same
> > > > > > > > > subsystem .prepare().
> > > > > > > > 
> > > > > > > > Well, I'm not sure about that.  Let me consider that for a
> > > > > > > > while.
> > > > > > > OK. let me be more clear about the situation i see in USB.
> > > > > > > Correct me if I am wrong, a USB device will always has at
> > > > > > > least one endpoint/ep_00 as a kid for control pipe, it is a
> > > > > > > logical device. So when device_prepare() is called, its
> > > > > > > call back is NULL which makes .direct_complete = 0. Since
> > > > > > > children device suspend is called before parents, the
> > > > > > > parents .direct_complete flag will always get cleared.
> > > > > > > 
> > > > > > > What i am trying to achieve here is to see if we avoid
> > > > > > > resuming built-in (hardwired connect_type) non-hub USB
> > > > > > > devices based on this new patchset. E.g. we don't want to
> > > > > > > resume/suspend USB camera every time in system
> > > > > > > suspend/resume cycle if they are already rpm suspended. We
> > > > > > > can save ~100ms resume time for the devices we have tested.
> > > > > > 
> > > > > > This is a good point, but I don't think it is at all related
> > > > > > to ignore_children.
> > > > > > 
> > > > > > Instead, it seems that the best way to solve it would be to
> > > > > > add a ->prepare() handler for usb_ep_device_type that would
> > > > > > always turn on direct_complete.
> > > > > > 
> > > > > yeah, that would solve the problem with EP device type. But what
> > > > > about other subdevices. e.g. for USB camera, uvcvideo device? We
> > > > > can add .prepare(return 1;) for each level but would it be
> > > > > better to have a flag similar to ignore_children if not
> > > > > ignore_children itself.
> > > > 
> > > > Something like that could always be added.
> > > or, how about if a device's .prepare() is NULL, we could
> > > assume .direct_resume() should be set. i.e.
> > 
> > You mean direct_complete (which is a flag, not a function), I suppose?
> > 
> yes.
> > Wouldn't that go a bit too far?  It seems to be based on the
> > assumption that all devices having no ->prepare() callback can be
> > safely left in runtime suspend over a system suspend/resume cycle,
> > but is that assumption actually satisfied for all such devices?
> > 
> yes, I agree it is risky though i don't see problems with my limited
> testing. But on the other side, it is too strict.
> I also tried adding .prepare( return 1;) to usb_ep_device_type pm ops,
> that didn't work either. The reason is that ep devices don't support
> runtime pm (disable_depth > 0). I think in this case ignore_children
> flag should be the right indicator to ignore pm_runtime_suspended()?

I guess an "ignore this device completely for PM" flag as suggested by
Alan would be better.

Rafael