[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

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 of complexity, the device was always resumed in
pm_runtime_force_resume().

More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
unset, we needed to resume the device as the subsystem/driver couldn't
rely on using runtime PM to do it.

As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
removed this combination, of using CONFIG_PM_SLEEP without the earlier
CONFIG_PM_RUNTIME.

For this reason we can now rely on the subsystem/driver to use runtime PM
to resume the device, instead of forcing that to be done in all cases. In
other words, let's defer this to a later point when it's actually needed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Note, this patch is based upon another not yet queued patch [1]. The reason is
simply because that [1] is a more important patch as it fixes a problem. It was
posted to linux-pm April 8th and I expect it (or a new revision of it) to be
applied before $subject patch.

[1]
https://patchwork.kernel.org/patch/8782851

---
 drivers/base/power/runtime.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b746904..a190ca0 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
 		goto out;
 	}
 
+	/*
+	 * The PM core increases the runtime PM usage count in the system PM
+	 * prepare phase. If the count is greather than 1 at this point, someone
+	 * else has also increased it. In such case, let's make sure to runtime
+	 * resume the device as that is likely what is expected. In other case
+	 * we trust the subsystem/driver to runtime resume the device when it's
+	 * actually needed.
+	 */
+	if (atomic_read(&dev->power.usage_count) < 2)
+		goto out;
+
 	ret = pm_runtime_set_active(dev);
 	if (ret)
 		goto out;
-- 
1.9.1

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Laurent Pinchart]

Hi Ulf,

Thank you for the patch.

On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
> 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 of complexity, the device was always resumed in
> pm_runtime_force_resume().
> 
> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> unset, we needed to resume the device as the subsystem/driver couldn't
> rely on using runtime PM to do it.
> 
> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> CONFIG_PM_RUNTIME.
> 
> For this reason we can now rely on the subsystem/driver to use runtime PM
> to resume the device, instead of forcing that to be done in all cases. In
> other words, let's defer this to a later point when it's actually needed.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Note, this patch is based upon another not yet queued patch [1]. The reason
> is simply because that [1] is a more important patch as it fixes a problem.
> It was posted to linux-pm April 8th and I expect it (or a new revision of
> it) to be applied before $subject patch.
> 
> [1]
> https://patchwork.kernel.org/patch/8782851
> 
> ---
>  drivers/base/power/runtime.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b746904..a190ca0 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>  		goto out;
>  	}
> 
> +	/*
> +	 * The PM core increases the runtime PM usage count in the system PM
> +	 * prepare phase. If the count is greather than 1 at this point, someone
> +	 * else has also increased it. In such case, let's make sure to runtime
> +	 * resume the device as that is likely what is expected. In other case
> +	 * we trust the subsystem/driver to runtime resume the device when it's
> +	 * actually needed.
> +	 */
> +	if (atomic_read(&dev->power.usage_count) < 2)
> +		goto out;
> +
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
>  		goto out;

This works in the sense that it prevents devices from being PM resumed at 
system resume time if not needed. However, devices that are part of a PM 
domain and that were idle before system suspend are suspended twice (with 
their .runtime_suspend() handler called twice), which is not good at all.

The first suspend occurs at system suspend time, with 
pm_runtime_force_suspend() rightfully suspending the device as the device is 
active (due to being woken up by pm_genpd_prepare()). The second suspend 
occurs at resume time due to device_complete() calling pm_runtime_put().

I've tracked the issue to the fact that pm_genpd_complete() calls 
pm_runtime_set_active() regardless of whether the device was PM resumed or 
not. As pm_runtime_force_suspend() doesn't resume devices with this patch 
applied, the pm_runtime_put() call from device_complete() will try to runtime 
suspend the device a second time as the state is incorrectly set to 
RPM_ACTIVE.

With the current genpd implementation this patch isn't needed (and neither is 
my patch), as genpd expects the device to be always active when the system is 
resumed. However, when genpd isn't used, pm_runtime_force_resume() needs to 
skip resuming devices that were suspended before system suspend. This patch 
looks good to me to fix that problem.

Do we need to fix genpd first ?

-- 
Regards,

Laurent Pinchart

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Laurent Pinchart]

On Thursday 21 Apr 2016 20:31:52 Laurent Pinchart wrote:
> Hi Ulf,
> 
> Thank you for the patch.
> 
> On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
> > 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 of complexity, the device was always resumed in
> > pm_runtime_force_resume().
> > 
> > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> > unset, we needed to resume the device as the subsystem/driver couldn't
> > rely on using runtime PM to do it.
> > 
> > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> > removed this combination, of using CONFIG_PM_SLEEP without the earlier
> > CONFIG_PM_RUNTIME.
> > 
> > For this reason we can now rely on the subsystem/driver to use runtime PM
> > to resume the device, instead of forcing that to be done in all cases. In
> > other words, let's defer this to a later point when it's actually needed.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> > 
> > Note, this patch is based upon another not yet queued patch [1]. The
> > reason
> > is simply because that [1] is a more important patch as it fixes a
> > problem.
> > It was posted to linux-pm April 8th and I expect it (or a new revision of
> > it) to be applied before $subject patch.
> > 
> > [1]
> > https://patchwork.kernel.org/patch/8782851
> > 
> > ---
> > 
> >  drivers/base/power/runtime.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index b746904..a190ca0 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
> > 
> >  		goto out;
> >  	
> >  	}
> > 
> > +	/*
> > +	 * The PM core increases the runtime PM usage count in the system PM
> > +	 * prepare phase. If the count is greather than 1 at this point, 
someone
> > +	 * else has also increased it. In such case, let's make sure to runtime
> > +	 * resume the device as that is likely what is expected. In other case
> > +	 * we trust the subsystem/driver to runtime resume the device when it's
> > +	 * actually needed.
> > +	 */
> > +	if (atomic_read(&dev->power.usage_count) < 2)
> > +		goto out;
> > +
> > 
> >  	ret = pm_runtime_set_active(dev);
> >  	if (ret)
> >  	
> >  		goto out;
> 
> This works in the sense that it prevents devices from being PM resumed at
> system resume time if not needed. However, devices that are part of a PM
> domain and that were idle before system suspend are suspended twice (with
> their .runtime_suspend() handler called twice), which is not good at all.
> 
> The first suspend occurs at system suspend time, with
> pm_runtime_force_suspend() rightfully suspending the device as the device is
> active (due to being woken up by pm_genpd_prepare()). The second suspend
> occurs at resume time due to device_complete() calling pm_runtime_put().
> 
> I've tracked the issue to the fact that pm_genpd_complete() calls
> pm_runtime_set_active() regardless of whether the device was PM resumed or
> not. As pm_runtime_force_suspend() doesn't resume devices with this patch
> applied, the pm_runtime_put() call from device_complete() will try to
> runtime suspend the device a second time as the state is incorrectly set to
> RPM_ACTIVE.
> 
> With the current genpd implementation this patch isn't needed (and neither
> is my patch), as genpd expects the device to be always active when the
> system is resumed. However, when genpd isn't used,
> pm_runtime_force_resume() needs to skip resuming devices that were
> suspended before system suspend. This patch looks good to me to fix that
> problem.
>
> Do we need to fix genpd first ?

And for the record, while this patch would require fixing genpd first, "[PATCH 
v2] PM / Runtime: Only force-resume device if it has been force-suspended" 
doesn't (at least as far as I understand the problem).

-- 
Regards,

Laurent Pinchart

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Kevin Hilman]

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

