[PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
designed to help driver being RPM-centric by offering an easy way to
manager runtime PM state during system suspend and resume. The first
function will force the device into runtime suspend at system suspend
time, while the second one will perform the reverse operation at system
resume time.

However, the pm_runtime_force_resume() really forces resume, regarding
of whether the device was running or already suspended before the call
to pm_runtime_force_suspend(). This results in devices being runtime
resumed at system resume time when they shouldn't.

Fix this by recording whether the device has been forcefully suspended
in pm_runtime_force_suspend() and condition resume in
pm_runtime_force_resume() to that state.

All current users of pm_runtime_force_resume() call the function
uncontionally in their system resume handler (some actually set it as
the resume handler), all after calling pm_runtime_force_suspend() at
system suspend time. The change in behaviour should thus be safe.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/base/power/runtime.c | 12 +++++++++---
 include/linux/pm.h           |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 4c7055009bd6..ad2189294c9b 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1400,6 +1400,7 @@ void pm_runtime_init(struct device *dev)
 	pm_suspend_ignore_children(dev, false);
 	dev->power.runtime_auto = true;
 
+	dev->power.is_force_suspended = false;
 	dev->power.request_pending = false;
 	dev->power.request = RPM_REQ_NONE;
 	dev->power.deferred_resume = false;
@@ -1475,6 +1476,7 @@ int pm_runtime_force_suspend(struct device *dev)
 		goto err;
 
 	pm_runtime_set_suspended(dev);
+	dev->power.is_force_suspended = true;
 	return 0;
 err:
 	pm_runtime_enable(dev);
@@ -1483,13 +1485,13 @@ err:
 EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
 
 /**
- * pm_runtime_force_resume - Force a device into resume state.
+ * pm_runtime_force_resume - Force a device into resume state if needed.
  * @dev: Device to resume.
  *
  * Prior invoking this function we expect the user to have brought the device
  * into low power state by a call to pm_runtime_force_suspend(). Here we reverse
- * those actions and brings the device into full power. We update the runtime PM
- * status and re-enables runtime PM.
+ * those actions and bring the device back to its runtime PM state before forced
+ * suspension. We update the runtime PM status and re-enables runtime PM.
  *
  * Typically this function may be invoked from a system resume callback to make
  * sure the device is put into full power state.
@@ -1499,6 +1501,9 @@ int pm_runtime_force_resume(struct device *dev)
 	int (*callback)(struct device *);
 	int ret = 0;
 
+	if (!dev->power.is_force_suspended)
+		goto out;
+
 	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 
 	if (!callback) {
@@ -1510,6 +1515,7 @@ int pm_runtime_force_resume(struct device *dev)
 	if (ret)
 		goto out;
 
+	dev->power.is_force_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_mark_last_busy(dev);
 out:
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 6a5d654f4447..bec15e0f244e 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -596,6 +596,7 @@ struct dev_pm_info {
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	unsigned int		memalloc_noio:1;
+	unsigned int		is_force_suspended:1;
 	enum rpm_request	request;
 	enum rpm_status		runtime_status;
 	int			runtime_error;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Kevin Hilman]

Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:

> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> designed to help driver being RPM-centric by offering an easy way to
> manager runtime PM state during system suspend and resume. The first

s/manager/manage/

> function will force the device into runtime suspend at system suspend
> time, while the second one will perform the reverse operation at system
> resume time.
>
> However, the pm_runtime_force_resume() really forces resume, regarding

s/regarding/regardless/

> of whether the device was running or already suspended before the call
> to pm_runtime_force_suspend(). This results in devices being runtime
> resumed at system resume time when they shouldn't.

Agreed.

> Fix this by recording whether the device has been forcefully suspended
> in pm_runtime_force_suspend() and condition resume in
> pm_runtime_force_resume() to that state.
>
> All current users of pm_runtime_force_resume() call the function
> uncontionally in their system resume handler (some actually set it as
> the resume handler), all after calling pm_runtime_force_suspend() at
> system suspend time. The change in behaviour should thus be safe.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

I agree this is the right approach, but Ulf should ack this too since
he's looked into all the strange corner case involved and may know of
something I've missed.

Kevin

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hi Kevin,

(CC'ing Ulf)

Thank you for the review.

On Thursday 03 March 2016 12:35:53 Kevin Hilman wrote:
> Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes:
> > The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> > designed to help driver being RPM-centric by offering an easy way to
> > manager runtime PM state during system suspend and resume. The first
> 
> s/manager/manage/
> 
> > function will force the device into runtime suspend at system suspend
> > time, while the second one will perform the reverse operation at system
> > resume time.
> > 
> > However, the pm_runtime_force_resume() really forces resume, regarding
> 
> s/regarding/regardless/
> 
> > of whether the device was running or already suspended before the call
> > to pm_runtime_force_suspend(). This results in devices being runtime
> > resumed at system resume time when they shouldn't.
> 
> Agreed.
> 
> > Fix this by recording whether the device has been forcefully suspended
> > in pm_runtime_force_suspend() and condition resume in
> > pm_runtime_force_resume() to that state.
> > 
> > All current users of pm_runtime_force_resume() call the function
> > uncontionally in their system resume handler (some actually set it as
> > the resume handler), all after calling pm_runtime_force_suspend() at
> > system suspend time. The change in behaviour should thus be safe.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> 
> I agree this is the right approach, but Ulf should ack this too since
> he's looked into all the strange corner case involved and may know of
> something I've missed.

Sure, the more reviewers, the merrier :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Sergei Shtylyov]

Hello.

On 03/03/2016 11:16 PM, Laurent Pinchart wrote:

    Noticed a few typos as well, some alreayd reported...

> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> designed to help driver being RPM-centric by offering an easy way to
> manager runtime PM state during system suspend and resume. The first

    Manage.

> function will force the device into runtime suspend at system suspend
> time, while the second one will perform the reverse operation at system
> resume time.
>
> However, the pm_runtime_force_resume() really forces resume, regarding

    Regardless.

> of whether the device was running or already suspended before the call
> to pm_runtime_force_suspend(). This results in devices being runtime
> resumed at system resume time when they shouldn't.
>
> Fix this by recording whether the device has been forcefully suspended
> in pm_runtime_force_suspend() and condition resume in
> pm_runtime_force_resume() to that state.
>
> All current users of pm_runtime_force_resume() call the function
> uncontionally in their system resume handler (some actually set it as

    Unconditionally.

> the resume handler), all after calling pm_runtime_force_suspend() at
> system suspend time. The change in behaviour should thus be safe.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/base/power/runtime.c | 12 +++++++++---
>   include/linux/pm.h           |  1 +
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 4c7055009bd6..ad2189294c9b 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
[...]
> @@ -1483,13 +1485,13 @@ err:
>   EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
>
>   /**
> - * pm_runtime_force_resume - Force a device into resume state.
> + * pm_runtime_force_resume - Force a device into resume state if needed.
>    * @dev: Device to resume.
>    *
>    * Prior invoking this function we expect the user to have brought the device
>    * into low power state by a call to pm_runtime_force_suspend(). Here we reverse
> - * those actions and brings the device into full power. We update the runtime PM
> - * status and re-enables runtime PM.
> + * those actions and bring the device back to its runtime PM state before forced
> + * suspension. We update the runtime PM status and re-enables runtime PM.

    Re-enable.

[...]

MBR, Sergei

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Ulf Hansson]

+ Alan

On 3 March 2016 at 21:16, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> designed to help driver being RPM-centric by offering an easy way to
> manager runtime PM state during system suspend and resume. The first
> function will force the device into runtime suspend at system suspend
> time, while the second one will perform the reverse operation at system
> resume time.
>
> However, the pm_runtime_force_resume() really forces resume, regarding
> of whether the device was running or already suspended before the call
> to pm_runtime_force_suspend(). This results in devices being runtime
> resumed at system resume time when they shouldn't.
>
> Fix this by recording whether the device has been forcefully suspended
> in pm_runtime_force_suspend() and condition resume in
> pm_runtime_force_resume() to that state.
>
> All current users of pm_runtime_force_resume() call the function
> uncontionally in their system resume handler (some actually set it as
> the resume handler), all after calling pm_runtime_force_suspend() at
> system suspend time. The change in behaviour should thus be safe.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/base/power/runtime.c | 12 +++++++++---
>  include/linux/pm.h           |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 4c7055009bd6..ad2189294c9b 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1400,6 +1400,7 @@ void pm_runtime_init(struct device *dev)
>         pm_suspend_ignore_children(dev, false);
>         dev->power.runtime_auto = true;
>
> +       dev->power.is_force_suspended = false;
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> @@ -1475,6 +1476,7 @@ int pm_runtime_force_suspend(struct device *dev)
>                 goto err;
>
>         pm_runtime_set_suspended(dev);
> +       dev->power.is_force_suspended = true;
>         return 0;
>  err:
>         pm_runtime_enable(dev);
> @@ -1483,13 +1485,13 @@ err:
>  EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
>
>  /**
> - * pm_runtime_force_resume - Force a device into resume state.
> + * pm_runtime_force_resume - Force a device into resume state if needed.
>   * @dev: Device to resume.
>   *
>   * Prior invoking this function we expect the user to have brought the device
>   * into low power state by a call to pm_runtime_force_suspend(). Here we reverse
> - * those actions and brings the device into full power. We update the runtime PM
> - * status and re-enables runtime PM.
> + * those actions and bring the device back to its runtime PM state before forced
> + * suspension. We update the runtime PM status and re-enables runtime PM.
>   *
>   * Typically this function may be invoked from a system resume callback to make
>   * sure the device is put into full power state.
> @@ -1499,6 +1501,9 @@ int pm_runtime_force_resume(struct device *dev)
>         int (*callback)(struct device *);
>         int ret = 0;
>
> +       if (!dev->power.is_force_suspended)
> +               goto out;
> +
>         callback = RPM_GET_CALLBACK(dev, runtime_resume);
>
>         if (!callback) {
> @@ -1510,6 +1515,7 @@ int pm_runtime_force_resume(struct device *dev)
>         if (ret)
>                 goto out;
>
> +       dev->power.is_force_suspended = false;
>         pm_runtime_set_active(dev);
>         pm_runtime_mark_last_busy(dev);
>  out:
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 6a5d654f4447..bec15e0f244e 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -596,6 +596,7 @@ struct dev_pm_info {
>         unsigned int            use_autosuspend:1;
>         unsigned int            timer_autosuspends:1;
>         unsigned int            memalloc_noio:1;
> +       unsigned int            is_force_suspended:1;
>         enum rpm_request        request;
>         enum rpm_status         runtime_status;
>         int                     runtime_error;
> --

Overall I have no objections to this change, as I think it's improving
the behaviour!

What I was thinking though, but it might be a bit controversial. :-)...
Instead of relying on whether we actually forced runtime suspend
earlier, why couldn't we instead check the runtime PM usage count of
the device?

Only when it's greater than zero, we shall do the forced resume of the
device, otherwise just re-enable runtime PM.

This would have the affect of leaving devices in runtime suspend,
until they really needs to be used again. It would thus decrease the
total system PM resume time.

Do you think this could work?

Kind regards
Uffe

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Alan Stern]

On Fri, 4 Mar 2016, Ulf Hansson wrote:

> + Alan
> 
> On 3 March 2016 at 21:16, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
> > The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> > designed to help driver being RPM-centric by offering an easy way to
> > manager runtime PM state during system suspend and resume. The first
> > function will force the device into runtime suspend at system suspend
> > time, while the second one will perform the reverse operation at system
> > resume time.
> >
> > However, the pm_runtime_force_resume() really forces resume, regarding
> > of whether the device was running or already suspended before the call
> > to pm_runtime_force_suspend(). This results in devices being runtime
> > resumed at system resume time when they shouldn't.
> >
> > Fix this by recording whether the device has been forcefully suspended
> > in pm_runtime_force_suspend() and condition resume in
> > pm_runtime_force_resume() to that state.
> >
> > All current users of pm_runtime_force_resume() call the function
> > uncontionally in their system resume handler (some actually set it as
> > the resume handler), all after calling pm_runtime_force_suspend() at
> > system suspend time. The change in behaviour should thus be safe.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>


> > @@ -1475,6 +1476,7 @@ int pm_runtime_force_suspend(struct device *dev)
> >                 goto err;
> >
> >         pm_runtime_set_suspended(dev);
> > +       dev->power.is_force_suspended = true;
> >         return 0;
> >  err:
> >         pm_runtime_enable(dev);

> > @@ -1510,6 +1515,7 @@ int pm_runtime_force_resume(struct device *dev)
> >         if (ret)
> >                 goto out;
> >
> > +       dev->power.is_force_suspended = false;
> >         pm_runtime_set_active(dev);
> >         pm_runtime_mark_last_busy(dev);
> >  out:

Setting a bitflag is not SMP-safe.  When you write to one of the
runtime-PM bits under dev->power, it is necessary to hold 
dev->power.lock.

> Overall I have no objections to this change, as I think it's improving
> the behaviour!
> 
> What I was thinking though, but it might be a bit controversial. :-)...
> Instead of relying on whether we actually forced runtime suspend
> earlier, why couldn't we instead check the runtime PM usage count of
> the device?
> 
> Only when it's greater than zero, we shall do the forced resume of the
> device, otherwise just re-enable runtime PM.
> 
> This would have the affect of leaving devices in runtime suspend,
> until they really needs to be used again. It would thus decrease the
> total system PM resume time.
> 
> Do you think this could work?

If you do this then there would be no need for is_force_suspended.  It 
seems like a good idea to me.

Alan Stern

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hi Ulf and Alan,

Thank you for the review.

On Friday 04 March 2016 10:24:10 Alan Stern wrote:
> On Fri, 4 Mar 2016, Ulf Hansson wrote:
> > On 3 March 2016 at 21:16, Laurent Pinchart wrote:
> >> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers are
> >> designed to help driver being RPM-centric by offering an easy way to
> >> manager runtime PM state during system suspend and resume. The first
> >> function will force the device into runtime suspend at system suspend
> >> time, while the second one will perform the reverse operation at system
> >> resume time.
> >> 
> >> However, the pm_runtime_force_resume() really forces resume, regarding
> >> of whether the device was running or already suspended before the call
> >> to pm_runtime_force_suspend(). This results in devices being runtime
> >> resumed at system resume time when they shouldn't.
> >> 
> >> Fix this by recording whether the device has been forcefully suspended
> >> in pm_runtime_force_suspend() and condition resume in
> >> pm_runtime_force_resume() to that state.
> >> 
> >> All current users of pm_runtime_force_resume() call the function
> >> uncontionally in their system resume handler (some actually set it as
> >> the resume handler), all after calling pm_runtime_force_suspend() at
> >> system suspend time. The change in behaviour should thus be safe.
> >> 
> >> Signed-off-by: Laurent Pinchart
> >> <laurent.pinchart+renesas@ideasonboard.com>
> >> 
> >> @@ -1475,6 +1476,7 @@ int pm_runtime_force_suspend(struct device *dev)
> >>                 goto err;
> >>         
> >>         pm_runtime_set_suspended(dev);
> >> +       dev->power.is_force_suspended = true;
> >>         return 0;
> >>  
> >>  err:
> >>         pm_runtime_enable(dev);
> >> @@ -1510,6 +1515,7 @@ int pm_runtime_force_resume(struct device *dev)
> >>         if (ret)
> >>                 goto out;
> >> 
> >> +       dev->power.is_force_suspended = false;
> >>         pm_runtime_set_active(dev);
> >>         pm_runtime_mark_last_busy(dev);
> >>  out:
>
> Setting a bitflag is not SMP-safe.  When you write to one of the
> runtime-PM bits under dev->power, it is necessary to hold
> dev->power.lock.
> 
> > Overall I have no objections to this change, as I think it's improving
> > the behaviour!
> > 
> > What I was thinking though, but it might be a bit controversial. :-)...
> > Instead of relying on whether we actually forced runtime suspend
> > earlier, why couldn't we instead check the runtime PM usage count of
> > the device?
> > 
> > Only when it's greater than zero, we shall do the forced resume of the
> > device, otherwise just re-enable runtime PM.
> > 
> > This would have the affect of leaving devices in runtime suspend,
> > until they really needs to be used again. It would thus decrease the
> > total system PM resume time.
> > 
> > Do you think this could work?
> 
> If you do this then there would be no need for is_force_suspended.  It
> seems like a good idea to me.

I agree, that's a better idea. Drivers shouldn't call 
pm_runtime_force_resume() if they haven't called pm_runtime_force_suspend(), 
so checking the PM use count should be fine. I'll modify the patch, test it 
and resubmit.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hello,

On Friday 04 March 2016 23:04:46 Laurent Pinchart wrote:
> On Friday 04 March 2016 10:24:10 Alan Stern wrote:
> > On Fri, 4 Mar 2016, Ulf Hansson wrote:
> >> On 3 March 2016 at 21:16, Laurent Pinchart wrote:
> >>> The pm_runtime_force_suspend() and pm_runtime_force_resume() helpers
> >>> are designed to help driver being RPM-centric by offering an easy way to
> >>> manager runtime PM state during system suspend and resume. The first
> >>> function will force the device into runtime suspend at system suspend
> >>> time, while the second one will perform the reverse operation at system
> >>> resume time.
> >>> 
> >>> However, the pm_runtime_force_resume() really forces resume, regarding
> >>> of whether the device was running or already suspended before the call
> >>> to pm_runtime_force_suspend(). This results in devices being runtime
> >>> resumed at system resume time when they shouldn't.
> >>> 
> >>> Fix this by recording whether the device has been forcefully suspended
> >>> in pm_runtime_force_suspend() and condition resume in
> >>> pm_runtime_force_resume() to that state.
> >>> 
> >>> All current users of pm_runtime_force_resume() call the function
> >>> uncontionally in their system resume handler (some actually set it as
> >>> the resume handler), all after calling pm_runtime_force_suspend() at
> >>> system suspend time. The change in behaviour should thus be safe.
> >>> 
> >>> Signed-off-by: Laurent Pinchart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> 
> >>> @@ -1475,6 +1476,7 @@ int pm_runtime_force_suspend(struct device *dev)
> >>>                 goto err;
> >>>         
> >>>         pm_runtime_set_suspended(dev);
> >>> +       dev->power.is_force_suspended = true;
> >>>         return 0;
> >>>  err:
> >>>         pm_runtime_enable(dev);
> >>> @@ -1510,6 +1515,7 @@ int pm_runtime_force_resume(struct device *dev)
> >>>         if (ret)
> >>>                 goto out;
> >>> 
> >>> +       dev->power.is_force_suspended = false;
> >>>         pm_runtime_set_active(dev);
> >>>         pm_runtime_mark_last_busy(dev);
> >>>  out:
> >
> > Setting a bitflag is not SMP-safe.  When you write to one of the
> > runtime-PM bits under dev->power, it is necessary to hold
> > dev->power.lock.
> > 
> >> Overall I have no objections to this change, as I think it's improving
> >> the behaviour!
> >> 
> >> What I was thinking though, but it might be a bit controversial. :-)...
> >> Instead of relying on whether we actually forced runtime suspend
> >> earlier, why couldn't we instead check the runtime PM usage count of
> >> the device?
> >> 
> >> Only when it's greater than zero, we shall do the forced resume of the
> >> device, otherwise just re-enable runtime PM.
> >> 
> >> This would have the affect of leaving devices in runtime suspend,
> >> until they really needs to be used again. It would thus decrease the
> >> total system PM resume time.
> >> 
> >> Do you think this could work?
> > 
> > If you do this then there would be no need for is_force_suspended.  It
> > seems like a good idea to me.
> 
> I agree, that's a better idea. Drivers shouldn't call
> pm_runtime_force_resume() if they haven't called pm_runtime_force_suspend(),
> so checking the PM use count should be fine. I'll modify the patch, test it
> and resubmit.

I gave it an unfortunately unsuccessful try. The problem I ran into is that 
device_prepare() calls pm_runtime_get_noresume() calls 
pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call being 
performed in device_complete(). The device power usage_count is thus always 
non-zero in the system resume handler, so I can't base the decision on that.

I also noticed that pm_genpd_prepare() runtime-resumes the device (when the 
power domain is in the GPD_STATE_ACTIVE state). I don't know why that is, but 
it means that in practice my device gets runtime-resumed when suspending the 
system while it could stay runtime-suspended in practice.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Alan Stern]

On Sun, 6 Mar 2016, Laurent Pinchart wrote:

> > >> What I was thinking though, but it might be a bit controversial. :-)...
> > >> Instead of relying on whether we actually forced runtime suspend
> > >> earlier, why couldn't we instead check the runtime PM usage count of
> > >> the device?
> > >> 
> > >> Only when it's greater than zero, we shall do the forced resume of the
> > >> device, otherwise just re-enable runtime PM.
> > >> 
> > >> This would have the affect of leaving devices in runtime suspend,
> > >> until they really needs to be used again. It would thus decrease the
> > >> total system PM resume time.
> > >> 
> > >> Do you think this could work?
> > > 
> > > If you do this then there would be no need for is_force_suspended.  It
> > > seems like a good idea to me.
> > 
> > I agree, that's a better idea. Drivers shouldn't call
> > pm_runtime_force_resume() if they haven't called pm_runtime_force_suspend(),
> > so checking the PM use count should be fine. I'll modify the patch, test it
> > and resubmit.
> 
> I gave it an unfortunately unsuccessful try. The problem I ran into is that 
> device_prepare() calls pm_runtime_get_noresume() calls 
> pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call being 
> performed in device_complete(). The device power usage_count is thus always 
> non-zero in the system resume handler, so I can't base the decision on that.

You could check for usage_count > 1 instead of > 0.  With a comment 
explaining why, of course.

Alan Stern

> I also noticed that pm_genpd_prepare() runtime-resumes the device (when the 
> power domain is in the GPD_STATE_ACTIVE state). I don't know why that is, but 
> it means that in practice my device gets runtime-resumed when suspending the 
> system while it could stay runtime-suspended in practice.

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Ulf Hansson]

[...]

>> I agree, that's a better idea. Drivers shouldn't call
>> pm_runtime_force_resume() if they haven't called pm_runtime_force_suspend(),
>> so checking the PM use count should be fine. I'll modify the patch, test it
>> and resubmit.
>
> I gave it an unfortunately unsuccessful try. The problem I ran into is that
> device_prepare() calls pm_runtime_get_noresume() calls
> pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call being
> performed in device_complete(). The device power usage_count is thus always
> non-zero in the system resume handler, so I can't base the decision on that.

As Alan said, let's just check against 1 instead.

>
> I also noticed that pm_genpd_prepare() runtime-resumes the device (when the
> power domain is in the GPD_STATE_ACTIVE state). I don't know why that is, but
> it means that in practice my device gets runtime-resumed when suspending the
> system while it could stay runtime-suspended in practice.

I am aware of this and it's on my TODO list of improvements of genpd,
The issue is related to an unoptimized behaviour for how genpd deal
with wakeups during system PM.

Kind regards
Uffe

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hello,

Reviving this old thread.

On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
> [...]
> 
> >> I agree, that's a better idea. Drivers shouldn't call
> >> pm_runtime_force_resume() if they haven't called
> >> pm_runtime_force_suspend(), so checking the PM use count should be fine.
> >> I'll modify the patch, test it and resubmit.
> > 
> > I gave it an unfortunately unsuccessful try. The problem I ran into is
> > that device_prepare() calls pm_runtime_get_noresume() calls
> > pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call
> > being performed in device_complete(). The device power usage_count is
> > thus always non-zero in the system resume handler, so I can't base the
> > decision on that.
>
> As Alan said, let's just check against 1 instead.

I gave this a try, and unfortunately it won't work.

pm_genpd_prepare() resumes devices without increasing the usage count, which 
leads to the device always being active in pm_runtime_force_suspend(). The 
usage count will be 1 if the device was suspended prior to entering system 
suspend (due to the pm_runtime_get_noresume() call in device_prepare()) or 
higher than 1 if the device was active.

However, pm_genpd_prepare() will not resume the device if suspend_power_off is 
set. In that case the device will be suspended with a usage count of 1 in 
pm_runtime_force_suspend() or active with a usage count higher than 1.

We thus can't detect at resume time whether we have force-suspended the device 
using the usage count.

Unless someone has another clever idea I'll keep the power.is_force_suspended 
flag and protect it with power.lock.

> > I also noticed that pm_genpd_prepare() runtime-resumes the device (when
> > the power domain is in the GPD_STATE_ACTIVE state). I don't know why that
> > is, but it means that in practice my device gets runtime-resumed when
> > suspending the system while it could stay runtime-suspended in practice.
> 
> I am aware of this and it's on my TODO list of improvements of genpd,
> The issue is related to an unoptimized behaviour for how genpd deal
> with wakeups during system PM.

Looking forward to seeing patches :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Ulf Hansson]

On 21 April 2016 at 01:30, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hello,
>
> Reviving this old thread.
>
> On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
>> [...]
>>
>> >> I agree, that's a better idea. Drivers shouldn't call
>> >> pm_runtime_force_resume() if they haven't called
>> >> pm_runtime_force_suspend(), so checking the PM use count should be fine.
>> >> I'll modify the patch, test it and resubmit.
>> >
>> > I gave it an unfortunately unsuccessful try. The problem I ran into is
>> > that device_prepare() calls pm_runtime_get_noresume() calls
>> > pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call
>> > being performed in device_complete(). The device power usage_count is
>> > thus always non-zero in the system resume handler, so I can't base the
>> > decision on that.
>>
>> As Alan said, let's just check against 1 instead.
>
> I gave this a try, and unfortunately it won't work.
>
> pm_genpd_prepare() resumes devices without increasing the usage count, which

It doesn't resume them, it only increases the usage count.

> leads to the device always being active in pm_runtime_force_suspend(). The
> usage count will be 1 if the device was suspended prior to entering system
> suspend (due to the pm_runtime_get_noresume() call in device_prepare()) or
> higher than 1 if the device was active.

It's only greater than 1 - if someone else than the PM core has
increased the usage count.

>
> However, pm_genpd_prepare() will not resume the device if suspend_power_off is
> set. In that case the device will be suspended with a usage count of 1 in
> pm_runtime_force_suspend() or active with a usage count higher than 1.
>
> We thus can't detect at resume time whether we have force-suspended the device
> using the usage count.

We don't need to detect this no more! Let me give you some background to why.

When the pm_runtime_force_suspend|resume() helpers were invented, we
still had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig
options.

To make sure these helpers worked for all combinations and without
introducing too much complexity, we needed to always resume the device
in pm_runtime_force_resume(). More precisely, when CONFIG_PM_SLEEP was
used without CONFIG_PM_RUNTIME, we needed to resume the device as the
driver/subsystem couldn't do it via a call to pm_runtime_get_sync()
(or similar API).

As we have merged CONFIG_PM_RUNTIME into CONFIG_PM, the above
described option has disappeared. This means the driver/bus indeed
will be able to use pm_runtime_get_sync() to resume the device at a
later point when needed.

>
> Unless someone has another clever idea I'll keep the power.is_force_suspended
> flag and protect it with power.lock.
>
>> > I also noticed that pm_genpd_prepare() runtime-resumes the device (when
>> > the power domain is in the GPD_STATE_ACTIVE state). I don't know why that
>> > is, but it means that in practice my device gets runtime-resumed when
>> > suspending the system while it could stay runtime-suspended in practice.
>>
>> I am aware of this and it's on my TODO list of improvements of genpd,
>> The issue is related to an unoptimized behaviour for how genpd deal
>> with wakeups during system PM.
>
> Looking forward to seeing patches :-)

I have been cooking a patch. Very soon I will post it and will make
sure you are on cc!

Of course any help in testing/reviewing will be highly appreciated!

Kind regards
Uffe

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hi Ulf,

On Thursday 21 Apr 2016 11:10:19 Ulf Hansson wrote:
> On 21 April 2016 at 01:30, Laurent Pinchart wrote:
> > On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
> >> [...]
> >> 
> >>>> I agree, that's a better idea. Drivers shouldn't call
> >>>> pm_runtime_force_resume() if they haven't called
> >>>> pm_runtime_force_suspend(), so checking the PM use count should be
> >>>> fine. I'll modify the patch, test it and resubmit.
> >>> 
> >>> I gave it an unfortunately unsuccessful try. The problem I ran into is
> >>> that device_prepare() calls pm_runtime_get_noresume() calls
> >>> pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call
> >>> being performed in device_complete(). The device power usage_count is
> >>> thus always non-zero in the system resume handler, so I can't base the
> >>> decision on that.
> >> 
> >> As Alan said, let's just check against 1 instead.
> > 
> > I gave this a try, and unfortunately it won't work.
> > 
> > pm_genpd_prepare() resumes devices without increasing the usage count,
> > which
>
> It doesn't resume them, it only increases the usage count.

Maybe there's something I don't get, but pm_genpd_prepare() ends with

        /*
         * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
         * so genpd_poweron() will return immediately, but if the device
         * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need
         * to make it operational.
         */
        pm_runtime_resume(dev);
        __pm_runtime_disable(dev, false);

        ret = pm_generic_prepare(dev);
        if (ret) {
                ... /* irrelevant error handling */
        }

        pm_runtime_put(dev);
        return ret;

The device is thus resumed. Note that the pm_runtime_put() call will decrease 
the usage count that was increased at the beginning of the function (and thus 
makes pm_genpd_prepare() neutral from a usage count point of view) but won't 
resume the device as the disable depth is increased by __pm_runtime_disable().

As far as I understand, the device is thus effectively active when entering 
the driver's system suspend handler, with a usage count equal to 1 or more. 
pm_runtime_force_suspend(), which decides whether to suspend the device based 
on the device pm state and not the usage count, will thus always pm suspend 
the device.

Is the above analysis correct ?

Now I'm a bit lost as to what happens (and what should happen) at resume time. 
Does genpd and the PM core try to suspend the device after calling the 
driver's resume handler (pm_runtime_force_resume() in this case) under some 
conditions, or does the resume handler have the last word in deciding the PM 
runtime status of the device after system resume ?

> > leads to the device always being active in pm_runtime_force_suspend(). The
> > usage count will be 1 if the device was suspended prior to entering system
> > suspend (due to the pm_runtime_get_noresume() call in device_prepare()) or
> > higher than 1 if the device was active.
> 
> It's only greater than 1 - if someone else than the PM core has
> increased the usage count.

That I agree with.

> > However, pm_genpd_prepare() will not resume the device if
> > suspend_power_off is set. In that case the device will be suspended with
> > a usage count of 1 in pm_runtime_force_suspend() or active with a usage
> > count higher than 1.
> > 
> > We thus can't detect at resume time whether we have force-suspended the
> > device using the usage count.
> 
> We don't need to detect this no more! Let me give you some background to
> why.
> 
> When the pm_runtime_force_suspend|resume() helpers were invented, we
> still had CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP as separate Kconfig
> options.
> 
> To make sure these helpers worked for all combinations and without
> introducing too much complexity, we needed to always resume the device
> in pm_runtime_force_resume(). More precisely, when CONFIG_PM_SLEEP was
> used without CONFIG_PM_RUNTIME, we needed to resume the device as the
> driver/subsystem couldn't do it via a call to pm_runtime_get_sync()
> (or similar API).
> 
> As we have merged CONFIG_PM_RUNTIME into CONFIG_PM, the above
> described option has disappeared. This means the driver/bus indeed
> will be able to use pm_runtime_get_sync() to resume the device at a
> later point when needed.
> 
> > Unless someone has another clever idea I'll keep the
> > power.is_force_suspended flag and protect it with power.lock.
> > 
> >> > I also noticed that pm_genpd_prepare() runtime-resumes the device (when
> >> > the power domain is in the GPD_STATE_ACTIVE state). I don't know why
> >> > that
> >> > is, but it means that in practice my device gets runtime-resumed when
> >> > suspending the system while it could stay runtime-suspended in
> >> > practice.
> >> 
> >> I am aware of this and it's on my TODO list of improvements of genpd,
> >> The issue is related to an unoptimized behaviour for how genpd deal
> >> with wakeups during system PM.
> > 
> > Looking forward to seeing patches :-)
> 
> I have been cooking a patch. Very soon I will post it and will make
> sure you are on cc!
> 
> Of course any help in testing/reviewing will be highly appreciated!

