[PATCH v3 0/4] PM / core: Extend behaviour for wakeup paths

[PATCH v3 0/4] PM / core: Extend behaviour for wakeup paths

[Ulf Hansson]

Changes in v3:
	- Dropped the DPM_FLAG_WAKEUP_PATH, thus re-placed patch 2 and patch 3
	with new patches.
	- Simplified patch4, according to suggestions by Rafael.
	- No changes to patch1.

The generic problem this series intends to solve, is that for some PM domains,
especially genpd, the current information in the dev->power.wakeup_path, is not
sufficient for a PM domain to fully understand how to treat devices in the
wakeup path during system suspend.

Particularly this applies to resources being consumed by the driver that has
configured system wakeup settings for its device. Currently the PM domain lacks
information about these consumed resources, which means it may decide to power
of their corresponding device and its attached PM domain, while they actually
would need to stay powered to allow system wakeup signals to be delivered.

Geert Uytterhoeven, has been working on these kind of problems for some Renesas
SoCs and there have been lots of discussions around this. So far I haven't
included any patches for dealing with the "OUT_BAND_WAKEUP" thingy, but I
suggest we do that in a second separate step, since it's not immediately needed
to fix the problems for the Renesas SoCs.

Ulf Hansson (4):
  PM / core: Assign the wakeup_path status flag in __device_prepare()
  PM / core: Propagate wakeup_path status flag in
    __device_suspend_late()
  PM / wakeup: Add device_set_wakeup_path() helper to control wakeup
    path
  PM / wakeup: Print warn if device gets enabled as wakeup source during
    sleep

 drivers/base/power/main.c   | 37 ++++++++++++++++++++-----------------
 drivers/base/power/wakeup.c |  3 +++
 include/linux/pm_wakeup.h   |  7 +++++++
 3 files changed, 30 insertions(+), 17 deletions(-)

-- 
2.7.4

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

[Ulf Hansson]

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 70398e7..7aeb91d 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1788,6 +1788,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);
 	}
@@ -1912,7 +1914,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 (dev->power.no_pm_callbacks) {
 		ret = 1;	/* Let device go direct_complete */
-- 
2.7.4

[PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()

[Ulf Hansson]

Currently the wakeup_path status flag becomes propagated from a child
device to its parent device at __device_suspend(). This allows a driver
dealing with a parent device to act on the flag from its ->suspend()
callback.

However, in situations when the wakeup_path status flag needs to be set
from a ->suspend_late() callback, its value doesn't get propagated to the
parent by the PM core. Let's address this limitation, by also propagating
the flag at __device_suspend_late().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/main.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 7aeb91d..612bf1b 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1476,6 +1476,22 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
 	return callback;
 }
 
+static void dpm_propagate_to_parent(struct device *dev)
+{
+	struct device *parent = dev->parent;
+
+	if (!parent)
+		return;
+
+	spin_lock_irq(&parent->power.lock);
+
+	parent->power.direct_complete = false;
+	if (dev->power.wakeup_path && !parent->power.ignore_children)
+		parent->power.wakeup_path = true;
+
+	spin_unlock_irq(&parent->power.lock);
+}
+
 /**
  * __device_suspend_late - Execute a "late suspend" callback for given device.
  * @dev: Device to handle.
@@ -1528,6 +1544,7 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 		goto Complete;
 	}
 
+	dpm_propagate_to_parent(dev);
 Skip:
 	dev->power.is_late_suspended = true;
 
@@ -1660,22 +1677,6 @@ static int legacy_suspend(struct device *dev, pm_message_t state,
 	return error;
 }
 
-static void dpm_propagate_to_parent(struct device *dev)
-{
-	struct device *parent = dev->parent;
-
-	if (!parent)
-		return;
-
-	spin_lock_irq(&parent->power.lock);
-
-	parent->power.direct_complete = false;
-	if (dev->power.wakeup_path && !parent->power.ignore_children)
-		parent->power.wakeup_path = true;
-
-	spin_unlock_irq(&parent->power.lock);
-}
-
 static void dpm_clear_suppliers_direct_complete(struct device *dev)
 {
 	struct device_link *link;
-- 
2.7.4

[PATCH v3 3/4] PM / wakeup: Add device_set_wakeup_path() helper to control wakeup path

[Ulf Hansson]

During system suspend, a driver may find that the wakeup setting is enabled
for its device and therefore configures it to deliver system wakeup
signals.

Additionally, sometimes the driver and its device, relies on some further
consumed resource, like an irqchip or a phy for example, to stay powered
on, as to be able to deliver system wakeup signals.

In general the driver deals with this, via raising an "enable count" of the
consumed resource or via a subsystem specific API, like irq_set_irq_wake()
or enable|disable_irq_wake() for an irqchip. However, this may not be
sufficient in cases when the resource's device may be attached to a PM
domain (genpd for example) or is attached to a non-trivial middle layer
(PCI for example).

To address cases like these, the existing ->dev.power.wakeup_path status
flag is there to help. As a matter of fact, genpd already monitors the flag
during system suspend and acts accordingly.

However, so far it has not been clear, if anybody else but the PM core is
allowed to set the ->dev.power.wakeup_path status flag, which is required
to make this work. For this reason, let's introduce a new helper function,
device_set_wakeup_path().

Typically a driver that manages a resource needed in the wakeup path,
should call device_set_wakeup_path() from its ->suspend() or
->suspend_late() callback.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/pm_wakeup.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
index 4c2cba7..4238dde 100644
--- a/include/linux/pm_wakeup.h
+++ b/include/linux/pm_wakeup.h
@@ -88,6 +88,11 @@ static inline bool device_may_wakeup(struct device *dev)
 	return dev->power.can_wakeup && !!dev->power.wakeup;
 }
 
+static inline void device_set_wakeup_path(struct device *dev)
+{
+	dev->power.wakeup_path = true;
+}
+
 /* drivers/base/power/wakeup.c */
 extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name);
 extern struct wakeup_source *wakeup_source_create(const char *name);
