[v2,3/4] PM / Domains: Take WAKEUP_PATH driver flag into account in genpd

Message ID 1514547423-18965-4-git-send-email-ulf.hansson@linaro.org
State New
Headers show
Series
  • PM / core: Extend behaviour for wakeup paths
Related show

Commit Message

Ulf Hansson Dec. 29, 2017, 11:37 a.m.
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(-)

-- 
2.7.4

Comments

Rafael J. Wysocki Dec. 30, 2017, 12:47 a.m. | #1
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

>
Ulf Hansson Jan. 2, 2018, 11:46 a.m. | #2
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
Rafael J. Wysocki Jan. 2, 2018, 11:52 a.m. | #3
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

Patch

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);