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

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

[Ulf Hansson]

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. After some
consideration I have dropped my first attempt [1] on how to address these
problems, so here's hopefully a better one.

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 need to fix the problems for the Renesas SoCs.

Geert's Renesas series [2], needs to be re-based on top of this series to make
it convert to use the new DPM_FLAG_WAKEUP_PATH flag, which is introduced in
this series. That leads to a tree-wise-dependency, so perhaps Rafael can host
an immutable branch the Renesas tree can pull in. Let's see.

[1]
https://www.spinics.net/lists/linux-renesas-soc/msg20122.html

[2]
https://www.spinics.net/lists/linux-renesas-soc/msg19947.html

Ulf Hansson (3):
  PM / core: Assign the wakeup_path status flag in __device_prepare()
  PM / core: Add WAKEUP_PATH driver flag
  PM / Domains: Take WAKEUP_PATH driver flag into account in genpd

 drivers/base/power/domain.c | 8 ++++++--
 drivers/base/power/main.c   | 5 ++++-
 include/linux/pm.h          | 7 +++++++
 3 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/3] 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 isn't supposed to
be changed during those system suspend phases. Nevertheless, there are a
cases not conforming to that behaviour, as device_set_wakeup_enable() is
indeed called from ->suspend() callbacks.

To address this, let's move the assignment of the wakeup_path status flag
to the __device_suspend() phase and more precisely, 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 (dev->power.no_pm_callbacks) {
 		ret = 1;	/* Let device go direct_complete */
-- 
2.7.4

[PATCH 2/3] PM / core: Add WAKEUP_PATH driver flag

[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 information about the wakeup path for resources, is currently
not available to a PM domain (unless it has some HW-logic that knows this),
which means that it may decide to power off a device and its PM domain. In
other words, it may prevent the wakeup signal from being delivered.

For this reason, let's introduce a new WAKEUP_PATH driver flag, to allow a
driver to indicate that it controls a resource needed in the wakeup path.
Make the PM core to check the WAKEUP_PATH flag immediately after the
->suspend() callback has been invoked and set the wakeup_path status flag
accordingly. In this way the wakeup_path flag also becomes propagated to
the parent device, which may be useful for some cases.

For a PM domain that needs the information about the wakeup path, it should
check the wakeup_path status flag for its attached devices in a later
suspend phase, but perhaps also the WAKEUP_PATH driver flag explicitly, in
case it has been set after the ->suspend() callback has been invoked for
the device.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/main.c | 3 ++-
 include/linux/pm.h        | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 810e5fb..1327726 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1620,7 +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))
+		if (device_may_wakeup(dev) ||
+		    dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_PATH))
 			dev->power.wakeup_path = true;
 		dpm_propagate_to_parent(dev);
 		dpm_clear_suppliers_direct_complete(dev);
diff --git a/include/linux/pm.h b/include/linux/pm.h
index e723b78..ebc6ef8 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -560,6 +560,7 @@ struct pm_subsys_data {
  * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
  * SMART_SUSPEND: No need to resume the device from runtime suspend.
  * LEAVE_SUSPENDED: Avoid resuming the device during system resume if possible.
+ * WAKEUP_PATH: The device is used in the wakeup path from system suspend.
  *
  * Setting SMART_PREPARE instructs bus types and PM domains which may want
  * system suspend/resume callbacks to be skipped for the device to return 0 from
@@ -576,11 +577,17 @@ struct pm_subsys_data {
  *
  * Setting LEAVE_SUSPENDED informs the PM core and middle-layer code that the
  * driver prefers the device to be left in suspend after system resume.
+ *
+ * Setting WAKEUP_PATH informs the PM core and the PM domain, that the device is
+ * a part of the wakeup path at system suspend. The PM core detects this flag
+ * and sets the wakeup_path status flag immeditaley after the ->suspend()
+ * callback has been invoked for the the device.
  */
 #define DPM_FLAG_NEVER_SKIP		BIT(0)
 #define DPM_FLAG_SMART_PREPARE		BIT(1)
 #define DPM_FLAG_SMART_SUSPEND		BIT(2)
 #define DPM_FLAG_LEAVE_SUSPENDED	BIT(3)
+#define DPM_FLAG_WAKEUP_PATH		BIT(4)
 
 struct dev_pm_info {
 	pm_message_t		power_state;
-- 
2.7.4

[PATCH 3/3] PM / Domains: Take WAKEUP_PATH driver flag into account in genpd

[Ulf Hansson]

In case the WAKEUP_PATH flag has been set in a later phase than from the
->suspend() callback, the PM core want 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)) &&
+	    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

Re: [PATCH 3/3] PM / Domains: Take WAKEUP_PATH driver flag into account in genpd

[Ulf Hansson]

On 15 December 2017 at 16:56, 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 want set the ->power.wakeup_path status

/s/want/don't

If another version is needed I fix, else perhaps you can just amend the patch!?

Kind regards
Uffe

> 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)) &&
> +           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
>

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

