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

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

[Krzysztof Kozlowski]

Hi,

Except adding lockdep assert to domains list mutex (3/8), all patches
are independent.  Including the fixes for unsafe loop iteration.

The last patch is RFC as this brings small overhead.

Best regards,
Krzysztof


Krzysztof Kozlowski (8):
  PM / Domains: Constify genpd pointer
  PM / domains: Protect reading loop over list of domains
  PM / domains: Add lockdep asserts for domains list mutex
  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 asserts for PM domain locks

 drivers/base/power/domain.c          | 66 ++++++++++++++++++++++++++++--------
 drivers/base/power/domain_governor.c | 12 +++----
 2 files changed, 58 insertions(+), 20 deletions(-)

-- 
2.9.3

[PATCH 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>
---
 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 da49a8383dc3..2e8d0f423507 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.9.3

[PATCH 2/8] PM / domains: Protect reading loop over list of domains

[Krzysztof Kozlowski]

The pm_genpd_present() iterates over list of domains so grabbing a
gpd_list_lock mutex is necessary before calling it.  Otherwise we could
end up in iterating over and modifying the list at the same time.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2e8d0f423507..2a6935dc0164 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1099,8 +1099,13 @@ 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))
+
+	mutex_lock(&gpd_list_lock);
+	if (!pm_genpd_present(genpd)) {
+		mutex_unlock(&gpd_list_lock);
 		return;
+	}
+	mutex_unlock(&gpd_list_lock);
 
 	if (suspend) {
 		genpd->suspended_count++;
-- 
2.9.3

[PATCH 3/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 2a6935dc0164..0f416563479b 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;
 
@@ -1326,6 +1328,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;
@@ -1533,6 +1537,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.9.3

[PATCH 4/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>
---
 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 0f416563479b..325962702bc8 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1405,7 +1405,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))
@@ -1421,7 +1421,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.9.3

[PATCH 5/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>
---
 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 325962702bc8..e426ceff1a59 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1794,12 +1794,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.9.3

[PATCH 6/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>
---
 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 e426ceff1a59..c1e22b613287 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1939,14 +1939,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.9.3

[PATCH 7/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>
---
 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.9.3

[RFC 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 c1e22b613287..17fecfd44607 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.9.3

Re: [PATCH 2/8] PM / domains: Protect reading loop over list of domains

[Krzysztof Kozlowski]

On Fri, Jun 09, 2017 at 06:08:47PM +0200, Krzysztof Kozlowski wrote:
> The pm_genpd_present() iterates over list of domains so grabbing a
> gpd_list_lock mutex is necessary before calling it.  Otherwise we could
> end up in iterating over and modifying the list at the same time.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/base/power/domain.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2e8d0f423507..2a6935dc0164 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1099,8 +1099,13 @@ 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))
> +
> +	mutex_lock(&gpd_list_lock);
> +	if (!pm_genpd_present(genpd)) {
> +		mutex_unlock(&gpd_list_lock);
>  		return;
> +	}
> +	mutex_unlock(&gpd_list_lock);

Eh, I might be too fast as this is not executed on my platform.  The
genpd_syscore_switch() seems to be called by clocksource_suspend() which
is called by syscore ops. Should be safe but actually not tested.

Someone with SH hardware is needed...

Best regards,
Krzysztof

Re: [PATCH 2/8] PM / domains: Protect reading loop over list of domains

[Ulf Hansson]

On 9 June 2017 at 18:16, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Fri, Jun 09, 2017 at 06:08:47PM +0200, Krzysztof Kozlowski wrote:
>> The pm_genpd_present() iterates over list of domains so grabbing a
>> gpd_list_lock mutex is necessary before calling it.  Otherwise we could
>> end up in iterating over and modifying the list at the same time.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>  drivers/base/power/domain.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 2e8d0f423507..2a6935dc0164 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -1099,8 +1099,13 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
>>       struct generic_pm_domain *genpd;
>>
>>       genpd = dev_to_genpd(dev);

Actually there may be potential problem here as we may end up trying
to use the container_of() call for a PM domain, not being a genpd but
something else.

>> -     if (!pm_genpd_present(genpd))
>> +
>> +     mutex_lock(&gpd_list_lock);
>> +     if (!pm_genpd_present(genpd)) {
>> +             mutex_unlock(&gpd_list_lock);
>>               return;
>> +     }
>> +     mutex_unlock(&gpd_list_lock);

Perhaps convert the hole thing above to call genpd_lookup_dev() instead?

>
> Eh, I might be too fast as this is not executed on my platform.  The
> genpd_syscore_switch() seems to be called by clocksource_suspend() which
> is called by syscore ops. Should be safe but actually not tested.
>
> Someone with SH hardware is needed...
>
> Best regards,
> Krzysztof
>

Kind regards
Uffe

Re: [PATCH 2/8] PM / domains: Protect reading loop over list of domains

[Krzysztof Kozlowski]

On Mon, Jun 12, 2017 at 02:57:21PM +0200, Ulf Hansson wrote:
> On 9 June 2017 at 18:16, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Fri, Jun 09, 2017 at 06:08:47PM +0200, Krzysztof Kozlowski wrote:
> >> The pm_genpd_present() iterates over list of domains so grabbing a
> >> gpd_list_lock mutex is necessary before calling it.  Otherwise we could
> >> end up in iterating over and modifying the list at the same time.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> ---
> >>  drivers/base/power/domain.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index 2e8d0f423507..2a6935dc0164 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -1099,8 +1099,13 @@ static void genpd_syscore_switch(struct device *dev, bool suspend)
> >>       struct generic_pm_domain *genpd;
> >>
> >>       genpd = dev_to_genpd(dev);
> 
> Actually there may be potential problem here as we may end up trying
> to use the container_of() call for a PM domain, not being a genpd but
> something else.
> 
> >> -     if (!pm_genpd_present(genpd))
> >> +
> >> +     mutex_lock(&gpd_list_lock);
> >> +     if (!pm_genpd_present(genpd)) {
> >> +             mutex_unlock(&gpd_list_lock);
> >>               return;
> >> +     }
> >> +     mutex_unlock(&gpd_list_lock);
> 
> Perhaps convert the hole thing above to call genpd_lookup_dev() instead?

Indeed, that would solve both problems. Thanks, I'll fix this.

Best regards,
Krzysztof