-- 
Regards,

Laurent Pinchart

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Ulf Hansson]

On 21 April 2016 at 14:41, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ulf,
>
> On Thursday 21 Apr 2016 11:10:19 Ulf Hansson wrote:
>> On 21 April 2016 at 01:30, Laurent Pinchart wrote:
>> > On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
>> >> [...]
>> >>
>> >>>> I agree, that's a better idea. Drivers shouldn't call
>> >>>> pm_runtime_force_resume() if they haven't called
>> >>>> pm_runtime_force_suspend(), so checking the PM use count should be
>> >>>> fine. I'll modify the patch, test it and resubmit.
>> >>>
>> >>> I gave it an unfortunately unsuccessful try. The problem I ran into is
>> >>> that device_prepare() calls pm_runtime_get_noresume() calls
>> >>> pm_runtime_get_noresume(), with the corresponding pm_runtime_put() call
>> >>> being performed in device_complete(). The device power usage_count is
>> >>> thus always non-zero in the system resume handler, so I can't base the
>> >>> decision on that.
>> >>
>> >> As Alan said, let's just check against 1 instead.
>> >
>> > I gave this a try, and unfortunately it won't work.
>> >
>> > pm_genpd_prepare() resumes devices without increasing the usage count,
>> > which
>>
>> It doesn't resume them, it only increases the usage count.
>
> Maybe there's something I don't get, but pm_genpd_prepare() ends with

