[PATCH 1/2] PM / Domains: Make genpd parameter of pm_genpd_present() const

[PATCH 1/2] PM / Domains: Make genpd parameter of pm_genpd_present() const

[Geert Uytterhoeven]

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/base/power/domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4072cc28ea..28d6e8bf746c4683 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -753,9 +753,9 @@ static inline void genpd_power_off_work_fn(struct work_struct *work) {}
  * pm_genpd_present - Check if the given PM domain has been initialized.
  * @genpd: PM domain to check.
  */
-static bool pm_genpd_present(struct generic_pm_domain *genpd)
+static bool pm_genpd_present(const struct generic_pm_domain *genpd)
 {
-	struct generic_pm_domain *gpd;
+	const struct generic_pm_domain *gpd;
 
 	if (IS_ERR_OR_NULL(genpd))
 		return false;
-- 
1.9.1

[PATCH 2/2] PM / Domains: Extract code to power off/on a PM domain

[Geert Uytterhoeven]

PM domains are powered on/off from various places. Some callers do
latency measurements, others don't. Consolidate using two helper
functions, which always measure the latencies, and update the stored
latencies when needed.

Other minor changes:
  - Use pr_warn() instead of pr_warning(),
  - There's no need to check genpd->name, %s handles NULL pointers fine,
  - Make the warning format strings identical, to save memory.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/base/power/domain.c | 101 ++++++++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 41 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 28d6e8bf746c4683..7b2007be51188ff8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -151,6 +151,59 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 	genpd->cpuidle_data->idle_state->exit_latency = usecs64;
 }
 
+static int do_genpd_power_on(struct generic_pm_domain *genpd)
+{
+	ktime_t time_start;
+	s64 elapsed_ns;
+	int ret;
+
+	if (!genpd->power_on)
+		return 0;
+
+	time_start = ktime_get();
+	ret = genpd->power_on(genpd);
+	if (ret)
+		return ret;
+
+	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
+	if (elapsed_ns <= genpd->power_on_latency_ns)
+		return ret;
+
+	genpd->power_on_latency_ns = elapsed_ns;
+	genpd->max_off_time_changed = true;
+	genpd_recalc_cpu_exit_latency(genpd);
+	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
+		genpd->name, "on", elapsed_ns);
+
+	return ret;
+}
+
+static int do_genpd_power_off(struct generic_pm_domain *genpd)
+{
+	ktime_t time_start;
+	s64 elapsed_ns;
+	int ret;
+
+	if (!genpd->power_off)
+		return 0;
+
+	time_start = ktime_get();
+	ret = genpd->power_off(genpd);
+	if (ret == -EBUSY)
+		return ret;
+
+	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
+	if (elapsed_ns <= genpd->power_off_latency_ns)
+		return ret;
+
+	genpd->power_off_latency_ns = elapsed_ns;
+	genpd->max_off_time_changed = true;
+	pr_warn("%s: Power-%s latency exceeded, new value %lld ns\n",
+		genpd->name, "off", elapsed_ns);
+
+	return ret;
+}
+
 /**
  * __pm_genpd_poweron - Restore power to a given PM domain and its masters.
  * @genpd: PM domain to power up.
@@ -222,25 +275,9 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 		}
 	}
 
-	if (genpd->power_on) {
-		ktime_t time_start = ktime_get();
-		s64 elapsed_ns;
-
-		ret = genpd->power_on(genpd);
-		if (ret)
-			goto err;
-
-		elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-		if (elapsed_ns > genpd->power_on_latency_ns) {
-			genpd->power_on_latency_ns = elapsed_ns;
-			genpd->max_off_time_changed = true;
-			genpd_recalc_cpu_exit_latency(genpd);
-			if (genpd->name)
-				pr_warning("%s: Power-on latency exceeded, "
-					"new value %lld ns\n", genpd->name,
-					elapsed_ns);
-		}
-	}
+	ret = do_genpd_power_on(genpd);
+	if (ret)
+		goto err;
 
  out:
 	genpd_set_active(genpd);
@@ -529,16 +566,11 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 	}
 
 	if (genpd->power_off) {
-		ktime_t time_start;
-		s64 elapsed_ns;
-
 		if (atomic_read(&genpd->sd_count) > 0) {
 			ret = -EBUSY;
 			goto out;
 		}
 
-		time_start = ktime_get();
-
 		/*
 		 * If sd_count > 0 at this point, one of the subdomains hasn't
 		 * managed to call pm_genpd_poweron() for the master yet after
@@ -547,21 +579,11 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		 * the pm_genpd_poweron() restore power for us (this shouldn't
 		 * happen very often).
 		 */
-		ret = genpd->power_off(genpd);
+		ret = do_genpd_power_off(genpd);
 		if (ret == -EBUSY) {
 			genpd_set_active(genpd);
 			goto out;
 		}
-
-		elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
-		if (elapsed_ns > genpd->power_off_latency_ns) {
-			genpd->power_off_latency_ns = elapsed_ns;
-			genpd->max_off_time_changed = true;
-			if (genpd->name)
-				pr_warning("%s: Power-off latency exceeded, "
-					"new value %lld ns\n", genpd->name,
-					elapsed_ns);
-		}
 	}
 
 	genpd->status = GPD_STATE_POWER_OFF;
@@ -796,8 +818,7 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
 	    || atomic_read(&genpd->sd_count) > 0)
 		return;
 
-	if (genpd->power_off)
-		genpd->power_off(genpd);
+	do_genpd_power_off(genpd);
 
 	genpd->status = GPD_STATE_POWER_OFF;
 
@@ -828,8 +849,7 @@ static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
 		genpd_sd_counter_inc(link->master);
 	}
 
-	if (genpd->power_on)
-		genpd->power_on(genpd);
+	do_genpd_power_on(genpd);
 
 	genpd->status = GPD_STATE_ACTIVE;
 }
@@ -1251,8 +1271,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 			 * If the domain was off before the hibernation, make
 			 * sure it will be off going forward.
 			 */
-			if (genpd->power_off)
-				genpd->power_off(genpd);
+			do_genpd_power_off(genpd);
 
 			return 0;
 		}
-- 
1.9.1

Re: [PATCH 2/2] PM / Domains: Extract code to power off/on a PM domain

[Ulf Hansson]

On 23 October 2014 14:12, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> PM domains are powered on/off from various places. Some callers do
> latency measurements, others don't. Consolidate using two helper
> functions, which always measure the latencies, and update the stored
> latencies when needed.
>
> Other minor changes:
>   - Use pr_warn() instead of pr_warning(),
>   - There's no need to check genpd->name, %s handles NULL pointers fine,
>   - Make the warning format strings identical, to save memory.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Though, some minor thoughts below.

> ---
>  drivers/base/power/domain.c | 101 ++++++++++++++++++++++++++------------------
>  1 file changed, 60 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 28d6e8bf746c4683..7b2007be51188ff8 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -151,6 +151,59 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
>         genpd->cpuidle_data->idle_state->exit_latency = usecs64;
>  }
>
> +static int do_genpd_power_on(struct generic_pm_domain *genpd)

Should we try to agree on the prefixes of the function names in genpd?
Currently there are a mix of them.

May I suggest we try to stick to this:

Exported functions:
pm_genpd_*
_pm_genpd_*
__pm_genpd_*

Static functions:
genpd_*
_genpd_*
__genpd_*

What do you think? Do you have any better suggestions?

Kind regards
Uffe

Re: [PATCH 2/2] PM / Domains: Extract code to power off/on a PM domain

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Oct 29, 2014 at 2:25 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 23 October 2014 14:12, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>> PM domains are powered on/off from various places. Some callers do
>> latency measurements, others don't. Consolidate using two helper
>> functions, which always measure the latencies, and update the stored
>> latencies when needed.
>>
>> Other minor changes:
>>   - Use pr_warn() instead of pr_warning(),
>>   - There's no need to check genpd->name, %s handles NULL pointers fine,
>>   - Make the warning format strings identical, to save memory.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks for your review.

>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -151,6 +151,59 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
>>         genpd->cpuidle_data->idle_state->exit_latency = usecs64;
>>  }
>>
>> +static int do_genpd_power_on(struct generic_pm_domain *genpd)
>
> Should we try to agree on the prefixes of the function names in genpd?
> Currently there are a mix of them.
>
> May I suggest we try to stick to this:
>
> Exported functions:
> pm_genpd_*
> _pm_genpd_*
> __pm_genpd_*
>
> Static functions:
> genpd_*
> _genpd_*
> __genpd_*
>
> What do you think? Do you have any better suggestions?

Sounds sane.

So:

s/do_genpd_power_on/genpd_power_on/
s/do_genpd_power_off/genpd_power_off/

(somehow I thought the latter were already in use, but I was wrong)

