[PATCH v3 0/2] Allow UVC devices to remain runtime-suspended when sleeping

[PATCH v3 0/2] Allow UVC devices to remain runtime-suspended when sleeping

[Tomeu Vizoso]

v3:	* Add a new power.force_direct_complete to let devices express that it's
	safe to let them be runtime-suspended at system sleep regardless of the state
	of their descendants

v2:	* Let creators of the input device to decide whether it should remain
	runtime suspended when the system goes into a sleep state
	* Don't enable PM runtime on all evdev handlers
	* Cope with another wrong wakeup setting in usb_dev_prepare

Hi,

this series contain what I needed to do in order to have my USB webcam to not
be resumed when the system resumes, reducing considerably the total time that
resuming takes.

It makes use of the facility that Rafael Wysocki added in aae4518b3 ("PM /
sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily").

Thanks,

Tomeu

Tomeu Vizoso (2):
  PM / sleep: Let devices force direct_complete
  [media] uvcvideo: Remain runtime-suspended at sleeps

 drivers/base/power/main.c          | 13 +++++++++----
 drivers/media/usb/uvc/uvc_driver.c |  2 ++
 include/linux/pm.h                 |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.3.5

[PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Tomeu Vizoso]

Introduce a new per-device flag power.force_direct_complete that will
instruct the PM core to ignore the runtime PM status of its descendants
when deciding whether to let this device remain in runtime suspend when
the system goes into a sleep power state.

This is needed because otherwise it would be needed to get dozens of
drivers to implement the prepare() callback and be runtime PM active
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>
---
 drivers/base/power/main.c | 13 +++++++++----
 include/linux/pm.h        |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 3d874ec..728c2dc 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1438,7 +1438,9 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 		if (parent) {
 			spin_lock_irq(&parent->power.lock);
 
-			dev->parent->power.direct_complete = false;
+			if (!dev->parent->power.force_direct_complete)
+				dev->parent->power.direct_complete = false;
+
 			if (dev->power.wakeup_path
 			    && !dev->parent->power.ignore_children)
 				dev->parent->power.wakeup_path = true;
@@ -1605,9 +1607,12 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	 * 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);
+	if (state.event == PM_EVENT_SUSPEND) {
+		spin_lock_irq(&dev->power.lock);
+		dev->power.direct_complete = ret > 0 ||
+			dev->power.force_direct_complete;
+		spin_unlock_irq(&dev->power.lock);
+	}
 	return 0;
 }
 
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 2d29c64..2e41cfd 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -553,6 +553,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			force_direct_complete:1;
 	spinlock_t		lock;
 #ifdef CONFIG_PM_SLEEP
 	struct list_head	entry;
-- 
2.3.5

[PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps

[Tomeu Vizoso]

When the system goes to sleep and afterwards resumes, a significant
amount of time is spent suspending and resuming devices that were
already runtime-suspended.

By setting the power.force_direct_complete flag, the PM core will ignore
the state of descendant devices and the device will be let in
runtime-suspend.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 5970dd6..ae75a70 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf,
 			"supported.\n", ret);
 	}
 
+	intf->dev.parent->power.force_direct_complete = true;
+
 	uvc_trace(UVC_TRACE_PROBE, "UVC device initialized.\n");
 	usb_enable_autosuspend(udev);
 	return 0;
-- 
2.3.5

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Greg Kroah-Hartman]

On Fri, Apr 17, 2015 at 05:24:49PM +0200, Tomeu Vizoso wrote:
> Introduce a new per-device flag power.force_direct_complete that will
> instruct the PM core to ignore the runtime PM status of its descendants
> when deciding whether to let this device remain in runtime suspend when
> the system goes into a sleep power state.
> 
> This is needed because otherwise it would be needed to get dozens of
> drivers to implement the prepare() callback and be runtime PM active
> 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>
> ---
>  drivers/base/power/main.c | 13 +++++++++----
>  include/linux/pm.h        |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3d874ec..728c2dc 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1438,7 +1438,9 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>  		if (parent) {
>  			spin_lock_irq(&parent->power.lock);
>  
> -			dev->parent->power.direct_complete = false;
> +			if (!dev->parent->power.force_direct_complete)
> +				dev->parent->power.direct_complete = false;
> +
>  			if (dev->power.wakeup_path
>  			    && !dev->parent->power.ignore_children)
>  				dev->parent->power.wakeup_path = true;
> @@ -1605,9 +1607,12 @@ static int device_prepare(struct device *dev, pm_message_t state)
>  	 * 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);
> +	if (state.event == PM_EVENT_SUSPEND) {
> +		spin_lock_irq(&dev->power.lock);
> +		dev->power.direct_complete = ret > 0 ||
> +			dev->power.force_direct_complete;
> +		spin_unlock_irq(&dev->power.lock);
> +	}
>  	return 0;
>  }
>  
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 2d29c64..2e41cfd 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -553,6 +553,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			force_direct_complete:1;