[Rafael J. Wysocki]

On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
> be changed during those system suspend phases. Nevertheless, there are a
> cases not conforming to that behaviour, as device_set_wakeup_enable() is
> indeed called from ->suspend() callbacks.

And why is this regarded as correct?

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

[Ulf Hansson]

On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
>> be changed during those system suspend phases. Nevertheless, there are a
>> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>> indeed called from ->suspend() callbacks.
>
> And why is this regarded as correct?

I am not saying that this behavior is correct. However, I am trying to
improve the situation, which doesn't hurt or does it?

More importantly, the next patch, which is about the wakeup path,
depends on this.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
> >> be changed during those system suspend phases. Nevertheless, there are a
> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
> >> indeed called from ->suspend() callbacks.
> >
> > And why is this regarded as correct?
> 
> I am not saying that this behavior is correct. However, I am trying to
> improve the situation, which doesn't hurt or does it?

Adding a workaround for them kind of encourages new code to do the same
thing, which actually may really hurt.  So I assume that these call sites are
all legacy and that's why you don't want to touch them for now, but in that
case the commit message should make it very clear that this is about legacy
only and new code should not call device_set_wakeup_enable() during suspend.

[Something should be printed to the log if wakeup source objects
are created while system suspend is in progress I guess or similar.]

> More importantly, the next patch, which is about the wakeup path,
> depends on this.

Honestly, this sounds like "We have this change we really really would like to
make, but there's incorrect code getting in the way, so let's paper over it
and be done."  Not very nice. :-/

How many drivers actually do call device_set_wakeup_enable() during suspend?

Thanks,
Rafael

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

[Ulf Hansson]

On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
>> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
>> >> be changed during those system suspend phases. Nevertheless, there are a
>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>> >> indeed called from ->suspend() callbacks.
>> >
>> > And why is this regarded as correct?
>>
>> I am not saying that this behavior is correct. However, I am trying to
>> improve the situation, which doesn't hurt or does it?
>
> Adding a workaround for them kind of encourages new code to do the same
> thing, which actually may really hurt.  So I assume that these call sites are
> all legacy and that's why you don't want to touch them for now, but in that
> case the commit message should make it very clear that this is about legacy
> only and new code should not call device_set_wakeup_enable() during suspend.

Yeah, makes sense. Let me clarify that!

>
> [Something should be printed to the log if wakeup source objects
> are created while system suspend is in progress I guess or similar.]

Yes, good idea. Let me cook a patch for that as well.

>
>> More importantly, the next patch, which is about the wakeup path,
>> depends on this.
>
> Honestly, this sounds like "We have this change we really really would like to
> make, but there's incorrect code getting in the way, so let's paper over it
> and be done."  Not very nice. :-/

Yeah it sounds a bit weird, I agree.

However, maybe if I update the changelog and fold in a patch printing
an error in case the APIs is being abused, that would sort out the
confusion?

Another option is simply to squash patch 1 and patch2, what do you prefer?

>
> How many drivers actually do call device_set_wakeup_enable() during suspend?

There are some ethernet/wifi drivers, although it hard to say how many
without a more thorough investigation.

