[PATCH v3 0/2] PM: direct_complete_default and pm_runtime_enable_recursive

[PATCH v3 0/2] PM: direct_complete_default and pm_runtime_enable_recursive

[Tomeu Vizoso]

Hi,

this is v3 of a series that adds API to make it more convenient for drivers to
opt into remaining runtime suspended when the system goes to sleep.

in this version I have replaced force_direct_complete with
direct_complete_default as suggested by Rafael and thus have had to add a
pm_runtime_enable_recursive() function call to make sure that all descendants
have runtime PM enabled.

Inheritance of the direct_complete_default flag has to be done in
device_pm_add instead of in pm_runtime_init because sometimes the parent won't
have been set yet by then.

Thanks,

Tomeu

Tomeu Vizoso (2):
  PM / sleep: Add power.direct_complete_default flag
  PM / Runtime: Add pm_runtime_enable_recursive

 Documentation/power/runtime_pm.txt |  8 +++++++-
 drivers/base/power/main.c          |  5 +++++
 drivers/base/power/runtime.c       | 15 +++++++++++++++
 include/linux/pm.h                 |  1 +
 include/linux/pm_runtime.h         |  1 +
 5 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.4.1

[PATCH v3 1/2] PM / sleep: Add power.direct_complete_default flag

[Tomeu Vizoso]

Introduce a new per-device flag power.direct_complete_default that will
instruct the PM core to let that device remain in runtime suspend when
the system goes into a sleep power state, without it having to implement
the prepare() callback.

This is useful because otherwise it would be needed to get dozens of
drivers to implement the prepare() callback even if they don't have a
1-to-1 relationship with a piece of HW.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

---

v3:	* Have the flag be a more direct substitute for prepare() as
suggested by Rafael
	* Inherit this flag from the parent device

v2:	* Fix wording as suggested by Kevin Hilman
---
 Documentation/power/runtime_pm.txt | 8 +++++++-
 drivers/base/power/main.c          | 5 +++++
 include/linux/pm.h                 | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index e76dc0a..caf7b53 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -228,6 +228,12 @@ defined in include/linux/pm.h:
   unsigned int ignore_children;
     - if set, the value of child_count is ignored (but still updated)
 
+  bool direct_complete_default;
+    - if set, the device will be left runtime-suspended when the system
+      transitions to a sleep state, unless there's a .prepare() callback that
+      returns a non-positive value. This flag is inherited from the parent
+      device when a device is added to the PM core's list of active devices.
+
   unsigned int disable_depth;
     - used for disabling the helper functions (they work normally if this is
       equal to zero); the initial value of it is 1 (i.e. runtime PM is
@@ -669,7 +675,7 @@ 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).
+information).  See also power.direct_complete_default.
 
 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
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 3d874ec..175ddec 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -128,6 +128,9 @@ void device_pm_add(struct device *dev)
 	if (dev->parent && dev->parent->power.is_prepared)
 		dev_warn(dev, "parent %s should not be sleeping\n",
 			dev_name(dev->parent));
+	if (dev->parent)
+		dev->power.direct_complete_default =
+			dev->parent->power.direct_complete_default;
 	list_add_tail(&dev->power.entry, &dpm_list);
 	mutex_unlock(&dpm_list_mtx);
 }
@@ -1589,6 +1592,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
 		trace_device_pm_callback_start(dev, info, state.event);
 		ret = callback(dev);
 		trace_device_pm_callback_end(dev, ret);