I realize that I did read *pm_genpd_prepare()* as *device_prepare()*,
which represents the behaviour of the PM core during the prepare phase
(drivers/base/power/main.c). In such cases my earlier reply would make
more sense, I believe.

Apologize for giving you the wrong input by not reading your response
in more detail!

>
>         /*
>          * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
>          * so genpd_poweron() will return immediately, but if the device
>          * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need
>          * to make it operational.
>          */
>         pm_runtime_resume(dev);
>         __pm_runtime_disable(dev, false);
>
>         ret = pm_generic_prepare(dev);
>         if (ret) {
>                 ... /* irrelevant error handling */
>         }
>
>         pm_runtime_put(dev);
>         return ret;
>
> The device is thus resumed. Note that the pm_runtime_put() call will decrease
> the usage count that was increased at the beginning of the function (and thus
> makes pm_genpd_prepare() neutral from a usage count point of view) but won't
> resume the device as the disable depth is increased by __pm_runtime_disable().
>
> As far as I understand, the device is thus effectively active when entering
> the driver's system suspend handler, with a usage count equal to 1 or more.
> pm_runtime_force_suspend(), which decides whether to suspend the device based
> on the device pm state and not the usage count, will thus always pm suspend
> the device.
>
> Is the above analysis correct ?

Yes.