@@ -174,6 +179,8 @@ static inline bool device_may_wakeup(struct device *dev)
 	return dev->power.can_wakeup && dev->power.should_wakeup;
 }
 
+static inline void device_set_wakeup_path(struct device *dev) {}
+
 static inline void __pm_stay_awake(struct wakeup_source *ws) {}
 
 static inline void pm_stay_awake(struct device *dev) {}
-- 
2.7.4

[PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep

[Ulf Hansson]

In general, wakeup settings are not supposed to be changed during any of
the system wide PM phases. The reason is simply that it would break
guarantees provided by the PM core, to properly act on active wakeup
sources.

However, there are exceptions to when, in particular, disabling a device as
wakeup source makes sense. For example, in cases when a driver realizes
that its device is dead during system suspend. For these scenarios, we
don't need to care about acting on the wakeup source correctly, because a
dead device shouldn't deliver wakeup signals.

To this reasoning and to help users to properly manage wakeup settings,
let's print a warning in cases someone calls device_wakeup_enable() during
system sleep.

Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/wakeup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index b7b8b2f..272c44b 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev)
 	if (!dev || !dev->power.can_wakeup)
 		return -EINVAL;
 
+	if (pm_suspend_target_state != PM_SUSPEND_ON)
+		dev_warn(dev, "don't enable as wakup source during sleep!\n");
+
 	ws = wakeup_source_register(dev_name(dev));
 	if (!ws)
 		return -ENOMEM;
-- 
2.7.4

Re: [PATCH v3 1/4] PM / core: Assign the wakeup_path status flag in __device_prepare()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 5:08 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>

I'm queuing this one up with an extra empty line added.

