[PATCH 0/6] PM / Domains: A few clean up and minor fixes

[PATCH 0/6] PM / Domains: A few clean up and minor fixes

[Aisheng Dong]

A few clean up and minor fixes

Dong Aisheng (6):
  PM / Domains: Improve warn for multiple states but no governor in
    genpd
  PM / Domains: Return for all error cases in _genpd_power_off
  PM / Domains: Remove the unnecessary ->power_off() check in
    genpd_power_off
  PM / Domains: Move the Subdomain check into _genpd_power_off
  PM / QoS: Fix a typo in file description
  PM / domains: Remove one unnecessary blank line

 drivers/base/power/domain.c          | 44 ++++++++++++++++--------------------
 drivers/base/power/domain_governor.c |  1 -
 drivers/base/power/qos.c             |  2 +-
 3 files changed, 21 insertions(+), 26 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] PM / Domains: Improve warn for multiple states but no governor in genpd

[Aisheng Dong]

It's possible a PM domain defines only one state and it does not need
a governor to work. For such case, a warning actually is not necessary.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2c334c0..394f9da 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1766,7 +1766,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
 		ret = genpd_set_default_power_state(genpd);
 		if (ret)
 			return ret;
-	} else if (!gov) {
+	} else if (!gov && genpd->state_count > 1) {
 		pr_warn("%s : no governor for states\n", genpd->name);
 	}
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] PM / Domains: Improve warn for multiple states but no governor in genpd

[Ulf Hansson]

On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> It's possible a PM domain defines only one state and it does not need
> a governor to work. For such case, a warning actually is not necessary.
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Fixes: 2c9b7f877203 ("PM / Domains: Deal with multiple states but no
governor in genpd")
Cc: stable@vger.kernel.org

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/base/power/domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2c334c0..394f9da 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1766,7 +1766,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>                 ret = genpd_set_default_power_state(genpd);
>                 if (ret)
>                         return ret;
> -       } else if (!gov) {
> +       } else if (!gov && genpd->state_count > 1) {
>                 pr_warn("%s : no governor for states\n", genpd->name);
>         }
>
> --
> 2.7.4
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/6] PM / Domains: Improve warn for multiple states but no governor in genpd

[Aisheng Dong]

> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, March 6, 2019 10:17 PM
> On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> >
> > It's possible a PM domain defines only one state and it does not need
> > a governor to work. For such case, a warning actually is not necessary.
> >
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Fixes: 2c9b7f877203 ("PM / Domains: Deal with multiple states but no
> governor in genpd")
> Cc: stable@vger.kernel.org
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> 

One question, this patch applies for current situation, but when we add two states
support with no governor (see my another patch series to support enter
deepest state for suspend but intermediate state for runtime pm), 
here will still warn.

Do you think if we should totally remove this warning or keep this patch
but change to use another simple governor later for multi states?
e.g. fixed intermediate state for runtime pm?

Regards
Dong Aisheng

> > ---
> >  drivers/base/power/domain.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 2c334c0..394f9da 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1766,7 +1766,7 @@ int pm_genpd_init(struct generic_pm_domain
> *genpd,
> >                 ret = genpd_set_default_power_state(genpd);
> >                 if (ret)
> >                         return ret;
> > -       } else if (!gov) {
> > +       } else if (!gov && genpd->state_count > 1) {
> >                 pr_warn("%s : no governor for states\n", genpd->name);
> >         }
> >
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] PM / Domains: Return for all error cases in _genpd_power_off

[Aisheng Dong]

It is strange to only return early for -EBUSY state and left other
errors to be still measured execution time.

As for error cases, the elapsed_ns computed actually is not quite
accurate and meaningful for governor to use. So let's simply return
for all error cases.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/domain.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 394f9da..f012576 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -457,19 +457,19 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 
 	time_start = ktime_get();
 	ret = genpd->power_off(genpd);
-	if (ret == -EBUSY)
+	if (ret)
 		return ret;
 
 	elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
 	if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
-		return ret;
+		return 0;
 
 	genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
 	genpd->max_off_time_changed = true;
 	pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
 		 genpd->name, "off", elapsed_ns);
 