> On Thursday 21 Apr 2016 20:31:52 Laurent Pinchart wrote:
>> Hi Ulf,
>> 
>> Thank you for the patch.
>> 
>> On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
>> > 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 of complexity, the device was always resumed in
>> > pm_runtime_force_resume().
>> > 
>> > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> > unset, we needed to resume the device as the subsystem/driver couldn't
>> > rely on using runtime PM to do it.
>> > 
>> > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> > removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> > CONFIG_PM_RUNTIME.
>> > 
>> > For this reason we can now rely on the subsystem/driver to use runtime PM
>> > to resume the device, instead of forcing that to be done in all cases. In
>> > other words, let's defer this to a later point when it's actually needed.
>> > 
>> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> > ---
>> > 
>> > Note, this patch is based upon another not yet queued patch [1]. The
>> > reason
>> > is simply because that [1] is a more important patch as it fixes a
>> > problem.
>> > It was posted to linux-pm April 8th and I expect it (or a new revision of
>> > it) to be applied before $subject patch.
>> > 
>> > [1]
>> > https://patchwork.kernel.org/patch/8782851
>> > 
>> > ---
>> > 
>> >  drivers/base/power/runtime.c | 11 +++++++++++
>> >  1 file changed, 11 insertions(+)
>> > 
>> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> > index b746904..a190ca0 100644
>> > --- a/drivers/base/power/runtime.c
>> > +++ b/drivers/base/power/runtime.c
>> > @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>> > 
>> >  		goto out;
>> >  	
>> >  	}
>> > 
>> > +	/*
>> > +	 * The PM core increases the runtime PM usage count in the system PM
>> > +	 * prepare phase. If the count is greather than 1 at this point, 
> someone
>> > +	 * else has also increased it. In such case, let's make sure to runtime
>> > +	 * resume the device as that is likely what is expected. In other case
>> > +	 * we trust the subsystem/driver to runtime resume the device when it's
>> > +	 * actually needed.
>> > +	 */
>> > +	if (atomic_read(&dev->power.usage_count) < 2)
>> > +		goto out;
>> > +
>> > 
>> >  	ret = pm_runtime_set_active(dev);
>> >  	if (ret)
>> >  	
>> >  		goto out;
>> 
>> This works in the sense that it prevents devices from being PM resumed at
>> system resume time if not needed. However, devices that are part of a PM
>> domain and that were idle before system suspend are suspended twice (with
>> their .runtime_suspend() handler called twice), which is not good at all.
>> 
>> The first suspend occurs at system suspend time, with
>> pm_runtime_force_suspend() rightfully suspending the device as the device is
>> active (due to being woken up by pm_genpd_prepare()). The second suspend
>> occurs at resume time due to device_complete() calling pm_runtime_put().
>> 
>> I've tracked the issue to the fact that pm_genpd_complete() calls
>> pm_runtime_set_active() regardless of whether the device was PM resumed or
>> not. As pm_runtime_force_suspend() doesn't resume devices with this patch
>> applied, the pm_runtime_put() call from device_complete() will try to
>> runtime suspend the device a second time as the state is incorrectly set to
>> RPM_ACTIVE.
>> 
>> With the current genpd implementation this patch isn't needed (and neither
>> is my patch), as genpd expects the device to be always active when the
>> system is resumed. However, when genpd isn't used,
>> pm_runtime_force_resume() needs to skip resuming devices that were
>> suspended before system suspend. This patch looks good to me to fix that
>> problem.
>>
>> Do we need to fix genpd first ?
>
> And for the record, while this patch would require fixing genpd first, "[PATCH 
> v2] PM / Runtime: Only force-resume device if it has been force-suspended" 
> doesn't (at least as far as I understand the problem).

Right, I'm thinking we should merge Laurent's patch first.  It fixes a
current problem, and won't get in the way of doing the genpd
improvements progressively.

Kevin

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 22 April 2016 at 22:27, Kevin Hilman <khilman@baylibre.com> wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> On Thursday 21 Apr 2016 20:31:52 Laurent Pinchart wrote:
>>> Hi Ulf,
>>>
>>> Thank you for the patch.
>>>
>>> On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
>>> > 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 of complexity, the device was always resumed in
>>> > pm_runtime_force_resume().
>>> >
>>> > More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>>> > unset, we needed to resume the device as the subsystem/driver couldn't
>>> > rely on using runtime PM to do it.
>>> >
>>> > As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>>> > removed this combination, of using CONFIG_PM_SLEEP without the earlier
>>> > CONFIG_PM_RUNTIME.
>>> >
>>> > For this reason we can now rely on the subsystem/driver to use runtime PM
>>> > to resume the device, instead of forcing that to be done in all cases. In
>>> > other words, let's defer this to a later point when it's actually needed.
>>> >
>>> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> > ---
>>> >
>>> > Note, this patch is based upon another not yet queued patch [1]. The
>>> > reason
>>> > is simply because that [1] is a more important patch as it fixes a
>>> > problem.
>>> > It was posted to linux-pm April 8th and I expect it (or a new revision of
>>> > it) to be applied before $subject patch.
>>> >
>>> > [1]
>>> > https://patchwork.kernel.org/patch/8782851
>>> >
>>> > ---
>>> >
>>> >  drivers/base/power/runtime.c | 11 +++++++++++
>>> >  1 file changed, 11 insertions(+)
>>> >
>>> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>> > index b746904..a190ca0 100644
>>> > --- a/drivers/base/power/runtime.c
>>> > +++ b/drivers/base/power/runtime.c
>>> > @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>>> >
>>> >            goto out;
>>> >
>>> >    }
>>> >
>>> > +  /*
>>> > +   * The PM core increases the runtime PM usage count in the system PM
>>> > +   * prepare phase. If the count is greather than 1 at this point,
>> someone
>>> > +   * else has also increased it. In such case, let's make sure to runtime
>>> > +   * resume the device as that is likely what is expected. In other case
>>> > +   * we trust the subsystem/driver to runtime resume the device when it's
>>> > +   * actually needed.
>>> > +   */
>>> > +  if (atomic_read(&dev->power.usage_count) < 2)
>>> > +          goto out;
>>> > +
>>> >
>>> >    ret = pm_runtime_set_active(dev);
>>> >    if (ret)
>>> >
>>> >            goto out;
>>>
>>> This works in the sense that it prevents devices from being PM resumed at
>>> system resume time if not needed. However, devices that are part of a PM
>>> domain and that were idle before system suspend are suspended twice (with
>>> their .runtime_suspend() handler called twice), which is not good at all.
>>>
>>> The first suspend occurs at system suspend time, with
>>> pm_runtime_force_suspend() rightfully suspending the device as the device is
>>> active (due to being woken up by pm_genpd_prepare()). The second suspend
>>> occurs at resume time due to device_complete() calling pm_runtime_put().
>>>
>>> I've tracked the issue to the fact that pm_genpd_complete() calls
>>> pm_runtime_set_active() regardless of whether the device was PM resumed or
>>> not. As pm_runtime_force_suspend() doesn't resume devices with this patch
>>> applied, the pm_runtime_put() call from device_complete() will try to
>>> runtime suspend the device a second time as the state is incorrectly set to
>>> RPM_ACTIVE.
>>>
>>> With the current genpd implementation this patch isn't needed (and neither
>>> is my patch), as genpd expects the device to be always active when the
>>> system is resumed. However, when genpd isn't used,
>>> pm_runtime_force_resume() needs to skip resuming devices that were
>>> suspended before system suspend. This patch looks good to me to fix that
>>> problem.
>>>
>>> Do we need to fix genpd first ?
>>
>> And for the record, while this patch would require fixing genpd first, "[PATCH
>> v2] PM / Runtime: Only force-resume device if it has been force-suspended"
>> doesn't (at least as far as I understand the problem).
>
> Right, I'm thinking we should merge Laurent's patch first.  It fixes a
> current problem, and won't get in the way of doing the genpd
> improvements progressively.

I believe I share Rafael's opinion, trying to avoid to add more state
flags to the PM core, unless really needed.

Now, even if we decide to pick up something like Laurent's "[PATCH V2
PM / Runtime: Only force-resume device if it has been
force-suspended", $subject patch is needed to enable further
improvements, don't you think?

