[PATCH] PM / Domains: Remove in_progress counter from struct generic_pm_domain

[PATCH] PM / Domains: Remove in_progress counter from struct generic_pm_domain

[Ulf Hansson]

Commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states..") changed
the power off sequence (pm_genpd_poweroff()), which from locking point of
view means the genpd mutex is held throughout the sequence.

The above change means the in_progress counter can't be updated while
pm_genpd_poweroff() is executing, which allows us to remove the counter.

Instead we inform pm_genpd_poweroff() via a bool parameter, to indicate
whether we call it from the scheduled work or from the ->runtime_suspend()
callback, since that all that matters.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 12 +++++-------
 include/linux/pm_domain.h   |  1 -
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index cf4950d..c785f2e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -315,11 +315,12 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 /**
  * pm_genpd_poweroff - Remove power from a given PM domain.
  * @genpd: PM domain to power down.
+ * @is_async: PM domain is powered down from a scheduled work
  *
  * If all of the @genpd's devices have been suspended and all of its subdomains
  * have been powered down, remove power from @genpd.
  */
-static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
+static int pm_genpd_poweroff(struct generic_pm_domain *genpd, bool is_async)
 {
 	struct pm_domain_data *pdd;
 	struct gpd_link *link;
@@ -351,7 +352,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 			not_suspended++;
 	}
 
-	if (not_suspended > genpd->in_progress)
+	if (not_suspended > 1 || (not_suspended == 1 && is_async))
 		return -EBUSY;
 
 	if (genpd->gov && genpd->gov->power_down_ok) {
@@ -399,7 +400,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 
 	mutex_lock(&genpd->lock);
-	pm_genpd_poweroff(genpd);
+	pm_genpd_poweroff(genpd, true);
 	mutex_unlock(&genpd->lock);
 }
 
@@ -445,9 +446,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return 0;
 
 	mutex_lock(&genpd->lock);
-	genpd->in_progress++;
-	pm_genpd_poweroff(genpd);
-	genpd->in_progress--;
+	pm_genpd_poweroff(genpd, false);
 	mutex_unlock(&genpd->lock);
 
 	return 0;
@@ -1462,7 +1461,6 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	mutex_init(&genpd->lock);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
-	genpd->in_progress = 0;
 	atomic_set(&genpd->sd_count, 0);
 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
 	genpd->device_count = 0;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 384b1d1..0eaa730 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -47,7 +47,6 @@ struct generic_pm_domain {
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
 	const char *name;
-	unsigned int in_progress;	/* Number of devices being suspended now */
 	atomic_t sd_count;	/* Number of subdomains with power "on" */
 	enum gpd_status status;	/* Current state of the domain */
 	unsigned int device_count;	/* Number of devices */
-- 
1.9.1

Re: [PATCH] PM / Domains: Remove in_progress counter from struct generic_pm_domain

[Lina Iyer]

On Tue, Sep 29 2015 at 07:12 -0600, Ulf Hansson wrote:
>Commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states..") changed
>the power off sequence (pm_genpd_poweroff()), which from locking point of
>view means the genpd mutex is held throughout the sequence.
>
>The above change means the in_progress counter can't be updated while
>pm_genpd_poweroff() is executing, which allows us to remove the counter.
>
>Instead we inform pm_genpd_poweroff() via a bool parameter, to indicate
>whether we call it from the scheduled work or from the ->runtime_suspend()
>callback, since that all that matters.
>
>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Lina Iyer <lina.iyer@linaro.org>
>---
> drivers/base/power/domain.c | 12 +++++-------
> include/linux/pm_domain.h   |  1 -
> 2 files changed, 5 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>index cf4950d..c785f2e 100644
>--- a/drivers/base/power/domain.c
>+++ b/drivers/base/power/domain.c
>@@ -315,11 +315,12 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> /**
>  * pm_genpd_poweroff - Remove power from a given PM domain.
>  * @genpd: PM domain to power down.
>+ * @is_async: PM domain is powered down from a scheduled work
>  *
>  * If all of the @genpd's devices have been suspended and all of its subdomains
>  * have been powered down, remove power from @genpd.
>  */
>-static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>+static int pm_genpd_poweroff(struct generic_pm_domain *genpd, bool is_async)
> {
> 	struct pm_domain_data *pdd;
> 	struct gpd_link *link;
>@@ -351,7 +352,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
> 			not_suspended++;
> 	}
>
>-	if (not_suspended > genpd->in_progress)
>+	if (not_suspended > 1 || (not_suspended == 1 && is_async))
> 		return -EBUSY;
>
> 	if (genpd->gov && genpd->gov->power_down_ok) {
>@@ -399,7 +400,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
> 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
>
> 	mutex_lock(&genpd->lock);
>-	pm_genpd_poweroff(genpd);
>+	pm_genpd_poweroff(genpd, true);
> 	mutex_unlock(&genpd->lock);
> }
>
>@@ -445,9 +446,7 @@ static int pm_genpd_runtime_suspend(struct device *dev)
> 		return 0;
>
> 	mutex_lock(&genpd->lock);
>-	genpd->in_progress++;
>-	pm_genpd_poweroff(genpd);
>-	genpd->in_progress--;
>+	pm_genpd_poweroff(genpd, false);
> 	mutex_unlock(&genpd->lock);
>
> 	return 0;
>@@ -1462,7 +1461,6 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
> 	mutex_init(&genpd->lock);
> 	genpd->gov = gov;
> 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>-	genpd->in_progress = 0;
> 	atomic_set(&genpd->sd_count, 0);
> 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
> 	genpd->device_count = 0;
>diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>index 384b1d1..0eaa730 100644
>--- a/include/linux/pm_domain.h
>+++ b/include/linux/pm_domain.h
>@@ -47,7 +47,6 @@ struct generic_pm_domain {
> 	struct dev_power_governor *gov;
> 	struct work_struct power_off_work;
> 	const char *name;
>-	unsigned int in_progress;	/* Number of devices being suspended now */
> 	atomic_t sd_count;	/* Number of subdomains with power "on" */
> 	enum gpd_status status;	/* Current state of the domain */
> 	unsigned int device_count;	/* Number of devices */
>-- 
>1.9.1
>

Re: [PATCH] PM / Domains: Remove in_progress counter from struct generic_pm_domain

[Rafael J. Wysocki]

On Tuesday, September 29, 2015 08:56:41 AM Lina Iyer wrote:
> On Tue, Sep 29 2015 at 07:12 -0600, Ulf Hansson wrote:
> >Commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states..") changed
> >the power off sequence (pm_genpd_poweroff()), which from locking point of
> >view means the genpd mutex is held throughout the sequence.
> >
> >The above change means the in_progress counter can't be updated while
> >pm_genpd_poweroff() is executing, which allows us to remove the counter.
> >
> >Instead we inform pm_genpd_poweroff() via a bool parameter, to indicate
> >whether we call it from the scheduled work or from the ->runtime_suspend()
> >callback, since that all that matters.
> >
> >Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Reviewed-by: Lina Iyer <lina.iyer@linaro.org>

Queued up for v4.4, thanks!

Rafael