| Message ID | 1515616316-5118-1-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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.
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); } /**
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(-)