* [PATCH] PM / Domains: Skip latency measurements if timekeeping is suspended
@ 2015-03-18 16:25 Geert Uytterhoeven
2015-03-18 22:30 ` Rafael J. Wysocki
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2015-03-18 16:25 UTC (permalink / raw)
To: Rafael J. Wysocki, Kevin Hilman, Ulf Hansson
Cc: linux-pm, linux-sh, linux-kernel, Geert Uytterhoeven
The PM Domain code uses ktime_get() to perform various latency
measurements. However, if ktime_get() is called while timekeeping is
suspended, the following warning is printed:
WARNING: CPU: 0 PID: 1340 at kernel/time/timekeeping.c:576 ktime_get+0x30/0xf4()
This happens when resuming the PM Domain that contains the clock events
source. Chain of operations is:
timekeeping_resume()
{
clockevents_resume()
sh_cmt_clock_event_resume()
pm_genpd_syscore_poweron()
pm_genpd_sync_poweron()
genpd_power_on()
ktime_get(), but timekeeping_suspended == 1
...
timekeeping_suspended = 0;
}
Skip all latency measurements if timekeeping is suspended to fix this.
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I'm not sure if this is needed for all latency measurements.
So far I only encountered it while powering-on a clock domain during
resume from s2ram.
---
drivers/base/power/domain.c | 39 +++++++++++++++++++++++++++++----------
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 45937f88e77c8889..ab2398cfcebb7732 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -18,6 +18,7 @@
#include <linux/sched.h>
#include <linux/suspend.h>
#include <linux/export.h>
+#include <linux/timekeeping.h>
#define GENPD_DEV_CALLBACK(genpd, type, callback, dev) \
({ \
@@ -33,16 +34,24 @@
#define GENPD_DEV_TIMED_CALLBACK(genpd, type, callback, dev, field, name) \
({ \
- ktime_t __start = ktime_get(); \
- type __retval = GENPD_DEV_CALLBACK(genpd, type, callback, dev); \
- s64 __elapsed = ktime_to_ns(ktime_sub(ktime_get(), __start)); \
- struct gpd_timing_data *__td = &dev_gpd_data(dev)->td; \
- if (!__retval && __elapsed > __td->field) { \
- __td->field = __elapsed; \
- dev_dbg(dev, name " latency exceeded, new value %lld ns\n", \
- __elapsed); \
- genpd->max_off_time_changed = true; \
- __td->constraint_changed = true; \
+ type __retval; \
+ if (unlikely(timekeeping_suspended)) { \
+ dev_dbg(dev, "Skipping %s timings\n", #callback); \
+ __retval = GENPD_DEV_CALLBACK(genpd, type, callback, dev); \
+ } else { \
+ ktime_t __start = ktime_get(); \
+ type __ret = GENPD_DEV_CALLBACK(genpd, type, callback, dev); \
+ s64 __elapsed = ktime_to_ns(ktime_sub(ktime_get(), __start)); \
+ struct gpd_timing_data *__td = &dev_gpd_data(dev)->td; \
+ if (!__ret && __elapsed > __td->field) { \
+ __td->field = __elapsed; \
+ dev_dbg(dev, \
+ name " latency exceeded, new value %lld ns\n", \
+ __elapsed); \
+ genpd->max_off_time_changed = true; \
+ __td->constraint_changed = true; \
+ } \
+ __retval = __ret; \
} \
__retval; \
})
@@ -161,6 +170,11 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
if (!genpd->power_on)
return 0;
+ if (unlikely(timekeeping_suspended)) {
+ pr_debug("%s: Skipping %s timings\n", genpd->name, "power_on");
+ return genpd->power_on(genpd);
+ }
+
time_start = ktime_get();
ret = genpd->power_on(genpd);
if (ret)
@@ -188,6 +202,11 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
if (!genpd->power_off)
return 0;
+ if (unlikely(timekeeping_suspended)) {
+ pr_debug("%s: Skipping %s timings\n", genpd->name, "power_off");
+ return genpd->power_off(genpd);
+ }
+
time_start = ktime_get();
ret = genpd->power_off(genpd);
if (ret == -EBUSY)
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] PM / Domains: Skip latency measurements if timekeeping is suspended
2015-03-18 16:25 [PATCH] PM / Domains: Skip latency measurements if timekeeping is suspended Geert Uytterhoeven
@ 2015-03-18 22:30 ` Rafael J. Wysocki
2015-03-19 8:54 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2015-03-18 22:30 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Kevin Hilman, Ulf Hansson, linux-pm, linux-sh, linux-kernel
On Wednesday, March 18, 2015 05:25:46 PM Geert Uytterhoeven wrote:
> The PM Domain code uses ktime_get() to perform various latency
> measurements. However, if ktime_get() is called while timekeeping is
> suspended, the following warning is printed:
>
> WARNING: CPU: 0 PID: 1340 at kernel/time/timekeeping.c:576 ktime_get+0x30/0xf4()
>
> This happens when resuming the PM Domain that contains the clock events
> source. Chain of operations is:
>
> timekeeping_resume()
> {
> clockevents_resume()
> sh_cmt_clock_event_resume()
> pm_genpd_syscore_poweron()
> pm_genpd_sync_poweron()
> genpd_power_on()
> ktime_get(), but timekeeping_suspended == 1
> ...
> timekeeping_suspended = 0;
> }
>
> Skip all latency measurements if timekeeping is suspended to fix this.
I don't think that this is where we should fix it. At least using
timekeeping_suspended outside of the timekeeping core would not be
welcome by its maintainers.
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> I'm not sure if this is needed for all latency measurements.
> So far I only encountered it while powering-on a clock domain during
> resume from s2ram.
The problem seems to be that the clock domain is powered on in a
syscore resume routine which happens to be called before timekeeping_resume().
It looks like we either need to force the right ordering somehow or have a
special variant of GENPD_DEV_TIMED_CALLBACK() for syscore suspend/resume that
won't do the latency measurement at all (which doesn't make much sense at
this point, because time is effectively "frozen" then).
Rafael
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] PM / Domains: Skip latency measurements if timekeeping is suspended
2015-03-18 22:30 ` Rafael J. Wysocki
@ 2015-03-19 8:54 ` Geert Uytterhoeven
0 siblings, 0 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2015-03-19 8:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Geert Uytterhoeven, Kevin Hilman, Ulf Hansson, Linux PM list,
Linux-sh list, linux-kernel
Hi Rafael,
On Wed, Mar 18, 2015 at 11:30 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, March 18, 2015 05:25:46 PM Geert Uytterhoeven wrote:
>> The PM Domain code uses ktime_get() to perform various latency
>> measurements. However, if ktime_get() is called while timekeeping is
>> suspended, the following warning is printed:
>>
>> WARNING: CPU: 0 PID: 1340 at kernel/time/timekeeping.c:576 ktime_get+0x30/0xf4()
>>
>> This happens when resuming the PM Domain that contains the clock events
>> source. Chain of operations is:
>>
>> timekeeping_resume()
>> {
>> clockevents_resume()
>> sh_cmt_clock_event_resume()
>> pm_genpd_syscore_poweron()
>> pm_genpd_sync_poweron()
>> genpd_power_on()
>> ktime_get(), but timekeeping_suspended == 1
>> ...
>> timekeeping_suspended = 0;
>> }
>>
>> Skip all latency measurements if timekeeping is suspended to fix this.
>
> I don't think that this is where we should fix it. At least using
> timekeeping_suspended outside of the timekeeping core would not be
> welcome by its maintainers.
It's a public symbol, declared in a header file ;-)
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> I'm not sure if this is needed for all latency measurements.
>> So far I only encountered it while powering-on a clock domain during
>> resume from s2ram.
>
> The problem seems to be that the clock domain is powered on in a
> syscore resume routine which happens to be called before timekeeping_resume().
The clock domain is powered on from _within_ timekeeping_resume().
> It looks like we either need to force the right ordering somehow or have a
> special variant of GENPD_DEV_TIMED_CALLBACK() for syscore suspend/resume that
> won't do the latency measurement at all (which doesn't make much sense at
> this point, because time is effectively "frozen" then).
That's an option.
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-03-19 8:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-18 16:25 [PATCH] PM / Domains: Skip latency measurements if timekeeping is suspended Geert Uytterhoeven
2015-03-18 22:30 ` Rafael J. Wysocki
2015-03-19 8:54 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).