Where have you documented this?  I foresee this just getting messier and
messier...

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Laurent Pinchart]

Hi Tomeu,

Thank you for the patch.

On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
> Introduce a new per-device flag power.force_direct_complete that will
> instruct the PM core to ignore the runtime PM status of its descendants
> when deciding whether to let this device remain in runtime suspend when
> the system goes into a sleep power state.
> 
> This is needed because otherwise it would be needed to get dozens of
> drivers to implement the prepare() callback and be runtime PM active
> even if they don't have a 1-to-1 relationship with a piece of HW.

I'll let PM experts comment on the approach, but I believe the new flag would 
benefit from being documented (likely in Documentation/power/devices.txt) :-)

> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  drivers/base/power/main.c | 13 +++++++++----
>  include/linux/pm.h        |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 3d874ec..728c2dc 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1438,7 +1438,9 @@ static int __device_suspend(struct device *dev,
> pm_message_t state, bool async) if (parent) {
>  			spin_lock_irq(&parent->power.lock);
> 
> -			dev->parent->power.direct_complete = false;
> +			if (!dev->parent->power.force_direct_complete)
> +				dev->parent->power.direct_complete = false;
> +
>  			if (dev->power.wakeup_path
>  			    && !dev->parent->power.ignore_children)
>  				dev->parent->power.wakeup_path = true;
> @@ -1605,9 +1607,12 @@ static int device_prepare(struct device *dev,
> pm_message_t state) * 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);
> +	if (state.event == PM_EVENT_SUSPEND) {
> +		spin_lock_irq(&dev->power.lock);
> +		dev->power.direct_complete = ret > 0 ||
> +			dev->power.force_direct_complete;
> +		spin_unlock_irq(&dev->power.lock);
> +	}
>  	return 0;
>  }
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 2d29c64..2e41cfd 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -553,6 +553,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			force_direct_complete:1;
>  	spinlock_t		lock;
>  #ifdef CONFIG_PM_SLEEP
>  	struct list_head	entry;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Alan Stern]

On Fri, 17 Apr 2015, Laurent Pinchart wrote:

> Hi Tomeu,
> 
> Thank you for the patch.
> 
> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
> > Introduce a new per-device flag power.force_direct_complete that will
> > instruct the PM core to ignore the runtime PM status of its descendants
> > when deciding whether to let this device remain in runtime suspend when
> > the system goes into a sleep power state.
> > 
> > This is needed because otherwise it would be needed to get dozens of
> > drivers to implement the prepare() callback and be runtime PM active
> > even if they don't have a 1-to-1 relationship with a piece of HW.
> 
> I'll let PM experts comment on the approach, but I believe the new flag would 
> benefit from being documented (likely in Documentation/power/devices.txt) :-)

Documentation/power/runtime_pm.txt is the right place.

However, I'm not sure that this is the sort of thing Rafael meant when 
he suggested adding a new flag.  I thought he meant the PM core would 
look at the new flag only if there was no ->prepare method at all.  
Then if the new flag was set, the PM core would act as though ->prepare 
had returned 1.  That way there would be no need to add silly little
one-line *_prepare() routines all over the place.

Maybe he had something else in mind, though...

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Alan Stern]

On Fri, 17 Apr 2015, Laurent Pinchart wrote:

> Hi Tomeu,
> 
> Thank you for the patch.
> 
> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
> > Introduce a new per-device flag power.force_direct_complete that will
> > instruct the PM core to ignore the runtime PM status of its descendants
> > when deciding whether to let this device remain in runtime suspend when
> > the system goes into a sleep power state.
> > 
> > This is needed because otherwise it would be needed to get dozens of
> > drivers to implement the prepare() callback and be runtime PM active
> > even if they don't have a 1-to-1 relationship with a piece of HW.
> 
> I'll let PM experts comment on the approach, but I believe the new flag would 
> benefit from being documented (likely in Documentation/power/devices.txt) :-)

Documentation/power/runtime_pm.txt is the right place.

However, I'm not sure that this is the sort of thing Rafael meant when 
he suggested adding a new flag.  I thought he meant the PM core would 
look at the new flag only if there was no ->prepare method at all.  
Then if the new flag was set, the PM core would act as though ->prepare 
had returned 1.  That way there would be no need to add silly little
one-line *_prepare() routines all over the place.

Maybe he had something else in mind, though...

Alan Stern

Re: [PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps

[Alan Stern]

On Fri, 17 Apr 2015, Tomeu Vizoso wrote:

> When the system goes to sleep and afterwards resumes, a significant
> amount of time is spent suspending and resuming devices that were
> already runtime-suspended.
> 
> By setting the power.force_direct_complete flag, the PM core will ignore
> the state of descendant devices and the device will be let in
> runtime-suspend.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 5970dd6..ae75a70 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf,
>  			"supported.\n", ret);
>  	}
>  
> +	intf->dev.parent->power.force_direct_complete = true;

