[PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[Ulf Hansson]

Runtime PM centric subsystems/drivers may re-use their runtime PM
callbacks for system PM. Typically that's done via using the runtime PM
helpers, pm_runtime_force_suspend|resume().

To genpd, this means its runtime PM callbacks may be invoked even when
runtime PM has been disabled for the device. By checking this condition,
it allows genpd to skip latency validation and measurement in the system
PM path. That's needed to enable drivers/subsystems to re-use the runtime
PM callbacks for system PM.

Fixes: ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the
power off sequence")
Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
Reported-by: Harunaga <nx-truong@jinso.co.jp>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

This patch has only been quick tested on ux500, so further tests are
welcome and needed. It was reported [1] to point out a certain commit
causing the regression, but further analyze actually tells that the
problem been there longer.

So, I decided to point the fixes tag to a commit from where it actually
becomes quite simple to address the problem. Earlier than that, is just
not worth the effort.

[1]
https://lkml.org/lkml/2015/11/16/748

---
 drivers/base/power/domain.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e03b1ad..8f0c2a3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -390,6 +390,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	struct generic_pm_domain *genpd;
 	bool (*stop_ok)(struct device *__dev);
 	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	bool system_pm = !pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -400,12 +401,18 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
+	/*
+	 * A runtime PM centric subsystem/driver may re-use the runtime PM
+	 * callbacks for system PM. In these cases, don't validate or measure
+	 * latencies.
+	 */
 	stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;
-	if (stop_ok && !stop_ok(dev))
+	if (!system_pm && stop_ok && !stop_ok(dev))
 		return -EBUSY;
 
 	/* Measure suspend latency. */
-	time_start = ktime_get();
+	if (!system_pm)
+		time_start = ktime_get();
 
 	ret = genpd_save_dev(genpd, dev);
 	if (ret)
@@ -417,6 +424,10 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return ret;
 	}
 
