[v2,3/4] PM / Domains: Take WAKEUP_PATH driver flag into account in 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(-)

-- 
2.7.4

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