On Tuesday, January 2, 2018 12:36:55 PM CET Ulf Hansson wrote:
> On 30 December 2017 at 01:44, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Fri, Dec 29, 2017 at 12:37 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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
