linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume
@ 2015-05-29 15:24 Geert Uytterhoeven
  2015-06-04 12:17 ` Ulf Hansson
  2015-06-15 23:41 ` Rafael J. Wysocki
  0 siblings, 2 replies; 3+ messages in thread
From: Geert Uytterhoeven @ 2015-05-29 15:24 UTC (permalink / raw)
  To: Rafael J. Wysocki, Kevin Hilman, Ulf Hansson
  Cc: linux-pm, linux-sh, linux-kernel, Geert Uytterhoeven

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

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+0x3

This happens when resuming the PM Domain that contains the clock events
source, which calls pm_genpd_syscore_poweron(). Chain of operations is:

    timekeeping_resume()
    {
	clockevents_resume()
	    sh_cmt_clock_event_resume()
		pm_genpd_syscore_poweron()
		    pm_genpd_sync_poweron()
			genpd_syscore_switch()
			    genpd_power_on()
				ktime_get(), but timekeeping_suspended == 1
	...
	timekeeping_suspended = 0;
    }

Fix this by adding a "timed" parameter to genpd_power_{on,off}() and
pm_genpd_sync_power{off,on}(), to indicate whether latency measurements
are allowed.  This parameter is passed as false in
genpd_syscore_switch() (i.e. during syscore suspend/resume), and true in
all other cases.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Seen on r8a7791/koelsch with CPG Clock Domain, _and_
genpd->power_{off,on}() populated for debugging purposes.

This is v2 of "PM / Domains: Skip latency measurements if timekeeping is
suspended".

Changes:
  v2:
    - Add a "timed" parameter instead of checking the
      timekeeping_suspended flag, which is supposed to be private to the
      time subsystem.
---
 drivers/base/power/domain.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2327613d453929db..cdd547bd67df8218 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -181,7 +181,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
 	genpd->cpuidle_data->idle_state->exit_latency = usecs64;
 }
 
-static int genpd_power_on(struct generic_pm_domain *genpd)
+static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
 {
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -190,6 +190,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	if (!genpd->power_on)
 		return 0;
 
+	if (!timed)
+		return genpd->power_on(genpd);
+
 	time_start = ktime_get();
 	ret = genpd->power_on(genpd);
 	if (ret)
@@ -208,7 +211,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
 	return ret;
 }
 
-static int genpd_power_off(struct generic_pm_domain *genpd)
+static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 {
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -217,6 +220,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
 	if (!genpd->power_off)
 		return 0;
 
+	if (!timed)
+		return genpd->power_off(genpd);
+
 	time_start = ktime_get();
 	ret = genpd->power_off(genpd);
 	if (ret == -EBUSY)
@@ -305,7 +311,7 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
 		}
 	}
 
-	ret = genpd_power_on(genpd);
+	ret = genpd_power_on(genpd, true);
 	if (ret)
 		goto err;
 
@@ -615,7 +621,7 @@ 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 = genpd_power_off(genpd, true);
 		if (ret == -EBUSY) {
 			genpd_set_active(genpd);
 			goto out;
@@ -827,6 +833,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
 /**
  * pm_genpd_sync_poweroff - Synchronously power off a PM domain and its masters.
  * @genpd: PM domain to power off, if possible.
+ * @timed: True if latency measurements are allowed.
  *
  * Check if the given PM domain can be powered off (during system suspend or
  * hibernation) and do that if so.  Also, in that case propagate to its masters.
@@ -836,7 +843,8 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
+static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
+				   bool timed)
 {
 	struct gpd_link *link;
 
@@ -847,26 +855,28 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
 	    || atomic_read(&genpd->sd_count) > 0)
 		return;
 
-	genpd_power_off(genpd);
+	genpd_power_off(genpd, timed);
 
 	genpd->status = GPD_STATE_POWER_OFF;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
 		genpd_sd_counter_dec(link->master);
-		pm_genpd_sync_poweroff(link->master);
+		pm_genpd_sync_poweroff(link->master, timed);
 	}
 }
 
 /**
  * pm_genpd_sync_poweron - Synchronously power on a PM domain and its masters.
  * @genpd: PM domain to power on.
+ * @timed: True if latency measurements are allowed.
  *
  * This function is only called in "noirq" and "syscore" stages of system power
  * transitions, so it need not acquire locks (all of the "noirq" callbacks are
  * executed sequentially, so it is guaranteed that it will never run twice in
  * parallel).
  */
-static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
+static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd,
+				  bool timed)
 {
 	struct gpd_link *link;
 
@@ -874,11 +884,11 @@ static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
 		return;
 
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
-		pm_genpd_sync_poweron(link->master);
+		pm_genpd_sync_poweron(link->master, timed);
 		genpd_sd_counter_inc(link->master);
 	}
 