+	} else if (dev->power.direct_complete_default) {
+		ret = 1;
 	}
 
 	device_unlock(dev);
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 4890743..fcb3451 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -565,6 +565,7 @@ struct dev_pm_info {
 	bool			ignore_children:1;
 	bool			early_init:1;	/* Owned by the PM core */
 	bool			direct_complete:1;	/* Owned by the PM core */
+	bool			direct_complete_default:1;	/* Ditto */
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
-- 
2.4.1

[PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

This function makes less cumbersome to enable runtime PM in a device and
its descendants.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/base/power/runtime.c | 15 +++++++++++++++
 include/linux/pm_runtime.h   |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 5070c4f..2c96f3f 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1189,6 +1189,21 @@ void pm_runtime_enable(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pm_runtime_enable);
 
+static int __pm_runtime_enable_recursive(struct device *dev, void *data)
+{
+	pm_runtime_enable(dev);
+
+	device_for_each_child(dev, NULL, __pm_runtime_enable_recursive);
+
+	return 0;
+}
+
+void pm_runtime_enable_recursive(struct device *dev)
+{
+	__pm_runtime_enable_recursive(dev, NULL);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_enable_recursive);
+
 /**
  * pm_runtime_forbid - Block runtime PM of a device.
  * @dev: Device to handle.
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 30e84d4..d393ac3 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -43,6 +43,7 @@ extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
+extern void pm_runtime_enable_recursive(struct device *dev);
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
-- 
2.4.1

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Tue, 19 May 2015, Tomeu Vizoso wrote:

> This function makes less cumbersome to enable runtime PM in a device and
> its descendants.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

I don't see the point of this.  In the scenario you have in mind, are
the device and all its descendants registered by the same
subsystem/driver?  If they are, can't the subsystem/driver code enable
runtime PM for each of them when they are registered, by adding a
single call in the right spot?

If they don't all belong to the same subsystem/driver, who is going to 
call your pm_runtime_enable_recursive routine?  No single caller will 
have the right to enable runtime PM for all these devices.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Add power.direct_complete_default flag

[Alan Stern]

On Tue, 19 May 2015, Tomeu Vizoso wrote:

> Introduce a new per-device flag power.direct_complete_default that will
> instruct the PM core to let that device remain in runtime suspend when
> the system goes into a sleep power state, without it having to implement
> the prepare() callback.
> 
> This is useful because otherwise it would be needed to get dozens of
> drivers to implement the prepare() callback even if they don't have a
> 1-to-1 relationship with a piece of HW.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

This seems to be a very reasonable approach, and much like what I 
envisioned from Rafael's original suggestion.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Add power.direct_complete_default flag

[Ulf Hansson]

On 19 May 2015 at 16:11, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
> Introduce a new per-device flag power.direct_complete_default that will
> instruct the PM core to let that device remain in runtime suspend when
> the system goes into a sleep power state, without it having to implement
> the prepare() callback.

The above wording make it sounds like the normal behavior is that the
PM core will runtime resume the device during the system PM suspend
phase. That's not the case.

As I understand it for the mentioned cases above, it will skip
invoking the system PM callbacks, if the device is already runtime
suspended.

>
>
> This is useful because otherwise it would be needed to get dozens of
> drivers to implement the prepare() callback even if they don't have a
> 1-to-1 relationship with a piece of HW.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> ---
>
> v3:     * Have the flag be a more direct substitute for prepare() as
> suggested by Rafael
>         * Inherit this flag from the parent device
>
> v2:     * Fix wording as suggested by Kevin Hilman
> ---
>  Documentation/power/runtime_pm.txt | 8 +++++++-
>  drivers/base/power/main.c          | 5 +++++
>  include/linux/pm.h                 | 1 +
>  3 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
> index e76dc0a..caf7b53 100644
> --- a/Documentation/power/runtime_pm.txt
> +++ b/Documentation/power/runtime_pm.txt
> @@ -228,6 +228,12 @@ defined in include/linux/pm.h:
>    unsigned int ignore_children;
>      - if set, the value of child_count is ignored (but still updated)
>
> +  bool direct_complete_default;
> +    - if set, the device will be left runtime-suspended when the system
> +      transitions to a sleep state, unless there's a .prepare() callback that
> +      returns a non-positive value. This flag is inherited from the parent
> +      device when a device is added to the PM core's list of active devices.

Similar comment as to the change log.

> +
>    unsigned int disable_depth;
>      - used for disabling the helper functions (they work normally if this is
>        equal to zero); the initial value of it is 1 (i.e. runtime PM is
> @@ -669,7 +675,7 @@ 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).
> +information).  See also power.direct_complete_default.
>
>  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
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3d874ec..175ddec 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -128,6 +128,9 @@ void device_pm_add(struct device *dev)
>         if (dev->parent && dev->parent->power.is_prepared)
>                 dev_warn(dev, "parent %s should not be sleeping\n",
>                         dev_name(dev->parent));
> +       if (dev->parent)
> +               dev->power.direct_complete_default =
> +                       dev->parent->power.direct_complete_default;
>         list_add_tail(&dev->power.entry, &dpm_list);
>         mutex_unlock(&dpm_list_mtx);
>  }
> @@ -1589,6 +1592,8 @@ static int device_prepare(struct device *dev, pm_message_t state)
>                 trace_device_pm_callback_start(dev, info, state.event);
>                 ret = callback(dev);
>                 trace_device_pm_callback_end(dev, ret);
> +       } else if (dev->power.direct_complete_default) {
> +               ret = 1;
>         }
>
>         device_unlock(dev);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 4890743..fcb3451 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -565,6 +565,7 @@ struct dev_pm_info {
>         bool                    ignore_children:1;
>         bool                    early_init:1;   /* Owned by the PM core */
>         bool                    direct_complete:1;      /* Owned by the PM core */
> +       bool                    direct_complete_default:1;      /* Ditto */
>         spinlock_t              lock;
>  #ifdef CONFIG_PM_SLEEP
>         struct list_head        entry;
> --

Besides the above minor things, this looks good to me.

Kind regards
Uffe

Re: [PATCH v3 1/2] PM / sleep: Add power.direct_complete_default flag

[Rafael J. Wysocki]

On Tuesday, May 19, 2015 04:11:15 PM Tomeu Vizoso wrote:
> Introduce a new per-device flag power.direct_complete_default that will
> instruct the PM core to let that device remain in runtime suspend when
> the system goes into a sleep power state, without it having to implement
> the prepare() callback.
> 
> This is useful because otherwise it would be needed to get dozens of
> drivers to implement the prepare() callback even if they don't have a
> 1-to-1 relationship with a piece of HW.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

The code changes look good, but please change the doc/changelog as
already suggested by Ulf.


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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Tuesday, May 19, 2015 01:49:15 PM Alan Stern wrote:
> On Tue, 19 May 2015, Tomeu Vizoso wrote:
> 
> > This function makes less cumbersome to enable runtime PM in a device and
> > its descendants.
> > 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> I don't see the point of this.  In the scenario you have in mind, are
> the device and all its descendants registered by the same
> subsystem/driver?  If they are, can't the subsystem/driver code enable
> runtime PM for each of them when they are registered, by adding a
> single call in the right spot?
> 
> If they don't all belong to the same subsystem/driver, who is going to 
> call your pm_runtime_enable_recursive routine?  No single caller will 
> have the right to enable runtime PM for all these devices.

Agreed.


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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 19 May 2015, Tomeu Vizoso wrote:
>
>> This function makes less cumbersome to enable runtime PM in a device and
>> its descendants.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> I don't see the point of this.  In the scenario you have in mind, are
> the device and all its descendants registered by the same
> subsystem/driver?

Not quite, the scenario here is a driver (uvcvideo) that deals with a
specific piece of hardware and knows that all the descendants of the
device it's bound to are virtual.

The subtree is:

1-1:1.0 (bound to uvcvideo)
        ep_87
        input4
                event4
        media0
        video0

I liked how the force_direct_complete flag played out here, but I
agree with Rafael that it can be abused as the PM domain or the bus
type weren't able to prevent going directly to complete.

This is my testing branch, btw:

https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5

> If they are, can't the subsystem/driver code enable
> runtime PM for each of them when they are registered, by adding a
> single call in the right spot?
>
> If they don't all belong to the same subsystem/driver, who is going to
> call your pm_runtime_enable_recursive routine?  No single caller will
> have the right to enable runtime PM for all these devices.

Yeah, I was thinking that uvcvideo might be able to decide that, but
I'm not really sure about it.

Thanks,

Tomeu

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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Wed, 20 May 2015, Tomeu Vizoso wrote:

> On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 19 May 2015, Tomeu Vizoso wrote:
> >
> >> This function makes less cumbersome to enable runtime PM in a device and
> >> its descendants.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >
> > I don't see the point of this.  In the scenario you have in mind, are
> > the device and all its descendants registered by the same
> > subsystem/driver?
> 
> Not quite, the scenario here is a driver (uvcvideo) that deals with a
> specific piece of hardware and knows that all the descendants of the
> device it's bound to are virtual.
> 
> The subtree is:
> 
> 1-1:1.0 (bound to uvcvideo)
>         ep_87
>         input4
>                 event4
>         media0
>         video0

Just because these sub-devices are virtual, it doesn't mean you can 
ignore the way they interact with runtime PM.

In the case of ep_87 this doesn't matter.  Endpoint devices (like all 
devices) are in the SUSPENDED state by default when they are created, 
and they never leave that state.

Does the uvcvideo interface go into runtime suspend at the right times? 
If it does then you shouldn't have anything more to worry about.

> I liked how the force_direct_complete flag played out here, but I
> agree with Rafael that it can be abused as the PM domain or the bus
> type weren't able to prevent going directly to complete.
> 
> This is my testing branch, btw:
> 
> https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5
> 
> > If they are, can't the subsystem/driver code enable
> > runtime PM for each of them when they are registered, by adding a
> > single call in the right spot?
> >
> > If they don't all belong to the same subsystem/driver, who is going to
> > call your pm_runtime_enable_recursive routine?  No single caller will
> > have the right to enable runtime PM for all these devices.
> 
> Yeah, I was thinking that uvcvideo might be able to decide that, but
> I'm not really sure about it.

A possible way around the problem is to use pm_suspend_ignore_children
on the uvcvideo interface.  But I'm not sure that would be the right 
thing to do.

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

On 20 May 2015 at 16:24, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 20 May 2015, Tomeu Vizoso wrote:
>
>> On 19 May 2015 at 19:49, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Tue, 19 May 2015, Tomeu Vizoso wrote:
>> >
>> >> This function makes less cumbersome to enable runtime PM in a device and
>> >> its descendants.
>> >>
>> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >
>> > I don't see the point of this.  In the scenario you have in mind, are
>> > the device and all its descendants registered by the same
>> > subsystem/driver?
>>
>> Not quite, the scenario here is a driver (uvcvideo) that deals with a
>> specific piece of hardware and knows that all the descendants of the
>> device it's bound to are virtual.
>>
>> The subtree is:
>>
>> 1-1:1.0 (bound to uvcvideo)
>>         ep_87
>>         input4
>>                 event4
>>         media0
>>         video0
>
> Just because these sub-devices are virtual, it doesn't mean you can
> ignore the way they interact with runtime PM.

Fair enough, but then, how are we expected to be able to use the
direct_complete facility if the core bails out if a descendant doesn't
have runtime PM enabled?

> In the case of ep_87 this doesn't matter.  Endpoint devices (like all
> devices) are in the SUSPENDED state by default when they are created,
> and they never leave that state.

