| Message ID | 1514547423-18965-4-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > In case the WAKEUP_PATH flag has been set in a later phase than from the > ->suspend() callback, the PM core don't set the ->power.wakeup_path status > flag for the device. Therefore, let's be safe and check it explicitly. > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/base/power/domain.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index f9dcc98..32b4ba7 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) > if (IS_ERR(genpd)) > return -EINVAL; > > - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) > + if ((dev->power.wakeup_path || > + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && Shouldn't dev->power.wakeup_path be always set if DPM_FLAG_WAKEUP_PATH is set as per the second patch in the series? > + genpd_is_active_wakeup(genpd)) > return 0; > > if (poweroff) > @@ -1093,7 +1095,9 @@ static int genpd_resume_noirq(struct device *dev) > if (IS_ERR(genpd)) > return -EINVAL; > > - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) > + if ((dev->power.wakeup_path || > + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && > + genpd_is_active_wakeup(genpd)) > return 0; > > genpd_lock(genpd); > -- > 2.7.4 >
On 30 December 2017 at 01:47, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> In case the WAKEUP_PATH flag has been set in a later phase than from the >> ->suspend() callback, the PM core don't set the ->power.wakeup_path status >> flag for the device. Therefore, let's be safe and check it explicitly. >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/base/power/domain.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index f9dcc98..32b4ba7 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) >> if (IS_ERR(genpd)) >> return -EINVAL; >> >> - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) >> + if ((dev->power.wakeup_path || >> + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && > > Shouldn't dev->power.wakeup_path be always set if DPM_FLAG_WAKEUP_PATH > is set as per the second patch in the series? Not if DPM_FLAG_WAKEUP_PATH is set from a driver's ->suspend_late() callback. To do that, the PM core would need to be adopted to set/propagate the "wakeup_path" flag at __device_suspend_late(), similar to what is done at __device_suspend(). [...] Kind regards Uffe
On Tue, Jan 2, 2018 at 12:46 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 30 December 2017 at 01:47, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >>> In case the WAKEUP_PATH flag has been set in a later phase than from the >>> ->suspend() callback, the PM core don't set the ->power.wakeup_path status >>> flag for the device. Therefore, let's be safe and check it explicitly. >>> >>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>> --- >>> drivers/base/power/domain.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >>> index f9dcc98..32b4ba7 100644 >>> --- a/drivers/base/power/domain.c >>> +++ b/drivers/base/power/domain.c >>> @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) >>> if (IS_ERR(genpd)) >>> return -EINVAL; >>> >>> - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) >>> + if ((dev->power.wakeup_path || >>> + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && >> >> Shouldn't dev->power.wakeup_path be always set if DPM_FLAG_WAKEUP_PATH >> is set as per the second patch in the series? > > Not if DPM_FLAG_WAKEUP_PATH is set from a driver's ->suspend_late() callback. > > To do that, the PM core would need to be adopted to set/propagate the > "wakeup_path" flag at __device_suspend_late(), similar to what is done > at __device_suspend(). Why not? The wakeup_path flag is only used during the "noirq" phase, so it can be propagated to parent at the end of __device_suspend_late() instead of doing that in __device_suspend() even. Thanks, Rafael
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index f9dcc98..32b4ba7 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1038,7 +1038,9 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) if (IS_ERR(genpd)) return -EINVAL; - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) + if ((dev->power.wakeup_path || + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && + genpd_is_active_wakeup(genpd)) return 0; if (poweroff) @@ -1093,7 +1095,9 @@ static int genpd_resume_noirq(struct device *dev) if (IS_ERR(genpd)) return -EINVAL; - if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd)) + if ((dev->power.wakeup_path || + dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH)) && + genpd_is_active_wakeup(genpd)) return 0; genpd_lock(genpd);
In case the WAKEUP_PATH flag has been set in a later phase than from the ->suspend() callback, the PM core don't set the ->power.wakeup_path status flag for the device. Therefore, let's be safe and check it explicitly. Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/base/power/domain.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)