This seems wrong.  The uvc driver is bound to intf, not to intf's
parent.  So it would be okay for the driver to set
intf->dev.power.force_direct_complete, but it's wrong to set
intf->dev.parent->power.force_direct_complete.

Alan Stern

Re: [PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps

[Alan Stern]

On Fri, 17 Apr 2015, Tomeu Vizoso wrote:

> When the system goes to sleep and afterwards resumes, a significant
> amount of time is spent suspending and resuming devices that were
> already runtime-suspended.
> 
> By setting the power.force_direct_complete flag, the PM core will ignore
> the state of descendant devices and the device will be let in
> runtime-suspend.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 5970dd6..ae75a70 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf,
>  			"supported.\n", ret);
>  	}
>  
> +	intf->dev.parent->power.force_direct_complete = true;

This seems wrong.  The uvc driver is bound to intf, not to intf's
parent.  So it would be okay for the driver to set
intf->dev.power.force_direct_complete, but it's wrong to set
intf->dev.parent->power.force_direct_complete.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Tomeu Vizoso]

On 17 April 2015 at 19:30, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 17 Apr 2015, Laurent Pinchart wrote:
>
>> Hi Tomeu,
>>
>> Thank you for the patch.
>>
>> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
>> > Introduce a new per-device flag power.force_direct_complete that will
>> > instruct the PM core to ignore the runtime PM status of its descendants
>> > when deciding whether to let this device remain in runtime suspend when
>> > the system goes into a sleep power state.
>> >
>> > This is needed because otherwise it would be needed to get dozens of
>> > drivers to implement the prepare() callback and be runtime PM active
>> > even if they don't have a 1-to-1 relationship with a piece of HW.
>>
>> I'll let PM experts comment on the approach, but I believe the new flag would
>> benefit from being documented (likely in Documentation/power/devices.txt) :-)
>
> Documentation/power/runtime_pm.txt is the right place.
>
> However, I'm not sure that this is the sort of thing Rafael meant when
> he suggested adding a new flag.  I thought he meant the PM core would
> look at the new flag only if there was no ->prepare method at all.
> Then if the new flag was set, the PM core would act as though ->prepare
> had returned 1.  That way there would be no need to add silly little
> one-line *_prepare() routines all over the place.
>
> Maybe he had something else in mind, though...

Yeah, I also interpreted it like that, but when I started looking at
how it would work, I found that it would be awkward if the uvcvideo
driver had to track all the devices that get attached below its
devices in order to set that flag to them.

When thinking about it, it occurred to me that it may make more sense
if we model this as a property of the device bound to the uvcvideo
driver, as what's happening here is that the uvcvideo driver knows
that it's safe to remain in runtime suspend when the system goes to
sleep, and that all its descendant devices can be ignored in that
regard.

Was meaning to explain this in the cover letter, but I forgot to, sorry.

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] [media] uvcvideo: Remain runtime-suspended at sleeps

[Tomeu Vizoso]

On 17 April 2015 at 19:32, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 17 Apr 2015, Tomeu Vizoso wrote:
>
>> When the system goes to sleep and afterwards resumes, a significant
>> amount of time is spent suspending and resuming devices that were
>> already runtime-suspended.
>>
>> By setting the power.force_direct_complete flag, the PM core will ignore
>> the state of descendant devices and the device will be let in
>> runtime-suspend.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> ---
>>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
>> index 5970dd6..ae75a70 100644
>> --- a/drivers/media/usb/uvc/uvc_driver.c
>> +++ b/drivers/media/usb/uvc/uvc_driver.c
>> @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf,
>>                       "supported.\n", ret);
>>       }
>>
>> +     intf->dev.parent->power.force_direct_complete = true;
>
> This seems wrong.  The uvc driver is bound to intf, not to intf's
> parent.  So it would be okay for the driver to set
> intf->dev.power.force_direct_complete, but it's wrong to set
> intf->dev.parent->power.force_direct_complete.

Agreed.

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 1/2] PM / sleep: Let devices force direct_complete

[Alan Stern]

On Mon, 20 Apr 2015, Tomeu Vizoso wrote:

> On 17 April 2015 at 19:30, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Fri, 17 Apr 2015, Laurent Pinchart wrote:
> >
> >> Hi Tomeu,
> >>
> >> Thank you for the patch.
> >>
> >> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
> >> > Introduce a new per-device flag power.force_direct_complete that will
> >> > instruct the PM core to ignore the runtime PM status of its descendants
> >> > when deciding whether to let this device remain in runtime suspend when
> >> > the system goes into a sleep power state.
> >> >
> >> > This is needed because otherwise it would be needed to get dozens of
> >> > drivers to implement the prepare() callback and be runtime PM active
> >> > even if they don't have a 1-to-1 relationship with a piece of HW.
> >>
> >> I'll let PM experts comment on the approach, but I believe the new flag would
> >> benefit from being documented (likely in Documentation/power/devices.txt) :-)
> >
> > Documentation/power/runtime_pm.txt is the right place.
> >
> > However, I'm not sure that this is the sort of thing Rafael meant when
> > he suggested adding a new flag.  I thought he meant the PM core would
> > look at the new flag only if there was no ->prepare method at all.
> > Then if the new flag was set, the PM core would act as though ->prepare
> > had returned 1.  That way there would be no need to add silly little
> > one-line *_prepare() routines all over the place.
> >
> > Maybe he had something else in mind, though...
> 
> Yeah, I also interpreted it like that, but when I started looking at
> how it would work, I found that it would be awkward if the uvcvideo
> driver had to track all the devices that get attached below its
> devices in order to set that flag to them.
> 
> When thinking about it, it occurred to me that it may make more sense
> if we model this as a property of the device bound to the uvcvideo
> driver, as what's happening here is that the uvcvideo driver knows
> that it's safe to remain in runtime suspend when the system goes to
> sleep, and that all its descendant devices can be ignored in that
> regard.

What you're proposing makes sense, but it is a significant change to 
the runtime PM core.  It should be submitted separately, not as part of 
an update to the UVC driver, and it should be discussed at length.

Basically, you want to mark certain devices to say that they will
_always_ use direct-suspend.  This means that all descendant devices
will be forced to use direct-suspend also, and therefore any driver
bound to one of these descendant devices will be unable to communicate
with it during a system sleep transition.  This is a non-trivial
restriction.

Among other things, it means that wakeup settings can't be altered 
during a sleep transition.  Therefore this should be allowed only for 
devices that are not wakeup-capable.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Tomeu Vizoso]

On 20 April 2015 at 16:12, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 20 Apr 2015, Tomeu Vizoso wrote:
>
>> On 17 April 2015 at 19:30, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Fri, 17 Apr 2015, Laurent Pinchart wrote:
>> >
>> >> Hi Tomeu,
>> >>
>> >> Thank you for the patch.
>> >>
>> >> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
>> >> > Introduce a new per-device flag power.force_direct_complete that will
>> >> > instruct the PM core to ignore the runtime PM status of its descendants
>> >> > when deciding whether to let this device remain in runtime suspend when
>> >> > the system goes into a sleep power state.
>> >> >
>> >> > This is needed because otherwise it would be needed to get dozens of
>> >> > drivers to implement the prepare() callback and be runtime PM active
>> >> > even if they don't have a 1-to-1 relationship with a piece of HW.
>> >>
>> >> I'll let PM experts comment on the approach, but I believe the new flag would
>> >> benefit from being documented (likely in Documentation/power/devices.txt) :-)
>> >
>> > Documentation/power/runtime_pm.txt is the right place.
>> >
>> > However, I'm not sure that this is the sort of thing Rafael meant when
>> > he suggested adding a new flag.  I thought he meant the PM core would
>> > look at the new flag only if there was no ->prepare method at all.
>> > Then if the new flag was set, the PM core would act as though ->prepare
>> > had returned 1.  That way there would be no need to add silly little
>> > one-line *_prepare() routines all over the place.
>> >
>> > Maybe he had something else in mind, though...
>>
>> Yeah, I also interpreted it like that, but when I started looking at
>> how it would work, I found that it would be awkward if the uvcvideo
>> driver had to track all the devices that get attached below its
>> devices in order to set that flag to them.
>>
>> When thinking about it, it occurred to me that it may make more sense
>> if we model this as a property of the device bound to the uvcvideo
>> driver, as what's happening here is that the uvcvideo driver knows
>> that it's safe to remain in runtime suspend when the system goes to
>> sleep, and that all its descendant devices can be ignored in that
>> regard.
>
> What you're proposing makes sense, but it is a significant change to
> the runtime PM core.  It should be submitted separately, not as part of
> an update to the UVC driver, and it should be discussed at length.
>
> Basically, you want to mark certain devices to say that they will
> _always_ use direct-suspend.  This means that all descendant devices
> will be forced to use direct-suspend also, and therefore any driver
> bound to one of these descendant devices will be unable to communicate
> with it during a system sleep transition.  This is a non-trivial
> restriction.
>
> Among other things, it means that wakeup settings can't be altered
> during a sleep transition.  Therefore this should be allowed only for
> devices that are not wakeup-capable.

Hi Rafael,

do you have any comments on this?

Thanks,