Note that there are lots of static pm_genpd_*() functions.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] PM / Domains: Extract code to power off/on a PM domain

[Ulf Hansson]

On 29 October 2014 14:32, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Oct 29, 2014 at 2:25 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 23 October 2014 14:12, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>> PM domains are powered on/off from various places. Some callers do
>>> latency measurements, others don't. Consolidate using two helper
>>> functions, which always measure the latencies, and update the stored
>>> latencies when needed.
>>>
>>> Other minor changes:
>>>   - Use pr_warn() instead of pr_warning(),
>>>   - There's no need to check genpd->name, %s handles NULL pointers fine,
>>>   - Make the warning format strings identical, to save memory.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thanks for your review.
>
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -151,6 +151,59 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
>>>         genpd->cpuidle_data->idle_state->exit_latency = usecs64;
>>>  }
>>>
>>> +static int do_genpd_power_on(struct generic_pm_domain *genpd)
>>
>> Should we try to agree on the prefixes of the function names in genpd?
>> Currently there are a mix of them.
>>
>> May I suggest we try to stick to this:
>>
>> Exported functions:
>> pm_genpd_*
>> _pm_genpd_*
>> __pm_genpd_*
>>
>> Static functions:
>> genpd_*
>> _genpd_*
>> __genpd_*
>>
>> What do you think? Do you have any better suggestions?
>
> Sounds sane.
>
> So:
>
> s/do_genpd_power_on/genpd_power_on/
> s/do_genpd_power_off/genpd_power_off/
>
> (somehow I thought the latter were already in use, but I was wrong)
>
> Note that there are lots of static pm_genpd_*() functions.

Yes, we could rename them such it would be easier to browse the code.

Kind regards
Uffe

Re: [PATCH 1/2] PM / Domains: Make genpd parameter of pm_genpd_present() const

[Kevin Hilman]

Geert Uytterhoeven <geert+renesas@glider.be> writes:

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Kevin Hilman <khilman@linaro.org>

Though I tend to prefer a brief changlog that answers "why", even if it may seem obvious.

Kevin


> ---
>  drivers/base/power/domain.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 40bc2f4072cc28ea..28d6e8bf746c4683 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -753,9 +753,9 @@ static inline void genpd_power_off_work_fn(struct work_struct *work) {}
>   * pm_genpd_present - Check if the given PM domain has been initialized.
>   * @genpd: PM domain to check.
>   */
> -static bool pm_genpd_present(struct generic_pm_domain *genpd)
> +static bool pm_genpd_present(const struct generic_pm_domain *genpd)
>  {
> -	struct generic_pm_domain *gpd;
> +	const struct generic_pm_domain *gpd;
>  
>  	if (IS_ERR_OR_NULL(genpd))
>  		return false;

Re: [PATCH 2/2] PM / Domains: Extract code to power off/on a PM domain

[Kevin Hilman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Ulf,
>
> On Wed, Oct 29, 2014 at 2:25 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 23 October 2014 14:12, Geert Uytterhoeven <geert+renesas@glider.be> wrote:
>>> PM domains are powered on/off from various places. Some callers do
>>> latency measurements, others don't. Consolidate using two helper
>>> functions, which always measure the latencies, and update the stored
>>> latencies when needed.
>>>
>>> Other minor changes:
>>>   - Use pr_warn() instead of pr_warning(),
>>>   - There's no need to check genpd->name, %s handles NULL pointers fine,
>>>   - Make the warning format strings identical, to save memory.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Thanks for your review.
>
>>> --- a/drivers/base/power/domain.c
>>> +++ b/drivers/base/power/domain.c
>>> @@ -151,6 +151,59 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
>>>         genpd->cpuidle_data->idle_state->exit_latency = usecs64;
>>>  }
>>>
>>> +static int do_genpd_power_on(struct generic_pm_domain *genpd)
>>
>> Should we try to agree on the prefixes of the function names in genpd?
>> Currently there are a mix of them.
>>
>> May I suggest we try to stick to this:
>>
>> Exported functions:
>> pm_genpd_*
>> _pm_genpd_*
>> __pm_genpd_*
>>
>> Static functions:
>> genpd_*
>> _genpd_*
>> __genpd_*
>>
>> What do you think? Do you have any better suggestions?
>
> Sounds sane.
>
> So:
>
> s/do_genpd_power_on/genpd_power_on/
> s/do_genpd_power_off/genpd_power_off/

With that name change, feel free to add

Reviewed-by: Kevin Hilman <khilman@linaro.org>

Kevin