I don't see why it doesn't matter for endpoints or the others. They
don't have runtime PM enabled, so no ancestor will be able to do
direct_complete.

> Does the uvcvideo interface go into runtime suspend at the right times?
> If it does then you shouldn't have anything more to worry about.

It goes in and out of runtime suspend just fine, but when the system
goes to sleep, it's runtime resumed and then suspended. And a full
resume takes typically very long for USB cameras.

>> I liked how the force_direct_complete flag played out here, but I
>> agree with Rafael that it can be abused as the PM domain or the bus
>> type weren't able to prevent going directly to complete.
>>
>> This is my testing branch, btw:
>>
>> https://git.collabora.com/cgit/user/tomeu/linux.git/log/?h=fast-resume-v5
>>
>> > If they are, can't the subsystem/driver code enable
>> > runtime PM for each of them when they are registered, by adding a
>> > single call in the right spot?
>> >
>> > If they don't all belong to the same subsystem/driver, who is going to
>> > call your pm_runtime_enable_recursive routine?  No single caller will
>> > have the right to enable runtime PM for all these devices.
>>
>> Yeah, I was thinking that uvcvideo might be able to decide that, but
>> I'm not really sure about it.
>
> A possible way around the problem is to use pm_suspend_ignore_children
> on the uvcvideo interface.  But I'm not sure that would be the right
> thing to do.

Would that mean that if a device has ignore_children then it could
still do direct_complete even if its descendants weren't able to?

Thanks,

Tomeu

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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Thu, 2 Jul 2015, Tomeu Vizoso wrote:

> > Just because these sub-devices are virtual, it doesn't mean you can
> > ignore the way they interact with runtime PM.
> 
> Fair enough, but then, how are we expected to be able to use the
> direct_complete facility if the core bails out if a descendant doesn't
> have runtime PM enabled?
> 
> > In the case of ep_87 this doesn't matter.  Endpoint devices (like all
> > devices) are in the SUSPENDED state by default when they are created,
> > and they never leave that state.
> 
> I don't see why it doesn't matter for endpoints or the others. They
> don't have runtime PM enabled, so no ancestor will be able to do
> direct_complete.

Ah, you're concerned about these lines near the start of
__device_suspend():

	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;
	}

Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
pm_runtime_status_suspended().  Then it won't matter whether the
descendant devices are enabled for runtime PM.

> > A possible way around the problem is to use pm_suspend_ignore_children
> > on the uvcvideo interface.  But I'm not sure that would be the right
> > thing to do.
> 
> Would that mean that if a device has ignore_children then it could
> still do direct_complete even if its descendants weren't able to?

I think we could justify that.  The ignore_children flag means we can 
communicate with the children even when the device is in runtime 
suspend, so there's no reason to force the device to leave runtime 
suspend during a system sleep.

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 2 Jul 2015, Tomeu Vizoso wrote:
>
>> > Just because these sub-devices are virtual, it doesn't mean you can
>> > ignore the way they interact with runtime PM.
>>
>> Fair enough, but then, how are we expected to be able to use the
>> direct_complete facility if the core bails out if a descendant doesn't
>> have runtime PM enabled?
>>
>> > In the case of ep_87 this doesn't matter.  Endpoint devices (like all
>> > devices) are in the SUSPENDED state by default when they are created,
>> > and they never leave that state.
>>
>> I don't see why it doesn't matter for endpoints or the others. They
>> don't have runtime PM enabled, so no ancestor will be able to do
>> direct_complete.
>
> Ah, you're concerned about these lines near the start of
> __device_suspend():
>
>         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;
>         }
>
> Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> pm_runtime_status_suspended().  Then it won't matter whether the
> descendant devices are enabled for runtime PM.

Yeah, that would remove the need for messing with the runtime PM
enable status of descendant devices, but I wonder why Rafael went that
way initially.

>> > A possible way around the problem is to use pm_suspend_ignore_children
>> > on the uvcvideo interface.  But I'm not sure that would be the right
>> > thing to do.
>>
>> Would that mean that if a device has ignore_children then it could
>> still do direct_complete even if its descendants weren't able to?
>
> I think we could justify that.  The ignore_children flag means we can
> communicate with the children even when the device is in runtime
> suspend, so there's no reason to force the device to leave runtime
> suspend during a system sleep.

