| Message ID | 1457036214-26136-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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
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 :-)
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
+ 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
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
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.
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.
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.
[...] >> 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
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 :-)
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
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!
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
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 ;-)
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;
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(-)