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

Message ID 1461196375-21768-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Laurent Pinchart April 20, 2016, 11:52 p.m. UTC
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
manage 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, 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
unconditionally 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>
---
 drivers/base/power/runtime.c | 19 ++++++++++++++++---
 include/linux/pm.h           |  1 +
 2 files changed, 17 insertions(+), 3 deletions(-)

Changes since v1:

- Fix typos
- Protect the is_force_suspended flag modifications with power.lock

Comments

Rafael J. Wysocki April 21, 2016, 7:52 p.m. UTC | #1
On Thursday, April 21, 2016 02:52:55 AM 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
> manage 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, 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
> unconditionally 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>

Ulf, any comments?

> ---
>  drivers/base/power/runtime.c | 19 ++++++++++++++++---
>  include/linux/pm.h           |  1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> Changes since v1:
> 
> - Fix typos
> - Protect the is_force_suspended flag modifications with power.lock
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 4c7055009bd6..8fc7fba811fa 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;
> @@ -1457,6 +1458,7 @@ void pm_runtime_remove(struct device *dev)
>  int pm_runtime_force_suspend(struct device *dev)
>  {
>  	int (*callback)(struct device *);
> +	unsigned long flags;
>  	int ret = 0;
>  
>  	pm_runtime_disable(dev);
> @@ -1475,6 +1477,10 @@ int pm_runtime_force_suspend(struct device *dev)
>  		goto err;
>  
>  	pm_runtime_set_suspended(dev);
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	dev->power.is_force_suspended = true;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +
>  	return 0;
>  err:
>  	pm_runtime_enable(dev);
> @@ -1483,13 +1489,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-enable runtime PM.
>   *
>   * Typically this function may be invoked from a system resume callback to make
>   * sure the device is put into full power state.
> @@ -1497,8 +1503,12 @@ EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
>  int pm_runtime_force_resume(struct device *dev)
>  {
>  	int (*callback)(struct device *);
> +	unsigned long flags;
>  	int ret = 0;
>  
> +	if (!dev->power.is_force_suspended)
> +		goto out;
> +
>  	callback = RPM_GET_CALLBACK(dev, runtime_resume);
>  
>  	if (!callback) {
> @@ -1510,6 +1520,9 @@ int pm_runtime_force_resume(struct device *dev)
>  	if (ret)
>  		goto out;
>  
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	dev->power.is_force_suspended = false;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
>  	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;
>
Laurent Pinchart April 21, 2016, 8:57 p.m. UTC | #2
Hi Rafael,

On Thursday 21 Apr 2016 21:52:56 Rafael J. Wysocki wrote:
> On Thursday, April 21, 2016 02:52:55 AM 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
> > manage 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, 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
> > unconditionally 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>
> 
> Ulf, any comments?

Ulf has proposed a different approach in "[PATCH] PM / Runtime: Defer resuming 
of the device in pm_runtime_force_resume()". I agree that using usage_count is 
better than introducing a new state flag in struct dev_pm_info, with a caveat: 
it doesn't work properly :-). We would have to fix genpd first, as commented 
in a reply to Ulf's patch.

> > ---
> > 
> >  drivers/base/power/runtime.c | 19 ++++++++++++++++---
> >  include/linux/pm.h           |  1 +
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> > 
> > Changes since v1:
> > 
> > - Fix typos
> > - Protect the is_force_suspended flag modifications with power.lock
> > 
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 4c7055009bd6..8fc7fba811fa 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;
> > 
> > @@ -1457,6 +1458,7 @@ void pm_runtime_remove(struct device *dev)
> > 
> >  int pm_runtime_force_suspend(struct device *dev)
> >  {
> >  
> >  	int (*callback)(struct device *);
> > 
> > +	unsigned long flags;
> > 
> >  	int ret = 0;
> >  	
> >  	pm_runtime_disable(dev);
> > 
> > @@ -1475,6 +1477,10 @@ int pm_runtime_force_suspend(struct device *dev)
> > 
> >  		goto err;
> >  	
> >  	pm_runtime_set_suspended(dev);
> > 
> > +	spin_lock_irqsave(&dev->power.lock, flags);
> > +	dev->power.is_force_suspended = true;
> > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > +
> > 
> >  	return 0;
> >  
> >  err:
> >  	pm_runtime_enable(dev);
> > 
> > @@ -1483,13 +1489,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-enable
> > runtime PM.> 
> >   *
> >   * Typically this function may be invoked from a system resume callback
> >   to make * sure the device is put into full power state.
> > 
> > @@ -1497,8 +1503,12 @@ EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
> > 
> >  int pm_runtime_force_resume(struct device *dev)
> >  {
> >  
> >  	int (*callback)(struct device *);
> > 
> > +	unsigned long flags;
> > 
> >  	int ret = 0;
> > 
> > +	if (!dev->power.is_force_suspended)
> > +		goto out;
> > +
> > 
> >  	callback = RPM_GET_CALLBACK(dev, runtime_resume);
> >  	
> >  	if (!callback) {
> > 
> > @@ -1510,6 +1520,9 @@ int pm_runtime_force_resume(struct device *dev)
> > 
> >  	if (ret)
> >  	
> >  		goto out;
> > 
> > +	spin_lock_irqsave(&dev->power.lock, flags);
> > +	dev->power.is_force_suspended = false;
> > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > 
> >  	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;
Rafael J. Wysocki April 21, 2016, 9:02 p.m. UTC | #3
On Thu, Apr 21, 2016 at 10:57 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rafael,
>
> On Thursday 21 Apr 2016 21:52:56 Rafael J. Wysocki wrote:
>> On Thursday, April 21, 2016 02:52:55 AM 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
>> > manage 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, 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
>> > unconditionally 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>
>>
>> Ulf, any comments?
>
> Ulf has proposed a different approach in "[PATCH] PM / Runtime: Defer resuming
> of the device in pm_runtime_force_resume()". I agree that using usage_count is
> better than introducing a new state flag in struct dev_pm_info, with a caveat:
> it doesn't work properly :-). We would have to fix genpd first, as commented
> in a reply to Ulf's patch.

OK, thanks!

Since I'd prefer to avoid adding more state flags too, I'll let you
guys noodle around this for a while more. :-)
Laurent Pinchart April 21, 2016, 9:07 p.m. UTC | #4
Hi Rafael,

On Thursday 21 Apr 2016 23:02:06 Rafael J. Wysocki wrote:
> On Thu, Apr 21, 2016 at 10:57 PM, Laurent Pinchart wrote:
> > On Thursday 21 Apr 2016 21:52:56 Rafael J. Wysocki wrote:
> >> On Thursday, April 21, 2016 02:52:55 AM 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
> >>> manage 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, 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
> >>> unconditionally 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>
> >> 
> >> Ulf, any comments?
> > 
> > Ulf has proposed a different approach in "[PATCH] PM / Runtime: Defer
> > resuming of the device in pm_runtime_force_resume()". I agree that using
> > usage_count is better than introducing a new state flag in struct
> > dev_pm_info, with a caveat: it doesn't work properly :-). We would have
> > to fix genpd first, as commented in a reply to Ulf's patch.
> 
> OK, thanks!
> 
> Since I'd prefer to avoid adding more state flags too, I'll let you
> guys noodle around this for a while more. :-)

Let's see what we can do in a reasonable time frame. We could decide to merge 
this patch as a temporary fix until the genpd rework is complete.
Ulf Hansson April 22, 2016, 7:15 a.m. UTC | #5
On 21 April 2016 at 23:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Rafael,
>
> On Thursday 21 Apr 2016 23:02:06 Rafael J. Wysocki wrote:
>> On Thu, Apr 21, 2016 at 10:57 PM, Laurent Pinchart wrote:
>> > On Thursday 21 Apr 2016 21:52:56 Rafael J. Wysocki wrote:
>> >> On Thursday, April 21, 2016 02:52:55 AM 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
>> >>> manage 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, 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
>> >>> unconditionally 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>
>> >>
>> >> Ulf, any comments?
>> >
>> > Ulf has proposed a different approach in "[PATCH] PM / Runtime: Defer
>> > resuming of the device in pm_runtime_force_resume()". I agree that using
>> > usage_count is better than introducing a new state flag in struct
>> > dev_pm_info, with a caveat: it doesn't work properly :-). We would have
>> > to fix genpd first, as commented in a reply to Ulf's patch.
>>
>> OK, thanks!
>>
>> Since I'd prefer to avoid adding more state flags too, I'll let you
>> guys noodle around this for a while more. :-)
>
> Let's see what we can do in a reasonable time frame. We could decide to merge
> this patch as a temporary fix until the genpd rework is complete.

Subsystems/driver that uses pm_runtime_force_suspend|resume() don't
necessarily need to have their devices attached to a genpd. In such
cases, $subject patch will be an improvement by itself.

Me personally would rather skip the intermediate step you propose, as
I prefer to properly change genpd with what is needed. Moreover, I am
already working on that so it shouldn't take too long before I can
post some patches.

Kind regards
Uffe

Patch
diff mbox

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 4c7055009bd6..8fc7fba811fa 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;
@@ -1457,6 +1458,7 @@  void pm_runtime_remove(struct device *dev)
 int pm_runtime_force_suspend(struct device *dev)
 {
 	int (*callback)(struct device *);
+	unsigned long flags;
 	int ret = 0;
 
 	pm_runtime_disable(dev);
@@ -1475,6 +1477,10 @@  int pm_runtime_force_suspend(struct device *dev)
 		goto err;
 
 	pm_runtime_set_suspended(dev);
+	spin_lock_irqsave(&dev->power.lock, flags);
+	dev->power.is_force_suspended = true;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
 	return 0;
 err:
 	pm_runtime_enable(dev);
@@ -1483,13 +1489,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-enable runtime PM.
  *
  * Typically this function may be invoked from a system resume callback to make
  * sure the device is put into full power state.
@@ -1497,8 +1503,12 @@  EXPORT_SYMBOL_GPL(pm_runtime_force_suspend);
 int pm_runtime_force_resume(struct device *dev)
 {
 	int (*callback)(struct device *);
+	unsigned long flags;
 	int ret = 0;
 
+	if (!dev->power.is_force_suspended)
+		goto out;
+
 	callback = RPM_GET_CALLBACK(dev, runtime_resume);
 
 	if (!callback) {
@@ -1510,6 +1520,9 @@  int pm_runtime_force_resume(struct device *dev)
 	if (ret)
 		goto out;
 
+	spin_lock_irqsave(&dev->power.lock, flags);
+	dev->power.is_force_suspended = false;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
 	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;