Besides those I found these more obvious ones:
drivers/ssb/pcihost_wrapper.c
drivers/staging/rtlwifi/core.c
drivers/tty/serial/atmel_serial.c
drivers/usb/core/hcd-pci.c

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
>>> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>> > On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
>>> >> be changed during those system suspend phases. Nevertheless, there are a
>>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>>> >> indeed called from ->suspend() callbacks.
>>> >
>>> > And why is this regarded as correct?
>>>
>>> I am not saying that this behavior is correct. However, I am trying to
>>> improve the situation, which doesn't hurt or does it?
>>
>> Adding a workaround for them kind of encourages new code to do the same
>> thing, which actually may really hurt.  So I assume that these call sites are
>> all legacy and that's why you don't want to touch them for now, but in that
>> case the commit message should make it very clear that this is about legacy
>> only and new code should not call device_set_wakeup_enable() during suspend.
>
> Yeah, makes sense. Let me clarify that!
>
>>
>> [Something should be printed to the log if wakeup source objects
>> are created while system suspend is in progress I guess or similar.]
>
> Yes, good idea. Let me cook a patch for that as well.
>
>>
>>> More importantly, the next patch, which is about the wakeup path,
>>> depends on this.
>>
>> Honestly, this sounds like "We have this change we really really would like to
>> make, but there's incorrect code getting in the way, so let's paper over it
>> and be done."  Not very nice. :-/
>
> Yeah it sounds a bit weird, I agree.
>
> However, maybe if I update the changelog and fold in a patch printing
> an error in case the APIs is being abused, that would sort out the
> confusion?

That should be sufficient IMO.

> Another option is simply to squash patch 1 and patch2, what do you prefer?
>
>>
>> How many drivers actually do call device_set_wakeup_enable() during suspend?
>
> There are some ethernet/wifi drivers, although it hard to say how many
> without a more thorough investigation.
>
> Besides those I found these more obvious ones:
> drivers/ssb/pcihost_wrapper.c
> drivers/staging/rtlwifi/core.c
> drivers/tty/serial/atmel_serial.c
> drivers/usb/core/hcd-pci.c

Ugh.  I need to look at the last one at least ...

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Sat, Dec 23, 2017 at 1:50 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Sat, Dec 23, 2017 at 1:03 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 22 December 2017 at 20:12, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Thursday, December 21, 2017 11:13:57 AM CET Ulf Hansson wrote:
>>>> On 21 December 2017 at 02:43, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> > On Fri, Dec 15, 2017 at 4:56 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 isn't supposed to
>>>> >> be changed during those system suspend phases. Nevertheless, there are a
>>>> >> cases not conforming to that behaviour, as device_set_wakeup_enable() is
>>>> >> indeed called from ->suspend() callbacks.
>>>> >
>>>> > And why is this regarded as correct?
>>>>
>>>> I am not saying that this behavior is correct. However, I am trying to
>>>> improve the situation, which doesn't hurt or does it?
>>>
>>> Adding a workaround for them kind of encourages new code to do the same
>>> thing, which actually may really hurt.  So I assume that these call sites are
>>> all legacy and that's why you don't want to touch them for now, but in that
>>> case the commit message should make it very clear that this is about legacy
>>> only and new code should not call device_set_wakeup_enable() during suspend.
>>
>> Yeah, makes sense. Let me clarify that!
>>
>>>
>>> [Something should be printed to the log if wakeup source objects
>>> are created while system suspend is in progress I guess or similar.]
>>
>> Yes, good idea. Let me cook a patch for that as well.
>>
>>>
>>>> More importantly, the next patch, which is about the wakeup path,
>>>> depends on this.
>>>
>>> Honestly, this sounds like "We have this change we really really would like to
>>> make, but there's incorrect code getting in the way, so let's paper over it
>>> and be done."  Not very nice. :-/
>>
>> Yeah it sounds a bit weird, I agree.
>>
>> However, maybe if I update the changelog and fold in a patch printing
>> an error in case the APIs is being abused, that would sort out the
>> confusion?
>
> That should be sufficient IMO.
>
>> Another option is simply to squash patch 1 and patch2, what do you prefer?
>>
>>>
>>> How many drivers actually do call device_set_wakeup_enable() during suspend?
>>
>> There are some ethernet/wifi drivers, although it hard to say how many
>> without a more thorough investigation.
>>
>> Besides those I found these more obvious ones:
>> drivers/ssb/pcihost_wrapper.c
>> drivers/staging/rtlwifi/core.c
>> drivers/tty/serial/atmel_serial.c
>> drivers/usb/core/hcd-pci.c
>
> Ugh.  I need to look at the last one at least ...