IIUIC, what you are proposing is to use ignore_children in a way
similar to how force_direct_complete was used in this patch?

http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292

That should work as well, but Rafael raised some objections and thus I
went with the present direct_complete_default, which should work if we
can relax the check as discussed above.

Thanks,

Tomeu

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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Fri, 3 Jul 2015, Tomeu Vizoso wrote:

> On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 2 Jul 2015, Tomeu Vizoso wrote:
> >
> >> > Just because these sub-devices are virtual, it doesn't mean you can
> >> > ignore the way they interact with runtime PM.
> >>
> >> Fair enough, but then, how are we expected to be able to use the
> >> direct_complete facility if the core bails out if a descendant doesn't
> >> have runtime PM enabled?
> >>
> >> > In the case of ep_87 this doesn't matter.  Endpoint devices (like all
> >> > devices) are in the SUSPENDED state by default when they are created,
> >> > and they never leave that state.
> >>
> >> I don't see why it doesn't matter for endpoints or the others. They
> >> don't have runtime PM enabled, so no ancestor will be able to do
> >> direct_complete.
> >
> > Ah, you're concerned about these lines near the start of
> > __device_suspend():
> >
> >         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;
> >         }
> >
> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> > pm_runtime_status_suspended().  Then it won't matter whether the
> > descendant devices are enabled for runtime PM.
> 
> Yeah, that would remove the need for messing with the runtime PM
> enable status of descendant devices, but I wonder why Rafael went that
> way initially.

I forget the details.  Probably it was just to be safe.  We probably 
thought that if a device was disabled for runtime PM then its runtime 
PM status might not be accurate.  But if direct_complete is set then it 
may be reasonable to assume that the runtime PM status _is_ accurate.

> >> > A possible way around the problem is to use pm_suspend_ignore_children
> >> > on the uvcvideo interface.  But I'm not sure that would be the right
> >> > thing to do.
> >>
> >> Would that mean that if a device has ignore_children then it could
> >> still do direct_complete even if its descendants weren't able to?
> >
> > I think we could justify that.  The ignore_children flag means we can
> > communicate with the children even when the device is in runtime
> > suspend, so there's no reason to force the device to leave runtime
> > suspend during a system sleep.
> 
> IIUIC, what you are proposing is to use ignore_children in a way
> similar to how force_direct_complete was used in this patch?
> 
> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292

That message doesn't contain a patch.

> That should work as well, but Rafael raised some objections and thus I
> went with the present direct_complete_default, which should work if we
> can relax the check as discussed above.

Rafael and I briefly discussed ignore_children while the original 
direct_complete patch was being designed.  We didn't come to any 
definite conclusion and decided to forget about it for the time being.  
Maybe now would be a good time to reconsider it.

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

On 3 July 2015 at 16:16, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 3 Jul 2015, Tomeu Vizoso wrote:
>
>> On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Thu, 2 Jul 2015, Tomeu Vizoso wrote:
>> >
>> >> > Just because these sub-devices are virtual, it doesn't mean you can
>> >> > ignore the way they interact with runtime PM.
>> >>
>> >> Fair enough, but then, how are we expected to be able to use the
>> >> direct_complete facility if the core bails out if a descendant doesn't
>> >> have runtime PM enabled?
>> >>
>> >> > In the case of ep_87 this doesn't matter.  Endpoint devices (like all
>> >> > devices) are in the SUSPENDED state by default when they are created,
>> >> > and they never leave that state.
>> >>
>> >> I don't see why it doesn't matter for endpoints or the others. They
>> >> don't have runtime PM enabled, so no ancestor will be able to do
>> >> direct_complete.
>> >
>> > Ah, you're concerned about these lines near the start of
>> > __device_suspend():
>> >
>> >         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;
>> >         }
>> >
>> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
>> > pm_runtime_status_suspended().  Then it won't matter whether the
>> > descendant devices are enabled for runtime PM.
>>
>> Yeah, that would remove the need for messing with the runtime PM
>> enable status of descendant devices, but I wonder why Rafael went that
>> way initially.
>
> I forget the details.  Probably it was just to be safe.  We probably
> thought that if a device was disabled for runtime PM then its runtime
> PM status might not be accurate.  But if direct_complete is set then it
> may be reasonable to assume that the runtime PM status _is_ accurate.

Cool.

>> >> > A possible way around the problem is to use pm_suspend_ignore_children
>> >> > on the uvcvideo interface.  But I'm not sure that would be the right
>> >> > thing to do.
>> >>
>> >> Would that mean that if a device has ignore_children then it could
>> >> still do direct_complete even if its descendants weren't able to?
>> >
>> > I think we could justify that.  The ignore_children flag means we can
>> > communicate with the children even when the device is in runtime
>> > suspend, so there's no reason to force the device to leave runtime
>> > suspend during a system sleep.
>>
>> IIUIC, what you are proposing is to use ignore_children in a way
>> similar to how force_direct_complete was used in this patch?
>>
>> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292
>
> That message doesn't contain a patch.

The patch is at the top of the thread:

http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292

>> That should work as well, but Rafael raised some objections and thus I
>> went with the present direct_complete_default, which should work if we
>> can relax the check as discussed above.
>
> Rafael and I briefly discussed ignore_children while the original
> direct_complete patch was being designed.  We didn't come to any
> definite conclusion and decided to forget about it for the time being.
> Maybe now would be a good time to reconsider it.

I would prefer to have ignore_children ignore whether the children of
a device were able to do direct_complete, rather than having a
direct_complete_default flag (plus not requiring that all its
descendants have runtime PM enabled).

Thanks,

Tomeu

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

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Fri, 3 Jul 2015, Tomeu Vizoso wrote:

> >> Yeah, that would remove the need for messing with the runtime PM
> >> enable status of descendant devices, but I wonder why Rafael went that
> >> way initially.
> >
> > I forget the details.  Probably it was just to be safe.  We probably
> > thought that if a device was disabled for runtime PM then its runtime
> > PM status might not be accurate.  But if direct_complete is set then it
> > may be reasonable to assume that the runtime PM status _is_ accurate.
> 
> Cool.

> > Rafael and I briefly discussed ignore_children while the original
> > direct_complete patch was being designed.  We didn't come to any
> > definite conclusion and decided to forget about it for the time being.
> > Maybe now would be a good time to reconsider it.
> 
> I would prefer to have ignore_children ignore whether the children of
> a device were able to do direct_complete, rather than having a
> direct_complete_default flag (plus not requiring that all its
> descendants have runtime PM enabled).

Okay, but remember that sometimes these "virtual" devices will exist 
beneath a device that needs to have ignore_children off.  So this won't 
be a complete solution to your problem.

Let's see what Rafael thinks about these two issues.  It seems to me
that the hardest part is dealing with drivers/subsystems that have no
runtime PM support.  In such cases, we have to be very careful not to
use direct_complete unless we know that the device does no power 
management at all.

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Friday, July 03, 2015 04:22:02 PM Tomeu Vizoso wrote:
> On 3 July 2015 at 16:16, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Fri, 3 Jul 2015, Tomeu Vizoso wrote:
> >
> >> On 2 July 2015 at 17:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Thu, 2 Jul 2015, Tomeu Vizoso wrote:
> >> >
> >> >> > Just because these sub-devices are virtual, it doesn't mean you can
> >> >> > ignore the way they interact with runtime PM.
> >> >>
> >> >> Fair enough, but then, how are we expected to be able to use the
> >> >> direct_complete facility if the core bails out if a descendant doesn't
> >> >> have runtime PM enabled?
> >> >>
> >> >> > In the case of ep_87 this doesn't matter.  Endpoint devices (like all
> >> >> > devices) are in the SUSPENDED state by default when they are created,
> >> >> > and they never leave that state.
> >> >>
> >> >> I don't see why it doesn't matter for endpoints or the others. They
> >> >> don't have runtime PM enabled, so no ancestor will be able to do
> >> >> direct_complete.
> >> >
> >> > Ah, you're concerned about these lines near the start of
> >> > __device_suspend():
> >> >
> >> >         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;
> >> >         }
> >> >
> >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> >> > pm_runtime_status_suspended().  Then it won't matter whether the
> >> > descendant devices are enabled for runtime PM.
> >>
> >> Yeah, that would remove the need for messing with the runtime PM
> >> enable status of descendant devices, but I wonder why Rafael went that
> >> way initially.
> >
> > I forget the details.  Probably it was just to be safe.  We probably
> > thought that if a device was disabled for runtime PM then its runtime
> > PM status might not be accurate.  But if direct_complete is set then it
> > may be reasonable to assume that the runtime PM status _is_ accurate.
> 
> Cool.

We're walking a grey area here.  What exactly does power.direct_complete mean
for devices whose runtime PM is disabled?

> >> >> > A possible way around the problem is to use pm_suspend_ignore_children
> >> >> > on the uvcvideo interface.  But I'm not sure that would be the right
> >> >> > thing to do.
> >> >>
> >> >> Would that mean that if a device has ignore_children then it could
> >> >> still do direct_complete even if its descendants weren't able to?
> >> >
> >> > I think we could justify that.  The ignore_children flag means we can
> >> > communicate with the children even when the device is in runtime
> >> > suspend, so there's no reason to force the device to leave runtime
> >> > suspend during a system sleep.
> >>
> >> IIUIC, what you are proposing is to use ignore_children in a way
> >> similar to how force_direct_complete was used in this patch?
> >>
> >> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292
> >
> > That message doesn't contain a patch.
> 
> The patch is at the top of the thread:
> 
> http://thread.gmane.org/gmane.linux.power-management.general/60198/focus=60292
> 
> >> That should work as well, but Rafael raised some objections and thus I
> >> went with the present direct_complete_default, which should work if we
> >> can relax the check as discussed above.
> >
> > Rafael and I briefly discussed ignore_children while the original
> > direct_complete patch was being designed.  We didn't come to any
> > definite conclusion and decided to forget about it for the time being.
> > Maybe now would be a good time to reconsider it.
> 
> I would prefer to have ignore_children ignore whether the children of
> a device were able to do direct_complete, rather than having a
> direct_complete_default flag (plus not requiring that all its
> descendants have runtime PM enabled).

Why?

Rafael

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Friday, July 03, 2015 11:11:19 AM Alan Stern wrote:
> On Fri, 3 Jul 2015, Tomeu Vizoso wrote:
> 
> > >> Yeah, that would remove the need for messing with the runtime PM
> > >> enable status of descendant devices, but I wonder why Rafael went that
> > >> way initially.
> > >
> > > I forget the details.  Probably it was just to be safe.  We probably
> > > thought that if a device was disabled for runtime PM then its runtime
> > > PM status might not be accurate.  But if direct_complete is set then it
> > > may be reasonable to assume that the runtime PM status _is_ accurate.
> > 
> > Cool.
> 
> > > Rafael and I briefly discussed ignore_children while the original
> > > direct_complete patch was being designed.  We didn't come to any
> > > definite conclusion and decided to forget about it for the time being.
> > > Maybe now would be a good time to reconsider it.
> > 
> > I would prefer to have ignore_children ignore whether the children of
> > a device were able to do direct_complete, rather than having a
> > direct_complete_default flag (plus not requiring that all its
> > descendants have runtime PM enabled).
> 
> Okay, but remember that sometimes these "virtual" devices will exist 
> beneath a device that needs to have ignore_children off.  So this won't 
> be a complete solution to your problem.
> 
> Let's see what Rafael thinks about these two issues.  It seems to me
> that the hardest part is dealing with drivers/subsystems that have no
> runtime PM support.  In such cases, we have to be very careful not to
> use direct_complete unless we know that the device does no power 
> management at all.

Precisely.

Thanks,
Rafael

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Sat, 4 Jul 2015, Rafael J. Wysocki wrote:

> > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> > >> > pm_runtime_status_suspended().  Then it won't matter whether the
> > >> > descendant devices are enabled for runtime PM.
> > >>
> > >> Yeah, that would remove the need for messing with the runtime PM
> > >> enable status of descendant devices, but I wonder why Rafael went that
> > >> way initially.
> > >
> > > I forget the details.  Probably it was just to be safe.  We probably
> > > thought that if a device was disabled for runtime PM then its runtime
> > > PM status might not be accurate.  But if direct_complete is set then it
> > > may be reasonable to assume that the runtime PM status _is_ accurate.
> > 
> > Cool.
> 
> We're walking a grey area here.  What exactly does power.direct_complete mean
> for devices whose runtime PM is disabled?

> > Let's see what Rafael thinks about these two issues.  It seems to me
> > that the hardest part is dealing with drivers/subsystems that have no
> > runtime PM support.  In such cases, we have to be very careful not to
> > use direct_complete unless we know that the device does no power
> > management at all.
> 
> Precisely.

