[v2,1/4] PM / core: Assign the wakeup_path status flag in __device_prepare()

The PM core in the device_prepare() phase, resets the wakeup_path status
flag to the value of device_may_wakeup(). This means if a ->prepare() or a
->suspend() callback for the device would update the device's wakeup
setting, this doesn't become reflected in the wakeup_path status flag.

In general this isn't a problem, because wakeup settings are not supposed
to be changed (via for example calling device_set_wakeup_enable()) during
any system wide suspend/resume phase.  Nevertheless there are some users,
which can be considered as legacy, that don't conform to this behaviour.

These legacy cases should be corrected, however until that is done, let's
address the issue from the PM core, by moving the assignment of the
wakeup_path status flag to the __device_suspend() phase and after the
->suspend() callback has been invoked.

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

---
 drivers/base/power/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.7.4

On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The PM core in the device_prepare() phase, resets the wakeup_path status

> flag to the value of device_may_wakeup(). This means if a ->prepare() or a

> ->suspend() callback for the device would update the device's wakeup

> setting, this doesn't become reflected in the wakeup_path status flag.

>

> In general this isn't a problem, because wakeup settings are not supposed

> to be changed (via for example calling device_set_wakeup_enable()) during

> any system wide suspend/resume phase.  Nevertheless there are some users,

> which can be considered as legacy, that don't conform to this behaviour.

>

> These legacy cases should be corrected, however until that is done, let's

> address the issue from the PM core, by moving the assignment of the

> wakeup_path status flag to the __device_suspend() phase and after the

> ->suspend() callback has been invoked.

>

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

> ---

>  drivers/base/power/main.c | 4 +++-

>  1 file changed, 3 insertions(+), 1 deletion(-)

>

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

> index 6e8cc5d..810e5fb 100644

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

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

> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)

>   End:

>         if (!error) {

>                 dev->power.is_suspended = true;

> +               if (device_may_wakeup(dev))

> +                       dev->power.wakeup_path = true;

>                 dpm_propagate_to_parent(dev);

>                 dpm_clear_suppliers_direct_complete(dev);

>         }

> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev, pm_message_t state)

>

>         device_lock(dev);

>

> -       dev->power.wakeup_path = device_may_wakeup(dev);

> +       dev->power.wakeup_path = false;


If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in
__device_suspend(), it wouldn't need to be cleared here I guess?

>

>         if (dev->power.no_pm_callbacks) {

>                 ret = 1;        /* Let device go direct_complete */

> --

> 2.7.4

>

On 30 December 2017 at 01:44, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> The PM core in the device_prepare() phase, resets the wakeup_path status

>> flag to the value of device_may_wakeup(). This means if a ->prepare() or a

>> ->suspend() callback for the device would update the device's wakeup

>> setting, this doesn't become reflected in the wakeup_path status flag.

>>

>> In general this isn't a problem, because wakeup settings are not supposed

>> to be changed (via for example calling device_set_wakeup_enable()) during

>> any system wide suspend/resume phase.  Nevertheless there are some users,

>> which can be considered as legacy, that don't conform to this behaviour.

>>

>> These legacy cases should be corrected, however until that is done, let's

>> address the issue from the PM core, by moving the assignment of the

>> wakeup_path status flag to the __device_suspend() phase and after the

>> ->suspend() callback has been invoked.

>>

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

>> ---

>>  drivers/base/power/main.c | 4 +++-

>>  1 file changed, 3 insertions(+), 1 deletion(-)

>>

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

>> index 6e8cc5d..810e5fb 100644

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

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

>> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)

>>   End:

>>         if (!error) {

>>                 dev->power.is_suspended = true;

>> +               if (device_may_wakeup(dev))

>> +                       dev->power.wakeup_path = true;

>>                 dpm_propagate_to_parent(dev);

>>                 dpm_clear_suppliers_direct_complete(dev);

>>         }

>> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev, pm_message_t state)

>>

>>         device_lock(dev);

>>

>> -       dev->power.wakeup_path = device_may_wakeup(dev);

>> +       dev->power.wakeup_path = false;

>

> If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in

> __device_suspend(), it wouldn't need to be cleared here I guess?

>


No that doesn't work, because it may override the value of the flag
wrongly, in case a child's wakeup_path flag that is set and has been
propagated to the parent.

>>

>>         if (dev->power.no_pm_callbacks) {

>>                 ret = 1;        /* Let device go direct_complete */

>> --

>> 2.7.4

>>


Kind regards
Uffe

On Tuesday, January 2, 2018 12:36:55 PM CET Ulf Hansson wrote:
> On 30 December 2017 at 01:44, Rafael J. Wysocki <rafael@kernel.org> wrote:

> > On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> >> The PM core in the device_prepare() phase, resets the wakeup_path status

> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a

> >> ->suspend() callback for the device would update the device's wakeup

> >> setting, this doesn't become reflected in the wakeup_path status flag.

> >>

> >> In general this isn't a problem, because wakeup settings are not supposed

> >> to be changed (via for example calling device_set_wakeup_enable()) during

> >> any system wide suspend/resume phase.  Nevertheless there are some users,

> >> which can be considered as legacy, that don't conform to this behaviour.

> >>

> >> These legacy cases should be corrected, however until that is done, let's

> >> address the issue from the PM core, by moving the assignment of the

> >> wakeup_path status flag to the __device_suspend() phase and after the

> >> ->suspend() callback has been invoked.

> >>

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

> >> ---

> >>  drivers/base/power/main.c | 4 +++-

> >>  1 file changed, 3 insertions(+), 1 deletion(-)

> >>

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

> >> index 6e8cc5d..810e5fb 100644

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

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

> >> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)

