PM / Domains: Don't skip driver's ->suspend|resume_noirq() callbacks

Message ID 1515616316-5118-1-git-send-email-ulf.hansson@linaro.org
State New
Headers show
Series
  • PM / Domains: Don't skip driver's ->suspend|resume_noirq() callbacks
Related show

Commit Message

Ulf Hansson Jan. 10, 2018, 8:31 p.m.
The commit 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")
started to respect driver's noirq callbacks, but while doing that it also
introduced a few potential problems.

More precisely, in genpd_finish_suspend() and genpd_resume_noirq() the
noirq callbacks at the driver level should be invoked, no matter of whether
dev->power.wakeup_path is set or not.

Additionally, the commit in question also made genpd_resume_noirq() to
ignore the return value from pm_runtime_force_resume().

Let's fix both these issues!

Fixes: 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---
 drivers/base/power/domain.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

-- 
2.7.4

Comments

Rafael J. Wysocki Jan. 11, 2018, 12:54 a.m. | #1
On Wed, Jan 10, 2018 at 9:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The commit 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")

> started to respect driver's noirq callbacks, but while doing that it also

> introduced a few potential problems.

>

> More precisely, in genpd_finish_suspend() and genpd_resume_noirq() the

> noirq callbacks at the driver level should be invoked, no matter of whether

> dev->power.wakeup_path is set or not.

>

> Additionally, the commit in question also made genpd_resume_noirq() to

> ignore the return value from pm_runtime_force_resume().

>

> Let's fix both these issues!

>

> Fixes: 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")

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

> ---

>  drivers/base/power/domain.c | 30 +++++++++++++++++-------------

>  1 file changed, 17 insertions(+), 13 deletions(-)

>

> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c

> index f9dcc98..48255ce 100644

> --- a/drivers/base/power/domain.c

> +++ b/drivers/base/power/domain.c

> @@ -1032,15 +1032,12 @@ static int genpd_prepare(struct device *dev)

>  static int genpd_finish_suspend(struct device *dev, bool poweroff)

>  {

>         struct generic_pm_domain *genpd;

> -       int ret;

> +       int ret = 0;

>

>         genpd = dev_to_genpd(dev);

>         if (IS_ERR(genpd))

>                 return -EINVAL;

>

> -       if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))

> -               return 0;

> -

>         if (poweroff)

>                 ret = pm_generic_poweroff_noirq(dev);

>         else

> @@ -1048,10 +1045,18 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)

>         if (ret)

>                 return ret;

>

> +       if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))

> +               return 0;

> +

>         if (genpd->dev_ops.stop && genpd->dev_ops.start) {

>                 ret = pm_runtime_force_suspend(dev);


Good catch!

> -               if (ret)

> +               if (ret) {

> +                       if (poweroff)

> +                               pm_generic_restore_noirq(dev);

> +                       else

> +                               pm_generic_resume_noirq(dev);

>                         return ret;

> +               }

>         }

>

>         genpd_lock(genpd);

> @@ -1085,7 +1090,7 @@ static int genpd_suspend_noirq(struct device *dev)

>  static int genpd_resume_noirq(struct device *dev)

>  {

>         struct generic_pm_domain *genpd;

> -       int ret = 0;

> +       int ret;

>

>         dev_dbg(dev, "%s()\n", __func__);

>

> @@ -1094,21 +1099,20 @@ static int genpd_resume_noirq(struct device *dev)

>                 return -EINVAL;

>

>         if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))

> -               return 0;

> +               return pm_generic_resume_noirq(dev);

>

>         genpd_lock(genpd);

>         genpd_sync_power_on(genpd, true, 0);

>         genpd->suspended_count--;

>         genpd_unlock(genpd);

>

> -       if (genpd->dev_ops.stop && genpd->dev_ops.start)

> +       if (genpd->dev_ops.stop && genpd->dev_ops.start) {

>                 ret = pm_runtime_force_resume(dev);

> +               if (ret)

> +                       return ret;

> +       }

>

> -       ret = pm_generic_resume_noirq(dev);

> -       if (ret)

> -               return ret;

> -

> -       return ret;

> +       return pm_generic_resume_noirq(dev);

>  }

>

>  /**

> --


Looks OK to me overall.

Patch

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index f9dcc98..48255ce 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1032,15 +1032,12 @@  static int genpd_prepare(struct device *dev)
 static int genpd_finish_suspend(struct device *dev, bool poweroff)
 {
 	struct generic_pm_domain *genpd;
-	int ret;
+	int ret = 0;
 
 	genpd = dev_to_genpd(dev);
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
-		return 0;
-
 	if (poweroff)
 		ret = pm_generic_poweroff_noirq(dev);
 	else
@@ -1048,10 +1045,18 @@  static int genpd_finish_suspend(struct device *dev, bool poweroff)
 	if (ret)
 		return ret;
 
+	if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
+		return 0;
+
 	if (genpd->dev_ops.stop && genpd->dev_ops.start) {
 		ret = pm_runtime_force_suspend(dev);
-		if (ret)
+		if (ret) {
+			if (poweroff)
+				pm_generic_restore_noirq(dev);
+			else
+				pm_generic_resume_noirq(dev);
 			return ret;
+		}
 	}
 
 	genpd_lock(genpd);
@@ -1085,7 +1090,7 @@  static int genpd_suspend_noirq(struct device *dev)
 static int genpd_resume_noirq(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
-	int ret = 0;
+	int ret;
 
 	dev_dbg(dev, "%s()\n", __func__);
 
@@ -1094,21 +1099,20 @@  static int genpd_resume_noirq(struct device *dev)
 		return -EINVAL;
 
 	if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
-		return 0;
+		return pm_generic_resume_noirq(dev);
 
 	genpd_lock(genpd);
 	genpd_sync_power_on(genpd, true, 0);
 	genpd->suspended_count--;
 	genpd_unlock(genpd);
 
-	if (genpd->dev_ops.stop && genpd->dev_ops.start)
+	if (genpd->dev_ops.stop && genpd->dev_ops.start) {
 		ret = pm_runtime_force_resume(dev);
+		if (ret)
+			return ret;
+	}
 
-	ret = pm_generic_resume_noirq(dev);
-	if (ret)
-		return ret;
-
-	return ret;
+	return pm_generic_resume_noirq(dev);
 }
 
 /**