Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662582 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] PM / Domains: Bunch of small improvements and fixes Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:10 +0200
    [PATCH 1/8] PM / Domains: Constify genpd pointer Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:10 +0200
    [PATCH 2/8] PM / domains: Protect reading loop over list of domains Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:10 +0200
      Re: [PATCH 2/8] PM / domains: Protect reading loop over list of  domains Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:20 +0200
        Re: [PATCH 2/8] PM / domains: Protect reading loop over list of domains Ulf Hansson <ulf.hansson@linaro.org> - 2017-06-12 15:00 +0200
          Re: [PATCH 2/8] PM / domains: Protect reading loop over list of  domains Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-12 15:20 +0200
    [PATCH 5/8] PM / domains: Fix unsafe iteration over modified list of domain providers Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:10 +0200
    [PATCH 3/8] PM / domains: Add lockdep asserts for domains list mutex Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:20 +0200
    [PATCH 4/8] PM / domains: Fix unsafe iteration over modified list of device links Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:20 +0200
    [PATCH 7/8] PM / domains: Fix missing default_power_down_ok comment Krzysztof Kozlowski <krzk@kernel.org> - 2017-06-09 18:20 +0200

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

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

[toc] | [next] | [standalone]

#1662584 — [PATCH 1/8] PM / Domains: Constify genpd pointer

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1662588 — [PATCH 5/8] PM / domains: Fix unsafe iteration over modified list of domain providers

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

[toc] | [prev] | [next] | [standalone]

#1662591 — [PATCH 3/8] PM / domains: Add lockdep asserts for domains list mutex

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

[toc] | [prev] | [next] | [standalone]

#1662592 — [PATCH 4/8] PM / domains: Fix unsafe iteration over modified list of device links

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

[toc] | [prev] | [next] | [standalone]

#1662595 — [PATCH 7/8] PM / domains: Fix missing default_power_down_ok comment

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web