All right, we can make a decision and document it.  The following seems
reasonable to me:

	If dev->power.direct_complete is set then the PM core will
	assume that dev->power.rpm_status is accurate even when
	dev->power.disable_depth > 0.  The core will obey the
	.direct_complete setting regardless of .disable_depth.

	As a consequence, devices that support system sleep but don't 
	support runtime PM must _never_ have .direct_complete set.

	On the other hand, if a device (such as a "virtual" device)
	requires no callbacks for either system sleep or runtime PM, 
	then there is no harm in setting .direct_complete.  Indeed,
	doing so may help speed up an ancestor device's sleep
	transition.

How does that sound?

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Saturday, July 04, 2015 10:37:55 AM Alan Stern wrote:
> On Sat, 4 Jul 2015, Rafael J. Wysocki wrote:
> 
> > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> > > >> > pm_runtime_status_suspended().  Then it won't matter whether the
> > > >> > descendant devices are enabled for runtime PM.
> > > >>
> > > >> Yeah, that would remove the need for messing with the runtime PM
> > > >> enable status of descendant devices, but I wonder why Rafael went that
> > > >> way initially.
> > > >
> > > > I forget the details.  Probably it was just to be safe.  We probably
> > > > thought that if a device was disabled for runtime PM then its runtime
> > > > PM status might not be accurate.  But if direct_complete is set then it
> > > > may be reasonable to assume that the runtime PM status _is_ accurate.
> > > 
> > > Cool.
> > 
> > We're walking a grey area here.  What exactly does power.direct_complete mean
> > for devices whose runtime PM is disabled?
> 
> > > Let's see what Rafael thinks about these two issues.  It seems to me
> > > that the hardest part is dealing with drivers/subsystems that have no
> > > runtime PM support.  In such cases, we have to be very careful not to
> > > use direct_complete unless we know that the device does no power
> > > management at all.
> > 
> > Precisely.
> 
> All right, we can make a decision and document it.  The following seems
> reasonable to me:
> 
> 	If dev->power.direct_complete is set then the PM core will
> 	assume that dev->power.rpm_status is accurate even when
> 	dev->power.disable_depth > 0.  The core will obey the
> 	.direct_complete setting regardless of .disable_depth.
> 
> 	As a consequence, devices that support system sleep but don't 
> 	support runtime PM must _never_ have .direct_complete set.
> 
> 	On the other hand, if a device (such as a "virtual" device)
> 	requires no callbacks for either system sleep or runtime PM, 
> 	then there is no harm in setting .direct_complete.  Indeed,
> 	doing so may help speed up an ancestor device's sleep
> 	transition.
> 
> How does that sound?

It would be workable I think, but I'd prefer the core to be told directly
about devices whose runtime PM status doesn't matter (because nothing changes
between "suspended" and "active"), so they may be treated in a special way
safely.

If we had that information, no special rules other than "that is a device
whose runtime PM status doesn't matter, so treat it accordingly" would be
necessary.

Thanks,
Rafael

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Monday, July 06, 2015 01:36:46 AM Rafael J. Wysocki wrote:
> On Saturday, July 04, 2015 10:37:55 AM Alan Stern wrote:
> > On Sat, 4 Jul 2015, Rafael J. Wysocki wrote:
> > 
> > > > >> > Perhaps the pm_runtime_suspended_if_enabled() test should be changed to
> > > > >> > pm_runtime_status_suspended().  Then it won't matter whether the
> > > > >> > descendant devices are enabled for runtime PM.
> > > > >>
> > > > >> Yeah, that would remove the need for messing with the runtime PM
> > > > >> enable status of descendant devices, but I wonder why Rafael went that
> > > > >> way initially.
> > > > >
> > > > > I forget the details.  Probably it was just to be safe.  We probably
> > > > > thought that if a device was disabled for runtime PM then its runtime
> > > > > PM status might not be accurate.  But if direct_complete is set then it
> > > > > may be reasonable to assume that the runtime PM status _is_ accurate.
> > > > 
> > > > Cool.
> > > 
> > > We're walking a grey area here.  What exactly does power.direct_complete mean
> > > for devices whose runtime PM is disabled?
> > 
> > > > Let's see what Rafael thinks about these two issues.  It seems to me
> > > > that the hardest part is dealing with drivers/subsystems that have no
> > > > runtime PM support.  In such cases, we have to be very careful not to
> > > > use direct_complete unless we know that the device does no power
> > > > management at all.
> > > 
> > > Precisely.
> > 
> > All right, we can make a decision and document it.  The following seems
> > reasonable to me:
> > 
> > 	If dev->power.direct_complete is set then the PM core will
> > 	assume that dev->power.rpm_status is accurate even when
> > 	dev->power.disable_depth > 0.  The core will obey the
> > 	.direct_complete setting regardless of .disable_depth.
> > 
> > 	As a consequence, devices that support system sleep but don't 
> > 	support runtime PM must _never_ have .direct_complete set.
> > 
> > 	On the other hand, if a device (such as a "virtual" device)
> > 	requires no callbacks for either system sleep or runtime PM, 
> > 	then there is no harm in setting .direct_complete.  Indeed,
> > 	doing so may help speed up an ancestor device's sleep
> > 	transition.
> > 
> > How does that sound?
> 
> It would be workable I think, but I'd prefer the core to be told directly
> about devices whose runtime PM status doesn't matter (because nothing changes
> between "suspended" and "active"), so they may be treated in a special way
> safely.
> 
> If we had that information, no special rules other than "that is a device
> whose runtime PM status doesn't matter, so treat it accordingly" would be
> necessary.

That said, a situation to consider is when device X is just a software device,
but it has children that correspond to physical hardware.  If that is the case,
the usual parent-children rules should apply to X and its children (ie. X should
only be marked as "suspended" if all of its children are suspended) and I see
no reason why the parent-children rules for direct_resume should not apply here.

Thanks,
Rafael

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Tue, 7 Jul 2015, Rafael J. Wysocki wrote:

> > > All right, we can make a decision and document it.  The following seems
> > > reasonable to me:
> > > 
> > > 	If dev->power.direct_complete is set then the PM core will
> > > 	assume that dev->power.rpm_status is accurate even when
> > > 	dev->power.disable_depth > 0.  The core will obey the
> > > 	.direct_complete setting regardless of .disable_depth.
> > > 
> > > 	As a consequence, devices that support system sleep but don't 
> > > 	support runtime PM must _never_ have .direct_complete set.
> > > 
> > > 	On the other hand, if a device (such as a "virtual" device)
> > > 	requires no callbacks for either system sleep or runtime PM, 
> > > 	then there is no harm in setting .direct_complete.  Indeed,
> > > 	doing so may help speed up an ancestor device's sleep
> > > 	transition.
> > > 
> > > How does that sound?
> > 
> > It would be workable I think, but I'd prefer the core to be told directly
> > about devices whose runtime PM status doesn't matter (because nothing changes
> > between "suspended" and "active"), so they may be treated in a special way
> > safely.
> > 
> > If we had that information, no special rules other than "that is a device
> > whose runtime PM status doesn't matter, so treat it accordingly" would be
> > necessary.
> 
> That said, a situation to consider is when device X is just a software device,
> but it has children that correspond to physical hardware.  If that is the case,
> the usual parent-children rules should apply to X and its children (ie. X should
> only be marked as "suspended" if all of its children are suspended) and I see
> no reason why the parent-children rules for direct_resume should not apply here.

Yes, this illustrates that in some ways we must not treat "virtual" or 
"software" devices specially.  Being "virtual" is not the same as 
having the ignore_children flag set.

The change I'm proposing is not related to whether a device is 
"virtual".  I'm just suggesting that the normal direct_complete rules 
should apply even when devices are runtime-PM-disabled.

This doesn't mean that their runtime PM status doesn't matter.  Just
the opposite, in fact -- it means that the PM core should pay attention
to the runtime PM status during a sleep transition even though
disabled_depth > 0.

Alan Stern

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Rafael J. Wysocki]

On Tuesday, July 07, 2015 10:55:59 AM Alan Stern wrote:
> On Tue, 7 Jul 2015, Rafael J. Wysocki wrote:
> 
> > > > All right, we can make a decision and document it.  The following seems
> > > > reasonable to me:
> > > > 
> > > > 	If dev->power.direct_complete is set then the PM core will
> > > > 	assume that dev->power.rpm_status is accurate even when
> > > > 	dev->power.disable_depth > 0.  The core will obey the
> > > > 	.direct_complete setting regardless of .disable_depth.
> > > > 
> > > > 	As a consequence, devices that support system sleep but don't 
> > > > 	support runtime PM must _never_ have .direct_complete set.
> > > > 
> > > > 	On the other hand, if a device (such as a "virtual" device)
> > > > 	requires no callbacks for either system sleep or runtime PM, 
> > > > 	then there is no harm in setting .direct_complete.  Indeed,
> > > > 	doing so may help speed up an ancestor device's sleep
> > > > 	transition.
> > > > 
> > > > How does that sound?
> > > 
> > > It would be workable I think, but I'd prefer the core to be told directly
> > > about devices whose runtime PM status doesn't matter (because nothing changes
> > > between "suspended" and "active"), so they may be treated in a special way
> > > safely.
> > > 
> > > If we had that information, no special rules other than "that is a device
> > > whose runtime PM status doesn't matter, so treat it accordingly" would be
> > > necessary.
> > 
> > That said, a situation to consider is when device X is just a software device,
> > but it has children that correspond to physical hardware.  If that is the case,
> > the usual parent-children rules should apply to X and its children (ie. X should
> > only be marked as "suspended" if all of its children are suspended) and I see
> > no reason why the parent-children rules for direct_resume should not apply here.
> 
> Yes, this illustrates that in some ways we must not treat "virtual" or 
> "software" devices specially.  Being "virtual" is not the same as 
> having the ignore_children flag set.
> 
> The change I'm proposing is not related to whether a device is 
> "virtual".  I'm just suggesting that the normal direct_complete rules 
> should apply even when devices are runtime-PM-disabled.
> 
> This doesn't mean that their runtime PM status doesn't matter.  Just
> the opposite, in fact -- it means that the PM core should pay attention
> to the runtime PM status during a sleep transition even though
> disabled_depth > 0.

I seem to have lost the context here, sorry about that.