Tomeu

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 04:26:39 PM Tomeu Vizoso wrote:
> On 20 April 2015 at 16:12, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Mon, 20 Apr 2015, Tomeu Vizoso wrote:
> >
> >> On 17 April 2015 at 19:30, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Fri, 17 Apr 2015, Laurent Pinchart wrote:
> >> >
> >> >> Hi Tomeu,
> >> >>
> >> >> Thank you for the patch.
> >> >>
> >> >> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
> >> >> > Introduce a new per-device flag power.force_direct_complete that will
> >> >> > instruct the PM core to ignore the runtime PM status of its descendants
> >> >> > when deciding whether to let this device remain in runtime suspend when
> >> >> > the system goes into a sleep power state.
> >> >> >
> >> >> > This is needed because otherwise it would be needed to get dozens of
> >> >> > drivers to implement the prepare() callback and be runtime PM active
> >> >> > even if they don't have a 1-to-1 relationship with a piece of HW.
> >> >>
> >> >> I'll let PM experts comment on the approach, but I believe the new flag would
> >> >> benefit from being documented (likely in Documentation/power/devices.txt) :-)
> >> >
> >> > Documentation/power/runtime_pm.txt is the right place.
> >> >
> >> > However, I'm not sure that this is the sort of thing Rafael meant when
> >> > he suggested adding a new flag.  I thought he meant the PM core would
> >> > look at the new flag only if there was no ->prepare method at all.
> >> > Then if the new flag was set, the PM core would act as though ->prepare
> >> > had returned 1.  That way there would be no need to add silly little
> >> > one-line *_prepare() routines all over the place.
> >> >
> >> > Maybe he had something else in mind, though...
> >>
> >> Yeah, I also interpreted it like that, but when I started looking at
> >> how it would work, I found that it would be awkward if the uvcvideo
> >> driver had to track all the devices that get attached below its
> >> devices in order to set that flag to them.
> >>
> >> When thinking about it, it occurred to me that it may make more sense
> >> if we model this as a property of the device bound to the uvcvideo
> >> driver, as what's happening here is that the uvcvideo driver knows
> >> that it's safe to remain in runtime suspend when the system goes to
> >> sleep, and that all its descendant devices can be ignored in that
> >> regard.
> >
> > What you're proposing makes sense, but it is a significant change to
> > the runtime PM core.  It should be submitted separately, not as part of
> > an update to the UVC driver, and it should be discussed at length.
> >
> > Basically, you want to mark certain devices to say that they will
> > _always_ use direct-suspend.  This means that all descendant devices
> > will be forced to use direct-suspend also, and therefore any driver
> > bound to one of these descendant devices will be unable to communicate
> > with it during a system sleep transition.  This is a non-trivial
> > restriction.
> >
> > Among other things, it means that wakeup settings can't be altered
> > during a sleep transition.  Therefore this should be allowed only for
> > devices that are not wakeup-capable.
> 
> Hi Rafael,
> 
> do you have any comments on this?

Well, what Alan has said sounds correct to me.

Rafael

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Ulf Hansson]

On 20 April 2015 at 16:12, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 20 Apr 2015, Tomeu Vizoso wrote:
>
>> On 17 April 2015 at 19:30, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Fri, 17 Apr 2015, Laurent Pinchart wrote:
>> >
>> >> Hi Tomeu,
>> >>
>> >> Thank you for the patch.
>> >>
>> >> On Friday 17 April 2015 17:24:49 Tomeu Vizoso wrote:
>> >> > Introduce a new per-device flag power.force_direct_complete that will
>> >> > instruct the PM core to ignore the runtime PM status of its descendants
>> >> > when deciding whether to let this device remain in runtime suspend when
>> >> > the system goes into a sleep power state.
>> >> >
>> >> > This is needed because otherwise it would be needed to get dozens of
>> >> > drivers to implement the prepare() callback and be runtime PM active
>> >> > even if they don't have a 1-to-1 relationship with a piece of HW.
>> >>
>> >> I'll let PM experts comment on the approach, but I believe the new flag would
>> >> benefit from being documented (likely in Documentation/power/devices.txt) :-)
>> >
>> > Documentation/power/runtime_pm.txt is the right place.
>> >
>> > However, I'm not sure that this is the sort of thing Rafael meant when
>> > he suggested adding a new flag.  I thought he meant the PM core would
>> > look at the new flag only if there was no ->prepare method at all.
>> > Then if the new flag was set, the PM core would act as though ->prepare
>> > had returned 1.  That way there would be no need to add silly little
>> > one-line *_prepare() routines all over the place.
>> >
>> > Maybe he had something else in mind, though...
>>
>> Yeah, I also interpreted it like that, but when I started looking at
>> how it would work, I found that it would be awkward if the uvcvideo
>> driver had to track all the devices that get attached below its
>> devices in order to set that flag to them.
>>
>> When thinking about it, it occurred to me that it may make more sense
>> if we model this as a property of the device bound to the uvcvideo
>> driver, as what's happening here is that the uvcvideo driver knows
>> that it's safe to remain in runtime suspend when the system goes to
>> sleep, and that all its descendant devices can be ignored in that
>> regard.
>
> What you're proposing makes sense, but it is a significant change to
> the runtime PM core.  It should be submitted separately, not as part of
> an update to the UVC driver, and it should be discussed at length.
>
> Basically, you want to mark certain devices to say that they will
> _always_ use direct-suspend.  This means that all descendant devices
> will be forced to use direct-suspend also, and therefore any driver
> bound to one of these descendant devices will be unable to communicate
> with it during a system sleep transition.  This is a non-trivial
> restriction.
>
> Among other things, it means that wakeup settings can't be altered
> during a sleep transition.  Therefore this should be allowed only for
> devices that are not wakeup-capable.
>

