All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] PM / Domains: Restore lock-less behaviour for the genpd syscore APIs
@ 2017-02-07 15:53 Ulf Hansson
  2017-02-07 18:29 ` Kevin Hilman
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ulf Hansson @ 2017-02-07 15:53 UTC (permalink / raw)
  To: Rafael J. Wysocki, Ulf Hansson, linux-pm
  Cc: Len Brown, Pavel Machek, Kevin Hilman, Geert Uytterhoeven,
	Lina Iyer, Jon Hunter, Marek Szyprowski, Brian Norris

The commit ef4f7e2c8335 ("PM / Domains: Fix asynchronous execution of
*noirq() callbacks") went too far, as it not only changed to use locks for
the *noirq callbacks, but also for the genpd syscore APIs.

This cause the following error, reported by Geert Uytterhoeven:

"This causes the following BUG on all my Renesas arm32 boards, where the
system timer is an IRQ safe device:

BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:232
in_atomic(): 0, irqs_disabled(): 128, pid: 1751, name: s2ram
CPU: 0 PID: 1751 Comm: s2ram Not tainted
4.10.0-rc7-koelsch-05643-g27f4c73972a614fe #3354
Hardware name: Generic R8A7791 (Flattened Device Tree)
[<c020e9c4>] (unwind_backtrace) from [<c020a40c>] (show_stack+0x10/0x14)
[<c020a40c>] (show_stack) from [<c03f9818>] (dump_stack+0x7c/0x9c)
[<c03f9818>] (dump_stack) from [<c0240020>] (___might_sleep+0x124/0x160)
[<c0240020>] (___might_sleep) from [<c06fedfc>] (mutex_lock+0x18/0x60)
[<c06fedfc>] (mutex_lock) from [<c04de340>]
(genpd_syscore_switch+0x2c/0x7c)
[<c04de340>] (genpd_syscore_switch) from [<c05ec328>]
(sh_cmt_clock_event_suspend+0x18/0x28)
[<c05ec328>] (sh_cmt_clock_event_suspend) from [<c027f9a4>]
(clockevents_suspend+0x40/0x54)
[<c027f9a4>] (clockevents_suspend) from [<c0276d48>]
(timekeeping_suspend+0x23c/0x278)
[<c0276d48>] (timekeeping_suspend) from [<c04cbb88>]
(syscore_suspend+0x88/0x138)
[<c04cbb88>] (syscore_suspend) from [<c025f618>]
(suspend_devices_and_enter+0x290/0x470)
[<c025f618>] (suspend_devices_and_enter) from [<c025fa20>]
(pm_suspend+0x228/0x280)
[<c025fa20>] (pm_suspend) from [<c025e60c>] (state_store+0xac/0xcc)
[<c025e60c>] (state_store) from [<c0340c4c>] (kernfs_fop_write+0x160/0x19c)
[<c0340c4c>] (kernfs_fop_write) from [<c02e3054>] (__vfs_write+0x20/0x108)
[<c02e3054>] (__vfs_write) from [<c02e4424>] (vfs_write+0xb8/0x144)
[<c02e4424>] (vfs_write) from [<c02e5014>] (SyS_write+0x40/0x80)
[<c02e5014>] (SyS_write) from [<c0206cc0>] (ret_fast_syscall+0x0/0x34)"

To fix this problem, restore the lock-less behaviour. However only for the
genpd syscore APIs.

Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Fixes: ef4f7e2c8335 ("PM / Domains: Fix asynchronous execution of *noirq() callbacks")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 48 +++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 271e208..3a75fb1 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -729,16 +729,17 @@ static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
 /**
  * genpd_sync_power_off - Synchronously power off a PM domain and its masters.
  * @genpd: PM domain to power off, if possible.
+ * @use_lock: use the lock.
  * @depth: nesting count for lockdep.
  *
  * 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.
  *
  * This function is only called in "noirq" and "syscore" stages of system power
- * transitions, but since the "noirq" callbacks may be executed asynchronously,
- * the lock must be held.
+ * transitions. The "noirq" callbacks may be executed asynchronously, thus in
+ * these cases the lock must be held.
  */
-static void genpd_sync_power_off(struct generic_pm_domain *genpd,
+static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 				 unsigned int depth)
 {
 	struct gpd_link *link;
@@ -759,22 +760,27 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd,
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
 		genpd_sd_counter_dec(link->master);
 
-		genpd_lock_nested(link->master, depth + 1);
-		genpd_sync_power_off(link->master, depth + 1);
-		genpd_unlock(link->master);
+		if (use_lock)
+			genpd_lock_nested(link->master, depth + 1);
+
+		genpd_sync_power_off(link->master, use_lock, depth + 1);
+
+		if (use_lock)
+			genpd_unlock(link->master);
 	}
 }
 
 /**
  * genpd_sync_power_on - Synchronously power on a PM domain and its masters.
  * @genpd: PM domain to power on.
+ * @use_lock: use the lock.
  * @depth: nesting count for lockdep.
  *
  * This function is only called in "noirq" and "syscore" stages of system power
- * transitions, but since the "noirq" callbacks may be executed asynchronously,
- * the lock must be held.
+ * transitions. The "noirq" callbacks may be executed asynchronously, thus in
+ * these cases the lock must be held.
  */
-static void genpd_sync_power_on(struct generic_pm_domain *genpd,
+static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 				unsigned int depth)
 {
 	struct gpd_link *link;
@@ -785,9 +791,13 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd,
 	list_for_each_entry(link, &genpd->slave_links, slave_node) {
 		genpd_sd_counter_inc(link->master);
 
-		genpd_lock_nested(link->master, depth + 1);
-		genpd_sync_power_on(link->master, depth + 1);
-		genpd_unlock(link->master);
+		if (use_lock)
+			genpd_lock_nested(link->master, depth + 1);
+
+		genpd_sync_power_on(link->master, use_lock, depth + 1);
+
+		if (use_lock)
+			genpd_unlock(link->master);
 	}
 
 	_genpd_power_on(genpd, false);
@@ -898,7 +908,7 @@ static int pm_genpd_suspend_noirq(struct device *dev)
 
 	genpd_lock(genpd);
 	genpd->suspended_count++;
-	genpd_sync_power_off(genpd, 0);
+	genpd_sync_power_off(genpd, true, 0);
 	genpd_unlock(genpd);
 
 	return 0;
@@ -925,7 +935,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
 		return 0;
 
 	genpd_lock(genpd);
-	genpd_sync_power_on(genpd, 0);
+	genpd_sync_power_on(genpd, true, 0);
 	genpd->suspended_count--;
 	genpd_unlock(genpd);
 
@@ -1016,7 +1026,7 @@ static int pm_genpd_restore_noirq(struct device *dev)
 		 */
 		genpd->status = GPD_STATE_POWER_OFF;
 
-	genpd_sync_power_on(genpd, 0);
+	genpd_sync_power_on(genpd, true, 0);
 	genpd_unlock(genpd);
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start)
@@ -1070,17 +1080,13 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
 	if (!pm_genpd_present(genpd))
 		return;
 
-	genpd_lock(genpd);
-
 	if (suspend) {
 		genpd->suspended_count++;
-		genpd_sync_power_off(genpd, 0);
+		genpd_sync_power_off(genpd, false, 0);
 	} else {
-		genpd_sync_power_on(genpd, 0);
+		genpd_sync_power_on(genpd, false, 0);
 		genpd->suspended_count--;
 	}
-
-	genpd_unlock(genpd);
 }
 
 void pm_genpd_syscore_poweroff(struct device *dev)
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-08 12:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-07 15:53 [PATCH] PM / Domains: Restore lock-less behaviour for the genpd syscore APIs Ulf Hansson
2017-02-07 18:29 ` Kevin Hilman
2017-02-08 12:06   ` Ulf Hansson
2017-02-08 12:06     ` Rafael J. Wysocki
2017-02-08 12:13       ` Ulf Hansson
2017-02-08 12:15         ` Rafael J. Wysocki
2017-02-08 12:35           ` Ulf Hansson
2017-02-07 19:09 ` Geert Uytterhoeven
2017-02-07 21:52 ` Lina Iyer
2017-02-08 12:41   ` Ulf Hansson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.