Although, genpd is not behaving as it should here. It must *not*
resume all devices, just because the domain is powered.

>
> Now I'm a bit lost as to what happens (and what should happen) at resume time.
> Does genpd and the PM core try to suspend the device after calling the
> driver's resume handler (pm_runtime_force_resume() in this case) under some
> conditions, or does the resume handler have the last word in deciding the PM
> runtime status of the device after system resume ?

The resume callback of genpd decides what will happen, as its a PM
domain and it sits on top of the hierarchy of a devices PM callbacks.

As you have realized, there are issues in genpd regarding the system
PM code, there have even been reports about it.

For example, genpd prevents subsystem/drivers from manage their
devices during system PM - when it start the system PM phase with the
domain in powered off state.

That's not an okay behaviour, as it can't know whether a device needs
to be used to serve a request after that point, causing the device to
be runtime resumed. As genpd prevents it to be suspended via system
PM, it will stay resumed during the system PM suspend phase, at least
that is my understanding and it seems like bad idea.

In general, the system PM code in genpd needs to be modernized. It
somewhat duplicating some code from the PM core and I think the code
can be greatly simplified.

I have started to work on this quite recently, once the first steps
are done I will consider optimizations, such as not runtime resuming
devices unnecessarily.

>
>> > leads to the device always being active in pm_runtime_force_suspend(). The
>> > usage count will be 1 if the device was suspended prior to entering system
>> > suspend (due to the pm_runtime_get_noresume() call in device_prepare()) or
>> > higher than 1 if the device was active.
>>
>> It's only greater than 1 - if someone else than the PM core has
>> increased the usage count.
>
> That I agree with.