I hesitated to send this reply, since it might add confusion. If
that's the case, please ignore it.

I have a long term vision to fully enable support for a runtime PM
centric configuration for drivers/subsystems. The idea is, that such
driver/subsystem should get system PM for "free".

The main goal is to simplify PM implementation for these drivers/subsystems.

They should need to implement the runtime PM callbacks only and not
the system PM ones. During system PM suspend, the requirement is that
the corresponding devices should be guaranteed to be "runtime PM
suspended". Somehow that then needs to be managed by the PM core.

I am not sure it's doable, but I wanted to bring it up within the
context of $subject patch, since it proposes yet another optimization
path for runtime PM during system PM.

Kind regards
Uffe

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Alan Stern]

On Thu, 30 Apr 2015, Ulf Hansson wrote:

> I hesitated to send this reply, since it might add confusion. If
> that's the case, please ignore it.
> 
> I have a long term vision to fully enable support for a runtime PM
> centric configuration for drivers/subsystems. The idea is, that such
> driver/subsystem should get system PM for "free".
> 
> The main goal is to simplify PM implementation for these drivers/subsystems.
> 
> They should need to implement the runtime PM callbacks only and not
> the system PM ones. During system PM suspend, the requirement is that
> the corresponding devices should be guaranteed to be "runtime PM
> suspended". Somehow that then needs to be managed by the PM core.
> 
> I am not sure it's doable, but I wanted to bring it up within the
> context of $subject patch, since it proposes yet another optimization
> path for runtime PM during system PM.

I suspect it is _not_ doable.  Consider a reasonable scenario: a driver
that does pm_runtime_get_sync() in its open routine and
pm_runtime_put() in its release routine.  If a user process holds the
device file open during a system suspend, it will be impossible for the
PM core to do a runtime suspend.

On the other hand, there's nothing to prevent drivers from setting 
their ->suspend and ->runtime_suspend structure members to point at the 
same routine.  The routine would need to handle the case where it was 
called for a system suspend while the device was already runtime 
suspended, but that doesn't seem too hard.  With the "direct-suspend" 
option, even this wouldn't be necessary.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Ulf Hansson]

On 30 April 2015 at 16:53, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 30 Apr 2015, Ulf Hansson wrote:
>
>> I hesitated to send this reply, since it might add confusion. If
>> that's the case, please ignore it.
>>
>> I have a long term vision to fully enable support for a runtime PM
>> centric configuration for drivers/subsystems. The idea is, that such
>> driver/subsystem should get system PM for "free".
>>
>> The main goal is to simplify PM implementation for these drivers/subsystems.
>>
>> They should need to implement the runtime PM callbacks only and not
>> the system PM ones. During system PM suspend, the requirement is that
>> the corresponding devices should be guaranteed to be "runtime PM
>> suspended". Somehow that then needs to be managed by the PM core.
>>
>> I am not sure it's doable, but I wanted to bring it up within the
>> context of $subject patch, since it proposes yet another optimization
>> path for runtime PM during system PM.
>
> I suspect it is _not_ doable.  Consider a reasonable scenario: a driver
> that does pm_runtime_get_sync() in its open routine and
> pm_runtime_put() in its release routine.  If a user process holds the
> device file open during a system suspend, it will be impossible for the
> PM core to do a runtime suspend.

Alan, thanks for your reply.

There are certainly drivers/subsystems that can't full-fill the
requirements to have the PM core to deal with what I propose. Somehow
drivers/subsystems would have to announce its capability for this.

Those drivers/subsystems I have been looking at, is dealing with I/O.
Typically platform/amba devices, which drivers has registered
subsystem specific callbacks at ->probe(). One of these callbacks are
invoked when there is an I/O request to serve from the subsystem's
core layer.

In the beginning of that callback, pm_runtime_get_sync() is invoked.
When the request has been served and the controller can be runtime PM
suspended, the driver call pm_runtime_put() or possibly
pm_runtime_put_autosuspend().

These drivers/subsystem may be considered as being "runtime PM
centric", since during system PM suspend they don't have any system PM
specific things to deal with. They only want to make sure their
devices becomes "runtime PM suspended".

There's no doubt that they can do that by implementing the system PM
->suspend() callbacks, in one way or the other.

To simplify PM implementation for these drivers/subsystems, it would
have been nice if the PM core could handle this "automagically", thus
drivers/subsystems wouldn't have to implement the system PM callbacks
at all. Reaching that point, would likely make it easier to understand
how to implement a "runtime PM centric" driver/subsystem.