> >>   End:

> >>         if (!error) {

> >>                 dev->power.is_suspended = true;

> >> +               if (device_may_wakeup(dev))

> >> +                       dev->power.wakeup_path = true;

> >>                 dpm_propagate_to_parent(dev);

> >>                 dpm_clear_suppliers_direct_complete(dev);

> >>         }

> >> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev, pm_message_t state)

> >>

> >>         device_lock(dev);

> >>

> >> -       dev->power.wakeup_path = device_may_wakeup(dev);

> >> +       dev->power.wakeup_path = false;

> >

> > If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in

> > __device_suspend(), it wouldn't need to be cleared here I guess?

> >

> 

> No that doesn't work, because it may override the value of the flag

> wrongly, in case a child's wakeup_path flag that is set and has been

> propagated to the parent.


I see, OK.

So it looks like the new driver flag is not really necessary.

The wakeup_path propagation may be done even later, like at the end of
__device_suspend_late(), which is fine, because it is not going to be used
before __device_suspend_noirq() anyway.

Then, drivers can set it from their ->suspend and ->suspend_late callbacks.

Thanks,
Rafael

On 2 January 2018 at 12:43, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, January 2, 2018 12:36:55 PM CET Ulf Hansson wrote:

>> On 30 December 2017 at 01:44, Rafael J. Wysocki <rafael@kernel.org> wrote:

>> > On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> >> The PM core in the device_prepare() phase, resets the wakeup_path status

>> >> flag to the value of device_may_wakeup(). This means if a ->prepare() or a

>> >> ->suspend() callback for the device would update the device's wakeup

>> >> setting, this doesn't become reflected in the wakeup_path status flag.

>> >>

>> >> In general this isn't a problem, because wakeup settings are not supposed

>> >> to be changed (via for example calling device_set_wakeup_enable()) during

>> >> any system wide suspend/resume phase.  Nevertheless there are some users,

>> >> which can be considered as legacy, that don't conform to this behaviour.

>> >>

>> >> These legacy cases should be corrected, however until that is done, let's

>> >> address the issue from the PM core, by moving the assignment of the

>> >> wakeup_path status flag to the __device_suspend() phase and after the

>> >> ->suspend() callback has been invoked.

>> >>

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

>> >> ---

>> >>  drivers/base/power/main.c | 4 +++-

>> >>  1 file changed, 3 insertions(+), 1 deletion(-)

>> >>

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

>> >> index 6e8cc5d..810e5fb 100644

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

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

>> >> @@ -1620,6 +1620,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)

>> >>   End:

>> >>         if (!error) {

>> >>                 dev->power.is_suspended = true;

>> >> +               if (device_may_wakeup(dev))

>> >> +                       dev->power.wakeup_path = true;

>> >>                 dpm_propagate_to_parent(dev);

>> >>                 dpm_clear_suppliers_direct_complete(dev);

>> >>         }

>> >> @@ -1744,7 +1746,7 @@ static int device_prepare(struct device *dev, pm_message_t state)

>> >>

>> >>         device_lock(dev);

>> >>

>> >> -       dev->power.wakeup_path = device_may_wakeup(dev);

>> >> +       dev->power.wakeup_path = false;

>> >

>> > If you did "dev->power.wakeup_path = device_may_wakeup(dev)" in

>> > __device_suspend(), it wouldn't need to be cleared here I guess?

>> >

>>

>> No that doesn't work, because it may override the value of the flag

>> wrongly, in case a child's wakeup_path flag that is set and has been

>> propagated to the parent.

>

> I see, OK.

>

> So it looks like the new driver flag is not really necessary.


Okay, I am fine with that. Please just check the other mail thread,
should we have a helper function that users call to set the flag?

>

> The wakeup_path propagation may be done even later, like at the end of

> __device_suspend_late(), which is fine, because it is not going to be used

> before __device_suspend_noirq() anyway.


I think we need to keep existing code in __device_suspend() as the
flag is used by drivers/ssb/pcihost_wrapper.c, from its ->suspend()
callback. Moreover, I think this is probably useful for other similar
scenarios of drivers dealing with parent devices.

However, adopting __device_suspend_late() to also deal the propagation
of the flag is a good idea.

>

> Then, drivers can set it from their ->suspend and ->suspend_late callbacks.


Yes, that would be great!

Kind regards
Uffe