Kind regards
Uffe

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 21 April 2016 at 19:31, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ulf,
>
> Thank you for the patch.
>
> On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
>> 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 of complexity, the device was always resumed in
>> pm_runtime_force_resume().
>>
>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> unset, we needed to resume the device as the subsystem/driver couldn't
>> rely on using runtime PM to do it.
>>
>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> CONFIG_PM_RUNTIME.
>>
>> For this reason we can now rely on the subsystem/driver to use runtime PM
>> to resume the device, instead of forcing that to be done in all cases. In
>> other words, let's defer this to a later point when it's actually needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> Note, this patch is based upon another not yet queued patch [1]. The reason
>> is simply because that [1] is a more important patch as it fixes a problem.
>> It was posted to linux-pm April 8th and I expect it (or a new revision of
>> it) to be applied before $subject patch.
>>
>> [1]
>> https://patchwork.kernel.org/patch/8782851
>>
>> ---
>>  drivers/base/power/runtime.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index b746904..a190ca0 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>>               goto out;
>>       }
>>
>> +     /*
>> +      * The PM core increases the runtime PM usage count in the system PM
>> +      * prepare phase. If the count is greather than 1 at this point, someone
>> +      * else has also increased it. In such case, let's make sure to runtime
>> +      * resume the device as that is likely what is expected. In other case
>> +      * we trust the subsystem/driver to runtime resume the device when it's
>> +      * actually needed.
>> +      */
>> +     if (atomic_read(&dev->power.usage_count) < 2)
>> +             goto out;
>> +
>>       ret = pm_runtime_set_active(dev);
>>       if (ret)
>>               goto out;
>
> This works in the sense that it prevents devices from being PM resumed at
> system resume time if not needed. However, devices that are part of a PM
> domain and that were idle before system suspend are suspended twice (with
> their .runtime_suspend() handler called twice), which is not good at all.
>
> The first suspend occurs at system suspend time, with
> pm_runtime_force_suspend() rightfully suspending the device as the device is
> active (due to being woken up by pm_genpd_prepare()). The second suspend
> occurs at resume time due to device_complete() calling pm_runtime_put().
>
> I've tracked the issue to the fact that pm_genpd_complete() calls
> pm_runtime_set_active() regardless of whether the device was PM resumed or
> not. As pm_runtime_force_suspend() doesn't resume devices with this patch
> applied, the pm_runtime_put() call from device_complete() will try to runtime
> suspend the device a second time as the state is incorrectly set to
> RPM_ACTIVE.
>
> With the current genpd implementation this patch isn't needed (and neither is
> my patch), as genpd expects the device to be always active when the system is
> resumed. However, when genpd isn't used, pm_runtime_force_resume() needs to
> skip resuming devices that were suspended before system suspend. This patch
> looks good to me to fix that problem.
>
> Do we need to fix genpd first ?

Following you reasoning, I agree!

Let's put this patch on hold for a little while. I am already working
on changing genpd, so it shouldn't take long before I can post some
additional genpd patches improving the behaviour.

Kind regards
Uffe

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Laurent Pinchart]

Hi Ulf,

On Monday 25 Apr 2016 15:32:52 Ulf Hansson wrote:
> On 21 April 2016 at 19:31, Laurent Pinchart wrote:
> > On Thursday 21 Apr 2016 12:34:02 Ulf Hansson wrote:
> >> 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 of complexity, the device was always resumed in
> >> pm_runtime_force_resume().
> >> 
> >> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> >> unset, we needed to resume the device as the subsystem/driver couldn't
> >> rely on using runtime PM to do it.
> >> 
> >> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> >> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> >> CONFIG_PM_RUNTIME.
> >> 
> >> For this reason we can now rely on the subsystem/driver to use runtime PM
> >> to resume the device, instead of forcing that to be done in all cases. In
> >> other words, let's defer this to a later point when it's actually needed.
> >> 
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> ---
> >> 
> >> Note, this patch is based upon another not yet queued patch [1]. The
> >> reason is simply because that [1] is a more important patch as it fixes a
> >> problem. It was posted to linux-pm April 8th and I expect it (or a new
> >> revision of it) to be applied before $subject patch.
> >> 
> >> [1]
> >> https://patchwork.kernel.org/patch/8782851
> >> 
> >> ---
> >> 
> >>  drivers/base/power/runtime.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >> 
> >> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> >> index b746904..a190ca0 100644
> >> --- a/drivers/base/power/runtime.c
> >> +++ b/drivers/base/power/runtime.c
> >> @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
> >>               goto out;
> >>       }
> >> 
> >> +     /*
> >> +      * The PM core increases the runtime PM usage count in the system
> >> +      * PM prepare phase. If the count is greather than 1 at this point,
> >> +      * someone else has also increased it. In such case, let's make
> >> +      * sure to runtime resume the device as that is likely what is
> >> +      * expected. In other case we trust the subsystem/driver to runtime
> >> +      * resume the device when it's actually needed.
> >> +      */
> >> +     if (atomic_read(&dev->power.usage_count) < 2)
> >> +             goto out;
> >> +
> >>       ret = pm_runtime_set_active(dev);
> >>       if (ret)
> >>               goto out;
> > 
> > This works in the sense that it prevents devices from being PM resumed at
> > system resume time if not needed. However, devices that are part of a PM
> > domain and that were idle before system suspend are suspended twice (with
> > their .runtime_suspend() handler called twice), which is not good at all.
> > 
> > The first suspend occurs at system suspend time, with
> > pm_runtime_force_suspend() rightfully suspending the device as the device
> > is active (due to being woken up by pm_genpd_prepare()). The second
> > suspend occurs at resume time due to device_complete() calling
> > pm_runtime_put().
> > 
> > I've tracked the issue to the fact that pm_genpd_complete() calls
> > pm_runtime_set_active() regardless of whether the device was PM resumed or
> > not. As pm_runtime_force_suspend() doesn't resume devices with this patch
> > applied, the pm_runtime_put() call from device_complete() will try to
> > runtime suspend the device a second time as the state is incorrectly set
> > to RPM_ACTIVE.
> > 
> > With the current genpd implementation this patch isn't needed (and neither
> > is my patch), as genpd expects the device to be always active when the
> > system is resumed. However, when genpd isn't used,
> > pm_runtime_force_resume() needs to skip resuming devices that were
> > suspended before system suspend. This patch looks good to me to fix that
> > problem.
> > 
> > Do we need to fix genpd first ?
> 
> Following you reasoning, I agree!
> 
> Let's put this patch on hold for a little while. I am already working
> on changing genpd, so it shouldn't take long before I can post some
> additional genpd patches improving the behaviour.