The drivers/usb/core/hcd-pci.c instance is actually valid, because it
calls device_set_wakeup_enable() to remove the wakeup source object in
case the device is dead.  Moreover, it does that in the "noirq" phase
which is not covered by your patch anyway.

Thanks,
Rafael

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

[Ulf Hansson]

[...]

>>>> How many drivers actually do call device_set_wakeup_enable() during suspend?
>>>
>>> There are some ethernet/wifi drivers, although it hard to say how many
>>> without a more thorough investigation.
>>>
>>> Besides those I found these more obvious ones:
>>> drivers/ssb/pcihost_wrapper.c
>>> drivers/staging/rtlwifi/core.c
>>> drivers/tty/serial/atmel_serial.c
>>> drivers/usb/core/hcd-pci.c
>>
>> Ugh.  I need to look at the last one at least ...
>
> The drivers/usb/core/hcd-pci.c instance is actually valid, because it
> calls device_set_wakeup_enable() to remove the wakeup source object in
> case the device is dead.  Moreover, it does that in the "noirq" phase
> which is not covered by your patch anyway.

Right, but this puzzles me a bit.

Could you explain what you mean by valid here? Is it okay to call
device_set_wakeup_enable() in the suspend phase after all? No?

My very vague idea at this point is, if we find cases where
device_set_wakeup_enable() makes sense to call during system suspend,
probably those could/should be replaces by using the "wakeup_path"
flag instead, somehow.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Saturday, December 23, 2017 4:17:58 PM CET Ulf Hansson wrote:
> [...]
> 
> >>>> How many drivers actually do call device_set_wakeup_enable() during suspend?
> >>>
> >>> There are some ethernet/wifi drivers, although it hard to say how many
> >>> without a more thorough investigation.
> >>>
> >>> Besides those I found these more obvious ones:
> >>> drivers/ssb/pcihost_wrapper.c
> >>> drivers/staging/rtlwifi/core.c
> >>> drivers/tty/serial/atmel_serial.c
> >>> drivers/usb/core/hcd-pci.c
> >>
> >> Ugh.  I need to look at the last one at least ...
> >
> > The drivers/usb/core/hcd-pci.c instance is actually valid, because it
> > calls device_set_wakeup_enable() to remove the wakeup source object in
> > case the device is dead.  Moreover, it does that in the "noirq" phase
> > which is not covered by your patch anyway.
> 
> Right, but this puzzles me a bit.
> 
> Could you explain what you mean by valid here? Is it okay to call
> device_set_wakeup_enable() in the suspend phase after all? No?

This is an exception and it would be better to call
device_wakeup_disable() directly here.

In general, calling device_set_wakeup_enable() during system suspend (to
"enable" device wakeup) and then during system resume (to "disable" it) may
be problematic.

First, if device_wakeup_enable() is run during system suspend, it will
create a wakeup source object, which then, quite obviously, can only cover
the time after its creation, so there is a window (between the start of the
system suspend transition and the creation of the wakeup source) during
which wakeup events may be missed.

That is not only relevant for devices supporting runtime PM, because input
device drivers, say, may choose to report system wakeup events every time
they see input (eg. from their interrupt handlers or similar) and wakeup
sources used for that should be present before system suspend begins.

Also, if the wakeup source is not present already, this effectively overrides
the choice made by user space which didn't set the device's "wakeup" attribute
in sysfs to "enabled", so it should not wake up the system.

Second, if device_wakeup_disable() is run during system resume, it effectively
destroys information that can be used to figure out why the system woke up
and, again, it may cause the choice made by user space to be effectively
overridden.

However, if the device is dead, calling device_wakeup_disable() on it is fine
at any time (as dead devices do not wake up the system as a rule).

> My very vague idea at this point is, if we find cases where
> device_set_wakeup_enable() makes sense to call during system suspend,
> probably those could/should be replaces by using the "wakeup_path"
> flag instead, somehow.

Not really in this particular case.  Clearing the wakeup enable bit for the
device plays the role of "don't bother any more" here.

Thanks,
Rafael