Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1350129 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended Ulf Hansson <ulf.hansson@linaro.org> - 2016-03-04 11:40 +0100
    Re: [PATCH] PM / Runtime: Only force-resume device if it has been  force-suspended Alan Stern <stern@rowland.harvard.edu> - 2016-03-04 16:30 +0100
      Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-03-04 22:10 +0100
        Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2016-03-06 16:40 +0100
          Re: [PATCH] PM / Runtime: Only force-resume device if it has been  force-suspended Alan Stern <stern@rowland.harvard.edu> - 2016-03-06 18:00 +0100
          Re: [PATCH] PM / Runtime: Only force-resume device if it has been force-suspended Ulf Hansson <ulf.hansson@linaro.org> - 2016-03-07 11:20 +0100

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

+ 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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1350589

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

[toc] | [prev] | [next] | [standalone]

#1351141

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1351514

[...]

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web