>
> On the other hand, there's nothing to prevent drivers from setting
> their ->suspend and ->runtime_suspend structure members to point at the
> same routine.  The routine would need to handle the case where it was
> called for a system suspend while the device was already runtime
> suspended, but that doesn't seem too hard.  With the "direct-suspend"
> option, even this wouldn't be necessary.

That would likely work, but again it would require drivers/subsystems
to assign system PM callbacks.

Kind regards
Uffe

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Alan Stern]

On Wed, 6 May 2015, Ulf Hansson wrote:

> On 30 April 2015 at 16:53, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 30 Apr 2015, Ulf Hansson wrote:
> >
> >> I hesitated to send this reply, since it might add confusion. If
> >> that's the case, please ignore it.
> >>
> >> I have a long term vision to fully enable support for a runtime PM
> >> centric configuration for drivers/subsystems. The idea is, that such
> >> driver/subsystem should get system PM for "free".
> >>
> >> The main goal is to simplify PM implementation for these drivers/subsystems.
> >>
> >> They should need to implement the runtime PM callbacks only and not
> >> the system PM ones. During system PM suspend, the requirement is that
> >> the corresponding devices should be guaranteed to be "runtime PM
> >> suspended". Somehow that then needs to be managed by the PM core.
> >>
> >> I am not sure it's doable, but I wanted to bring it up within the
> >> context of $subject patch, since it proposes yet another optimization
> >> path for runtime PM during system PM.
> >
> > I suspect it is _not_ doable.  Consider a reasonable scenario: a driver
> > that does pm_runtime_get_sync() in its open routine and
> > pm_runtime_put() in its release routine.  If a user process holds the
> > device file open during a system suspend, it will be impossible for the
> > PM core to do a runtime suspend.
> 
> Alan, thanks for your reply.
> 
> There are certainly drivers/subsystems that can't full-fill the
> requirements to have the PM core to deal with what I propose. Somehow
> drivers/subsystems would have to announce its capability for this.
> 
> Those drivers/subsystems I have been looking at, is dealing with I/O.
> Typically platform/amba devices, which drivers has registered
> subsystem specific callbacks at ->probe(). One of these callbacks are
> invoked when there is an I/O request to serve from the subsystem's
> core layer.
> 
> In the beginning of that callback, pm_runtime_get_sync() is invoked.
> When the request has been served and the controller can be runtime PM
> suspended, the driver call pm_runtime_put() or possibly
> pm_runtime_put_autosuspend().
> 
> These drivers/subsystem may be considered as being "runtime PM
> centric", since during system PM suspend they don't have any system PM
> specific things to deal with. They only want to make sure their
> devices becomes "runtime PM suspended".
> 
> There's no doubt that they can do that by implementing the system PM
> ->suspend() callbacks, in one way or the other.
> 
> To simplify PM implementation for these drivers/subsystems, it would
> have been nice if the PM core could handle this "automagically", thus
> drivers/subsystems wouldn't have to implement the system PM callbacks
> at all. Reaching that point, would likely make it easier to understand
> how to implement a "runtime PM centric" driver/subsystem.

The drivers/subsystems don't have to implement these things, because
you have _already_ implemented them: pm_runtime_force_suspend() and 
pm_runtime_force_resume().  A driver/subsystem merely has to store 
pointers to these routines in its dev_pm_ops structure.

> > On the other hand, there's nothing to prevent drivers from setting
> > their ->suspend and ->runtime_suspend structure members to point at the
> > same routine.  The routine would need to handle the case where it was
> > called for a system suspend while the device was already runtime
> > suspended, but that doesn't seem too hard.  With the "direct-suspend"
> > option, even this wouldn't be necessary.
> 
> That would likely work, but again it would require drivers/subsystems
> to assign system PM callbacks.

You said just above that the driver/subsystem would have to announce
its capability for this somehow.  Using suitable callback pointers
would be a good way to make that announcement.

Alan Stern

Re: [PATCH v3 1/2] PM / sleep: Let devices force direct_complete

[Ulf Hansson]

