[PATCH v3 0/8] PM / Domains: Bunch of small improvements and fixes

[PATCH v3 0/8] PM / Domains: Bunch of small improvements and fixes

[Krzysztof Kozlowski]

Hi,

Changes since v2:
=================
1. Add Ulf's acks.
2. Re-order patches so the un-acked commits are at the end.

Changes since v1:
=================
1. Patch 2/8: Follow Ulf's advice and use genpd_lookup_dev() which also
   solves risk of calling this for non-genpd (thus I added Ulf's
   Reported-by).



The last patch is RFC as this brings small overhead.

Best regards,
Krzysztof


Krzysztof Kozlowski (8):
  PM / Domains: Constify genpd pointer
  PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd
    device
  PM / Domains: Fix unsafe iteration over modified list of device links
  PM / Domains: Fix unsafe iteration over modified list of domain
    providers
  PM / Domains: Fix unsafe iteration over modified list of domains
  PM / Domains: Fix missing default_power_down_ok comment
  PM / Domains: Add lockdep asserts for domains list mutex
  PM / Domains: Add asserts for PM domain locks

 drivers/base/power/domain.c          | 63 +++++++++++++++++++++++++++---------
 drivers/base/power/domain_governor.c | 12 +++----
 2 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.11.0

[PATCH v3 1/8] PM / Domains: Constify genpd pointer

[Krzysztof Kozlowski]

Mark pointer to struct generic_pm_domain const (either passed in
argument or used localy in a function), whenever it is not modifed by
the function itself.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index df2b282f48e5..01e31d9f6c94 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -126,7 +126,7 @@ static const struct genpd_lock_ops genpd_spin_ops = {
 #define genpd_is_always_on(genpd)	(genpd->flags & GENPD_FLAG_ALWAYS_ON)
 
 static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
-		struct generic_pm_domain *genpd)
+		const struct generic_pm_domain *genpd)
 {
 	bool ret;
 
@@ -181,12 +181,14 @@ static struct generic_pm_domain *dev_to_genpd(struct device *dev)
 	return pd_to_genpd(dev->pm_domain);
 }
 
-static int genpd_stop_dev(struct generic_pm_domain *genpd, struct device *dev)
+static int genpd_stop_dev(const struct generic_pm_domain *genpd,
+			  struct device *dev)
 {
 	return GENPD_DEV_CALLBACK(genpd, int, stop, dev);
 }
 
-static int genpd_start_dev(struct generic_pm_domain *genpd, struct device *dev)
+static int genpd_start_dev(const struct generic_pm_domain *genpd,
+			   struct device *dev)
 {
 	return GENPD_DEV_CALLBACK(genpd, int, start, dev);
 }
@@ -738,7 +740,7 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd)
 
 #ifdef CONFIG_PM_SLEEP
 