> ---
>  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 70398e7..7aeb91d 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1788,6 +1788,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);
>         }
> @@ -1912,7 +1914,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 (dev->power.no_pm_callbacks) {
>                 ret = 1;        /* Let device go direct_complete */
> --
> 2.7.4
>

Re: [PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Currently the wakeup_path status flag becomes propagated from a child
> device to its parent device at __device_suspend(). This allows a driver
> dealing with a parent device to act on the flag from its ->suspend()
> callback.
>
> However, in situations when the wakeup_path status flag needs to be set
> from a ->suspend_late() callback, its value doesn't get propagated to the
> parent by the PM core. Let's address this limitation, by also propagating
> the flag at __device_suspend_late().

This doesn't need to be done twice, so doing it once in
__device_suspend_late() should be sufficient.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/main.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 7aeb91d..612bf1b 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1476,6 +1476,22 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>         return callback;
>  }
>
> +static void dpm_propagate_to_parent(struct device *dev)
> +{
> +       struct device *parent = dev->parent;
> +
> +       if (!parent)
> +               return;
> +
> +       spin_lock_irq(&parent->power.lock);
> +
> +       parent->power.direct_complete = false;
> +       if (dev->power.wakeup_path && !parent->power.ignore_children)
> +               parent->power.wakeup_path = true;
> +
> +       spin_unlock_irq(&parent->power.lock);
> +}

Clearing the parent's direct_complete in __device_suspend_late() is
incorrect, because it potentially overwrites the value previously used
by __device_suspend() for the parent.

IMO it also need not be done under parent->power.lock, however,
because the other parent's power. flags should not be updated in
parallel with this update anyway, so maybe just move the parent's
direct_complete update to __device_suspend(), rename
dpm_propagate_to_parent() to something like
dpm_propagate_wakeup_to_parent() and call it from
__device_suspend_late() only?

Thanks,
Rafael

Re: [PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()

[Ulf Hansson]

On 5 January 2018 at 13:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Currently the wakeup_path status flag becomes propagated from a child
>> device to its parent device at __device_suspend(). This allows a driver
>> dealing with a parent device to act on the flag from its ->suspend()
>> callback.
>>
>> However, in situations when the wakeup_path status flag needs to be set
>> from a ->suspend_late() callback, its value doesn't get propagated to the
>> parent by the PM core. Let's address this limitation, by also propagating
>> the flag at __device_suspend_late().
>
> This doesn't need to be done twice, so doing it once in
> __device_suspend_late() should be sufficient.

Unfortunately no.

For example that would break drivers/ssb/pcihost_wrapper.c, as it uses
the flag from its ->suspend() callback.

Also, I expect there may be more cases when the flag may be useful to
check from a ->suspend() callback, rather than from a ->suspend_late()
(or in later phase).

>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/main.c | 33 +++++++++++++++++----------------
>>  1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 7aeb91d..612bf1b 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1476,6 +1476,22 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>>         return callback;
>>  }
>>
>> +static void dpm_propagate_to_parent(struct device *dev)
>> +{
>> +       struct device *parent = dev->parent;
>> +
>> +       if (!parent)
>> +               return;
>> +
>> +       spin_lock_irq(&parent->power.lock);
>> +
>> +       parent->power.direct_complete = false;
>> +       if (dev->power.wakeup_path && !parent->power.ignore_children)
>> +               parent->power.wakeup_path = true;
>> +
>> +       spin_unlock_irq(&parent->power.lock);
>> +}
>
> Clearing the parent's direct_complete in __device_suspend_late() is
> incorrect, because it potentially overwrites the value previously used
> by __device_suspend() for the parent.

That is already taken care of in __device_suspend_late(), with the below check:

        if (dev->power.syscore || dev->power.direct_complete)
                goto Complete;

 In other words, the dpm_propagate_to_parent() isn't called when
dev->power.direct_complete is set.

>
> IMO it also need not be done under parent->power.lock, however,
> because the other parent's power. flags should not be updated in
> parallel with this update anyway, so maybe just move the parent's
> direct_complete update to __device_suspend(), rename
> dpm_propagate_to_parent() to something like
> dpm_propagate_wakeup_to_parent() and call it from
> __device_suspend_late() only?

I can do this, if you think this is more clear, but according to the
above, it's shouldn't be needed.

Kind regards
Uffe

Re: [PATCH v3 2/4] PM / core: Propagate wakeup_path status flag in __device_suspend_late()

[Rafael J. Wysocki]

On Fri, Jan 5, 2018 at 6:12 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 5 January 2018 at 13:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Currently the wakeup_path status flag becomes propagated from a child
>>> device to its parent device at __device_suspend(). This allows a driver
>>> dealing with a parent device to act on the flag from its ->suspend()
>>> callback.
>>>
>>> However, in situations when the wakeup_path status flag needs to be set
>>> from a ->suspend_late() callback, its value doesn't get propagated to the
>>> parent by the PM core. Let's address this limitation, by also propagating
>>> the flag at __device_suspend_late().
>>
>> This doesn't need to be done twice, so doing it once in
>> __device_suspend_late() should be sufficient.
>
> Unfortunately no.
>
> For example that would break drivers/ssb/pcihost_wrapper.c, as it uses
> the flag from its ->suspend() callback.

But what drivers/ssb/pcihost_wrapper.c does is just wrong!

I guess we'd need a special variant of pci_prepare_to_sleep() with an
extra "wakeup" arg for it to do the right thing and I don't see why it
cannot do all that in ->suspend_late.

> Also, I expect there may be more cases when the flag may be useful to
> check from a ->suspend() callback, rather than from a ->suspend_late()
> (or in later phase).

I'm not inclined to believe it until I see a convincing example. :-)

And it obviously won't take the later updates of power.wakeup_path into account.

Anyway, for the time being please at least add a comment next to the
first invocation of this routine to explain why it is needed in there.

>
>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/base/power/main.c | 33 +++++++++++++++++----------------
>>>  1 file changed, 17 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index 7aeb91d..612bf1b 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1476,6 +1476,22 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>>>         return callback;
>>>  }
>>>
>>> +static void dpm_propagate_to_parent(struct device *dev)
>>> +{
>>> +       struct device *parent = dev->parent;
>>> +
>>> +       if (!parent)
>>> +               return;
>>> +
>>> +       spin_lock_irq(&parent->power.lock);
>>> +
>>> +       parent->power.direct_complete = false;
>>> +       if (dev->power.wakeup_path && !parent->power.ignore_children)
>>> +               parent->power.wakeup_path = true;
>>> +
>>> +       spin_unlock_irq(&parent->power.lock);
>>> +}
>>
>> Clearing the parent's direct_complete in __device_suspend_late() is
>> incorrect, because it potentially overwrites the value previously used
>> by __device_suspend() for the parent.
>
> That is already taken care of in __device_suspend_late(), with the below check:
>
>         if (dev->power.syscore || dev->power.direct_complete)
>                 goto Complete;
>
>  In other words, the dpm_propagate_to_parent() isn't called when
> dev->power.direct_complete is set.

