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

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

[Krzysztof Kozlowski]

Hi,

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

Description:
============
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: Handle safely genpd_syscore_switch() call on non-genpd
    device
  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          | 63 +++++++++++++++++++++++++++---------
 drivers/base/power/domain_governor.c | 12 +++----
 2 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.9.3

[PATCH v2 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

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

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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 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
>

[RFT v2 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>

---

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 2e8d0f423507..983f086ab235 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.9.3

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

[Ulf Hansson]

On 12 June 2017 at 17:17, 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>
>
> ---
>
> Not tested on real hardware.

When tested you may add my:

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 2e8d0f423507..983f086ab235 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.9.3
>

[PATCH v2 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 983f086ab235..9d3d3c2a5979 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.9.3

Re: [PATCH v2 3/8] PM / Domains: Add lockdep asserts for domains list mutex

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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.

I think it seems a bit silly to use these lockdep checks as these
functions are as you state above, static functions. Moreover there are
called from a quite limited amount of places.

Do you really think this add some value?

Kind regards
Uffe

>
> 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 983f086ab235..9d3d3c2a5979 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.9.3
>

Re: [PATCH v2 3/8] PM / Domains: Add lockdep asserts for domains list mutex

[Krzysztof Kozlowski]

On Mon, Jun 12, 2017 at 09:09:59PM +0200, Ulf Hansson wrote:
> On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > 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.
> 
> I think it seems a bit silly to use these lockdep checks as these
> functions are as you state above, static functions. Moreover there are
> called from a quite limited amount of places.
> 
> Do you really think this add some value?

In ideal world, these would not need lockdeps because we do not make
mistakes. I agree, that mostly the exposed functions to other kernel
modules should be protected, as lockdep annotation is a more advanced
way of documenting the need of locking.

Does it mean that file-scope functions should not be annotated? Even in
such case function can be misused as the code is getting more and more
complicated.

What is best example, one of these calls (pm_genpd_present()) was
already used in wrong way, without locking when iterating over the list.
Having lockdep would point this early.

Best regards,
Krzysztof


> 
> Kind regards
> Uffe
> 
> >
> > 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 983f086ab235..9d3d3c2a5979 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.9.3
> >

Re: [PATCH v2 3/8] PM / Domains: Add lockdep asserts for domains list mutex

[Ulf Hansson]

On 13 June 2017 at 09:12, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Mon, Jun 12, 2017 at 09:09:59PM +0200, Ulf Hansson wrote:
>> On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> > 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.
>>
>> I think it seems a bit silly to use these lockdep checks as these
>> functions are as you state above, static functions. Moreover there are
>> called from a quite limited amount of places.
>>
>> Do you really think this add some value?
>
> In ideal world, these would not need lockdeps because we do not make
> mistakes. I agree, that mostly the exposed functions to other kernel
> modules should be protected, as lockdep annotation is a more advanced
> way of documenting the need of locking.
>
> Does it mean that file-scope functions should not be annotated? Even in
> such case function can be misused as the code is getting more and more
> complicated.
>
> What is best example, one of these calls (pm_genpd_present()) was
> already used in wrong way, without locking when iterating over the list.
> Having lockdep would point this early.

Well, I think we found a potential bug because of reviewing the code.
Not because of adding lockdep_assert_held().

Perhaps adding protection with lockdep_assert_held() could be
meaningful also for static functions, but only in cases of complicated
code. I don't think that is the case here.

Anyway, if other people thinks $subject patch is good idea, then I
drop my case. :-)

[...]

Kind regards
Uffe

[PATCH v2 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 9d3d3c2a5979..d1e438024e46 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1400,7 +1400,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))
@@ -1416,7 +1416,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

Re: [PATCH v2 4/8] PM / Domains: Fix unsafe iteration over modified list of device links

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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 9d3d3c2a5979..d1e438024e46 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1400,7 +1400,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))
> @@ -1416,7 +1416,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 v2 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 d1e438024e46..b74b5111957a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1789,12 +1789,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

Re: [PATCH v2 5/8] PM / Domains: Fix unsafe iteration over modified list of domain providers

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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 d1e438024e46..b74b5111957a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1789,12 +1789,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 v2 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 b74b5111957a..33467beaeea4 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1934,14 +1934,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

Re: [PATCH v2 6/8] PM / Domains: Fix unsafe iteration over modified list of domains

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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 b74b5111957a..33467beaeea4 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1934,14 +1934,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 v2 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

Re: [PATCH v2 7/8] PM / Domains: Fix missing default_power_down_ok comment

[Ulf Hansson]

On 12 June 2017 at 17:17, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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.9.3
>

[RFC v2 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 33467beaeea4..36db9a06f0cc 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 v2 0/8] PM / Domains: Bunch of small improvements and fixes

[Rafael J. Wysocki]

On Monday, June 12, 2017 05:17:40 PM Krzysztof Kozlowski wrote:
> Hi,
> 
> 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).
> 
> Description:
> ============
> 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.

I've applied the [1/8] from this set, but I realize that some other patches in
it have been ACKed by Ulf, so can you please resend those with the tags?

Unless, of course, they depend on the ones that have not been ACKed still.

Thanks,
Rafael

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

[Krzysztof Kozlowski]

On Wed, Jun 28, 2017 at 12:51:45AM +0200, Rafael J. Wysocki wrote:
> On Monday, June 12, 2017 05:17:40 PM Krzysztof Kozlowski wrote:
> > Hi,
> > 
> > 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).
> > 
> > Description:
> > ============
> > 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.
> 
> I've applied the [1/8] from this set, but I realize that some other patches in
> it have been ACKed by Ulf, so can you please resend those with the tags?
> 
> Unless, of course, they depend on the ones that have not been ACKed still.

Sure, I'll re-spin with acks.

Best regards,
Krzysztof