I'd like to see something merged for v4.7 if possible. I agree that my patch 
isn't a long term solution (we want to avoid adding additional fields to the 
device power structure), but it has the benefit of being available now and 
fixing the problem I ran into with drivers that would be broken on v4.7 
without a fix. Do you think you could get a better fix ready in time for v4.7 
? If so I'm fine with dropping this patch, but otherwise I'd prefer to get it 
merged and reverted as part of your better implementation for v4.8.

-- 
Regards,

Laurent Pinchart

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

[...]

>
>> Following you reasoning, I agree!
>>
>> Let's put this patch on hold for a little while. I am already working
>> on changing genpd, so it shouldn't take long before I can post some
>> additional genpd patches improving the behaviour.
>
> I'd like to see something merged for v4.7 if possible. I agree that my patch
> isn't a long term solution (we want to avoid adding additional fields to the
> device power structure), but it has the benefit of being available now and
> fixing the problem I ran into with drivers that would be broken on v4.7
> without a fix. Do you think you could get a better fix ready in time for v4.7
> ? If so I'm fine with dropping this patch, but otherwise I'd prefer to get it
> merged and reverted as part of your better implementation for v4.8.

My impression was that devices becomes unnecessary resumed when they
don't need to. They won't stay resumed as the PM core invokes
pm_runtime_put() in the system PM complete phase.

So, in the end I think we are trying to optimize a behaviour here, but
not fix something that is "broken", correct?

Anyway, I have no objections to your proposed solution, so I leave it
to Rafael and Kevin to decide what to do.

>From my side I will continue with the improvements for the system PM
support in genpd.

Kind regards
Uffe

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Laurent Pinchart]

Hello,

On Wednesday 27 Apr 2016 16:23:49 Ulf Hansson wrote:
> [...]
> 
> >> Following you reasoning, I agree!
> >> 
> >> Let's put this patch on hold for a little while. I am already working
> >> on changing genpd, so it shouldn't take long before I can post some
> >> additional genpd patches improving the behaviour.
> > 
> > I'd like to see something merged for v4.7 if possible. I agree that my
> > patch isn't a long term solution (we want to avoid adding additional
> > fields to the device power structure), but it has the benefit of being
> > available now and fixing the problem I ran into with drivers that would
> > be broken on v4.7 without a fix. Do you think you could get a better fix
> > ready in time for v4.7 ? If so I'm fine with dropping this patch, but
> > otherwise I'd prefer to get it merged and reverted as part of your better
> > implementation for v4.8.
> 
> My impression was that devices becomes unnecessary resumed when they
> don't need to. They won't stay resumed as the PM core invokes
> pm_runtime_put() in the system PM complete phase.
> 
> So, in the end I think we are trying to optimize a behaviour here, but
> not fix something that is "broken", correct?
> 
> Anyway, I have no objections to your proposed solution, so I leave it
> to Rafael and Kevin to decide what to do.

Kevin, Rafael, any comment ? I need to fix PM support in a driver that is 
currently broken partly due to this issue. Which of "PM / Runtime: Only force-
resume device if it has been force-suspended" and this patch should we merge, 
if any ?

> From my side I will continue with the improvements for the system PM
> support in genpd.