The idea seems to be to rely on the fact that the RPM status for all devices
is initially RPM_SUSPENDED and that never changes if runtime PM is never
enabled for the device, so in that particular case it would be OK to treat
the "power.direct_complete set + RPM status == RPM_SUSPENDED" combination
as valid even though runtime PM has never been enabled for the device in
question (provided that power.direct_complete will never be set for "real"
devices that don't support runtime PM).  Is that correct?

That seems to be fragile, but I have no strong opinion.

Let's do that change if it allows us to make forward progress here.  Please
feel free to submit a documentation patch along the lines you've suggested.

Thanks,
Rafael

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Wed, 8 Jul 2015, Rafael J. Wysocki wrote:

> I seem to have lost the context here, sorry about that.
> 
> The idea seems to be to rely on the fact that the RPM status for all devices
> is initially RPM_SUSPENDED and that never changes if runtime PM is never
> enabled for the device, so in that particular case it would be OK to treat
> the "power.direct_complete set + RPM status == RPM_SUSPENDED" combination
> as valid even though runtime PM has never been enabled for the device in
> question (provided that power.direct_complete will never be set for "real"
> devices that don't support runtime PM).  Is that correct?

I would have expressed it slightly differently, but yes, that's correct.

> That seems to be fragile, but I have no strong opinion.

In itself it's not all that bad, I think.  In the presence of Tomeu's
new direct_complete_default flag, however, it does seem quite fragile.  

We may want to do the direct_complete_default thing in a different way.  
For example, the PM core could automatically set the direct_complete
flag if a device has _none_ of the system suspend callbacks (i.e., no
prepare, suspend, suspend_late, suspend_noirq, resume_noirq,
resume_early, resume, or complete).  Although it would be a little
awkward to check this, it would be safer than inheriting
direct_complete_default from the parent and it ought to solve Tomeu's
problem just as well.

> Let's do that change if it allows us to make forward progress here.  Please
> feel free to submit a documentation patch along the lines you've suggested.

Here's a proposed patch to illustrate what I have in mind.  Since it 
removes the only usage of pm_runtime_suspended_if_enabled(), it also 
removes the definition of that function.

Alan Stern



Index: usb-4.1/drivers/base/power/main.c
===================================================================
--- usb-4.1.orig/drivers/base/power/main.c
+++ usb-4.1/drivers/base/power/main.c
@@ -1374,7 +1374,7 @@ static int __device_suspend(struct devic
 	if (dev->power.direct_complete) {
 		if (pm_runtime_status_suspended(dev)) {
 			pm_runtime_disable(dev);
-			if (pm_runtime_suspended_if_enabled(dev))
+			if (pm_runtime_status_suspended(dev))
 				goto Complete;
 
 			pm_runtime_enable(dev);
Index: usb-4.1/Documentation/power/devices.txt
===================================================================
--- usb-4.1.orig/Documentation/power/devices.txt
+++ usb-4.1/Documentation/power/devices.txt
@@ -341,6 +341,13 @@ the phases are:
 	and is entirely responsible for bringing the device back to the
 	functional state as appropriate.
 
+	Note that this direct-complete procedure applies even if the device is
+	disabled for runtime PM; only the runtime-PM status matters.  It follows
+	that if a device has system-sleep callbacks but does not support runtime
+	PM, then its prepare callback must never return a positive value.  This
+	is because all devices are initially set to runtime-suspended with
+	runtime PM disabled.
+
     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,
Index: usb-4.1/Documentation/power/runtime_pm.txt
===================================================================
--- usb-4.1.orig/Documentation/power/runtime_pm.txt
+++ usb-4.1/Documentation/power/runtime_pm.txt
@@ -445,10 +445,6 @@ 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
Index: usb-4.1/include/linux/pm_runtime.h
===================================================================
--- usb-4.1.orig/include/linux/pm_runtime.h
+++ usb-4.1/include/linux/pm_runtime.h
@@ -98,11 +98,6 @@ 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;
@@ -164,7 +159,6 @@ 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) {}

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Tomeu Vizoso]

On 8 July 2015 at 22:31, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 8 Jul 2015, Rafael J. Wysocki wrote:
>
>> I seem to have lost the context here, sorry about that.
>>
>> The idea seems to be to rely on the fact that the RPM status for all devices
>> is initially RPM_SUSPENDED and that never changes if runtime PM is never
>> enabled for the device, so in that particular case it would be OK to treat
>> the "power.direct_complete set + RPM status == RPM_SUSPENDED" combination
>> as valid even though runtime PM has never been enabled for the device in
>> question (provided that power.direct_complete will never be set for "real"
>> devices that don't support runtime PM).  Is that correct?
>
> I would have expressed it slightly differently, but yes, that's correct.
>
>> That seems to be fragile, but I have no strong opinion.
>
> In itself it's not all that bad, I think.  In the presence of Tomeu's
> new direct_complete_default flag, however, it does seem quite fragile.
>
> We may want to do the direct_complete_default thing in a different way.
> For example, the PM core could automatically set the direct_complete
> flag if a device has _none_ of the system suspend callbacks (i.e., no
> prepare, suspend, suspend_late, suspend_noirq, resume_noirq,
> resume_early, resume, or complete).  Although it would be a little
> awkward to check this, it would be safer than inheriting
> direct_complete_default from the parent and it ought to solve Tomeu's
> problem just as well.

Yeah, I think this is an improvement. Will give it a try.

>> Let's do that change if it allows us to make forward progress here.  Please
>> feel free to submit a documentation patch along the lines you've suggested.
>
> Here's a proposed patch to illustrate what I have in mind.  Since it
> removes the only usage of pm_runtime_suspended_if_enabled(), it also
> removes the definition of that function.

Will this patch be picked up as-is or should I add it to my series
with a proper changelog?

Thanks,

Tomeu


> Alan Stern
>
>
>
> Index: usb-4.1/drivers/base/power/main.c
> ===================================================================
> --- usb-4.1.orig/drivers/base/power/main.c
> +++ usb-4.1/drivers/base/power/main.c
> @@ -1374,7 +1374,7 @@ static int __device_suspend(struct devic
>         if (dev->power.direct_complete) {
>                 if (pm_runtime_status_suspended(dev)) {
>                         pm_runtime_disable(dev);
> -                       if (pm_runtime_suspended_if_enabled(dev))
> +                       if (pm_runtime_status_suspended(dev))
>                                 goto Complete;
>
>                         pm_runtime_enable(dev);
> Index: usb-4.1/Documentation/power/devices.txt
> ===================================================================
> --- usb-4.1.orig/Documentation/power/devices.txt
> +++ usb-4.1/Documentation/power/devices.txt
> @@ -341,6 +341,13 @@ the phases are:
>         and is entirely responsible for bringing the device back to the
>         functional state as appropriate.
>
> +       Note that this direct-complete procedure applies even if the device is
> +       disabled for runtime PM; only the runtime-PM status matters.  It follows
> +       that if a device has system-sleep callbacks but does not support runtime
> +       PM, then its prepare callback must never return a positive value.  This
> +       is because all devices are initially set to runtime-suspended with
> +       runtime PM disabled.
> +
>      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,
> Index: usb-4.1/Documentation/power/runtime_pm.txt
> ===================================================================
> --- usb-4.1.orig/Documentation/power/runtime_pm.txt
> +++ usb-4.1/Documentation/power/runtime_pm.txt
> @@ -445,10 +445,6 @@ 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
> Index: usb-4.1/include/linux/pm_runtime.h
> ===================================================================
> --- usb-4.1.orig/include/linux/pm_runtime.h
> +++ usb-4.1/include/linux/pm_runtime.h
> @@ -98,11 +98,6 @@ 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;
> @@ -164,7 +159,6 @@ 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) {}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] PM / Runtime: Add pm_runtime_enable_recursive

[Alan Stern]

On Tue, 14 Jul 2015, Tomeu Vizoso wrote:

> > We may want to do the direct_complete_default thing in a different way.
> > For example, the PM core could automatically set the direct_complete
> > flag if a device has _none_ of the system suspend callbacks (i.e., no
> > prepare, suspend, suspend_late, suspend_noirq, resume_noirq,
> > resume_early, resume, or complete).  Although it would be a little
> > awkward to check this, it would be safer than inheriting
> > direct_complete_default from the parent and it ought to solve Tomeu's
> > problem just as well.
> 
> Yeah, I think this is an improvement. Will give it a try.

Sounds good.

> > Here's a proposed patch to illustrate what I have in mind.  Since it
> > removes the only usage of pm_runtime_suspended_if_enabled(), it also
> > removes the definition of that function.
> 
> Will this patch be picked up as-is or should I add it to my series
> with a proper changelog?

You can add it to your series with my S-O-B:

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

Alan Stern