-	genpd_power_on(genpd);
+	genpd_power_on(genpd, timed);
 
 	genpd->status = GPD_STATE_ACTIVE;
 }
@@ -1056,7 +1066,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
 	genpd->suspended_count++;
-	pm_genpd_sync_poweroff(genpd);
+	pm_genpd_sync_poweroff(genpd, true);
 
 	return 0;
 }
@@ -1086,7 +1096,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 	 * guaranteed that this function will never run twice in parallel for
 	 * the same PM domain, so it is not necessary to use locking here.
 	 */
-	pm_genpd_sync_poweron(genpd);
+	pm_genpd_sync_poweron(genpd, true);
 	genpd->suspended_count--;
 
 	return genpd_start_dev(genpd, dev);
@@ -1300,7 +1310,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.
 			 */
-			genpd_power_off(genpd);
+			genpd_power_off(genpd, true);
 
 			return 0;
 		}
@@ -1309,7 +1319,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 	if (genpd->suspend_power_off)
 		return 0;
 
-	pm_genpd_sync_poweron(genpd);
+	pm_genpd_sync_poweron(genpd, true);
 
 	return genpd_start_dev(genpd, dev);
 }
@@ -1367,9 +1377,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
 
 	if (suspend) {
 		genpd->suspended_count++;
-		pm_genpd_sync_poweroff(genpd);
+		pm_genpd_sync_poweroff(genpd, false);
 	} else {
-		pm_genpd_sync_poweron(genpd);
+		pm_genpd_sync_poweron(genpd, false);
 		genpd->suspended_count--;
 	}
 }
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume
  2015-05-29 15:24 [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume Geert Uytterhoeven
@ 2015-06-04 12:17 ` Ulf Hansson
  2015-06-15 23:41 ` Rafael J. Wysocki
  1 sibling, 0 replies; 3+ messages in thread
From: Ulf Hansson @ 2015-06-04 12:17 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Rafael J. Wysocki, Kevin Hilman, linux-pm, Linux-sh list,
	linux-kernel, Geert Uytterhoeven

On 29 May 2015 at 17:24, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
>
> 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+0x3
>
> This happens when resuming the PM Domain that contains the clock events
> source, which calls pm_genpd_syscore_poweron(). Chain of operations is:
>
>     timekeeping_resume()
>     {
>         clockevents_resume()
>             sh_cmt_clock_event_resume()
>                 pm_genpd_syscore_poweron()
>                     pm_genpd_sync_poweron()
>                         genpd_syscore_switch()
>                             genpd_power_on()
>                                 ktime_get(), but timekeeping_suspended == 1
>         ...
>         timekeeping_suspended = 0;
>     }
>
> Fix this by adding a "timed" parameter to genpd_power_{on,off}() and
> pm_genpd_sync_power{off,on}(), to indicate whether latency measurements
> are allowed.  This parameter is passed as false in
> genpd_syscore_switch() (i.e. during syscore suspend/resume), and true in
> all other cases.

Hmm...

Will it ever make sense to do these time measurement once a system PM
suspend sequence has started? Especially since those measurements
might not be represent a typical case.

Could we turn of time measurement once system PM has started and turn
it on again once resumed?

Just and idea...

Kind regards
Uffe

>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Seen on r8a7791/koelsch with CPG Clock Domain, _and_
> genpd->power_{off,on}() populated for debugging purposes.
>
> This is v2 of "PM / Domains: Skip latency measurements if timekeeping is
> suspended".
>
> Changes:
>   v2:
>     - Add a "timed" parameter instead of checking the
>       timekeeping_suspended flag, which is supposed to be private to the
>       time subsystem.
> ---
>  drivers/base/power/domain.c | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2327613d453929db..cdd547bd67df8218 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -181,7 +181,7 @@ static void genpd_recalc_cpu_exit_latency(struct generic_pm_domain *genpd)
>         genpd->cpuidle_data->idle_state->exit_latency = usecs64;
>  }
>
> -static int genpd_power_on(struct generic_pm_domain *genpd)
> +static int genpd_power_on(struct generic_pm_domain *genpd, bool timed)
>  {
>         ktime_t time_start;
>         s64 elapsed_ns;
> @@ -190,6 +190,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
>         if (!genpd->power_on)
>                 return 0;
>
> +       if (!timed)
> +               return genpd->power_on(genpd);
> +
>         time_start = ktime_get();
>         ret = genpd->power_on(genpd);
>         if (ret)
> @@ -208,7 +211,7 @@ static int genpd_power_on(struct generic_pm_domain *genpd)
>         return ret;
>  }
>
> -static int genpd_power_off(struct generic_pm_domain *genpd)
> +static int genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>  {
>         ktime_t time_start;
>         s64 elapsed_ns;
> @@ -217,6 +220,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd)
>         if (!genpd->power_off)
>                 return 0;
>
> +       if (!timed)
> +               return genpd->power_off(genpd);
> +
>         time_start = ktime_get();
>         ret = genpd->power_off(genpd);
>         if (ret == -EBUSY)
> @@ -305,7 +311,7 @@ static int __pm_genpd_poweron(struct generic_pm_domain *genpd)
>                 }
>         }
>
> -       ret = genpd_power_on(genpd);
> +       ret = genpd_power_on(genpd, true);
>         if (ret)
>                 goto err;
>
> @@ -615,7 +621,7 @@ 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 = genpd_power_off(genpd, true);
>                 if (ret == -EBUSY) {
>                         genpd_set_active(genpd);
>                         goto out;
> @@ -827,6 +833,7 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
>  /**
>   * pm_genpd_sync_poweroff - Synchronously power off a PM domain and its masters.
>   * @genpd: PM domain to power off, if possible.
> + * @timed: True if latency measurements are allowed.
>   *
>   * Check if the given PM domain can be powered off (during system suspend or
>   * hibernation) and do that if so.  Also, in that case propagate to its masters.
> @@ -836,7 +843,8 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
>   * executed sequentially, so it is guaranteed that it will never run twice in
>   * parallel).
>   */
> -static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
> +static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd,
> +                                  bool timed)
>  {
>         struct gpd_link *link;
>
> @@ -847,26 +855,28 @@ static void pm_genpd_sync_poweroff(struct generic_pm_domain *genpd)
>             || atomic_read(&genpd->sd_count) > 0)
>                 return;
>
> -       genpd_power_off(genpd);
> +       genpd_power_off(genpd, timed);
>
>         genpd->status = GPD_STATE_POWER_OFF;
>
>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
>                 genpd_sd_counter_dec(link->master);
> -               pm_genpd_sync_poweroff(link->master);
> +               pm_genpd_sync_poweroff(link->master, timed);
>         }
>  }
>
>  /**
>   * pm_genpd_sync_poweron - Synchronously power on a PM domain and its masters.
>   * @genpd: PM domain to power on.
> + * @timed: True if latency measurements are allowed.
>   *
>   * This function is only called in "noirq" and "syscore" stages of system power
>   * transitions, so it need not acquire locks (all of the "noirq" callbacks are
>   * executed sequentially, so it is guaranteed that it will never run twice in
>   * parallel).
>   */
> -static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
> +static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd,
> +                                 bool timed)
>  {
>         struct gpd_link *link;
>
> @@ -874,11 +884,11 @@ static void pm_genpd_sync_poweron(struct generic_pm_domain *genpd)
>                 return;
>
>         list_for_each_entry(link, &genpd->slave_links, slave_node) {
> -               pm_genpd_sync_poweron(link->master);
> +               pm_genpd_sync_poweron(link->master, timed);
>                 genpd_sd_counter_inc(link->master);
>         }
>
> -       genpd_power_on(genpd);
> +       genpd_power_on(genpd, timed);
>
>         genpd->status = GPD_STATE_ACTIVE;
>  }
> @@ -1056,7 +1066,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
>          * the same PM domain, so it is not necessary to use locking here.
>          */
>         genpd->suspended_count++;
> -       pm_genpd_sync_poweroff(genpd);
> +       pm_genpd_sync_poweroff(genpd, true);
>
>         return 0;
>  }
> @@ -1086,7 +1096,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
>          * guaranteed that this function will never run twice in parallel for
>          * the same PM domain, so it is not necessary to use locking here.
>          */
> -       pm_genpd_sync_poweron(genpd);
> +       pm_genpd_sync_poweron(genpd, true);
>         genpd->suspended_count--;
>
>         return genpd_start_dev(genpd, dev);
> @@ -1300,7 +1310,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.
>                          */
> -                       genpd_power_off(genpd);
> +                       genpd_power_off(genpd, true);
>
>                         return 0;
>                 }
> @@ -1309,7 +1319,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
>         if (genpd->suspend_power_off)
>                 return 0;
>
> -       pm_genpd_sync_poweron(genpd);
> +       pm_genpd_sync_poweron(genpd, true);
>
>         return genpd_start_dev(genpd, dev);
>  }
> @@ -1367,9 +1377,9 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
>
>         if (suspend) {
>                 genpd->suspended_count++;
> -               pm_genpd_sync_poweroff(genpd);
> +               pm_genpd_sync_poweroff(genpd, false);
>         } else {
> -               pm_genpd_sync_poweron(genpd);
> +               pm_genpd_sync_poweron(genpd, false);
>                 genpd->suspended_count--;
>         }
>  }
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume
  2015-05-29 15:24 [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume Geert Uytterhoeven
  2015-06-04 12:17 ` Ulf Hansson
@ 2015-06-15 23:41 ` Rafael J. Wysocki
  1 sibling, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2015-06-15 23:41 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Kevin Hilman, Ulf Hansson, linux-pm, linux-sh, linux-kernel,
	Geert Uytterhoeven

On Friday, May 29, 2015 05:24:23 PM Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> 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+0x3
> 
> This happens when resuming the PM Domain that contains the clock events
> source, which calls pm_genpd_syscore_poweron(). Chain of operations is:
> 
>     timekeeping_resume()
>     {
> 	clockevents_resume()
> 	    sh_cmt_clock_event_resume()
> 		pm_genpd_syscore_poweron()
> 		    pm_genpd_sync_poweron()
> 			genpd_syscore_switch()
> 			    genpd_power_on()
> 				ktime_get(), but timekeeping_suspended == 1
> 	...
> 	timekeeping_suspended = 0;
>     }
> 
> Fix this by adding a "timed" parameter to genpd_power_{on,off}() and
> pm_genpd_sync_power{off,on}(), to indicate whether latency measurements
> are allowed.  This parameter is passed as false in
> genpd_syscore_switch() (i.e. during syscore suspend/resume), and true in
> all other cases.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Queued up for 4.2, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-15 23:15 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-29 15:24 [PATCH v2] PM / Domains: Skip timings during syscore suspend/resume Geert Uytterhoeven
2015-06-04 12:17 ` Ulf Hansson
2015-06-15 23:41 ` Rafael J. Wysocki

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).