Thanks.

-- 
Regards,

Laurent Pinchart

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Rafael J. Wysocki]

On Thu, May 12, 2016 at 9:01 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hello,
>
> On Wednesday 27 Apr 2016 16:23:49 Ulf Hansson wrote:
>> [...]
>>
>> >> Following you reasoning, I agree!
>> >>
>> >> Let's put this patch on hold for a little while. I am already working
>> >> on changing genpd, so it shouldn't take long before I can post some
>> >> additional genpd patches improving the behaviour.
>> >
>> > I'd like to see something merged for v4.7 if possible. I agree that my
>> > patch isn't a long term solution (we want to avoid adding additional
>> > fields to the device power structure), but it has the benefit of being
>> > available now and fixing the problem I ran into with drivers that would
>> > be broken on v4.7 without a fix. Do you think you could get a better fix
>> > ready in time for v4.7 ? If so I'm fine with dropping this patch, but
>> > otherwise I'd prefer to get it merged and reverted as part of your better
>> > implementation for v4.8.
>>
>> My impression was that devices becomes unnecessary resumed when they
>> don't need to. They won't stay resumed as the PM core invokes
>> pm_runtime_put() in the system PM complete phase.
>>
>> So, in the end I think we are trying to optimize a behaviour here, but
>> not fix something that is "broken", correct?
>>
>> Anyway, I have no objections to your proposed solution, so I leave it
>> to Rafael and Kevin to decide what to do.
>
> Kevin, Rafael, any comment ? I need to fix PM support in a driver that is
> currently broken partly due to this issue. Which of "PM / Runtime: Only force-
> resume device if it has been force-suspended" and this patch should we merge,
> if any ?

My and Kevin's assumption was that Ulf would provide a better fix
shortly, but I'm not sure how realistic that is.

Ulf?

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 12 May 2016 at 22:28, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, May 12, 2016 at 9:01 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hello,
>>
>> On Wednesday 27 Apr 2016 16:23:49 Ulf Hansson wrote:
>>> [...]
>>>
>>> >> Following you reasoning, I agree!
>>> >>
>>> >> Let's put this patch on hold for a little while. I am already working
>>> >> on changing genpd, so it shouldn't take long before I can post some
>>> >> additional genpd patches improving the behaviour.
>>> >
>>> > I'd like to see something merged for v4.7 if possible. I agree that my
>>> > patch isn't a long term solution (we want to avoid adding additional
>>> > fields to the device power structure), but it has the benefit of being
>>> > available now and fixing the problem I ran into with drivers that would
>>> > be broken on v4.7 without a fix. Do you think you could get a better fix
>>> > ready in time for v4.7 ? If so I'm fine with dropping this patch, but
>>> > otherwise I'd prefer to get it merged and reverted as part of your better
>>> > implementation for v4.8.
>>>
>>> My impression was that devices becomes unnecessary resumed when they
>>> don't need to. They won't stay resumed as the PM core invokes
>>> pm_runtime_put() in the system PM complete phase.
>>>
>>> So, in the end I think we are trying to optimize a behaviour here, but
>>> not fix something that is "broken", correct?
>>>
>>> Anyway, I have no objections to your proposed solution, so I leave it
>>> to Rafael and Kevin to decide what to do.
>>
>> Kevin, Rafael, any comment ? I need to fix PM support in a driver that is
>> currently broken partly due to this issue. Which of "PM / Runtime: Only force-
>> resume device if it has been force-suspended" and this patch should we merge,
>> if any ?
>
> My and Kevin's assumption was that Ulf would provide a better fix
> shortly, but I'm not sure how realistic that is.

First, is this really a fix we are talking about or an optimization? Laurent?

Second, I currently have patches for genpd etc that I think may deals
with this better, although I need a day or two before I can post them.

Additionally, "[PATCH v3 1/2] PM / Domains: Allow genpd to power on
during the system PM phases", has been posted [1] already.
That change, will prevent Laurent's approach to work and it's because
genpd will force a runtime resume of the device in the system PM
prepare phase. This means pm_genpd_force_resume() will anyway *always*
resume the device.

Kind regards
Uffe

[1]
http://www.spinics.net/lists/dri-devel/msg107210.html

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Rafael J. Wysocki]