-static bool genpd_dev_active_wakeup(struct generic_pm_domain *genpd,
+static bool genpd_dev_active_wakeup(const struct generic_pm_domain *genpd,
 				    struct device *dev)
 {
 	return GENPD_DEV_CALLBACK(genpd, bool, active_wakeup, dev);
@@ -840,7 +842,8 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
  * signal remote wakeup from the system's working state as needed by runtime PM.
  * Return 'true' in either of the above cases.
  */
-static bool resume_needed(struct device *dev, struct generic_pm_domain *genpd)
+static bool resume_needed(struct device *dev,
+			  const struct generic_pm_domain *genpd)
 {
 	bool active_wakeup;
 
@@ -975,7 +978,7 @@ static int pm_genpd_resume_noirq(struct device *dev)
  */
 static int pm_genpd_freeze_noirq(struct device *dev)
 {
-	struct generic_pm_domain *genpd;
+	const struct generic_pm_domain *genpd;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
@@ -999,7 +1002,7 @@ static int pm_genpd_freeze_noirq(struct device *dev)
  */
 static int pm_genpd_thaw_noirq(struct device *dev)
 {
-	struct generic_pm_domain *genpd;
+	const struct generic_pm_domain *genpd;
 	int ret = 0;
 
 	dev_dbg(dev, "%s()\n", __func__);
-- 
2.11.0

[PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Krzysztof Kozlowski]

genpd_syscore_switch() had two problems:
1. It silently assumed that device, it is being called for, belongs to
   generic power domain and used container_of() on its power domain
   pointer.  Such assumption might not be true always.

2. It iterated over list of generic power domains without holding
   gpd_list_lock mutex thus list could have been modified in the same
   time.

Usage of genpd_lookup_dev() solves both problems as it is safe a call
for non-generic power domains and uses mutex when iterating.

Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

---

Not tested on real hardware.
---
 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 01e31d9f6c94..d31a4434b8b3 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
 {
 	struct generic_pm_domain *genpd;
 
-	genpd = dev_to_genpd(dev);
-	if (!pm_genpd_present(genpd))
+	genpd = genpd_lookup_dev(dev);
+	if (!genpd)
 		return;
 
 	if (suspend) {
-- 
2.11.0

[PATCH v3 3/8] PM / Domains: Fix unsafe iteration over modified list of device links

[Krzysztof Kozlowski]

pm_genpd_remove_subdomain() iterates over domain's master_links list and
removes matching element thus it has to use safe version of list
iteration.

Fixes: f721889ff65a ("PM / Domains: Support for generic I/O PM domains (v8)")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 d31a4434b8b3..ebbce6ef0850 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1396,7 +1396,7 @@ EXPORT_SYMBOL_GPL(pm_genpd_add_subdomain);
 int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 			      struct generic_pm_domain *subdomain)
 {
-	struct gpd_link *link;
+	struct gpd_link *l, *link;
 	int ret = -EINVAL;
 
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
@@ -1412,7 +1412,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 		goto out;
 	}
 
-	list_for_each_entry(link, &genpd->master_links, master_node) {
+	list_for_each_entry_safe(link, l, &genpd->master_links, master_node) {
 		if (link->slave != subdomain)
 			continue;
 
-- 
2.11.0

[PATCH v3 4/8] PM / Domains: Fix unsafe iteration over modified list of domain providers

[Krzysztof Kozlowski]

of_genpd_del_provider() iterates over list of domain provides and
removes matching element thus it has to use safe version of list
iteration.

Fixes: aa42240ab254 ("PM / Domains: Add generic OF-based PM domain look-up")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 ebbce6ef0850..793f9f7f3f2d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1783,12 +1783,12 @@ EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
  */
 void of_genpd_del_provider(struct device_node *np)
 {
-	struct of_genpd_provider *cp;
+	struct of_genpd_provider *cp, *tmp;
 	struct generic_pm_domain *gpd;
 
 	mutex_lock(&gpd_list_lock);
 	mutex_lock(&of_genpd_mutex);
-	list_for_each_entry(cp, &of_genpd_providers, link) {
+	list_for_each_entry_safe(cp, tmp, &of_genpd_providers, link) {
 		if (cp->node == np) {
 			/*
 			 * For each PM domain associated with the
-- 
2.11.0

[PATCH v3 5/8] PM / Domains: Fix unsafe iteration over modified list of domains

[Krzysztof Kozlowski]

of_genpd_remove_last() iterates over list of domains and removes
matching element thus it has to use safe version of list iteration.

Fixes: 17926551c98a ("PM / Domains: Add support for removing nested PM domains by provider")
Cc: <stable@vger.kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 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 793f9f7f3f2d..be34cbf3793e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1928,14 +1928,14 @@ EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
  */
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
 {
-	struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
+	struct generic_pm_domain *gpd, *tmp, *genpd = ERR_PTR(-ENOENT);
 	int ret;
 
 	if (IS_ERR_OR_NULL(np))
 		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&gpd_list_lock);
-	list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
+	list_for_each_entry_safe(gpd, tmp, &gpd_list, gpd_list_node) {
 		if (gpd->provider == &np->fwnode) {
 			ret = genpd_remove(gpd);
 			genpd = ret ? ERR_PTR(ret) : gpd;
-- 
2.11.0

[PATCH v3 6/8] PM / Domains: Fix missing default_power_down_ok comment

[Krzysztof Kozlowski]

Commit fc5cbf0c94b6 ("PM / Domains: Support for multiple states") split
out some code out of default_power_down_ok() function so the
documentation has to be moved to appropriate place.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain_governor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 2e0fce711135..281f949c5ffe 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -92,12 +92,6 @@ static bool default_suspend_ok(struct device *dev)
 	return td->cached_suspend_ok;
 }
 
-/**
- * default_power_down_ok - Default generic PM domain power off governor routine.
- * @pd: PM domain to check.
- *
- * This routine must be executed under the PM domain's lock.
- */
 static bool __default_power_down_ok(struct dev_pm_domain *pd,
 				     unsigned int state)
 {
@@ -187,6 +181,12 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
 	return true;
 }
 
+/**
+ * default_power_down_ok - Default generic PM domain power off governor routine.
+ * @pd: PM domain to check.
+ *
+ * This routine must be executed under the PM domain's lock.
+ */
 static bool default_power_down_ok(struct dev_pm_domain *pd)
 {
 	struct generic_pm_domain *genpd = pd_to_genpd(pd);
-- 
2.11.0

[PATCH v3 7/8] PM / Domains: Add lockdep asserts for domains list mutex

[Krzysztof Kozlowski]

Add lockdep checks for holding mutex protecting the list of domains.
This might expose misuse even though only file-scope functions use it
for now.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/base/power/domain.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index be34cbf3793e..1a47c5ff6a2f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -726,6 +726,8 @@ static bool pm_genpd_present(const struct generic_pm_domain *genpd)
 {
 	const struct generic_pm_domain *gpd;
 
+	lockdep_assert_held(&gpd_list_lock);
+
 	if (IS_ERR_OR_NULL(genpd))
 		return false;
 
@@ -1321,6 +1323,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
 	struct gpd_link *link, *itr;
 	int ret = 0;
 
+	lockdep_assert_held(&gpd_list_lock);
+
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
 	    || genpd == subdomain)
 		return -EINVAL;
@@ -1528,6 +1532,8 @@ static int genpd_remove(struct generic_pm_domain *genpd)
 {
 	struct gpd_link *l, *link;
 
+	lockdep_assert_held(&gpd_list_lock);
+
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-- 
2.11.0

[RFC v3 8/8] PM / Domains: Add asserts for PM domain locks

[Krzysztof Kozlowski]

Add lockdep checks for holding domain lock in few places where this is
required.  This might expose misuse even though only file-scope
functions use it for now.

Regular lockdep asserts can be entirely discarded by preprocessor, however
domain code uses mixed type of lock: spinlock or mutex.  This means that
these asserts will not be thrown away entirely.  Instead, always
at least two pointer dereferences (p->lock_ops->assert_held) and
probably one function call (assert_held()) will be made.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/base/power/domain.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 1a47c5ff6a2f..1c3bd7434675 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -40,12 +40,18 @@ static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
 struct genpd_lock_ops {
+	void (*assert_held)(struct generic_pm_domain *genpd);
 	void (*lock)(struct generic_pm_domain *genpd);
 	void (*lock_nested)(struct generic_pm_domain *genpd, int depth);
 	int (*lock_interruptible)(struct generic_pm_domain *genpd);
 	void (*unlock)(struct generic_pm_domain *genpd);
 };
 
+static void genpd_assert_held_mtx(struct generic_pm_domain *genpd)
+{
+	lockdep_assert_held(&genpd->mlock);
+}
+
 static void genpd_lock_mtx(struct generic_pm_domain *genpd)
 {
 	mutex_lock(&genpd->mlock);
@@ -68,12 +74,18 @@ static void genpd_unlock_mtx(struct generic_pm_domain *genpd)
 }
 
 static const struct genpd_lock_ops genpd_mtx_ops = {
+	.assert_held = genpd_assert_held_mtx,
 	.lock = genpd_lock_mtx,
 	.lock_nested = genpd_lock_nested_mtx,
 	.lock_interruptible = genpd_lock_interruptible_mtx,
 	.unlock = genpd_unlock_mtx,
 };
 
+static void genpd_assert_held_spin(struct generic_pm_domain *genpd)
+{
+	lockdep_assert_held(&genpd->slock);
+}
+
 static void genpd_lock_spin(struct generic_pm_domain *genpd)
 	__acquires(&genpd->slock)
 {
@@ -110,12 +122,14 @@ static void genpd_unlock_spin(struct generic_pm_domain *genpd)
 }
 
 static const struct genpd_lock_ops genpd_spin_ops = {
+	.assert_held = genpd_assert_held_spin,
 	.lock = genpd_lock_spin,
 	.lock_nested = genpd_lock_nested_spin,
 	.lock_interruptible = genpd_lock_interruptible_spin,
 	.unlock = genpd_unlock_spin,
 };
 
+#define genpd_assert_held(p)		p->lock_ops->assert_held(p)
 #define genpd_lock(p)			p->lock_ops->lock(p)
 #define genpd_lock_nested(p, d)		p->lock_ops->lock_nested(p, d)
 #define genpd_lock_interruptible(p)	p->lock_ops->lock_interruptible(p)
@@ -299,6 +313,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	struct gpd_link *link;
 	unsigned int not_suspended = 0;
 
+	genpd_assert_held(genpd);
+
 	/*
 	 * Do not try to power off the domain in the following situations:
 	 * (1) The domain is already in the "power off" state.
@@ -385,6 +401,8 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
 	struct gpd_link *link;
 	int ret = 0;
 
+	genpd_assert_held(genpd);
+
 	if (genpd_status_on(genpd))
 		return 0;
 
@@ -766,6 +784,9 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 {
 	struct gpd_link *link;
 
+	if (use_lock)
+		genpd_assert_held(genpd);
+
 	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
 		return;
 
@@ -808,6 +829,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
 {
 	struct gpd_link *link;
 
+	if (use_lock)
+		genpd_assert_held(genpd);
+
 	if (genpd_status_on(genpd))
 		return;
 
-- 
2.11.0

Re: [PATCH v3 0/8] PM / Domains: Bunch of small improvements and fixes

[Rafael J. Wysocki]

On Wednesday, June 28, 2017 04:56:15 PM Krzysztof Kozlowski wrote:
> Hi,
> 
> Changes since v2:
> =================
> 1. Add Ulf's acks.
> 2. Re-order patches so the un-acked commits are at the end.
> 
> Changes since v1:
> =================
> 1. Patch 2/8: Follow Ulf's advice and use genpd_lookup_dev() which also
>    solves risk of calling this for non-genpd (thus I added Ulf's
>    Reported-by).

[1-6/8] applied, the remaining two need ACKs from the genpd people.

Thanks,
Rafael

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Geert Uytterhoeven]

Hi Krzysztof, Rafael,

On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> genpd_syscore_switch() had two problems:
> 1. It silently assumed that device, it is being called for, belongs to
>    generic power domain and used container_of() on its power domain
>    pointer.  Such assumption might not be true always.
>
> 2. It iterated over list of generic power domains without holding
>    gpd_list_lock mutex thus list could have been modified in the same
>    time.
>
> Usage of genpd_lookup_dev() solves both problems as it is safe a call
> for non-generic power domains and uses mutex when iterating.
>
> Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
request "[GIT PULL] Power management updates for v4.13-rc1".

> Not tested on real hardware.

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

PM: Syncing filesystems ... done.
PM: Preparing system for sleep (mem)
Freezing user space processes ... (elapsed 0.001 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
PM: Suspending system (mem)
PM: suspend of devices complete after 122.841 msecs
PM: suspend devices took 0.130 seconds
PM: late suspend of devices complete after 2.682 msecs
PM: noirq suspend of devices complete after 29.951 msecs
Disabling non-boot CPUs ...
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
CPU: 0 PID: 1730 Comm: s2ram Not tainted
4.12.0-koelsch-07061-g810fee9afeba15ef #3592
Hardware name: Generic R8A7791 (Flattened Device Tree)
[<c020e9f4>] (unwind_backtrace) from [<c020a484>] (show_stack+0x10/0x14)
[<c020a484>] (show_stack) from [<c04017e8>] (dump_stack+0x7c/0x9c)
[<c04017e8>] (dump_stack) from [<c0240284>] (___might_sleep+0x124/0x160)
[<c0240284>] (___might_sleep) from [<c0717cfc>] (mutex_lock+0x18/0x60)
[<c0717cfc>] (mutex_lock) from [<c04de11c>] (genpd_lookup_dev+0x38/0x94)
[<c04de11c>] (genpd_lookup_dev) from [<c04dfd34>]
(pm_genpd_syscore_poweroff+0x8/0x2c)
[<c04dfd34>] (pm_genpd_syscore_poweroff) from [<c05fcb70>]
(sh_cmt_clock_event_suspend+0x18/0x28)
[<c05fcb70>] (sh_cmt_clock_event_suspend) from [<c027f174>]
(clockevents_suspend+0x40/0x54)
[<c027f174>] (clockevents_suspend) from [<c02762d8>]
(timekeeping_suspend+0x23c/0x278)
[<c02762d8>] (timekeeping_suspend) from [<c04ce028>]
(syscore_suspend+0x88/0x138)
[<c04ce028>] (syscore_suspend) from [<c025d740>]
(suspend_devices_and_enter+0x308/0x4fc)
[<c025d740>] (suspend_devices_and_enter) from [<c025db5c>]
(pm_suspend+0x228/0x280)
[<c025db5c>] (pm_suspend) from [<c025c6b8>] (state_store+0xac/0xcc)
[<c025c6b8>] (state_store) from [<c0342f9c>] (kernfs_fop_write+0x170/0x1b0)
[<c0342f9c>] (kernfs_fop_write) from [<c02e47dc>] (__vfs_write+0x20/0x108)
[<c02e47dc>] (__vfs_write) from [<c02e5b80>] (vfs_write+0xb8/0x144)
[<c02e5b80>] (vfs_write) from [<c02e6804>] (SyS_write+0x40/0x80)
[<c02e6804>] (SyS_write) from [<c0206c40>] (ret_fast_syscall+0x0/0x34)

Reverting the commit fixes the issue.

> ---
>  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 01e31d9f6c94..d31a4434b8b3 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
>  {
>         struct generic_pm_domain *genpd;
>
> -       genpd = dev_to_genpd(dev);
> -       if (!pm_genpd_present(genpd))
> +       genpd = genpd_lookup_dev(dev);
> +       if (!genpd)
>                 return;
>
>         if (suspend) {

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 v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Krzysztof Kozlowski]

On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof, Rafael,
> 
> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > genpd_syscore_switch() had two problems:
> > 1. It silently assumed that device, it is being called for, belongs to
> >    generic power domain and used container_of() on its power domain
> >    pointer.  Such assumption might not be true always.
> >
> > 2. It iterated over list of generic power domains without holding
> >    gpd_list_lock mutex thus list could have been modified in the same
> >    time.
> >
> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
> > for non-generic power domains and uses mutex when iterating.
> >
> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
> request "[GIT PULL] Power management updates for v4.13-rc1".
> 
> > Not tested on real hardware.
> 
> This causes the following BUG during s2ram on all my Renesas arm32 boards,
> where the system timer is an IRQ safe device:
> 
> PM: Syncing filesystems ... done.
> PM: Preparing system for sleep (mem)
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> OOM killer disabled.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Suspending system (mem)
> PM: suspend of devices complete after 122.841 msecs
> PM: suspend devices took 0.130 seconds
> PM: late suspend of devices complete after 2.682 msecs
> PM: noirq suspend of devices complete after 29.951 msecs
> Disabling non-boot CPUs ...
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238

Hi,

Thanks for report!

Damn it, although I couldn't find this in the code, but I was fearing
that this ends up in atomic section. That would kind of explain why
mutex was not there [1].

Anyway, the buggy code was there already. Instead of "sleeping in atomic
section" there was no locking at all... In this context this was
probably safe because it was executed *after* disabling non-boot CPUs
but then the function cannot be called in other contexts.

I am not sure I will be capable of developing the proper fix as I do not
have the hardware and I do not know all stuff happening in sh suspend.
Probably reverting this and living with non-locked path would be the
safest choice.

[1] https://patchwork.kernel.org/patch/9778903/

Best regards,
Krzysztof


> in_atomic(): 0, irqs_disabled(): 128, pid: 1730, name: s2ram
> CPU: 0 PID: 1730 Comm: s2ram Not tainted
> 4.12.0-koelsch-07061-g810fee9afeba15ef #3592
> Hardware name: Generic R8A7791 (Flattened Device Tree)
> [<c020e9f4>] (unwind_backtrace) from [<c020a484>] (show_stack+0x10/0x14)
> [<c020a484>] (show_stack) from [<c04017e8>] (dump_stack+0x7c/0x9c)
> [<c04017e8>] (dump_stack) from [<c0240284>] (___might_sleep+0x124/0x160)
> [<c0240284>] (___might_sleep) from [<c0717cfc>] (mutex_lock+0x18/0x60)
> [<c0717cfc>] (mutex_lock) from [<c04de11c>] (genpd_lookup_dev+0x38/0x94)
> [<c04de11c>] (genpd_lookup_dev) from [<c04dfd34>]
> (pm_genpd_syscore_poweroff+0x8/0x2c)
> [<c04dfd34>] (pm_genpd_syscore_poweroff) from [<c05fcb70>]
> (sh_cmt_clock_event_suspend+0x18/0x28)
> [<c05fcb70>] (sh_cmt_clock_event_suspend) from [<c027f174>]
> (clockevents_suspend+0x40/0x54)
> [<c027f174>] (clockevents_suspend) from [<c02762d8>]
> (timekeeping_suspend+0x23c/0x278)
> [<c02762d8>] (timekeeping_suspend) from [<c04ce028>]
> (syscore_suspend+0x88/0x138)
> [<c04ce028>] (syscore_suspend) from [<c025d740>]
> (suspend_devices_and_enter+0x308/0x4fc)
> [<c025d740>] (suspend_devices_and_enter) from [<c025db5c>]
> (pm_suspend+0x228/0x280)
> [<c025db5c>] (pm_suspend) from [<c025c6b8>] (state_store+0xac/0xcc)
> [<c025c6b8>] (state_store) from [<c0342f9c>] (kernfs_fop_write+0x170/0x1b0)
> [<c0342f9c>] (kernfs_fop_write) from [<c02e47dc>] (__vfs_write+0x20/0x108)
> [<c02e47dc>] (__vfs_write) from [<c02e5b80>] (vfs_write+0xb8/0x144)
> [<c02e5b80>] (vfs_write) from [<c02e6804>] (SyS_write+0x40/0x80)
> [<c02e6804>] (SyS_write) from [<c0206c40>] (ret_fast_syscall+0x0/0x34)
> 
> Reverting the commit fixes the issue.
> 
> > ---
> >  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 01e31d9f6c94..d31a4434b8b3 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1098,8 +1098,8 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
> >  {
> >         struct generic_pm_domain *genpd;
> >
> > -       genpd = dev_to_genpd(dev);
> > -       if (!pm_genpd_present(genpd))
> > +       genpd = genpd_lookup_dev(dev);
> > +       if (!genpd)
> >                 return;
> >
> >         if (suspend) {
> 
> 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 v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Geert Uytterhoeven]

Hi Krzysztof,

On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
>> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > genpd_syscore_switch() had two problems:
>> > 1. It silently assumed that device, it is being called for, belongs to
>> >    generic power domain and used container_of() on its power domain
>> >    pointer.  Such assumption might not be true always.
>> >
>> > 2. It iterated over list of generic power domains without holding
>> >    gpd_list_lock mutex thus list could have been modified in the same
>> >    time.
>> >
>> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
>> > for non-generic power domains and uses mutex when iterating.
>> >
>> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
>> request "[GIT PULL] Power management updates for v4.13-rc1".
>>
>> > Not tested on real hardware.
>>
>> This causes the following BUG during s2ram on all my Renesas arm32 boards,
>> where the system timer is an IRQ safe device:
>>
>> PM: Syncing filesystems ... done.
>> PM: Preparing system for sleep (mem)
>> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> OOM killer disabled.
>> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> PM: Suspending system (mem)
>> PM: suspend of devices complete after 122.841 msecs
>> PM: suspend devices took 0.130 seconds
>> PM: late suspend of devices complete after 2.682 msecs
>> PM: noirq suspend of devices complete after 29.951 msecs
>> Disabling non-boot CPUs ...
>> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
>
> Thanks for report!
>
> Damn it, although I couldn't find this in the code, but I was fearing
> that this ends up in atomic section. That would kind of explain why
> mutex was not there [1].
>
> Anyway, the buggy code was there already. Instead of "sleeping in atomic
> section" there was no locking at all... In this context this was
> probably safe because it was executed *after* disabling non-boot CPUs
> but then the function cannot be called in other contexts.
>
> I am not sure I will be capable of developing the proper fix as I do not
> have the hardware and I do not know all stuff happening in sh suspend.
> Probably reverting this and living with non-locked path would be the
> safest choice.
>
> [1] https://patchwork.kernel.org/patch/9778903/

AFAIU, all syscore stuff runs in atomic context.

Don't worry, you're not the only one.
This bug report was almost 100% the same as an earlier one for a patch
from Ulf ;-)
(cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
*noirq() callbacks")

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 v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Krzysztof Kozlowski]

On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> > genpd_syscore_switch() had two problems:
> >> > 1. It silently assumed that device, it is being called for, belongs to
> >> >    generic power domain and used container_of() on its power domain
> >> >    pointer.  Such assumption might not be true always.
> >> >
> >> > 2. It iterated over list of generic power domains without holding
> >> >    gpd_list_lock mutex thus list could have been modified in the same
> >> >    time.
> >> >
> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
> >> > for non-generic power domains and uses mutex when iterating.
> >> >
> >> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >>
> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
> >> request "[GIT PULL] Power management updates for v4.13-rc1".
> >>
> >> > Not tested on real hardware.
> >>
> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
> >> where the system timer is an IRQ safe device:
> >>
> >> PM: Syncing filesystems ... done.
> >> PM: Preparing system for sleep (mem)
> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
> >> OOM killer disabled.
> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> >> PM: Suspending system (mem)
> >> PM: suspend of devices complete after 122.841 msecs
> >> PM: suspend devices took 0.130 seconds
> >> PM: late suspend of devices complete after 2.682 msecs
> >> PM: noirq suspend of devices complete after 29.951 msecs
> >> Disabling non-boot CPUs ...
> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> >
> > Thanks for report!
> >
> > Damn it, although I couldn't find this in the code, but I was fearing
> > that this ends up in atomic section. That would kind of explain why
> > mutex was not there [1].
> >
> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
> > section" there was no locking at all... In this context this was
> > probably safe because it was executed *after* disabling non-boot CPUs
> > but then the function cannot be called in other contexts.
> >
> > I am not sure I will be capable of developing the proper fix as I do not
> > have the hardware and I do not know all stuff happening in sh suspend.
> > Probably reverting this and living with non-locked path would be the
> > safest choice.
> >
> > [1] https://patchwork.kernel.org/patch/9778903/
> 
> AFAIU, all syscore stuff runs in atomic context.

Indeed... The confusing part is that this code is syscore only from
the name, it is not hooked in to syscore_ops. Although going by call
chain (through sh clocksource drivers) we end up in
timekeeping_suspend() which is a syscore op.

I wonder whether it would be useful - after reverting my commit - to add
an assert (which is a stronger API requirement than only documentation "may
only be called during the system core (syscore) suspend") like:
	WARN_ON(num_online_cpus() > 1));
as without mutexes this should not be executed with more than one online
CPU.

Best regards,
Krzysztof

> 
> Don't worry, you're not the only one.
> This bug report was almost 100% the same as an earlier one for a patch
> from Ulf ;-)
> (cfr. "[RESEND PATCH 0/2] PM / Domains: Fix asynchronous execution of
> *noirq() callbacks")
> 
> 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 v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Rafael J. Wysocki]

On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
>> Hi Krzysztof,
>>
>> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
>> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> >> > genpd_syscore_switch() had two problems:
>> >> > 1. It silently assumed that device, it is being called for, belongs to
>> >> >    generic power domain and used container_of() on its power domain
>> >> >    pointer.  Such assumption might not be true always.
>> >> >
>> >> > 2. It iterated over list of generic power domains without holding
>> >> >    gpd_list_lock mutex thus list could have been modified in the same
>> >> >    time.
>> >> >
>> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
>> >> > for non-generic power domains and uses mutex when iterating.
>> >> >
>> >> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> >> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >>
>> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
>> >> request "[GIT PULL] Power management updates for v4.13-rc1".
>> >>
>> >> > Not tested on real hardware.
>> >>
>> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
>> >> where the system timer is an IRQ safe device:
>> >>
>> >> PM: Syncing filesystems ... done.
>> >> PM: Preparing system for sleep (mem)
>> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> >> OOM killer disabled.
>> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> >> PM: Suspending system (mem)
>> >> PM: suspend of devices complete after 122.841 msecs
>> >> PM: suspend devices took 0.130 seconds
>> >> PM: late suspend of devices complete after 2.682 msecs
>> >> PM: noirq suspend of devices complete after 29.951 msecs
>> >> Disabling non-boot CPUs ...
>> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
>> >
>> > Thanks for report!
>> >
>> > Damn it, although I couldn't find this in the code, but I was fearing
>> > that this ends up in atomic section. That would kind of explain why
>> > mutex was not there [1].
>> >
>> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
>> > section" there was no locking at all... In this context this was
>> > probably safe because it was executed *after* disabling non-boot CPUs
>> > but then the function cannot be called in other contexts.
>> >
>> > I am not sure I will be capable of developing the proper fix as I do not
>> > have the hardware and I do not know all stuff happening in sh suspend.
>> > Probably reverting this and living with non-locked path would be the
>> > safest choice.
>> >
>> > [1] https://patchwork.kernel.org/patch/9778903/
>>
>> AFAIU, all syscore stuff runs in atomic context.
>
> Indeed... The confusing part is that this code is syscore only from
> the name, it is not hooked in to syscore_ops. Although going by call
> chain (through sh clocksource drivers) we end up in
> timekeeping_suspend() which is a syscore op.
>
> I wonder whether it would be useful - after reverting my commit - to add
> an assert (which is a stronger API requirement than only documentation "may
> only be called during the system core (syscore) suspend") like:
>         WARN_ON(num_online_cpus() > 1));
> as without mutexes this should not be executed with more than one online
> CPU.

Or maybe WARN_ON_ONCE(!in_atomic())?

I'm queuing up a revert of the $subject commit.

Thanks,
Rafael

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Krzysztof Kozlowski]

On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
> >> Hi Krzysztof,
> >>
> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
> >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >> >> > genpd_syscore_switch() had two problems:
> >> >> > 1. It silently assumed that device, it is being called for, belongs to
> >> >> >    generic power domain and used container_of() on its power domain
> >> >> >    pointer.  Such assumption might not be true always.
> >> >> >
> >> >> > 2. It iterated over list of generic power domains without holding
> >> >> >    gpd_list_lock mutex thus list could have been modified in the same
> >> >> >    time.
> >> >> >
> >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
> >> >> > for non-generic power domains and uses mutex when iterating.
> >> >> >
> >> >> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> >> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> >> >>
> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
> >> >> request "[GIT PULL] Power management updates for v4.13-rc1".
> >> >>
> >> >> > Not tested on real hardware.
> >> >>
> >> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
> >> >> where the system timer is an IRQ safe device:
> >> >>
> >> >> PM: Syncing filesystems ... done.
> >> >> PM: Preparing system for sleep (mem)
> >> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
> >> >> OOM killer disabled.
> >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> >> >> PM: Suspending system (mem)
> >> >> PM: suspend of devices complete after 122.841 msecs
> >> >> PM: suspend devices took 0.130 seconds
> >> >> PM: late suspend of devices complete after 2.682 msecs
> >> >> PM: noirq suspend of devices complete after 29.951 msecs
> >> >> Disabling non-boot CPUs ...
> >> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> >> >
> >> > Thanks for report!
> >> >
> >> > Damn it, although I couldn't find this in the code, but I was fearing
> >> > that this ends up in atomic section. That would kind of explain why
> >> > mutex was not there [1].
> >> >
> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
> >> > section" there was no locking at all... In this context this was
> >> > probably safe because it was executed *after* disabling non-boot CPUs
> >> > but then the function cannot be called in other contexts.
> >> >
> >> > I am not sure I will be capable of developing the proper fix as I do not
> >> > have the hardware and I do not know all stuff happening in sh suspend.
> >> > Probably reverting this and living with non-locked path would be the
> >> > safest choice.
> >> >
> >> > [1] https://patchwork.kernel.org/patch/9778903/
> >>
> >> AFAIU, all syscore stuff runs in atomic context.
> >
> > Indeed... The confusing part is that this code is syscore only from
> > the name, it is not hooked in to syscore_ops. Although going by call
> > chain (through sh clocksource drivers) we end up in
> > timekeeping_suspend() which is a syscore op.
> >
> > I wonder whether it would be useful - after reverting my commit - to add
> > an assert (which is a stronger API requirement than only documentation "may
> > only be called during the system core (syscore) suspend") like:
> >         WARN_ON(num_online_cpus() > 1));
> > as without mutexes this should not be executed with more than one online
> > CPU.
> 
> Or maybe WARN_ON_ONCE(!in_atomic())?

You could be in atomic section on this CPU and still have other CPUs
online playing with gpd_list (without any protection from locking).
This function is safe only on non-SMP case.

Best regards,
Krzysztof
 
> I'm queuing up a revert of the $subject commit.
> 
> Thanks,
> Rafael

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Rafael J. Wysocki]

On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 09:54:10PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Jul 4, 2017 at 8:36 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > On Tue, Jul 04, 2017 at 08:19:47PM +0200, Geert Uytterhoeven wrote:
>> >> Hi Krzysztof,
>> >>
>> >> On Tue, Jul 4, 2017 at 8:10 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> >> > On Tue, Jul 04, 2017 at 03:01:15PM +0200, Geert Uytterhoeven wrote:
>> >> >> On Wed, Jun 28, 2017 at 4:56 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> >> >> > genpd_syscore_switch() had two problems:
>> >> >> > 1. It silently assumed that device, it is being called for, belongs to
>> >> >> >    generic power domain and used container_of() on its power domain
>> >> >> >    pointer.  Such assumption might not be true always.
>> >> >> >
>> >> >> > 2. It iterated over list of generic power domains without holding
>> >> >> >    gpd_list_lock mutex thus list could have been modified in the same
>> >> >> >    time.
>> >> >> >
>> >> >> > Usage of genpd_lookup_dev() solves both problems as it is safe a call
>> >> >> > for non-generic power domains and uses mutex when iterating.
>> >> >> >
>> >> >> > Reported-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> >> >> > Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> >>
>> >> >> This is commit 8b55e55ee44356d6 in pm/linux-next, also part of the pull
>> >> >> request "[GIT PULL] Power management updates for v4.13-rc1".
>> >> >>
>> >> >> > Not tested on real hardware.
>> >> >>
>> >> >> This causes the following BUG during s2ram on all my Renesas arm32 boards,
>> >> >> where the system timer is an IRQ safe device:
>> >> >>
>> >> >> PM: Syncing filesystems ... done.
>> >> >> PM: Preparing system for sleep (mem)
>> >> >> Freezing user space processes ... (elapsed 0.001 seconds) done.
>> >> >> OOM killer disabled.
>> >> >> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
>> >> >> PM: Suspending system (mem)
>> >> >> PM: suspend of devices complete after 122.841 msecs
>> >> >> PM: suspend devices took 0.130 seconds
>> >> >> PM: late suspend of devices complete after 2.682 msecs
>> >> >> PM: noirq suspend of devices complete after 29.951 msecs
>> >> >> Disabling non-boot CPUs ...
>> >> >> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
>> >> >
>> >> > Thanks for report!
>> >> >
>> >> > Damn it, although I couldn't find this in the code, but I was fearing
>> >> > that this ends up in atomic section. That would kind of explain why
>> >> > mutex was not there [1].
>> >> >
>> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
>> >> > section" there was no locking at all... In this context this was
>> >> > probably safe because it was executed *after* disabling non-boot CPUs
>> >> > but then the function cannot be called in other contexts.
>> >> >
>> >> > I am not sure I will be capable of developing the proper fix as I do not
>> >> > have the hardware and I do not know all stuff happening in sh suspend.
>> >> > Probably reverting this and living with non-locked path would be the
>> >> > safest choice.
>> >> >
>> >> > [1] https://patchwork.kernel.org/patch/9778903/
>> >>
>> >> AFAIU, all syscore stuff runs in atomic context.
>> >
>> > Indeed... The confusing part is that this code is syscore only from
>> > the name, it is not hooked in to syscore_ops. Although going by call
>> > chain (through sh clocksource drivers) we end up in
>> > timekeeping_suspend() which is a syscore op.
>> >
>> > I wonder whether it would be useful - after reverting my commit - to add
>> > an assert (which is a stronger API requirement than only documentation "may
>> > only be called during the system core (syscore) suspend") like:
>> >         WARN_ON(num_online_cpus() > 1));
>> > as without mutexes this should not be executed with more than one online
>> > CPU.
>>
>> Or maybe WARN_ON_ONCE(!in_atomic())?
>
> You could be in atomic section on this CPU and still have other CPUs
> online playing with gpd_list (without any protection from locking).
> This function is safe only on non-SMP case.

Well, not quite.

It is safe if you can guarantee that no other CPUs will touch the data
structure in question concurrently, which pretty much is the case for
timekeeping_suspend() even though it may be invoked without taking the
other CPUs offline (from the suspend-to-idle core path).

Thanks,
Rafael

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Krzysztof Kozlowski]

On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
 >> >> > Thanks for report!
> >> >> >
> >> >> > Damn it, although I couldn't find this in the code, but I was fearing
> >> >> > that this ends up in atomic section. That would kind of explain why
> >> >> > mutex was not there [1].
> >> >> >
> >> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
> >> >> > section" there was no locking at all... In this context this was
> >> >> > probably safe because it was executed *after* disabling non-boot CPUs
> >> >> > but then the function cannot be called in other contexts.
> >> >> >
> >> >> > I am not sure I will be capable of developing the proper fix as I do not
> >> >> > have the hardware and I do not know all stuff happening in sh suspend.
> >> >> > Probably reverting this and living with non-locked path would be the
> >> >> > safest choice.
> >> >> >
> >> >> > [1] https://patchwork.kernel.org/patch/9778903/
> >> >>
> >> >> AFAIU, all syscore stuff runs in atomic context.
> >> >
> >> > Indeed... The confusing part is that this code is syscore only from
> >> > the name, it is not hooked in to syscore_ops. Although going by call
> >> > chain (through sh clocksource drivers) we end up in
> >> > timekeeping_suspend() which is a syscore op.
> >> >
> >> > I wonder whether it would be useful - after reverting my commit - to add
> >> > an assert (which is a stronger API requirement than only documentation "may
> >> > only be called during the system core (syscore) suspend") like:
> >> >         WARN_ON(num_online_cpus() > 1));
> >> > as without mutexes this should not be executed with more than one online
> >> > CPU.
> >>
> >> Or maybe WARN_ON_ONCE(!in_atomic())?
> >
> > You could be in atomic section on this CPU and still have other CPUs
> > online playing with gpd_list (without any protection from locking).
> > This function is safe only on non-SMP case.
> 
> Well, not quite.
> 
> It is safe if you can guarantee that no other CPUs will touch the data
> structure in question concurrently, which pretty much is the case for
> timekeeping_suspend() even though it may be invoked without taking the
> other CPUs offline (from the suspend-to-idle core path).

Right, that would work fine for that case.

However I was rather thinking that we have an in-kernel API (exported)
so someone might by mistake try to use it in different contexts. For
example in some atomic section but on a platform which offlines CPUs
later. Thus it would be called in some imaginary suspend path but with
CPUs still being online. Partially it is already mentioned in documentation
although I am not sure that on every possible architecture syscore ops
are called after disabling non-boot CPUs...

And anyway for in-kernel API having a explicit assert is a stronger way
of documenting requirement than a comment.

Best regards,
Krzysztof

Re: [PATCH v3 2/8] PM / Domains: Handle safely genpd_syscore_switch() call on non-genpd device

[Rafael J. Wysocki]

On Tue, Jul 4, 2017 at 10:20 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, Jul 04, 2017 at 10:12:13PM +0200, Rafael J. Wysocki wrote:
>> On Tue, Jul 4, 2017 at 10:05 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>  >> >> > Thanks for report!
>> >> >> >
>> >> >> > Damn it, although I couldn't find this in the code, but I was fearing
>> >> >> > that this ends up in atomic section. That would kind of explain why
>> >> >> > mutex was not there [1].
>> >> >> >
>> >> >> > Anyway, the buggy code was there already. Instead of "sleeping in atomic
>> >> >> > section" there was no locking at all... In this context this was
>> >> >> > probably safe because it was executed *after* disabling non-boot CPUs
>> >> >> > but then the function cannot be called in other contexts.
>> >> >> >
>> >> >> > I am not sure I will be capable of developing the proper fix as I do not
>> >> >> > have the hardware and I do not know all stuff happening in sh suspend.
>> >> >> > Probably reverting this and living with non-locked path would be the
>> >> >> > safest choice.
>> >> >> >
>> >> >> > [1] https://patchwork.kernel.org/patch/9778903/
>> >> >>
>> >> >> AFAIU, all syscore stuff runs in atomic context.
>> >> >
>> >> > Indeed... The confusing part is that this code is syscore only from
>> >> > the name, it is not hooked in to syscore_ops. Although going by call
>> >> > chain (through sh clocksource drivers) we end up in
>> >> > timekeeping_suspend() which is a syscore op.
>> >> >
>> >> > I wonder whether it would be useful - after reverting my commit - to add
>> >> > an assert (which is a stronger API requirement than only documentation "may
>> >> > only be called during the system core (syscore) suspend") like:
>> >> >         WARN_ON(num_online_cpus() > 1));
>> >> > as without mutexes this should not be executed with more than one online
>> >> > CPU.
>> >>
>> >> Or maybe WARN_ON_ONCE(!in_atomic())?
>> >
>> > You could be in atomic section on this CPU and still have other CPUs
>> > online playing with gpd_list (without any protection from locking).
>> > This function is safe only on non-SMP case.
>>
>> Well, not quite.
>>
>> It is safe if you can guarantee that no other CPUs will touch the data
>> structure in question concurrently, which pretty much is the case for
>> timekeeping_suspend() even though it may be invoked without taking the
>> other CPUs offline (from the suspend-to-idle core path).
>
> Right, that would work fine for that case.
>
> However I was rather thinking that we have an in-kernel API (exported)
> so someone might by mistake try to use it in different contexts. For
> example in some atomic section but on a platform which offlines CPUs
> later. Thus it would be called in some imaginary suspend path but with
> CPUs still being online. Partially it is already mentioned in documentation
> although I am not sure that on every possible architecture syscore ops
> are called after disabling non-boot CPUs...

Yes, they are.  Nonboot CPUs are disabled by the core.

Anyway, while I see your point, it would be rather hard to find an assertion
that would also work for the suspend-to-idle timekeeping_suspend() invocation
case.

Thanks,
Rafael