Which means that the parent's direct_complete is only cleared when it
is unset already, so this isn't incorrect, but just pointless.

Well, "pointless" doesn't really sound good to me too ...

>>
>> IMO it also need not be done under parent->power.lock, however,
>> because the other parent's power. flags should not be updated in
>> parallel with this update anyway, so maybe just move the parent's
>> direct_complete update to __device_suspend(), rename
>> dpm_propagate_to_parent() to something like
>> dpm_propagate_wakeup_to_parent() and call it from
>> __device_suspend_late() only?
>
> I can do this, if you think this is more clear, but according to the
> above, it's shouldn't be needed.

Yes, please.

Apart from avoiding a pointless update of the parent's direct_complete
I think that it's better to keep it close to the update of
direct_complete for suppliers.

Thanks,
Rafael

Re: [PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep

[Rafael J. Wysocki]

On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> In general, wakeup settings are not supposed to be changed during any of
> the system wide PM phases. The reason is simply that it would break
> guarantees provided by the PM core, to properly act on active wakeup
> sources.
>
> However, there are exceptions to when, in particular, disabling a device as
> wakeup source makes sense. For example, in cases when a driver realizes
> that its device is dead during system suspend. For these scenarios, we
> don't need to care about acting on the wakeup source correctly, because a
> dead device shouldn't deliver wakeup signals.
>
> To this reasoning and to help users to properly manage wakeup settings,
> let's print a warning in cases someone calls device_wakeup_enable() during
> system sleep.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/wakeup.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index b7b8b2f..272c44b 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev)
>         if (!dev || !dev->power.can_wakeup)
>                 return -EINVAL;
>
> +       if (pm_suspend_target_state != PM_SUSPEND_ON)
> +               dev_warn(dev, "don't enable as wakup source during sleep!\n");
> +
>         ws = wakeup_source_register(dev_name(dev));
>         if (!ws)
>                 return -ENOMEM;
> --

It looks like we need to defer this until we fix a couple of issues ...

Thanks,
Rafael

Re: [PATCH v3 3/4] PM / wakeup: Add device_set_wakeup_path() helper to control wakeup path

[Rafael J. Wysocki]

On Tuesday, January 2, 2018 5:08:52 PM CET Ulf Hansson wrote:
> During system suspend, a driver may find that the wakeup setting is enabled
> for its device and therefore configures it to deliver system wakeup
> signals.
> 
> Additionally, sometimes the driver and its device, relies on some further
> consumed resource, like an irqchip or a phy for example, to stay powered
> on, as to be able to deliver system wakeup signals.
> 
> In general the driver deals with this, via raising an "enable count" of the
> consumed resource or via a subsystem specific API, like irq_set_irq_wake()
> or enable|disable_irq_wake() for an irqchip. However, this may not be
> sufficient in cases when the resource's device may be attached to a PM
> domain (genpd for example) or is attached to a non-trivial middle layer
> (PCI for example).
> 
> To address cases like these, the existing ->dev.power.wakeup_path status
> flag is there to help. As a matter of fact, genpd already monitors the flag
> during system suspend and acts accordingly.
> 
> However, so far it has not been clear, if anybody else but the PM core is
> allowed to set the ->dev.power.wakeup_path status flag, which is required
> to make this work. For this reason, let's introduce a new helper function,
> device_set_wakeup_path().
> 
> Typically a driver that manages a resource needed in the wakeup path,
> should call device_set_wakeup_path() from its ->suspend() or
> ->suspend_late() callback.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  include/linux/pm_wakeup.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index 4c2cba7..4238dde 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -88,6 +88,11 @@ static inline bool device_may_wakeup(struct device *dev)
>  	return dev->power.can_wakeup && !!dev->power.wakeup;
>  }
>  
> +static inline void device_set_wakeup_path(struct device *dev)
> +{
> +	dev->power.wakeup_path = true;
> +}
> +
>  /* drivers/base/power/wakeup.c */
>  extern void wakeup_source_prepare(struct wakeup_source *ws, const char *name);
>  extern struct wakeup_source *wakeup_source_create(const char *name);
> @@ -174,6 +179,8 @@ static inline bool device_may_wakeup(struct device *dev)
>  	return dev->power.can_wakeup && dev->power.should_wakeup;
>  }
>  
> +static inline void device_set_wakeup_path(struct device *dev) {}
> +
>  static inline void __pm_stay_awake(struct wakeup_source *ws) {}
>  
>  static inline void pm_stay_awake(struct device *dev) {}
> 