Okay, great! :-)

[...]

Again, sorry for the giving the wrong input earlier!

Future wise, I will keep you on cc on any related genpd patches.

Kind regards
Uffe

Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended

[Laurent Pinchart]

Hi Ulf,

On Thursday 21 Apr 2016 15:52:06 Ulf Hansson wrote:
> On 21 April 2016 at 14:41, Laurent Pinchart wrote:
> > On Thursday 21 Apr 2016 11:10:19 Ulf Hansson wrote:
> >> On 21 April 2016 at 01:30, Laurent Pinchart wrote:
> >>> On Monday 07 Mar 2016 11:10:08 Ulf Hansson wrote:
> >>>> [...]
> >>>> 
> >>>>>> I agree, that's a better idea. Drivers shouldn't call
> >>>>>> pm_runtime_force_resume() if they haven't called
> >>>>>> pm_runtime_force_suspend(), so checking the PM use count should be
> >>>>>> fine. I'll modify the patch, test it and resubmit.
> >>>>> 
> >>>>> I gave it an unfortunately unsuccessful try. The problem I ran into
> >>>>> is that device_prepare() calls pm_runtime_get_noresume() calls
> >>>>> pm_runtime_get_noresume(), with the corresponding pm_runtime_put()
> >>>>> call being performed in device_complete(). The device power
> >>>>> usage_count is thus always non-zero in the system resume handler, so
> >>>>> I can't base the decision on that.
> >>>> 
> >>>> As Alan said, let's just check against 1 instead.
> >>> 
> >>> I gave this a try, and unfortunately it won't work.
> >>> 
> >>> pm_genpd_prepare() resumes devices without increasing the usage count,
> >>> which
> >> 
> >> It doesn't resume them, it only increases the usage count.
> > 
> > Maybe there's something I don't get, but pm_genpd_prepare() ends with
> 
> I realize that I did read *pm_genpd_prepare()* as *device_prepare()*,
> which represents the behaviour of the PM core during the prepare phase
> (drivers/base/power/main.c). In such cases my earlier reply would make
> more sense, I believe.
> 
> Apologize for giving you the wrong input by not reading your response
> in more detail!