+	/* Don't try poweroff in system PM as it's prevented anyway. */
+	if (system_pm)
+		return 0;
+
 	/* Update suspend latency value if the measured time exceeds it. */
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
 	if (elapsed_ns > td->suspend_latency_ns) {
@@ -453,6 +464,7 @@ static int pm_genpd_runtime_resume(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
 	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	bool system_pm = !pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
 	int ret;
@@ -464,8 +476,8 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/* If power.irq_safe, the PM domain is never powered off. */
-	if (dev->power.irq_safe) {
+	/* If power.irq_safe or system PM, the PM domain remains powered. */
+	if (dev->power.irq_safe || system_pm) {
 		timed = false;
 		goto out;
 	}
-- 
1.9.1

Re: [PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Runtime PM centric subsystems/drivers may re-use their runtime PM
> callbacks for system PM. Typically that's done via using the runtime PM
> helpers, pm_runtime_force_suspend|resume().
>
> To genpd, this means its runtime PM callbacks may be invoked even when
> runtime PM has been disabled for the device. By checking this condition,
> it allows genpd to skip latency validation and measurement in the system
> PM path. That's needed to enable drivers/subsystems to re-use the runtime
> PM callbacks for system PM.
>
> Fixes: ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the
> power off sequence")
> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
> Reported-by: Harunaga <nx-truong@jinso.co.jp>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> This patch has only been quick tested on ux500, so further tests are
> welcome and needed. It was reported [1] to point out a certain commit
> causing the regression, but further analyze actually tells that the
> problem been there longer.
>
> So, I decided to point the fixes tag to a commit from where it actually
> becomes quite simple to address the problem. Earlier than that, is just
> not worth the effort.
>
> [1]
> https://lkml.org/lkml/2015/11/16/748
>
> ---
>  drivers/base/power/domain.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..8f0c2a3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -390,6 +390,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>  	struct generic_pm_domain *genpd;
>  	bool (*stop_ok)(struct device *__dev);
>  	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> +	bool system_pm = !pm_runtime_enabled(dev);

nit: I think this 'system_pm' variable name is confusing, especaly
because it just means runtime PM *not* enabled.

Is !pm_runtime_enabled() really sufficient to determine if these are
being called in the context of system PM?

Kevin

Re: [PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[Ulf Hansson]

On 17 November 2015 at 22:32, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> Runtime PM centric subsystems/drivers may re-use their runtime PM
>> callbacks for system PM. Typically that's done via using the runtime PM
>> helpers, pm_runtime_force_suspend|resume().
>>
>> To genpd, this means its runtime PM callbacks may be invoked even when
>> runtime PM has been disabled for the device. By checking this condition,
>> it allows genpd to skip latency validation and measurement in the system
>> PM path. That's needed to enable drivers/subsystems to re-use the runtime
>> PM callbacks for system PM.
>>
>> Fixes: ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the
>> power off sequence")
>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
>> Reported-by: Harunaga <nx-truong@jinso.co.jp>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> This patch has only been quick tested on ux500, so further tests are
>> welcome and needed. It was reported [1] to point out a certain commit
>> causing the regression, but further analyze actually tells that the
>> problem been there longer.
>>
>> So, I decided to point the fixes tag to a commit from where it actually
>> becomes quite simple to address the problem. Earlier than that, is just
>> not worth the effort.
>>
>> [1]
>> https://lkml.org/lkml/2015/11/16/748
>>
>> ---
>>  drivers/base/power/domain.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index e03b1ad..8f0c2a3 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -390,6 +390,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>>       struct generic_pm_domain *genpd;
>>       bool (*stop_ok)(struct device *__dev);
>>       struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
>> +     bool system_pm = !pm_runtime_enabled(dev);
>
> nit: I think this 'system_pm' variable name is confusing, especaly
> because it just means runtime PM *not* enabled.

Okay! Perhaps "runtime_pm" would be better?

>
> Is !pm_runtime_enabled() really sufficient to determine if these are
> being called in the context of system PM?

I guess pm_runtime_force_suspend() could be used from other places but
a system PM callback, such as a ->remove() callback for example. So,
indeed you have point!

Nevertheless, I still think we can rely on the condition from
pm_runtime_enabled(dev), as it's only relevant to validate/measure dev
PM QOS latencies when runtime PM is enabled.

With this in mind, let me cook a new version.

Thanks for reviewing!

Kind regards
Uffe

Re: [PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 17 November 2015 at 22:32, Kevin Hilman <khilman@kernel.org> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>>> Runtime PM centric subsystems/drivers may re-use their runtime PM
>>> callbacks for system PM. Typically that's done via using the runtime PM
>>> helpers, pm_runtime_force_suspend|resume().
>>>
>>> To genpd, this means its runtime PM callbacks may be invoked even when
>>> runtime PM has been disabled for the device. By checking this condition,
>>> it allows genpd to skip latency validation and measurement in the system
>>> PM path. That's needed to enable drivers/subsystems to re-use the runtime
>>> PM callbacks for system PM.
>>>
>>> Fixes: ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the
>>> power off sequence")
>>> Reported-by: Cao Minh Hiep <cm-hiep@jinso.co.jp>
>>> Reported-by: Harunaga <nx-truong@jinso.co.jp>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>
>>> This patch has only been quick tested on ux500, so further tests are
>>> welcome and needed. It was reported [1] to point out a certain commit
>>> causing the regression, but further analyze actually tells that the
>>> problem been there longer.
>>>
>>> So, I decided to point the fixes tag to a commit from where it actually
>>> becomes quite simple to address the problem. Earlier than that, is just
>>> not worth the effort.
>>>
>>> [1]
>>> https://lkml.org/lkml/2015/11/16/748
>>>
>>> ---
>>>  drivers/base/power/domain.c | 20 ++++++++++++++++----
>>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>>> index e03b1ad..8f0c2a3 100644
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -390,6 +390,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>>>       struct generic_pm_domain *genpd;
>>>       bool (*stop_ok)(struct device *__dev);
>>>       struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
>>> +     bool system_pm = !pm_runtime_enabled(dev);
>>
>> nit: I think this 'system_pm' variable name is confusing, especaly
>> because it just means runtime PM *not* enabled.
>
> Okay! Perhaps "runtime_pm" would be better?
>

Yes, or rpm_disabled (or rpm_enabled), which is what it's actually
checking.

>>
>> Is !pm_runtime_enabled() really sufficient to determine if these are
>> being called in the context of system PM?
>
> I guess pm_runtime_force_suspend() could be used from other places but
> a system PM callback, such as a ->remove() callback for example. So,
> indeed you have point!
>
> Nevertheless, I still think we can rely on the condition from
> pm_runtime_enabled(dev), as it's only relevant to validate/measure dev
> PM QOS latencies when runtime PM is enabled.

Yeah, good point.

Kevin

Re: [PATCH] PM / Domains: Allow runtime PM callbacks to be re-used during system PM

[Cao Minh Hiep]

Hi Ulf Hansson
Thanks for your fixed patch!

We have just patched and tested this patch on upstream v4.4-rc1 again.

Results:
The problem related Runtime PM has disappeared.
PM driver works well without any problems.

Thanks you and best regards.
Jinso/Cao Minh Hiep.


On 2015年11月18日 01:42, Ulf Hansson wrote:
> Runtime PM centric subsystems/drivers may re-use their runtime PM
> callbacks for system PM. Typically that's done via using the runtime PM
> helpers, pm_runtime_force_suspend|resume().
>
> To genpd, this means its runtime PM callbacks may be invoked even when
> runtime PM has been disabled for the device. By checking this condition,
> it allows genpd to skip latency validation and measurement in the system
> PM path. That's needed to enable drivers/subsystems to re-use the runtime
> PM callbacks for system PM.
>
> Fixes: ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the
> power off sequence")
> Reported-by: Cao Minh Hiep<cm-hiep@jinso.co.jp>
> Reported-by: Harunaga<nx-truong@jinso.co.jp>
> Signed-off-by: Ulf Hansson<ulf.hansson@linaro.org>
> ---
>
> This patch has only been quick tested on ux500, so further tests are
> welcome and needed. It was reported [1] to point out a certain commit
> causing the regression, but further analyze actually tells that the
> problem been there longer.
>
> So, I decided to point the fixes tag to a commit from where it actually
> becomes quite simple to address the problem. Earlier than that, is just
> not worth the effort.
>
> [1]
> https://lkml.org/lkml/2015/11/16/748
>
> ---
>   drivers/base/power/domain.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e03b1ad..8f0c2a3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -390,6 +390,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>   	struct generic_pm_domain *genpd;
>   	bool (*stop_ok)(struct device *__dev);
>   	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> +	bool system_pm = !pm_runtime_enabled(dev);
>   	ktime_t time_start;
>   	s64 elapsed_ns;
>   	int ret;
> @@ -400,12 +401,18 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>   	if (IS_ERR(genpd))
>   		return -EINVAL;
>   
> +	/*
> +	 * A runtime PM centric subsystem/driver may re-use the runtime PM
> +	 * callbacks for system PM. In these cases, don't validate or measure
> +	 * latencies.
> +	 */
>   	stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;
> -	if (stop_ok && !stop_ok(dev))
> +	if (!system_pm && stop_ok && !stop_ok(dev))
>   		return -EBUSY;
>   
>   	/* Measure suspend latency. */
> -	time_start = ktime_get();
> +	if (!system_pm)
> +		time_start = ktime_get();
>   
>   	ret = genpd_save_dev(genpd, dev);
>   	if (ret)
> @@ -417,6 +424,10 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>   		return ret;
>   	}
>   
> +	/* Don't try poweroff in system PM as it's prevented anyway. */
> +	if (system_pm)
> +		return 0;
> +
>   	/* Update suspend latency value if the measured time exceeds it. */
>   	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
>   	if (elapsed_ns > td->suspend_latency_ns) {
> @@ -453,6 +464,7 @@ static int pm_genpd_runtime_resume(struct device *dev)
>   {
>   	struct generic_pm_domain *genpd;
>   	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> +	bool system_pm = !pm_runtime_enabled(dev);
>   	ktime_t time_start;
>   	s64 elapsed_ns;
>   	int ret;
> @@ -464,8 +476,8 @@ static int pm_genpd_runtime_resume(struct device *dev)
>   	if (IS_ERR(genpd))
>   		return -EINVAL;
>   
> -	/* If power.irq_safe, the PM domain is never powered off. */
> -	if (dev->power.irq_safe) {
> +	/* If power.irq_safe or system PM, the PM domain remains powered. */
> +	if (dev->power.irq_safe || system_pm) {
>   		timed = false;
>   		goto out;
>   	}
> -- 1.9.1