Applied, thanks!

Re: [PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep

[Ulf Hansson]

On 6 January 2018 at 00:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> In general, wakeup settings are not supposed to be changed during any of
>> the system wide PM phases. The reason is simply that it would break
>> guarantees provided by the PM core, to properly act on active wakeup
>> sources.
>>
>> However, there are exceptions to when, in particular, disabling a device as
>> wakeup source makes sense. For example, in cases when a driver realizes
>> that its device is dead during system suspend. For these scenarios, we
>> don't need to care about acting on the wakeup source correctly, because a
>> dead device shouldn't deliver wakeup signals.
>>
>> To this reasoning and to help users to properly manage wakeup settings,
>> let's print a warning in cases someone calls device_wakeup_enable() during
>> system sleep.
>>
>> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  drivers/base/power/wakeup.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>> index b7b8b2f..272c44b 100644
>> --- a/drivers/base/power/wakeup.c
>> +++ b/drivers/base/power/wakeup.c
>> @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev)
>>         if (!dev || !dev->power.can_wakeup)
>>                 return -EINVAL;
>>
>> +       if (pm_suspend_target_state != PM_SUSPEND_ON)
>> +               dev_warn(dev, "don't enable as wakup source during sleep!\n");
>> +
>>         ws = wakeup_source_register(dev_name(dev));
>>         if (!ws)
>>                 return -ENOMEM;
>> --
>
> It looks like we need to defer this until we fix a couple of issues ...

Yes.

Or, perhaps changing from dev_warn() to dev_dbg(), as it could provide
us with some more detailed information, compared to a "git grep"
research?

Kind regards
Uffe

Re: [PATCH v3 4/4] PM / wakeup: Print warn if device gets enabled as wakeup source during sleep

[Rafael J. Wysocki]

On Mon, Jan 8, 2018 at 12:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 6 January 2018 at 00:57, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Tue, Jan 2, 2018 at 5:08 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> In general, wakeup settings are not supposed to be changed during any of
>>> the system wide PM phases. The reason is simply that it would break
>>> guarantees provided by the PM core, to properly act on active wakeup
>>> sources.
>>>
>>> However, there are exceptions to when, in particular, disabling a device as
>>> wakeup source makes sense. For example, in cases when a driver realizes
>>> that its device is dead during system suspend. For these scenarios, we
>>> don't need to care about acting on the wakeup source correctly, because a
>>> dead device shouldn't deliver wakeup signals.
>>>
>>> To this reasoning and to help users to properly manage wakeup settings,
>>> let's print a warning in cases someone calls device_wakeup_enable() during
>>> system sleep.
>>>
>>> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  drivers/base/power/wakeup.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
>>> index b7b8b2f..272c44b 100644
>>> --- a/drivers/base/power/wakeup.c
>>> +++ b/drivers/base/power/wakeup.c
>>> @@ -268,6 +268,9 @@ int device_wakeup_enable(struct device *dev)
>>>         if (!dev || !dev->power.can_wakeup)
>>>                 return -EINVAL;
>>>
>>> +       if (pm_suspend_target_state != PM_SUSPEND_ON)
>>> +               dev_warn(dev, "don't enable as wakup source during sleep!\n");
>>> +
>>>         ws = wakeup_source_register(dev_name(dev));
>>>         if (!ws)
>>>                 return -ENOMEM;
>>> --
>>
>> It looks like we need to defer this until we fix a couple of issues ...
>
> Yes.
>
> Or, perhaps changing from dev_warn() to dev_dbg(), as it could provide
> us with some more detailed information, compared to a "git grep"
> research?

That would work too.

Thanks,
Rafael