[PATCH v2 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[PATCH v2 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

The generic problem this series is trying to solve, is that for some bus types
and PM domains, it's not sufficient to only check the return value from
device_may_wakeup(), to fully understand how to treat the device during system
suspend.

One particular case that suffers from this, is the generic PM domain (aka genpd)
and that is taken care of in the final change in this series.

The special case this series address, is to enable drivers to instruct bus types
and PM domains, that the device need to remain in its current power state in
case it should be able to generate wakeup signals during system suspend. This is
done by an opt-in method, however bus types and PM domains that has additional
knowledge about devices' wakeup settings may still override the setting.

Geert Uytterhoeven, has been working on some related problems for some Renesas
SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
devices/drivers, which are used together with genpd. My intent is that this
series enables a solution for those problems.

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

Changes in v2:
	- See change logs for each patch.


Ulf Hansson (3):
  PM / core: Re-factor some code dealing with parents in
    __device_suspend()
  PM / core: Add IN_BAND_WAKEUP driver flag
  PM / Domains: Take wakeup_path_in_band status flag into account

 Documentation/driver-api/pm/devices.rst | 11 +++++++++++
 drivers/base/power/domain.c             |  6 ++++--
 drivers/base/power/main.c               | 35 ++++++++++++++++++++++-----------
 include/linux/pm.h                      |  6 ++++++
 4 files changed, 44 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] PM / core: Re-factor some code dealing with parents in __device_suspend()

[Ulf Hansson]

Let's make the code a bit more readable by moving some of the code, which
deals with adjustments for parent devices in __device_suspend(), into its
own function.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

Changes in v2:
	- Added Geert's Reviewed-by tag.

---
 drivers/base/power/main.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 6c6f1c7..8089e72 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1422,6 +1422,22 @@ 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;
@@ -1530,19 +1546,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 
  End:
 	if (!error) {
-		struct device *parent = dev->parent;
-
 		dev->power.is_suspended = true;
-		if (parent) {
-			spin_lock_irq(&parent->power.lock);
-
-			dev->parent->power.direct_complete = false;
-			if (dev->power.wakeup_path
-			    && !dev->parent->power.ignore_children)
-				dev->parent->power.wakeup_path = true;
-
-			spin_unlock_irq(&parent->power.lock);
-		}
+		dpm_propagate_to_parent(dev);
 		dpm_clear_suppliers_direct_complete(dev);
 	}
 
-- 
2.7.4

Re: [PATCH v2 1/3] PM / core: Re-factor some code dealing with parents in __device_suspend()

[Rafael J. Wysocki]

On Monday, November 13, 2017 4:46:41 PM CET Ulf Hansson wrote:
> Let's make the code a bit more readable by moving some of the code, which
> deals with adjustments for parent devices in __device_suspend(), into its
> own function.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Applied, thanks!

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

[Ulf Hansson]

For some bus types and PM domains, it's not sufficient to only check the
return value from device_may_wakeup(), to fully understand how to configure
wakeup settings for the device during system suspend.

In particular, sometimes the device may need to remain in its power state,
in case the driver has configured it for in band IRQs, as to be able to
generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
driver flag, to enable drivers to instruct bus types and PM domains about
this setting.

Of course, in case the bus type and PM domain has additional information
about wakeup settings of the device, they may override the behaviour.

Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
types and PM domains should treat the device's parent during system
suspend. Therefore, in __device_suspend(), let the PM core propagate the
wakeup setting by using a status flag in the struct dev_pm_info for the
parent. This also makes it consistent with how the existing "wakeup_path"
status flag is being assigned.

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

Changes in v2:
	- Fixed comments from Rafael:
	  - Rename flag to IN_BAND_WAKEUP.
	  - Clarify doc and changelog.
	  - Use a separate status flag when propagating to parents in PM core.
	- I didn't add Geert's Reviewed-by, because of new changes.

---
 Documentation/driver-api/pm/devices.rst | 11 +++++++++++
 drivers/base/power/main.c               |  8 +++++++-
 include/linux/pm.h                      |  6 ++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
index 53c1b0b..b7fddf2 100644
--- a/Documentation/driver-api/pm/devices.rst
+++ b/Documentation/driver-api/pm/devices.rst
@@ -414,6 +414,17 @@ when the system is in the sleep state.  For example, :c:func:`enable_irq_wake()`
 might identify GPIO signals hooked up to a switch or other external hardware,
 and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
 
+Sometimes :c:func:`device_may_wakeup(dev)` is not sufficient to understand how a
+wakeup signal needs to be configured. Particularly, in some cases the driver
+needs to instruct bus types and PM domains, that it has configured the device to
+use an in-band wakeup setting, which may require the device to remain in its
+power state to be able to generate wakeup signals. Therefore, drivers can set
+``DPM_FLAG_IN_BAND_WAKEUP`` in :c:member:`power.driver_flags`, by passing the
+flag to :c:func:`dev_pm_set_driver_flags` helper, which instructs bus types and
+PM domains to use this configuration. However, the bus types and PM domains may
+override this setting, in case they have additional information about wakeup
+settings of the device.
+
 If any of these callbacks returns an error, the system won't enter the desired
 low-power state.  Instead, the PM core will unwind its actions by resuming all
 the devices that were suspended.
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 8089e72..8ec3189 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1432,8 +1432,11 @@ static void dpm_propagate_to_parent(struct device *dev)
 	spin_lock_irq(&parent->power.lock);
 
 	parent->power.direct_complete = false;
-	if (dev->power.wakeup_path && !parent->power.ignore_children)
+	if (dev->power.wakeup_path && !parent->power.ignore_children) {
 		parent->power.wakeup_path = true;
+		parent->power.wakeup_path_in_band =
+			dev->power.wakeup_path_in_band;
+	}
 
 	spin_unlock_irq(&parent->power.lock);
 }
@@ -1547,6 +1550,8 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
  End:
 	if (!error) {
 		dev->power.is_suspended = true;
+		if (dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP))
+			dev->power.wakeup_path_in_band = true;
 		dpm_propagate_to_parent(dev);
 		dpm_clear_suppliers_direct_complete(dev);
 	}