No worries, thanks for taking the time to reply now.

> >     /*
> >      * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
> >      * so genpd_poweron() will return immediately, but if the device
> >      * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need
> >      * to make it operational.
> >      */
> >     pm_runtime_resume(dev);
> >     __pm_runtime_disable(dev, false);
> >         
> >     ret = pm_generic_prepare(dev);
> >     if (ret) {
> >             ... /* irrelevant error handling */
> >     }
> >         
> >     pm_runtime_put(dev);
> >     return ret;
> > 
> > The device is thus resumed. Note that the pm_runtime_put() call will
> > decrease the usage count that was increased at the beginning of the
> > function (and thus makes pm_genpd_prepare() neutral from a usage count
> > point of view) but won't resume the device as the disable depth is
> > increased by __pm_runtime_disable().
> > 
> > As far as I understand, the device is thus effectively active when
> > entering the driver's system suspend handler, with a usage count equal to
> > 1 or more. pm_runtime_force_suspend(), which decides whether to suspend
> > the device based on the device pm state and not the usage count, will thus
> > always pm suspend the device.
> > 
> > Is the above analysis correct ?
> 
> Yes.
> 
> Although, genpd is not behaving as it should here. It must *not*
> resume all devices, just because the domain is powered.

We agree on that.

> > Now I'm a bit lost as to what happens (and what should happen) at resume
> > time. Does genpd and the PM core try to suspend the device after calling
> > the driver's resume handler (pm_runtime_force_resume() in this case)
> > under some conditions, or does the resume handler have the last word in
> > deciding the PM runtime status of the device after system resume ?
> 
> The resume callback of genpd decides what will happen, as its a PM
> domain and it sits on top of the hierarchy of a devices PM callbacks.
> 
> As you have realized, there are issues in genpd regarding the system
> PM code, there have even been reports about it.
> 
> For example, genpd prevents subsystem/drivers from manage their
> devices during system PM - when it start the system PM phase with the
> domain in powered off state.
> 
> That's not an okay behaviour, as it can't know whether a device needs
> to be used to serve a request after that point, causing the device to
> be runtime resumed. As genpd prevents it to be suspended via system
> PM, it will stay resumed during the system PM suspend phase, at least
> that is my understanding and it seems like bad idea.
> 
> In general, the system PM code in genpd needs to be modernized. It
> somewhat duplicating some code from the PM core and I think the code
> can be greatly simplified.
> 
> I have started to work on this quite recently, once the first steps
> are done I will consider optimizations, such as not runtime resuming
> devices unnecessarily.
> 
> >>> leads to the device always being active in pm_runtime_force_suspend().
> >>> The usage count will be 1 if the device was suspended prior to entering
> >>> system suspend (due to the pm_runtime_get_noresume() call in
> >>> device_prepare()) or higher than 1 if the device was active.
> >> 
> >> It's only greater than 1 - if someone else than the PM core has
> >> increased the usage count.
> > 
> > That I agree with.
> 
> Okay, great! :-)
> 
> [...]
> 
> Again, sorry for the giving the wrong input earlier!
> 
> Future wise, I will keep you on cc on any related genpd patches.

Yeah \o/ more patches in my inbox, thank you so much ;-)

-- 
Regards,

Laurent Pinchart