On Thursday, April 21, 2016 12:34:02 PM Ulf Hansson wrote:
> 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 of complexity, the device was always resumed in
> pm_runtime_force_resume().
> 
> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
> unset, we needed to resume the device as the subsystem/driver couldn't
> rely on using runtime PM to do it.
> 
> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
> removed this combination, of using CONFIG_PM_SLEEP without the earlier
> CONFIG_PM_RUNTIME.
> 
> For this reason we can now rely on the subsystem/driver to use runtime PM
> to resume the device, instead of forcing that to be done in all cases. In
> other words, let's defer this to a later point when it's actually needed.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Note, this patch is based upon another not yet queued patch [1]. The reason is
> simply because that [1] is a more important patch as it fixes a problem. It was
> posted to linux-pm April 8th and I expect it (or a new revision of it) to be
> applied before $subject patch.
> 
> [1]
> https://patchwork.kernel.org/patch/8782851
> 
> ---
>  drivers/base/power/runtime.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b746904..a190ca0 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>  		goto out;
>  	}
>  
> +	/*
> +	 * The PM core increases the runtime PM usage count in the system PM
> +	 * prepare phase. If the count is greather than 1 at this point, someone
> +	 * else has also increased it. In such case, let's make sure to runtime
> +	 * resume the device as that is likely what is expected. In other case

That doesn't sound quite right.  I'd rather say "In that case, invoke the
runtime resume callback for the device as that is likely what is expected" here.

> +	 * we trust the subsystem/driver to runtime resume the device when it's
> +	 * actually needed.
> +	 */
> +	if (atomic_read(&dev->power.usage_count) < 2)
> +		goto out;
> +
>  	ret = pm_runtime_set_active(dev);
>  	if (ret)
>  		goto out;
> 

Thanks,
Rafael

[PATCH] PM / Runtime: Defer resuming of the device in pm_runtime_force_resume()

[Ulf Hansson]

On 21 April 2016 at 21:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, April 21, 2016 12:34:02 PM Ulf Hansson wrote:
>> 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 of complexity, the device was always resumed in
>> pm_runtime_force_resume().
>>
>> More precisely, when CONFIG_PM_SLEEP was set and CONFIG_PM_RUNTIME was
>> unset, we needed to resume the device as the subsystem/driver couldn't
>> rely on using runtime PM to do it.
>>
>> As the CONFIG_PM_RUNTIME option was merged into CONFIG_PM a while ago, it
>> removed this combination, of using CONFIG_PM_SLEEP without the earlier
>> CONFIG_PM_RUNTIME.
>>
>> For this reason we can now rely on the subsystem/driver to use runtime PM
>> to resume the device, instead of forcing that to be done in all cases. In
>> other words, let's defer this to a later point when it's actually needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> Note, this patch is based upon another not yet queued patch [1]. The reason is
>> simply because that [1] is a more important patch as it fixes a problem. It was
>> posted to linux-pm April 8th and I expect it (or a new revision of it) to be
>> applied before $subject patch.
>>
>> [1]
>> https://patchwork.kernel.org/patch/8782851
>>
>> ---
>>  drivers/base/power/runtime.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>> index b746904..a190ca0 100644
>> --- a/drivers/base/power/runtime.c
>> +++ b/drivers/base/power/runtime.c
>> @@ -1506,6 +1506,17 @@ int pm_runtime_force_resume(struct device *dev)
>>               goto out;
>>       }
>>
>> +     /*
>> +      * The PM core increases the runtime PM usage count in the system PM
>> +      * prepare phase. If the count is greather than 1 at this point, someone
>> +      * else has also increased it. In such case, let's make sure to runtime
>> +      * resume the device as that is likely what is expected. In other case
>
> That doesn't sound quite right.  I'd rather say "In that case, invoke the
> runtime resume callback for the device as that is likely what is expected" here.

I definitely agree, let me send a respin in a while.

Besides this, I assume you are happy with the approach?

>
>> +      * we trust the subsystem/driver to runtime resume the device when it's
>> +      * actually needed.
>> +      */
>> +     if (atomic_read(&dev->power.usage_count) < 2)
>> +             goto out;
>> +
>>       ret = pm_runtime_set_active(dev);
>>       if (ret)
>>               goto out;
>>
>
> Thanks,
> Rafael
>

Kind regards
Uffe