@@ -1671,6 +1676,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_in_band = false;
 
 	if (dev->power.no_pm_callbacks) {
 		ret = 1;	/* Let device go direct_complete */
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 65d3911..4efb16a 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -559,6 +559,7 @@ struct pm_subsys_data {
  * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
  * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
  * SMART_SUSPEND: No need to resume the device from runtime suspend.
+ * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled.
  *
  * 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
@@ -572,10 +573,14 @@ struct pm_subsys_data {
  * necessary from the driver's perspective.  It also may cause them to skip
  * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by
  * the driver if they decide to leave the device in runtime suspend.
+ *
+ * Setting WAKEUP_POWERED instructs bus types and PM domains that the device
+ * needs to remain powered in system suspend, in case wakeup is enabled for it.
  */
 #define DPM_FLAG_NEVER_SKIP	BIT(0)
 #define DPM_FLAG_SMART_PREPARE	BIT(1)
 #define DPM_FLAG_SMART_SUSPEND	BIT(2)
+#define DPM_FLAG_IN_BAND_WAKEUP	BIT(3)
 
 struct dev_pm_info {
 	pm_message_t		power_state;
@@ -595,6 +600,7 @@ struct dev_pm_info {
 	struct completion	completion;
 	struct wakeup_source	*wakeup;
 	bool			wakeup_path:1;
+	bool			wakeup_path_in_band:1;
 	bool			syscore:1;
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
 #else
-- 
2.7.4

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Vincent Guittot]

Hi Ulf,

you have left some old naming WAKEUP_POWERED in your patch

On 13 November 2017 at 16:46, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> For some bus types and PM domains, it's not sufficient to only check the
> return value from device_may_wakeup(), to fully understand how to configure
> wakeup settings for the device during system suspend.
>
[snip]

> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 65d3911..4efb16a 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -559,6 +559,7 @@ struct pm_subsys_data {
>   * NEVER_SKIP: Do not skip system suspend/resume callbacks for the device.
>   * SMART_PREPARE: Check the return value of the driver's ->prepare callback.
>   * SMART_SUSPEND: No need to resume the device from runtime suspend.
> + * WAKEUP_POWERED: Keep the device powered if it has wakeup enabled.

it should be IN_BAND_WAKEUP ?

>   *
>   * 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
> @@ -572,10 +573,14 @@ struct pm_subsys_data {
>   * necessary from the driver's perspective.  It also may cause them to skip
>   * invocations of the ->suspend_late and ->suspend_noirq callbacks provided by
>   * the driver if they decide to leave the device in runtime suspend.
> + *
> + * Setting WAKEUP_POWERED instructs bus types and PM domains that the device

it should be IN_BAND_WAKEUP ?

> + * needs to remain powered in system suspend, in case wakeup is enabled for it.
>   */
>  #define DPM_FLAG_NEVER_SKIP    BIT(0)
>  #define DPM_FLAG_SMART_PREPARE BIT(1)
>  #define DPM_FLAG_SMART_SUSPEND BIT(2)
> +#define DPM_FLAG_IN_BAND_WAKEUP        BIT(3)
>
>  struct dev_pm_info {
>         pm_message_t            power_state;
> @@ -595,6 +600,7 @@ struct dev_pm_info {
>         struct completion       completion;
>         struct wakeup_source    *wakeup;
>         bool                    wakeup_path:1;
> +       bool                    wakeup_path_in_band:1;
>         bool                    syscore:1;
>         bool                    no_pm_callbacks:1;      /* Owned by the PM core */
>  #else
> --
> 2.7.4
>

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Rafael J. Wysocki]

On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
> For some bus types and PM domains, it's not sufficient to only check the
> return value from device_may_wakeup(), to fully understand how to configure
> wakeup settings for the device during system suspend.
> 
> In particular, sometimes the device may need to remain in its power state,
> in case the driver has configured it for in band IRQs, as to be able to
> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
> driver flag, to enable drivers to instruct bus types and PM domains about
> this setting.
> 
> Of course, in case the bus type and PM domain has additional information
> about wakeup settings of the device, they may override the behaviour.
> 
> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
> types and PM domains should treat the device's parent during system
> suspend. Therefore, in __device_suspend(), let the PM core propagate the
> wakeup setting by using a status flag in the struct dev_pm_info for the
> parent. This also makes it consistent with how the existing "wakeup_path"
> status flag is being assigned.

I've been thinking about this quite a bit recently and my conclusion is that
the flag makes perfect sence (as it covers a valid use case), but I would
define it and design the handling of it a bit differently.

First off, the genpd changes in patch [3/3] effectively skip the "noirq"
driver callbacks for the device, but question is why this is just "noirq".
Arguably, the "late suspend" callback should be skipped too (the regular
"suspend" one not necessarily, because the state of the device can still
be changed via runtime PM at that point).  Moreover, if we go for the
skipping at the suspend time, all of the resume should be skipped for the
device too, because it has not been suspended in the first place.

But, if the device is in suspend at the suspend time already, the
resume part should not be skipped for it at all (unless it has
LEAVE_SUSPENDED set too), so it looks like the skipping should only
happen if the device has not been suspended when the system transition
starts.

Second, it looks like the flag should be handled in the core in
accordance with what genpd does with it or drivers will have to
check the middle layer configuration.

So below is my version of the core part (on top of the series I posted
earlier today: https://marc.info/?l=linux-pm&m=151286423304445&w=2)
and the genpd one should be analogous IMO.

---
 Documentation/driver-api/pm/devices.rst |   21 ++++++++-
 drivers/base/power/main.c               |   71 +++++++++++++++++++++++++-------
 include/linux/pm.h                      |    8 +++
 3 files changed, 82 insertions(+), 18 deletions(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/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.
+ * IN_BAND_WAKEUP: Avoid suspending the device if configured for system wakeup.
  *
  * 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,16 @@ 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 IN_BAND_WAKEUP informs the PM core and middle-layer code that, as far
+ * as the driver knows, the device cannot be suspended to be able to wake up the
+ * system from sleep.
  */
 #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_IN_BAND_WAKEUP		BIT(4)
 
 struct dev_pm_info {
 	pm_message_t		power_state;
@@ -604,6 +610,7 @@ struct dev_pm_info {
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
 	unsigned int		must_resume:1;	/* Owned by the PM core */
 	unsigned int		may_skip_resume:1;	/* Set by subsystems */
+	unsigned int		skip_suspend:1;	/* Owned by the PM core */
 #else
 	unsigned int		should_wakeup:1;
 #endif
@@ -774,6 +781,7 @@ extern void pm_generic_complete(struct d
 
 extern void dev_pm_skip_next_resume_phases(struct device *dev);
 extern bool dev_pm_may_skip_resume(struct device *dev);
+extern bool dev_pm_skip_suspend_and_not_suspended(struct device *dev);
 extern bool dev_pm_smart_suspend_and_suspended(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
Index: linux-pm/Documentation/driver-api/pm/devices.rst
===================================================================
--- linux-pm.orig/Documentation/driver-api/pm/devices.rst
+++ linux-pm/Documentation/driver-api/pm/devices.rst
@@ -401,6 +401,10 @@ the phases are: ``prepare``, ``suspend``
 	generated by some other device after its own device had been set to low
 	power.
 
+If any of these callbacks returns an error, the system won't enter the desired
+low-power state.  Instead, the PM core will unwind its actions by resuming all
+the devices that were suspended.
+
 At the end of these phases, drivers should have stopped all I/O transactions
 (DMA, IRQs), saved enough state that they can re-initialize or restore previous
 state (as needed by the hardware), and placed the device into a low-power state.
@@ -414,9 +418,20 @@ when the system is in the sleep state.
 might identify GPIO signals hooked up to a switch or other external hardware,
 and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
 
-If any of these callbacks returns an error, the system won't enter the desired
-low-power state.  Instead, the PM core will unwind its actions by resuming all
-the devices that were suspended.
+Some devices can only generate any signals if they are not suspended at all.  If
+:c:func:`device_may_wakeup(dev)` returns ``true`` for such a device, it should
+not be suspended during system-wide transitions to sleep states.  Device drivers
+can indicate that condition to the PM core and middle-layer code (bus types or
+PM domains) by setting the ``DPM_FLAG_IN_BAND_WAKEUP`` driver flag (at the probe
+time).  The PM core and middle-layer code respond to that by avoiding to suspend
+the device unless they have additional information on the device's wakeup
+capabilities (for example, they may know that the device is in fact capable of
+generating wakeup signals from a low-power state which the driver may not be
+aware of).  In particular, the PM core will not invoke driver callbacks in the
+``suspend_late`` and ``suspend_noirq`` phases for devices with
+``DPM_FLAG_IN_BAND_WAKEUP`` set and for their ancestors and suppliers, but it
+still will invoke middle-layer callbacks for them (if present) and those
+callbacks are then responsible for handling the devices as appropriate.
 
 
 Leaving System Suspend
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -609,6 +609,14 @@ static pm_callback_t dpm_subsys_suspend_
 						pm_message_t state,
 						const char **info_p);
 
+static bool device_no_subsys_suspend(struct device *dev, pm_message_t state)
+{
+	pm_message_t suspend_msg = suspend_event(state);
+
+	return !dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
+		!dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL);
+}
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -645,25 +653,26 @@ static int device_resume_noirq(struct de
 	if (skip_resume)
 		goto Skip;
 
-	if (dev_pm_smart_suspend_and_suspended(dev)) {
-		pm_message_t suspend_msg = suspend_event(state);
-
-		/*
-		 * If "freeze" callbacks have been skipped during a transition
-		 * related to hibernation, the subsequent "thaw" callbacks must
-		 * be skipped too or bad things may happen.  Otherwise, resume
-		 * callbacks are going to be run for the device, so its runtime
-		 * PM status must be changed to reflect the new state after the
-		 * transition under way.
-		 */
-		if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
-		    !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
+	if (device_no_subsys_suspend(dev, state)) {
+		if (dev_pm_smart_suspend_and_suspended(dev)) {
+			/*
+			 * If "freeze" callbacks have been skipped during a
+			 * transition related to hibernation, the subsequent
+			 * "thaw" callbacks must be skipped too or bad things
+			 * may happen.  Otherwise, resume callbacks are going to
+			 * be run for the device, so its runtime PM status must
+			 * be changed to reflect the new state after the
+			 * transition under way.
+			 */
 			if (state.event == PM_EVENT_THAW) {
 				skip_resume = true;
 				goto Skip;
 			} else {
 				pm_runtime_set_active(dev);
 			}
+		} else if (dev_pm_skip_suspend_and_not_suspended(dev)) {
+			skip_resume = true;
+			goto Skip;
 		}
 	}
 
@@ -1317,7 +1326,8 @@ static int __device_suspend_noirq(struct
 
 	no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL);
 
-	if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
+	if ((dev_pm_smart_suspend_and_suspended(dev) ||
+	     dev_pm_skip_suspend_and_not_suspended(dev)) && no_subsys_cb)
 		goto Skip;
 
 	if (dev->driver && dev->driver->pm) {
@@ -1450,6 +1460,22 @@ int dpm_suspend_noirq(pm_message_t state
 	return ret;
 }
 
+static void dpm_superior_set_skip_suspend(struct device *dev)
+{
+	struct device_link *link;
+	int idx;
+
+	if (dev->parent)
+		dev->parent->power.skip_suspend = true;
+
+	idx = device_links_read_lock();
+
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+		link->supplier->power.skip_suspend = true;
+
+	device_links_read_unlock(idx);
+}
+
 static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
 						pm_message_t state,
 						const char **info_p)
@@ -1511,11 +1537,17 @@ static int __device_suspend_late(struct
 	if (dev->power.syscore || dev->power.direct_complete)
 		goto Complete;
 
+	if ((state.event == PM_EVENT_SUSPEND || state.event == PM_EVENT_HIBERNATE) &&
+	    dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP) &&
+	    device_may_wakeup(dev))
+		dev->power.skip_suspend = true;
+
 	callback = dpm_subsys_suspend_late_cb(dev, state, &info);
 	if (callback)
 		goto Run;
 
-	if (dev_pm_smart_suspend_and_suspended(dev) &&
+	if ((dev_pm_smart_suspend_and_suspended(dev) ||
+	     dev_pm_skip_suspend_and_not_suspended(dev)) &&
 	    !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
 		goto Skip;
 
@@ -1534,6 +1566,9 @@ Run:
 Skip:
 	dev->power.is_late_suspended = true;
 
+	if (dev->power.skip_suspend)
+		dpm_superior_set_skip_suspend(dev);
+
 Complete:
 	TRACE_SUSPEND(error);
 	complete_all(&dev->power.completion);
@@ -1746,6 +1781,7 @@ static int __device_suspend(struct devic
 
 	dev->power.may_skip_resume = false;
 	dev->power.must_resume = false;
+	dev->power.skip_suspend = false;
 
 	dpm_watchdog_set(&wd, dev);
 	device_lock(dev);
@@ -2112,6 +2148,11 @@ void device_pm_check_callbacks(struct de
 	spin_unlock_irq(&dev->power.lock);
 }
 
+bool dev_pm_skip_suspend_and_not_suspended(struct device *dev)
+{
+	return dev->power.skip_suspend && !pm_runtime_status_suspended(dev);
+}
+
 bool dev_pm_smart_suspend_and_suspended(struct device *dev)
 {
 	return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Ulf Hansson]

On 10 December 2017 at 03:30, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
>> For some bus types and PM domains, it's not sufficient to only check the
>> return value from device_may_wakeup(), to fully understand how to configure
>> wakeup settings for the device during system suspend.
>>
>> In particular, sometimes the device may need to remain in its power state,
>> in case the driver has configured it for in band IRQs, as to be able to
>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
>> driver flag, to enable drivers to instruct bus types and PM domains about
>> this setting.
>>
>> Of course, in case the bus type and PM domain has additional information
>> about wakeup settings of the device, they may override the behaviour.
>>
>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
>> types and PM domains should treat the device's parent during system
>> suspend. Therefore, in __device_suspend(), let the PM core propagate the
>> wakeup setting by using a status flag in the struct dev_pm_info for the
>> parent. This also makes it consistent with how the existing "wakeup_path"
>> status flag is being assigned.
>
> I've been thinking about this quite a bit recently and my conclusion is that
> the flag makes perfect sence (as it covers a valid use case), but I would
> define it and design the handling of it a bit differently.
>
> First off, the genpd changes in patch [3/3] effectively skip the "noirq"
> driver callbacks for the device, but question is why this is just "noirq".
> Arguably, the "late suspend" callback should be skipped too (the regular
> "suspend" one not necessarily, because the state of the device can still
> be changed via runtime PM at that point).  Moreover, if we go for the
> skipping at the suspend time, all of the resume should be skipped for the
> device too, because it has not been suspended in the first place.

No, sorry, but this is not the purpose of the flag. Actually there is
no reason at all to why genpd needs to skip invoking the driver's
noirq callbacks, no matter of whether the flag is set or not.

Moreover, skipping invoking callbacks would not work for those cases
where the driver needs to configure its wakeup setting in those
callbacks. Or if the driver has anything additional todo, like
disabling a request queue or whatever things, that it needs to manage
in those callbacks.

Instead this is about telling genpd that it must not power off the PM
domain (and in the special case of when genpd has the
->dev_ops.stop|start() assigned for the device, avoid to call them).

For that reason, I don't think the PM core needs to be involved,
besides what the patch already does.

Kind regards
Uffe

>
> But, if the device is in suspend at the suspend time already, the
> resume part should not be skipped for it at all (unless it has
> LEAVE_SUSPENDED set too), so it looks like the skipping should only
> happen if the device has not been suspended when the system transition
> starts.
>
> Second, it looks like the flag should be handled in the core in
> accordance with what genpd does with it or drivers will have to
> check the middle layer configuration.
>
> So below is my version of the core part (on top of the series I posted
> earlier today: https://marc.info/?l=linux-pm&m=151286423304445&w=2)
> and the genpd one should be analogous IMO.
>
> ---
>  Documentation/driver-api/pm/devices.rst |   21 ++++++++-
>  drivers/base/power/main.c               |   71 +++++++++++++++++++++++++-------
>  include/linux/pm.h                      |    8 +++
>  3 files changed, 82 insertions(+), 18 deletions(-)
>
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/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.
> + * IN_BAND_WAKEUP: Avoid suspending the device if configured for system wakeup.
>   *
>   * 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,16 @@ 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 IN_BAND_WAKEUP informs the PM core and middle-layer code that, as far
> + * as the driver knows, the device cannot be suspended to be able to wake up the
> + * system from sleep.
>   */
>  #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_IN_BAND_WAKEUP                BIT(4)
>
>  struct dev_pm_info {
>         pm_message_t            power_state;
> @@ -604,6 +610,7 @@ struct dev_pm_info {
>         bool                    no_pm_callbacks:1;      /* Owned by the PM core */
>         unsigned int            must_resume:1;  /* Owned by the PM core */
>         unsigned int            may_skip_resume:1;      /* Set by subsystems */
> +       unsigned int            skip_suspend:1; /* Owned by the PM core */
>  #else
>         unsigned int            should_wakeup:1;
>  #endif
> @@ -774,6 +781,7 @@ extern void pm_generic_complete(struct d
>
>  extern void dev_pm_skip_next_resume_phases(struct device *dev);
>  extern bool dev_pm_may_skip_resume(struct device *dev);
> +extern bool dev_pm_skip_suspend_and_not_suspended(struct device *dev);
>  extern bool dev_pm_smart_suspend_and_suspended(struct device *dev);
>
>  #else /* !CONFIG_PM_SLEEP */
> Index: linux-pm/Documentation/driver-api/pm/devices.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/pm/devices.rst
> +++ linux-pm/Documentation/driver-api/pm/devices.rst
> @@ -401,6 +401,10 @@ the phases are: ``prepare``, ``suspend``
>         generated by some other device after its own device had been set to low
>         power.
>
> +If any of these callbacks returns an error, the system won't enter the desired
> +low-power state.  Instead, the PM core will unwind its actions by resuming all
> +the devices that were suspended.
> +
>  At the end of these phases, drivers should have stopped all I/O transactions
>  (DMA, IRQs), saved enough state that they can re-initialize or restore previous
>  state (as needed by the hardware), and placed the device into a low-power state.
> @@ -414,9 +418,20 @@ when the system is in the sleep state.
>  might identify GPIO signals hooked up to a switch or other external hardware,
>  and :c:func:`pci_enable_wake()` does something similar for the PCI PME signal.
>
> -If any of these callbacks returns an error, the system won't enter the desired
> -low-power state.  Instead, the PM core will unwind its actions by resuming all
> -the devices that were suspended.
> +Some devices can only generate any signals if they are not suspended at all.  If
> +:c:func:`device_may_wakeup(dev)` returns ``true`` for such a device, it should
> +not be suspended during system-wide transitions to sleep states.  Device drivers
> +can indicate that condition to the PM core and middle-layer code (bus types or
> +PM domains) by setting the ``DPM_FLAG_IN_BAND_WAKEUP`` driver flag (at the probe
> +time).  The PM core and middle-layer code respond to that by avoiding to suspend
> +the device unless they have additional information on the device's wakeup
> +capabilities (for example, they may know that the device is in fact capable of
> +generating wakeup signals from a low-power state which the driver may not be
> +aware of).  In particular, the PM core will not invoke driver callbacks in the
> +``suspend_late`` and ``suspend_noirq`` phases for devices with
> +``DPM_FLAG_IN_BAND_WAKEUP`` set and for their ancestors and suppliers, but it
> +still will invoke middle-layer callbacks for them (if present) and those
> +callbacks are then responsible for handling the devices as appropriate.
>
>
>  Leaving System Suspend
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -609,6 +609,14 @@ static pm_callback_t dpm_subsys_suspend_
>                                                 pm_message_t state,
>                                                 const char **info_p);
>
> +static bool device_no_subsys_suspend(struct device *dev, pm_message_t state)
> +{
> +       pm_message_t suspend_msg = suspend_event(state);
> +
> +       return !dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
> +               !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL);
> +}
> +
>  /**
>   * device_resume_noirq - Execute a "noirq resume" callback for given device.
>   * @dev: Device to handle.
> @@ -645,25 +653,26 @@ static int device_resume_noirq(struct de
>         if (skip_resume)
>                 goto Skip;
>
> -       if (dev_pm_smart_suspend_and_suspended(dev)) {
> -               pm_message_t suspend_msg = suspend_event(state);
> -
> -               /*
> -                * If "freeze" callbacks have been skipped during a transition
> -                * related to hibernation, the subsequent "thaw" callbacks must
> -                * be skipped too or bad things may happen.  Otherwise, resume
> -                * callbacks are going to be run for the device, so its runtime
> -                * PM status must be changed to reflect the new state after the
> -                * transition under way.
> -                */
> -               if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
> -                   !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
> +       if (device_no_subsys_suspend(dev, state)) {
> +               if (dev_pm_smart_suspend_and_suspended(dev)) {
> +                       /*
> +                        * If "freeze" callbacks have been skipped during a
> +                        * transition related to hibernation, the subsequent
> +                        * "thaw" callbacks must be skipped too or bad things
> +                        * may happen.  Otherwise, resume callbacks are going to
> +                        * be run for the device, so its runtime PM status must
> +                        * be changed to reflect the new state after the
> +                        * transition under way.
> +                        */
>                         if (state.event == PM_EVENT_THAW) {
>                                 skip_resume = true;
>                                 goto Skip;
>                         } else {
>                                 pm_runtime_set_active(dev);
>                         }
> +               } else if (dev_pm_skip_suspend_and_not_suspended(dev)) {
> +                       skip_resume = true;
> +                       goto Skip;
>                 }
>         }
>
> @@ -1317,7 +1326,8 @@ static int __device_suspend_noirq(struct
>
>         no_subsys_cb = !dpm_subsys_suspend_late_cb(dev, state, NULL);
>
> -       if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
> +       if ((dev_pm_smart_suspend_and_suspended(dev) ||
> +            dev_pm_skip_suspend_and_not_suspended(dev)) && no_subsys_cb)
>                 goto Skip;
>
>         if (dev->driver && dev->driver->pm) {
> @@ -1450,6 +1460,22 @@ int dpm_suspend_noirq(pm_message_t state
>         return ret;
>  }
>
> +static void dpm_superior_set_skip_suspend(struct device *dev)
> +{
> +       struct device_link *link;
> +       int idx;
> +
> +       if (dev->parent)
> +               dev->parent->power.skip_suspend = true;
> +
> +       idx = device_links_read_lock();
> +
> +       list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> +               link->supplier->power.skip_suspend = true;
> +
> +       device_links_read_unlock(idx);
> +}
> +
>  static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>                                                 pm_message_t state,
>                                                 const char **info_p)
> @@ -1511,11 +1537,17 @@ static int __device_suspend_late(struct
>         if (dev->power.syscore || dev->power.direct_complete)
>                 goto Complete;
>
> +       if ((state.event == PM_EVENT_SUSPEND || state.event == PM_EVENT_HIBERNATE) &&
> +           dev_pm_test_driver_flags(dev, DPM_FLAG_IN_BAND_WAKEUP) &&
> +           device_may_wakeup(dev))
> +               dev->power.skip_suspend = true;
> +
>         callback = dpm_subsys_suspend_late_cb(dev, state, &info);
>         if (callback)
>                 goto Run;
>
> -       if (dev_pm_smart_suspend_and_suspended(dev) &&
> +       if ((dev_pm_smart_suspend_and_suspended(dev) ||
> +            dev_pm_skip_suspend_and_not_suspended(dev)) &&
>             !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
>                 goto Skip;
>
> @@ -1534,6 +1566,9 @@ Run:
>  Skip:
>         dev->power.is_late_suspended = true;
>
> +       if (dev->power.skip_suspend)
> +               dpm_superior_set_skip_suspend(dev);
> +
>  Complete:
>         TRACE_SUSPEND(error);
>         complete_all(&dev->power.completion);
> @@ -1746,6 +1781,7 @@ static int __device_suspend(struct devic
>
>         dev->power.may_skip_resume = false;
>         dev->power.must_resume = false;
> +       dev->power.skip_suspend = false;
>
>         dpm_watchdog_set(&wd, dev);
>         device_lock(dev);
> @@ -2112,6 +2148,11 @@ void device_pm_check_callbacks(struct de
>         spin_unlock_irq(&dev->power.lock);
>  }
>
> +bool dev_pm_skip_suspend_and_not_suspended(struct device *dev)
> +{
> +       return dev->power.skip_suspend && !pm_runtime_status_suspended(dev);
> +}
> +
>  bool dev_pm_smart_suspend_and_suspended(struct device *dev)
>  {
>         return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) &&
>

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Geert Uytterhoeven]

Hi Rafael, Ulf,

On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
>> For some bus types and PM domains, it's not sufficient to only check the
>> return value from device_may_wakeup(), to fully understand how to configure
>> wakeup settings for the device during system suspend.
>>
>> In particular, sometimes the device may need to remain in its power state,
>> in case the driver has configured it for in band IRQs, as to be able to
>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
>> driver flag, to enable drivers to instruct bus types and PM domains about
>> this setting.
>>
>> Of course, in case the bus type and PM domain has additional information
>> about wakeup settings of the device, they may override the behaviour.
>>
>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
>> types and PM domains should treat the device's parent during system
>> suspend. Therefore, in __device_suspend(), let the PM core propagate the
>> wakeup setting by using a status flag in the struct dev_pm_info for the
>> parent. This also makes it consistent with how the existing "wakeup_path"
>> status flag is being assigned.
>
> I've been thinking about this quite a bit recently and my conclusion is that
> the flag makes perfect sence (as it covers a valid use case), but I would
> define it and design the handling of it a bit differently.

I'm also still thinking about this, cfr. my recent silence w.r.t.
these matters...

On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the
pre-DT legacy clock domain), a device needs to be kept running if it is a
wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g.
irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM
Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP
flag (but see the exception below)
Hence for this class of SoCs, this would duplicate the existing
dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would
prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the
older SH-Mobile SoCs, but not for newer R-Car and RZ series).
For other SoC families, the situation may be different.

For us, the only exception are devices where the wakeup-source is not the
device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the
device driver knows if the device is to be woken up through a dedicated CD
line, or through a GPIO CD. So that would call for a method to opt-out,
e.g. OUT_BAND_WAKEUP.

To complicate matters, some drivers may be used on SoCs where the device
needs to be kept running (clock and/or power domain), and on SoCs where the
device is always running. This difference is typically handled by genpd,
and the device driver may not even be aware. Of course the driver can just
set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
clocks and/or power-domains properties itself, duplicating genpd
core/driver code).

So what about

         if (IN_BAND_WAKEUP ||
            (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
                ... suspend device...
        }

?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Ulf Hansson]

On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rafael, Ulf,
>
> On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
>>> For some bus types and PM domains, it's not sufficient to only check the
>>> return value from device_may_wakeup(), to fully understand how to configure
>>> wakeup settings for the device during system suspend.
>>>
>>> In particular, sometimes the device may need to remain in its power state,
>>> in case the driver has configured it for in band IRQs, as to be able to
>>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
>>> driver flag, to enable drivers to instruct bus types and PM domains about
>>> this setting.
>>>
>>> Of course, in case the bus type and PM domain has additional information
>>> about wakeup settings of the device, they may override the behaviour.
>>>
>>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
>>> types and PM domains should treat the device's parent during system
>>> suspend. Therefore, in __device_suspend(), let the PM core propagate the
>>> wakeup setting by using a status flag in the struct dev_pm_info for the
>>> parent. This also makes it consistent with how the existing "wakeup_path"
>>> status flag is being assigned.
>>
>> I've been thinking about this quite a bit recently and my conclusion is that
>> the flag makes perfect sence (as it covers a valid use case), but I would
>> define it and design the handling of it a bit differently.
>
> I'm also still thinking about this, cfr. my recent silence w.r.t.
> these matters...
>
> On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the
> pre-DT legacy clock domain), a device needs to be kept running if it is a
> wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g.
> irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM
> Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP
> flag (but see the exception below)

Could you elaborate a bit about the wakeup-path? Let me start and you
can fill in.

In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of
the device to its parent device, via the ->power.wakeup_path_in_band
flag. That becomes additional new information, as we already has the
PM core to propagate the value of device_may_wakeup() (if true) to the
parent device, via the ->power.wakeup_path flag.

I assume that isn't sufficient, because the wakeup path isn't
reflected in the child/parent hierarchy?

So, I guess this is about drivers who deals with wakeup enabled
devices and which are consumers of other resources (irqchips for
example). As part of the wakup path, these resources also needs to be
kept running, right?

In your case, what other resources besides the irqchip, can be
considered as a part of the wakeup path, but also being outside the
parent/child hierarchy of the wakeup enabled device?

Note that, an important fact as of today, is that when
GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the
->power.wakeup_path flag for the device. Both conditions must be true,
else genpd powers off the PM domain (and the device).

In other words, all devices being part of the wakeup path must either
have device_may_wakeup() to return true for it or the
->power.wakeup_path set, else genpd will power off the PM domain,
regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not.

> Hence for this class of SoCs, this would duplicate the existing
> dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would
> prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the
> older SH-Mobile SoCs, but not for newer R-Car and RZ series).
> For other SoC families, the situation may be different.
>
> For us, the only exception are devices where the wakeup-source is not the
> device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the
> device driver knows if the device is to be woken up through a dedicated CD
> line, or through a GPIO CD. So that would call for a method to opt-out,
> e.g. OUT_BAND_WAKEUP.

The main reason to why I chosen the IN_BAND_WAKEUP method, is because
genpd's *default* method is to power off the PM domain (and the
device) even if wakeup is enabled for the device. Hence, genpd treats
all wakeups as default being out-of-band IRQs.

Having an OUT_BAND_WAKEUP flag, would thus only make sense for those
genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you
are saying as well?

>
> To complicate matters, some drivers may be used on SoCs where the device
> needs to be kept running (clock and/or power domain), and on SoCs where the
> device is always running. This difference is typically handled by genpd,
> and the device driver may not even be aware. Of course the driver can just
> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
> clocks and/or power-domains properties itself, duplicating genpd
> core/driver code).
>
> So what about
>
>          if (IN_BAND_WAKEUP ||
>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {

We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?

>                 ... suspend device...
>         }

Kind regards
Uffe

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
>>>> For some bus types and PM domains, it's not sufficient to only check the
>>>> return value from device_may_wakeup(), to fully understand how to configure
>>>> wakeup settings for the device during system suspend.
>>>>
>>>> In particular, sometimes the device may need to remain in its power state,
>>>> in case the driver has configured it for in band IRQs, as to be able to
>>>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
>>>> driver flag, to enable drivers to instruct bus types and PM domains about
>>>> this setting.
>>>>
>>>> Of course, in case the bus type and PM domain has additional information
>>>> about wakeup settings of the device, they may override the behaviour.
>>>>
>>>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
>>>> types and PM domains should treat the device's parent during system
>>>> suspend. Therefore, in __device_suspend(), let the PM core propagate the
>>>> wakeup setting by using a status flag in the struct dev_pm_info for the
>>>> parent. This also makes it consistent with how the existing "wakeup_path"
>>>> status flag is being assigned.
>>>
>>> I've been thinking about this quite a bit recently and my conclusion is that
>>> the flag makes perfect sence (as it covers a valid use case), but I would
>>> define it and design the handling of it a bit differently.
>>
>> I'm also still thinking about this, cfr. my recent silence w.r.t.
>> these matters...
>>
>> On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the
>> pre-DT legacy clock domain), a device needs to be kept running if it is a
>> wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g.
>> irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM
>> Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP
>> flag (but see the exception below)
>
> Could you elaborate a bit about the wakeup-path? Let me start and you
> can fill in.
>
> In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of
> the device to its parent device, via the ->power.wakeup_path_in_band
> flag. That becomes additional new information, as we already has the
> PM core to propagate the value of device_may_wakeup() (if true) to the
> parent device, via the ->power.wakeup_path flag.
>
> I assume that isn't sufficient, because the wakeup path isn't
> reflected in the child/parent hierarchy?

That's correct. Interrupts are not part of the parent/child relationship.
Traditionally, irqchips were not platform devices, but now many irqchips
used on embedded platforms are.

> So, I guess this is about drivers who deals with wakeup enabled
> devices and which are consumers of other resources (irqchips for
> example). As part of the wakup path, these resources also needs to be
> kept running, right?

Correct.

> In your case, what other resources besides the irqchip, can be
> considered as a part of the wakeup path, but also being outside the
> parent/child hierarchy of the wakeup enabled device?

GPIOs used for wake-up? E.g. SDHI GPIO. We don't seem to have that
enabled for the Renesas SDHI drivers, though.

For gpio-keys, this is handled through enable_irq_wake(), so again it
uses an irqchip.

> Note that, an important fact as of today, is that when
> GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the
> ->power.wakeup_path flag for the device. Both conditions must be true,
> else genpd powers off the PM domain (and the device).
>
> In other words, all devices being part of the wakeup path must either
> have device_may_wakeup() to return true for it or the
> ->power.wakeup_path set, else genpd will power off the PM domain,
> regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not.

Correct.

>> Hence for this class of SoCs, this would duplicate the existing
>> dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would
>> prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the
>> older SH-Mobile SoCs, but not for newer R-Car and RZ series).
>> For other SoC families, the situation may be different.
>>
>> For us, the only exception are devices where the wakeup-source is not the
>> device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the
>> device driver knows if the device is to be woken up through a dedicated CD
>> line, or through a GPIO CD. So that would call for a method to opt-out,
>> e.g. OUT_BAND_WAKEUP.
>
> The main reason to why I chosen the IN_BAND_WAKEUP method, is because
> genpd's *default* method is to power off the PM domain (and the
> device) even if wakeup is enabled for the device. Hence, genpd treats
> all wakeups as default being out-of-band IRQs.

OK.

> Having an OUT_BAND_WAKEUP flag, would thus only make sense for those
> genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you
> are saying as well?

Yes.

IMHO this is orthogonal to the device: the device driver knows if the
device itself is generating the wake-up event (e.g. Wake-on-LAN, dedicated
SDHI CD), or some other device is taking care (e.g. GPIO SDHI CD).
The device driver may not know if the device needs to be kept awake to
to handle wake-up events, that may be platform knowledge handled by genpd.

>> To complicate matters, some drivers may be used on SoCs where the device
>> needs to be kept running (clock and/or power domain), and on SoCs where the
>> device is always running. This difference is typically handled by genpd,
>> and the device driver may not even be aware. Of course the driver can just
>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
>> clocks and/or power-domains properties itself, duplicating genpd
>> core/driver code).
>>
>> So what about
>>
>>          if (IN_BAND_WAKEUP ||
>>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
>
> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?
>
>>                 ... suspend device...
>>         }

Oops, inverted logic. I should not write technical emails on Sunday morning.

Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or
if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Ulf Hansson]

On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Sun, Dec 10, 2017 at 3:30 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Monday, November 13, 2017 4:46:42 PM CET Ulf Hansson wrote:
>>>>> For some bus types and PM domains, it's not sufficient to only check the
>>>>> return value from device_may_wakeup(), to fully understand how to configure
>>>>> wakeup settings for the device during system suspend.
>>>>>
>>>>> In particular, sometimes the device may need to remain in its power state,
>>>>> in case the driver has configured it for in band IRQs, as to be able to
>>>>> generate wakeup signals. Therefore, define and document an IN_BAND_WAKEUP
>>>>> driver flag, to enable drivers to instruct bus types and PM domains about
>>>>> this setting.
>>>>>
>>>>> Of course, in case the bus type and PM domain has additional information
>>>>> about wakeup settings of the device, they may override the behaviour.
>>>>>
>>>>> Using the IN_BAND_WAKEUP driver flag for a device, may also affect how bus
>>>>> types and PM domains should treat the device's parent during system
>>>>> suspend. Therefore, in __device_suspend(), let the PM core propagate the
>>>>> wakeup setting by using a status flag in the struct dev_pm_info for the
>>>>> parent. This also makes it consistent with how the existing "wakeup_path"
>>>>> status flag is being assigned.
>>>>
>>>> I've been thinking about this quite a bit recently and my conclusion is that
>>>> the flag makes perfect sence (as it covers a valid use case), but I would
>>>> define it and design the handling of it a bit differently.
>>>
>>> I'm also still thinking about this, cfr. my recent silence w.r.t.
>>> these matters...
>>>
>>> On Renesas ARM SoCs (and at least some SH SoCs, but these are stuck in the
>>> pre-DT legacy clock domain), a device needs to be kept running if it is a
>>> wake-up source (e.g. WoL), or if it's part of the wake-up patch (e.g.
>>> irqchip). So in the absence of the GENPD_FLAG_ACTIVE_WAKEUP flag in the PM
>>> Domain driver, all individual drivers would need to set the IN_BAND_WAKEUP
>>> flag (but see the exception below)
>>
>> Could you elaborate a bit about the wakeup-path? Let me start and you
>> can fill in.
>>
>> In $subject patch, the PM core propagates the IN_BAND_WAKEUP flag of
>> the device to its parent device, via the ->power.wakeup_path_in_band
>> flag. That becomes additional new information, as we already has the
>> PM core to propagate the value of device_may_wakeup() (if true) to the
>> parent device, via the ->power.wakeup_path flag.
>>
>> I assume that isn't sufficient, because the wakeup path isn't
>> reflected in the child/parent hierarchy?
>
> That's correct. Interrupts are not part of the parent/child relationship.
> Traditionally, irqchips were not platform devices, but now many irqchips
> used on embedded platforms are.
>
>> So, I guess this is about drivers who deals with wakeup enabled
>> devices and which are consumers of other resources (irqchips for
>> example). As part of the wakup path, these resources also needs to be
>> kept running, right?
>
> Correct.
>
>> In your case, what other resources besides the irqchip, can be
>> considered as a part of the wakeup path, but also being outside the
>> parent/child hierarchy of the wakeup enabled device?
>
> GPIOs used for wake-up? E.g. SDHI GPIO. We don't seem to have that
> enabled for the Renesas SDHI drivers, though.
>
> For gpio-keys, this is handled through enable_irq_wake(), so again it
> uses an irqchip.

Yes, this is clearly a common use case. Although, I am sure we have more.

For example, phys may need a very similar treatment, in case their
consumers requires its phy to be running - as to be able to receive
signals for wakeups. Anyway, let's discuss other cases separately and
focus on yours with the irqchip for now.

>
>> Note that, an important fact as of today, is that when
>> GENPD_FLAG_ACTIVE_WAKEUP is set for the genpd, genpd also checks the
>> ->power.wakeup_path flag for the device. Both conditions must be true,
>> else genpd powers off the PM domain (and the device).
>>
>> In other words, all devices being part of the wakeup path must either
>> have device_may_wakeup() to return true for it or the
>> ->power.wakeup_path set, else genpd will power off the PM domain,
>> regardless of whether the GENPD_FLAG_ACTIVE_WAKEUP flag is set or not.
>
> Correct.

Okay, thanks for confirming this.

This also clarifies why you set the ->power.wakeup_path flag for the
renesas irqchip device in your other series [1]. That makes perfect
sense to me, in this regards.

>
>>> Hence for this class of SoCs, this would duplicate the existing
>>> dev->power.wakeup and dev->power.wakeup_path flags, so that's why I would
>>> prefer using GENPD_FLAG_ACTIVE_WAKEUP instead (like we already do for the
>>> older SH-Mobile SoCs, but not for newer R-Car and RZ series).
>>> For other SoC families, the situation may be different.
>>>
>>> For us, the only exception are devices where the wakeup-source is not the
>>> device itself, but a GPIO, e.g. SDHI card detect. In such a case, only the
>>> device driver knows if the device is to be woken up through a dedicated CD
>>> line, or through a GPIO CD. So that would call for a method to opt-out,
>>> e.g. OUT_BAND_WAKEUP.
>>
>> The main reason to why I chosen the IN_BAND_WAKEUP method, is because
>> genpd's *default* method is to power off the PM domain (and the
>> device) even if wakeup is enabled for the device. Hence, genpd treats
>> all wakeups as default being out-of-band IRQs.
>
> OK.
>
>> Having an OUT_BAND_WAKEUP flag, would thus only make sense for those
>> genpds that has GENPD_FLAG_ACTIVE_WAKEUP set, I guess that is what you
>> are saying as well?
>
> Yes.
>
> IMHO this is orthogonal to the device: the device driver knows if the
> device itself is generating the wake-up event (e.g. Wake-on-LAN, dedicated
> SDHI CD), or some other device is taking care (e.g. GPIO SDHI CD).
> The device driver may not know if the device needs to be kept awake to
> to handle wake-up events, that may be platform knowledge handled by genpd.

Agree.

>
>>> To complicate matters, some drivers may be used on SoCs where the device
>>> needs to be kept running (clock and/or power domain), and on SoCs where the
>>> device is always running. This difference is typically handled by genpd,
>>> and the device driver may not even be aware. Of course the driver can just
>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
>>> clocks and/or power-domains properties itself, duplicating genpd
>>> core/driver code).
>>>
>>> So what about
>>>
>>>          if (IN_BAND_WAKEUP ||
>>>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
>>
>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?
>>
>>>                 ... suspend device...
>>>         }
>
> Oops, inverted logic. I should not write technical emails on Sunday morning.
>
> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or
> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't.
>

Putting together the pieces of information received here, you have
convinced me that we should stick to use the current
GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for
start dealing with in-band-wakeups.

Then, as you suggest, we need a method for the driver to set an
opt-out flag for its device, in case it configures an out-band-wakeup.
In other words, we should have an OUT_BAND_WAKEUP flag instead of an
IN_BAND_WAKEUP flag.

That together with an option of allowing "consumed resource-devices"
(irqchip) to be included in the wakeup path. I am thinking, perhaps
another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks
at and sets ->power.wakeup_path flag for the device and its parents.

Let me re-spin this series. Perhaps I fold in some of the changes from
your series as well, as to provide people with a complete picture.

Any further comments? Does it makes sense?

Kind regards
Uffe

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

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> That together with an option of allowing "consumed resource-devices"
> (irqchip) to be included in the wakeup path. I am thinking, perhaps
> another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks
> at and sets ->power.wakeup_path flag for the device and its parents.

This is complicated by the fact that currently the device and irq
subsystems don't really share data. E.g. {en,dis}able_irq_wake() and
irq_set_irq_wake() don't take a device parameter, only an interrupt number,
and conversion from interrupt numbers to devices is non-trivial.
That's why I handled it in the irqchip drivers in the series referenced
below.

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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Ulf Hansson]

On 12 December 2017 at 09:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> That together with an option of allowing "consumed resource-devices"
>> (irqchip) to be included in the wakeup path. I am thinking, perhaps
>> another driver PM flag (DPM_FLAG_WAKEUP_PATH), that the PM core looks
>> at and sets ->power.wakeup_path flag for the device and its parents.
>
> This is complicated by the fact that currently the device and irq
> subsystems don't really share data. E.g. {en,dis}able_irq_wake() and
> irq_set_irq_wake() don't take a device parameter, only an interrupt number,
> and conversion from interrupt numbers to devices is non-trivial.
> That's why I handled it in the irqchip drivers in the series referenced
> below.

Yes, that seems like a good way forward for now.

We can always look into how to extend the irq subsystem to deal with
this internally, but I rather leave that to be done as a separate
task.

[...]

Kind regards
Uffe

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> To complicate matters, some drivers may be used on SoCs where the device
>>>> needs to be kept running (clock and/or power domain), and on SoCs where the
>>>> device is always running. This difference is typically handled by genpd,
>>>> and the device driver may not even be aware. Of course the driver can just
>>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
>>>> clocks and/or power-domains properties itself, duplicating genpd
>>>> core/driver code).
>>>>
>>>> So what about
>>>>
>>>>          if (IN_BAND_WAKEUP ||
>>>>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
>>>
>>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?
>>>
>>>>                 ... suspend device...
>>>>         }
>>
>> Oops, inverted logic. I should not write technical emails on Sunday morning.
>>
>> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or
>> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't.
>
> Putting together the pieces of information received here, you have
> convinced me that we should stick to use the current
> GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for
> start dealing with in-band-wakeups.

Thank you!

So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active
wakeup behavior"
(https://www.spinics.net/lists/linux-renesas-soc/msg19941.html)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Ulf Hansson]

On 14 December 2017 at 11:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> To complicate matters, some drivers may be used on SoCs where the device
>>>>> needs to be kept running (clock and/or power domain), and on SoCs where the
>>>>> device is always running. This difference is typically handled by genpd,
>>>>> and the device driver may not even be aware. Of course the driver can just
>>>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
>>>>> clocks and/or power-domains properties itself, duplicating genpd
>>>>> core/driver code).
>>>>>
>>>>> So what about
>>>>>
>>>>>          if (IN_BAND_WAKEUP ||
>>>>>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
>>>>
>>>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?
>>>>
>>>>>                 ... suspend device...
>>>>>         }
>>>
>>> Oops, inverted logic. I should not write technical emails on Sunday morning.
>>>
>>> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or
>>> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't.
>>
>> Putting together the pieces of information received here, you have
>> convinced me that we should stick to use the current
>> GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for
>> start dealing with in-band-wakeups.
>
> Thank you!
>
> So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active
> wakeup behavior"
> (https://www.spinics.net/lists/linux-renesas-soc/msg19941.html)

Yes! I just added my reviewed-by tag to these.

Kind regards
Uffe

Re: [PATCH v2 2/3] PM / core: Add IN_BAND_WAKEUP driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Dec 14, 2017 at 3:13 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 14 December 2017 at 11:52, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Mon, Dec 11, 2017 at 9:59 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 11 December 2017 at 11:48, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Mon, Dec 11, 2017 at 11:24 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> On 10 December 2017 at 11:16, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>>> To complicate matters, some drivers may be used on SoCs where the device
>>>>>> needs to be kept running (clock and/or power domain), and on SoCs where the
>>>>>> device is always running. This difference is typically handled by genpd,
>>>>>> and the device driver may not even be aware. Of course the driver can just
>>>>>> set IN_BAND_WAKEUP regardless, (else it has to check for the presence of
>>>>>> clocks and/or power-domains properties itself, duplicating genpd
>>>>>> core/driver code).
>>>>>>
>>>>>> So what about
>>>>>>
>>>>>>          if (IN_BAND_WAKEUP ||
>>>>>>             (GENPD_FLAG_ACTIVE_WAKEUP && !OUT_BAND_WAKEUP)) {
>>>>>
>>>>> We don't want to suspend the device in case of IN_BAND_WAKEUP, right!?
>>>>>
>>>>>>                 ... suspend device...
>>>>>>         }
>>>>
>>>> Oops, inverted logic. I should not write technical emails on Sunday morning.
>>>>
>>>> Yes, the device must be kept awake if either IN_BAND_WAKEUP is set, or
>>>> if GENPD_FLAG_ACTIVE_WAKEUP is set but OUT_BAND_WAKEUP isn't.
>>>
>>> Putting together the pieces of information received here, you have
>>> convinced me that we should stick to use the current
>>> GENPD_FLAG_ACTIVE_WAKEUP for now, which allows genpds to opt-in for
>>> start dealing with in-band-wakeups.
>>
>> Thank you!
>>
>> So I'll move forward with "[PATCH v2 0/3] PM / Domain: renesas: Fix active
>> wakeup behavior"
>> (https://www.spinics.net/lists/linux-renesas-soc/msg19941.html)
>
> Yes! I just added my reviewed-by tag to these.

Thanks a lot!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 3/3] PM / Domains: Take wakeup_path_in_band status flag into account

[Ulf Hansson]

Make genpd to take the wakeup_path_in_band status flag into account during
system suspend/resume. More precisely, in case the flag has been set by the
PM core, let's leave the device in full power state and prevent the PM
domain from being powered off.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---

Changes in v2:
	- Added Geert's Reviewed-by tag.
	- Changed to look at status flag instead of driver flag.
---
 drivers/base/power/domain.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 24e39ce..21cbea0 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1037,7 +1037,8 @@ 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->power.wakeup_path_in_band || genpd_is_active_wakeup(genpd)))
 		return 0;
 
 	if (poweroff)
@@ -1092,7 +1093,8 @@ 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->power.wakeup_path_in_band || genpd_is_active_wakeup(genpd)))
 		return 0;
 
 	genpd_lock(genpd);
-- 
2.7.4

Re: [PATCH v2 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

On 13 November 2017 at 16:46, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The generic problem this series is trying to solve, is that for some bus types
> and PM domains, it's not sufficient to only check the return value from
> device_may_wakeup(), to fully understand how to treat the device during system
> suspend.
>
> One particular case that suffers from this, is the generic PM domain (aka genpd)
> and that is taken care of in the final change in this series.
>
> The special case this series address, is to enable drivers to instruct bus types
> and PM domains, that the device need to remain in its current power state in
> case it should be able to generate wakeup signals during system suspend. This is
> done by an opt-in method, however bus types and PM domains that has additional
> knowledge about devices' wakeup settings may still override the setting.
>
> Geert Uytterhoeven, has been working on some related problems for some Renesas
> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
> devices/drivers, which are used together with genpd. My intent is that this
> series enables a solution for those problems.
>
> [1]
> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>
> Changes in v2:
>         - See change logs for each patch.
>
>
> Ulf Hansson (3):
>   PM / core: Re-factor some code dealing with parents in
>     __device_suspend()
>   PM / core: Add IN_BAND_WAKEUP driver flag
>   PM / Domains: Take wakeup_path_in_band status flag into account
>

Realized that the header of the cover letter still has the old
WAKEUP_POWERED flag name in it. It should be "IN_BAND_WAKEUP", as
following patches shows.

Apologize for noise!

Br
Uffe