On 6 May 2015 at 16:43, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 6 May 2015, Ulf Hansson wrote:
>
>> On 30 April 2015 at 16:53, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Thu, 30 Apr 2015, Ulf Hansson wrote:
>> >
>> >> I hesitated to send this reply, since it might add confusion. If
>> >> that's the case, please ignore it.
>> >>
>> >> I have a long term vision to fully enable support for a runtime PM
>> >> centric configuration for drivers/subsystems. The idea is, that such
>> >> driver/subsystem should get system PM for "free".
>> >>
>> >> The main goal is to simplify PM implementation for these drivers/subsystems.
>> >>
>> >> They should need to implement the runtime PM callbacks only and not
>> >> the system PM ones. During system PM suspend, the requirement is that
>> >> the corresponding devices should be guaranteed to be "runtime PM
>> >> suspended". Somehow that then needs to be managed by the PM core.
>> >>
>> >> I am not sure it's doable, but I wanted to bring it up within the
>> >> context of $subject patch, since it proposes yet another optimization
>> >> path for runtime PM during system PM.
>> >
>> > I suspect it is _not_ doable.  Consider a reasonable scenario: a driver
>> > that does pm_runtime_get_sync() in its open routine and
>> > pm_runtime_put() in its release routine.  If a user process holds the
>> > device file open during a system suspend, it will be impossible for the
>> > PM core to do a runtime suspend.
>>
>> Alan, thanks for your reply.
>>
>> There are certainly drivers/subsystems that can't full-fill the
>> requirements to have the PM core to deal with what I propose. Somehow
>> drivers/subsystems would have to announce its capability for this.
>>
>> Those drivers/subsystems I have been looking at, is dealing with I/O.
>> Typically platform/amba devices, which drivers has registered
>> subsystem specific callbacks at ->probe(). One of these callbacks are
>> invoked when there is an I/O request to serve from the subsystem's
>> core layer.
>>
>> In the beginning of that callback, pm_runtime_get_sync() is invoked.
>> When the request has been served and the controller can be runtime PM
>> suspended, the driver call pm_runtime_put() or possibly
>> pm_runtime_put_autosuspend().
>>
>> These drivers/subsystem may be considered as being "runtime PM
>> centric", since during system PM suspend they don't have any system PM
>> specific things to deal with. They only want to make sure their
>> devices becomes "runtime PM suspended".
>>
>> There's no doubt that they can do that by implementing the system PM
>> ->suspend() callbacks, in one way or the other.
>>
>> To simplify PM implementation for these drivers/subsystems, it would
>> have been nice if the PM core could handle this "automagically", thus
>> drivers/subsystems wouldn't have to implement the system PM callbacks
>> at all. Reaching that point, would likely make it easier to understand
>> how to implement a "runtime PM centric" driver/subsystem.
>
> The drivers/subsystems don't have to implement these things, because
> you have _already_ implemented them: pm_runtime_force_suspend() and
> pm_runtime_force_resume().  A driver/subsystem merely has to store
> pointers to these routines in its dev_pm_ops structure.

Yes, apparently I have touched this topic earlier. :-)

As we are moving towards removing the Kconfig option CONFIG_PM_SLEEP
(CONFIG_PM_RUNTIME is already removed) and only have the CONFIG_PM,
parts of what the above helper functions addresses becomes redundant.

>
>> > On the other hand, there's nothing to prevent drivers from setting
>> > their ->suspend and ->runtime_suspend structure members to point at the
>> > same routine.  The routine would need to handle the case where it was
>> > called for a system suspend while the device was already runtime
>> > suspended, but that doesn't seem too hard.  With the "direct-suspend"
>> > option, even this wouldn't be necessary.
>>
>> That would likely work, but again it would require drivers/subsystems
>> to assign system PM callbacks.
>
> You said just above that the driver/subsystem would have to announce
> its capability for this somehow.  Using suitable callback pointers
> would be a good way to make that announcement.

I was thinking, for simplicity purpose, that we could try to move this
a step forward.

Having a API like pm_runtime_centric() (or whatever name we can come
up with) to announce the capability, could be easier to understand and
use. Of course that's my opinion.

Kind regards
Uffe

Re: [PATCH v3 2/2] [media] uvcvideo: Remain runtime-suspended at sleeps

[Laurent Pinchart]

Hi Tomeu,

On Monday 20 April 2015 09:11:36 Tomeu Vizoso wrote:
> On 17 April 2015 at 19:32, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Fri, 17 Apr 2015, Tomeu Vizoso wrote:
> >> When the system goes to sleep and afterwards resumes, a significant
> >> amount of time is spent suspending and resuming devices that were
> >> already runtime-suspended.
> >> 
> >> By setting the power.force_direct_complete flag, the PM core will ignore
> >> the state of descendant devices and the device will be let in
> >> runtime-suspend.
> >> 
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> ---
> >> 
> >>  drivers/media/usb/uvc/uvc_driver.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> >> b/drivers/media/usb/uvc/uvc_driver.c index 5970dd6..ae75a70 100644
> >> --- a/drivers/media/usb/uvc/uvc_driver.c
> >> +++ b/drivers/media/usb/uvc/uvc_driver.c
> >> @@ -1945,6 +1945,8 @@ static int uvc_probe(struct usb_interface *intf,
> >> 
> >>                       "supported.\n", ret);
> >>       
> >>       }
> >> 
> >> +     intf->dev.parent->power.force_direct_complete = true;
> > 
> > This seems wrong.  The uvc driver is bound to intf, not to intf's
> > parent.  So it would be okay for the driver to set
> > intf->dev.power.force_direct_complete, but it's wrong to set
> > intf->dev.parent->power.force_direct_complete.
> 
> Agreed.

Do you plan to resubmit this patch series with the above fix ? I know you've 
had a hard time trying to find an approach that could get accepted, but please 
rest assured that your work on the uvcvideo driver is appreciated.

-- 
Regards,

Laurent Pinchart