-	return ret;
+	return 0;
 }
 
 /**
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] PM / Domains: Return for all error cases in _genpd_power_off

[Ulf Hansson]

On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> It is strange to only return early for -EBUSY state and left other
> errors to be still measured execution time.
>
> As for error cases, the elapsed_ns computed actually is not quite
> accurate and meaningful for governor to use. So let's simply return
> for all error cases.
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>  drivers/base/power/domain.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 394f9da..f012576 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -457,19 +457,19 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>
>         time_start = ktime_get();
>         ret = genpd->power_off(genpd);
> -       if (ret == -EBUSY)
> +       if (ret)
>                 return ret;
>
>         elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
>         if (elapsed_ns <= genpd->states[state_idx].power_off_latency_ns)
> -               return ret;
> +               return 0;
>
>         genpd->states[state_idx].power_off_latency_ns = elapsed_ns;
>         genpd->max_off_time_changed = true;
>         pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
>                  genpd->name, "off", elapsed_ns);
>
> -       return ret;
> +       return 0;
>  }
>
>  /**
> --
> 2.7.4
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] PM / Domains: Remove the unnecessary ->power_off() check in genpd_power_off

[Aisheng Dong]

There's already a check in _genpd_power_off, so caller can directly call it
without checking.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/domain.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index f012576..591f37f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -501,6 +501,7 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	struct pm_domain_data *pdd;
 	struct gpd_link *link;
 	unsigned int not_suspended = 0;
+	int ret;
 
 	/*
 	 * Do not try to power off the domain in the following situations:
@@ -546,24 +547,20 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	if (!genpd->gov)
 		genpd->state_idx = 0;
 
-	if (genpd->power_off) {
-		int ret;
-
-		if (atomic_read(&genpd->sd_count) > 0)
-			return -EBUSY;
+	if (atomic_read(&genpd->sd_count) > 0)
+		return -EBUSY;
 
-		/*
-		 * If sd_count > 0 at this point, one of the subdomains hasn't
-		 * managed to call genpd_power_on() for the master yet after
-		 * incrementing it.  In that case genpd_power_on() will wait
-		 * for us to drop the lock, so we can call .power_off() and let
-		 * the genpd_power_on() restore power for us (this shouldn't
-		 * happen very often).
-		 */
-		ret = _genpd_power_off(genpd, true);
-		if (ret)
-			return ret;
-	}
+	/*
+	 * If sd_count > 0 at this point, one of the subdomains hasn't
+	 * managed to call genpd_power_on() for the master yet after
+	 * incrementing it.  In that case genpd_power_on() will wait
+	 * for us to drop the lock, so we can call .power_off() and let
+	 * the genpd_power_on() restore power for us (this shouldn't
+	 * happen very often).
+	 */
+	ret = _genpd_power_off(genpd, true);
+	if (ret)
+		return ret;
 
 	genpd->status = GPD_STATE_POWER_OFF;
 	genpd_update_accounting(genpd);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] PM / Domains: Move the Subdomain check into _genpd_power_off

[Aisheng Dong]

Move the Subdomain check into _genpd_power_off, then the caller does
not have to check it each time. This also ensures a double check
of &genpd->sd_count before really power off domain in case it's
increased asynchronously by subdomains. This is the same behavior
as the original genpd_power_off() does.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/domain.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 591f37f..61cd500 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -452,6 +452,9 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
 	if (!genpd->power_off)
 		return 0;
 
+	if (atomic_read(&genpd->sd_count) > 0)
+		return -EBUSY;
+
 	if (!timed)
 		return genpd->power_off(genpd);
 
@@ -547,9 +550,6 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
 	if (!genpd->gov)
 		genpd->state_idx = 0;
 
-	if (atomic_read(&genpd->sd_count) > 0)
-		return -EBUSY;
-
 	/*
 	 * If sd_count > 0 at this point, one of the subdomains hasn't
 	 * managed to call genpd_power_on() for the master yet after
@@ -962,8 +962,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
 	if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
 		return;
 
-	if (genpd->suspended_count != genpd->device_count
-	    || atomic_read(&genpd->sd_count) > 0)
+	if (genpd->suspended_count != genpd->device_count)
 		return;
 
 	/* Choose the deepest state when suspending */
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] PM / Domains: Move the Subdomain check into _genpd_power_off

[Ulf Hansson]

On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> Move the Subdomain check into _genpd_power_off, then the caller does
> not have to check it each time. This also ensures a double check
> of &genpd->sd_count before really power off domain in case it's
> increased asynchronously by subdomains. This is the same behavior
> as the original genpd_power_off() does.
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Please squash this with patch 3, as to avoid an intermediate change in behavior.

Otherwise both patch 3 and patch4 looks good to me.

Kind regards
Uffe

> ---
>  drivers/base/power/domain.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 591f37f..61cd500 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -452,6 +452,9 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>         if (!genpd->power_off)
>                 return 0;
>
> +       if (atomic_read(&genpd->sd_count) > 0)
> +               return -EBUSY;
> +
>         if (!timed)
>                 return genpd->power_off(genpd);
>
> @@ -547,9 +550,6 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
>         if (!genpd->gov)
>                 genpd->state_idx = 0;
>
> -       if (atomic_read(&genpd->sd_count) > 0)
> -               return -EBUSY;
> -
>         /*
>          * If sd_count > 0 at this point, one of the subdomains hasn't
>          * managed to call genpd_power_on() for the master yet after
> @@ -962,8 +962,7 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
>         if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
>                 return;
>
> -       if (genpd->suspended_count != genpd->device_count
> -           || atomic_read(&genpd->sd_count) > 0)
> +       if (genpd->suspended_count != genpd->device_count)
>                 return;
>
>         /* Choose the deepest state when suspending */
> --
> 2.7.4
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 4/6] PM / Domains: Move the Subdomain check into _genpd_power_off

[Aisheng Dong]

> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: Wednesday, March 6, 2019 10:27 PM
> On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> >
> > Move the Subdomain check into _genpd_power_off, then the caller does
> > not have to check it each time. This also ensures a double check of
> > &genpd->sd_count before really power off domain in case it's increased
> > asynchronously by subdomains. This is the same behavior as the
> > original genpd_power_off() does.
> >
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> 
> Please squash this with patch 3, as to avoid an intermediate change in
> behavior.
> 
> Otherwise both patch 3 and patch4 looks good to me.
> 

Thanks, I will take this as a reviewed-by tag.
And will resend with patch3&4 squashed later.

Regards
Dong Aisheng

> Kind regards
> Uffe
> 
> > ---
> >  drivers/base/power/domain.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 591f37f..61cd500 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -452,6 +452,9 @@ static int _genpd_power_off(struct
> generic_pm_domain *genpd, bool timed)
> >         if (!genpd->power_off)
> >                 return 0;
> >
> > +       if (atomic_read(&genpd->sd_count) > 0)
> > +               return -EBUSY;
> > +
> >         if (!timed)
> >                 return genpd->power_off(genpd);
> >
> > @@ -547,9 +550,6 @@ static int genpd_power_off(struct
> generic_pm_domain *genpd, bool one_dev_on,
> >         if (!genpd->gov)
> >                 genpd->state_idx = 0;
> >
> > -       if (atomic_read(&genpd->sd_count) > 0)
> > -               return -EBUSY;
> > -
> >         /*
> >          * If sd_count > 0 at this point, one of the subdomains hasn't
> >          * managed to call genpd_power_on() for the master yet after
> > @@ -962,8 +962,7 @@ static void genpd_sync_power_off(struct
> generic_pm_domain *genpd, bool use_lock,
> >         if (!genpd_status_on(genpd) || genpd_is_always_on(genpd))
> >                 return;
> >
> > -       if (genpd->suspended_count != genpd->device_count
> > -           || atomic_read(&genpd->sd_count) > 0)
> > +       if (genpd->suspended_count != genpd->device_count)
> >                 return;
> >
> >         /* Choose the deepest state when suspending */
> > --
> > 2.7.4
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] PM / QoS: Fix a typo in file description

[Aisheng Dong]

Fix a typo in file description

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/qos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 3382542..f80e402 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -22,7 +22,7 @@
  * per-device constraint data struct.
  *
  * Note about the per-device constraint data struct allocation:
- * . The per-device constraints data struct ptr is tored into the device
+ * . The per-device constraints data struct ptr is stored into the device
  *    dev_pm_info.
  * . To minimize the data usage by the per-device constraints, the data struct
  *   is only allocated at the first call to dev_pm_qos_add_request.
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] PM / domains: Remove one unnecessary blank line

[Aisheng Dong]

Remove one unnecessary blank line

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/base/power/domain_governor.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 99896fb..4d07e38 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -128,7 +128,6 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
 	off_on_time_ns = genpd->states[state].power_off_latency_ns +
 		genpd->states[state].power_on_latency_ns;
 
-
 	min_off_time_ns = -1;
 	/*
 	 * Check if subdomains can be off for enough time.
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] PM / domains: Remove one unnecessary blank line

[Ulf Hansson]

On Wed, 6 Mar 2019 at 14:25, Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> Remove one unnecessary blank line
>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/base/power/domain_governor.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index 99896fb..4d07e38 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -128,7 +128,6 @@ static bool __default_power_down_ok(struct dev_pm_domain *pd,
>         off_on_time_ns = genpd->states[state].power_off_latency_ns +
>                 genpd->states[state].power_on_latency_ns;
>
> -

Normally I would do these kind of cleanups while working on some
closely related code. However, I have no problem with it, if Rafael
want's to pick it up.

>         min_off_time_ns = -1;
>         /*
>          * Check if subdomains can be off for enough time.
> --
> 2.7.4
>

Kind regards
Uffe

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] PM / Domains: A few clean up and minor fixes

[Rafael J. Wysocki]

On Wednesday, March 6, 2019 2:25:08 PM CET Aisheng Dong wrote:
> A few clean up and minor fixes
> 
> Dong Aisheng (6):
>   PM / Domains: Improve warn for multiple states but no governor in
>     genpd
>   PM / Domains: Return for all error cases in _genpd_power_off
>   PM / Domains: Remove the unnecessary ->power_off() check in
>     genpd_power_off
>   PM / Domains: Move the Subdomain check into _genpd_power_off
>   PM / QoS: Fix a typo in file description
>   PM / domains: Remove one unnecessary blank line
> 
>  drivers/base/power/domain.c          | 44 ++++++++++++++++--------------------
>  drivers/base/power/domain_governor.c |  1 -
>  drivers/base/power/qos.c             |  2 +-
>  3 files changed, 21 insertions(+), 26 deletions(-)

I've applied patches [1-2/6] (with tags from Ulf) and [5-6/6] from this series
and I'm expecting an update to replace patches [3-4/6].


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel