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

[PATCH 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 stay powered in case wakeup signals
is enabled for it.

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 


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

 Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
 drivers/base/power/domain.c             | 14 ++++++++++----
 drivers/base/power/main.c               | 33 +++++++++++++++++++++------------
 include/linux/pm.h                      |  5 +++++
 4 files changed, 48 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH 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>
---
 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 1/3] PM / core: Re-factor some code dealing with parents in __device_suspend()

[Geert Uytterhoeven]

On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> 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>

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 2/3] PM / core: Add WAKEUP_POWERED 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 treat the
device during system suspend.

In particular, sometimes the device may need to stay in full power state,
to have wakeup signals enabled for it. Therefore, define and document a
WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
exactly about that.

During __device_suspend() in the PM core, let's make sure to also propagate
the setting of the flag to the parent device, when wakeup signals are
enabled and unless the parent has the "ignore_children" flag set. This
makes it also consistent with how the existing "wakeup_path" flag is being
assigned.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
 drivers/base/power/main.c               |  6 +++++-
 include/linux/pm.h                      |  5 +++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
index 53c1b0b..1ca2d0f 100644
--- a/Documentation/driver-api/pm/devices.rst
+++ b/Documentation/driver-api/pm/devices.rst
@@ -414,6 +414,18 @@ 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.
 
+Moreover, in case wakeup signals are enabled for a device, some bus types and
+PM domains may manage the device slightly differently during system suspend. For
+example, sometimes the device needs to stay in full power state, to have wakeup
+signals enabled for it. In cases when the wakeup settings are mostly managed by
+the driver, it may not be sufficient for bus types and PM domains to only check
+the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
+are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
+:c:member:`power.driver_flags`, by passing the flag to
+:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
+domains to leave the device in full power state, when wakeup signals are enabled
+for it.
+
 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..f64f945 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1432,9 +1432,13 @@ 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;
 
+		if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
+			parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
+	}
+
 	spin_unlock_irq(&parent->power.lock);
 }
 
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 65d3911..34c2404 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_WAKEUP_POWERED	BIT(3)
 
 struct dev_pm_info {
 	pm_message_t		power_state;
-- 
2.7.4

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

[Geert Uytterhoeven]

On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
> device during system suspend.
>
> In particular, sometimes the device may need to stay in full power state,
> to have wakeup signals enabled for it. Therefore, define and document a
> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
> exactly about that.
>
> During __device_suspend() in the PM core, let's make sure to also propagate
> the setting of the flag to the parent device, when wakeup signals are
> enabled and unless the parent has the "ignore_children" flag set. This
> makes it also consistent with how the existing "wakeup_path" flag is being
> assigned.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 2/3] PM / core: Add WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
> device during system suspend.
>
> In particular, sometimes the device may need to stay in full power state,
> to have wakeup signals enabled for it. Therefore, define and document a
> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
> exactly about that.
>
> During __device_suspend() in the PM core, let's make sure to also propagate
> the setting of the flag to the parent device, when wakeup signals are
> enabled and unless the parent has the "ignore_children" flag set. This
> makes it also consistent with how the existing "wakeup_path" flag is being
> assigned.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>  drivers/base/power/main.c               |  6 +++++-
>  include/linux/pm.h                      |  5 +++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
> index 53c1b0b..1ca2d0f 100644
> --- a/Documentation/driver-api/pm/devices.rst
> +++ b/Documentation/driver-api/pm/devices.rst
> @@ -414,6 +414,18 @@ 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.
>
> +Moreover, in case wakeup signals are enabled for a device, some bus types and
> +PM domains may manage the device slightly differently during system suspend. For
> +example, sometimes the device needs to stay in full power state, to have wakeup
> +signals enabled for it. In cases when the wakeup settings are mostly managed by
> +the driver, it may not be sufficient for bus types and PM domains to only check
> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
> +:c:member:`power.driver_flags`, by passing the flag to
> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
> +domains to leave the device in full power state, when wakeup signals are enabled
> +for it.

IMO this is a bit unclear.

First off, how does the driver know that the device has to be in full
power for wakeup to work?

Second, this requirement sort of implies that the device cannot go
into a low-power state during runtime suspend too, so it basically
remains stays at full power even when runtime-suspended.

Does it then mean that the middle layer is expected to simply avoid
changing the power state of the device when enabled to wake up the
system, or is there more to that?  In the former case, it may be
better to rename the flag to something along the lines of "don't
change power state if wakeup enabled".

> +
>  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..f64f945 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1432,9 +1432,13 @@ 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;
>
> +               if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
> +                       parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
> +       }
> +
>         spin_unlock_irq(&parent->power.lock);
>  }
>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 65d3911..34c2404 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_WAKEUP_POWERED        BIT(3)

I'd prefer this to be BIT(4).

>
>  struct dev_pm_info {
>         pm_message_t            power_state;
> --
> 2.7.4
>

Thanks,
Rafael

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

[Ulf Hansson]

On 9 November 2017 at 01:24, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
>> device during system suspend.
>>
>> In particular, sometimes the device may need to stay in full power state,
>> to have wakeup signals enabled for it. Therefore, define and document a
>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>> exactly about that.
>>
>> During __device_suspend() in the PM core, let's make sure to also propagate
>> the setting of the flag to the parent device, when wakeup signals are
>> enabled and unless the parent has the "ignore_children" flag set. This
>> makes it also consistent with how the existing "wakeup_path" flag is being
>> assigned.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>>  drivers/base/power/main.c               |  6 +++++-
>>  include/linux/pm.h                      |  5 +++++
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
>> index 53c1b0b..1ca2d0f 100644
>> --- a/Documentation/driver-api/pm/devices.rst
>> +++ b/Documentation/driver-api/pm/devices.rst
>> @@ -414,6 +414,18 @@ 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.
>>
>> +Moreover, in case wakeup signals are enabled for a device, some bus types and
>> +PM domains may manage the device slightly differently during system suspend. For
>> +example, sometimes the device needs to stay in full power state, to have wakeup
>> +signals enabled for it. In cases when the wakeup settings are mostly managed by
>> +the driver, it may not be sufficient for bus types and PM domains to only check
>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>> +:c:member:`power.driver_flags`, by passing the flag to
>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>> +domains to leave the device in full power state, when wakeup signals are enabled
>> +for it.
>
> IMO this is a bit unclear.
>
> First off, how does the driver know that the device has to be in full
> power for wakeup to work?

Because the device is accessed as part of dealing with the wakeup.

>
> Second, this requirement sort of implies that the device cannot go
> into a low-power state during runtime suspend too, so it basically
> remains stays at full power even when runtime-suspended.

No, not really, because that depends on the current situation.

An Ethernet device can surely go into a low power state, at runtime
suspend, when the interface is down, for example.

>
> Does it then mean that the middle layer is expected to simply avoid
> changing the power state of the device when enabled to wake up the
> system, or is there more to that?  In the former case, it may be
> better to rename the flag to something along the lines of "don't
> change power state if wakeup enabled".

Yes, correct.

I can try to clarify that in the description and unless you have a
suggestion for a better name of the flag, I try to come up with
something for that too.

>>  #define DPM_FLAG_NEVER_SKIP    BIT(0)
>>  #define DPM_FLAG_SMART_PREPARE BIT(1)
>>  #define DPM_FLAG_SMART_SUSPEND BIT(2)
>> +#define DPM_FLAG_WAKEUP_POWERED        BIT(3)
>
> I'd prefer this to be BIT(4).

OK.

I guess you can always also amend my patch, depending on in what order
you merge things. :-)

[...]

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 9:44 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 01:24, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
>>> device during system suspend.
>>>
>>> In particular, sometimes the device may need to stay in full power state,
>>> to have wakeup signals enabled for it. Therefore, define and document a
>>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>>> exactly about that.
>>>
>>> During __device_suspend() in the PM core, let's make sure to also propagate
>>> the setting of the flag to the parent device, when wakeup signals are
>>> enabled and unless the parent has the "ignore_children" flag set. This
>>> makes it also consistent with how the existing "wakeup_path" flag is being
>>> assigned.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>>>  drivers/base/power/main.c               |  6 +++++-
>>>  include/linux/pm.h                      |  5 +++++
>>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
>>> index 53c1b0b..1ca2d0f 100644
>>> --- a/Documentation/driver-api/pm/devices.rst
>>> +++ b/Documentation/driver-api/pm/devices.rst
>>> @@ -414,6 +414,18 @@ 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.
>>>
>>> +Moreover, in case wakeup signals are enabled for a device, some bus types and
>>> +PM domains may manage the device slightly differently during system suspend. For
>>> +example, sometimes the device needs to stay in full power state, to have wakeup
>>> +signals enabled for it. In cases when the wakeup settings are mostly managed by
>>> +the driver, it may not be sufficient for bus types and PM domains to only check
>>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
>>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>>> +:c:member:`power.driver_flags`, by passing the flag to
>>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>>> +domains to leave the device in full power state, when wakeup signals are enabled
>>> +for it.
>>
>> IMO this is a bit unclear.
>>
>> First off, how does the driver know that the device has to be in full
>> power for wakeup to work?
>
> Because the device is accessed as part of dealing with the wakeup.

Yes, it is, In the working state of the system.  In the system wakeup
case it may not be.

Essentially, what happens then is that driver-provided interrupt
handlers don't run as a rule and system wakeup is triggered by the
low-level handler at the IRQ chip level.  Next, the PM callbacks
invoked for the device are expected to clean up the wakeup status etc.

Of course, power still is necessary for that to work, but it may be
not be in-band.  That may be either in-band power used for normal
operations and provided through the interconnect used by the device or
it may be special wakeup power provided out-of-band.

Also the wakeup signal itself may be an in-band device interrupt (like
the ones used for signaling IO events during normal operation) or it
may be a special wakeup signal (like PCI PME) in which case, from the
driver's perspective, the wakeup signaling is out-of-band.

Usually, the driver doesn't know how this is set up for the particular
device in the particular platform and hence my question. :-)

The case at hand seems to be when the wakeup power is in-band or the
wakeup signal is an in-band interrupt (in which case power needs to be
provided to the interconnect at least).

If they both are out-of-band, the middle layer should know that,
because as a rule it will be involved in setting up both of them.
Otherwise, in principle, it should assume that in-band power needs to
be provided for wakeup to work and avoid turning things off if wakeup
is enabled.

>>
>> Second, this requirement sort of implies that the device cannot go
>> into a low-power state during runtime suspend too, so it basically
>> remains stays at full power even when runtime-suspended.
>
> No, not really, because that depends on the current situation.
>
> An Ethernet device can surely go into a low power state, at runtime
> suspend, when the interface is down, for example.

But then it is not expected to generate wakeup signals I suppose.

[BTW, I wonder how it detects when the cable is connected again to it.
I know what happens in PCI NICs, but that clearly is not the case
here.]

Well, anyway, it looks like the case when the device is
runtime-suspended right before system suspend and it is going to stay
suspended is not interesting here, because the state will be retained
whatever it is then.

An interesting case seems to be when the device is not
runtime-suspended when system suspend triggers.

>>
>> Does it then mean that the middle layer is expected to simply avoid
>> changing the power state of the device when enabled to wake up the
>> system, or is there more to that?  In the former case, it may be
>> better to rename the flag to something along the lines of "don't
>> change power state if wakeup enabled".
>
> Yes, correct.
>
> I can try to clarify that in the description and unless you have a
> suggestion for a better name of the flag, I try to come up with
> something for that too.

I was thinking about something like DPM_FLAG_IN_BAND_WAKEUP or similar.

But please note that there are cases in which the middle layer has
information on what power states to put devices into for wakeup to
work and IMO that should take precedence over the flag as a rule.

>>>  #define DPM_FLAG_NEVER_SKIP    BIT(0)
>>>  #define DPM_FLAG_SMART_PREPARE BIT(1)
>>>  #define DPM_FLAG_SMART_SUSPEND BIT(2)
>>> +#define DPM_FLAG_WAKEUP_POWERED        BIT(3)
>>
>> I'd prefer this to be BIT(4).
>
> OK.
>
> I guess you can always also amend my patch, depending on in what order
> you merge things. :-)

Right.

Thanks,
Rafael

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

[Ulf Hansson]

[...]

>>>
>>> First off, how does the driver know that the device has to be in full
>>> power for wakeup to work?
>>
>> Because the device is accessed as part of dealing with the wakeup.
>
> Yes, it is, In the working state of the system.  In the system wakeup
> case it may not be.
>
> Essentially, what happens then is that driver-provided interrupt
> handlers don't run as a rule and system wakeup is triggered by the
> low-level handler at the IRQ chip level.  Next, the PM callbacks
> invoked for the device are expected to clean up the wakeup status etc.
>
> Of course, power still is necessary for that to work, but it may be
> not be in-band.  That may be either in-band power used for normal
> operations and provided through the interconnect used by the device or
> it may be special wakeup power provided out-of-band.
>
> Also the wakeup signal itself may be an in-band device interrupt (like
> the ones used for signaling IO events during normal operation) or it
> may be a special wakeup signal (like PCI PME) in which case, from the
> driver's perspective, the wakeup signaling is out-of-band.
>
> Usually, the driver doesn't know how this is set up for the particular
> device in the particular platform and hence my question. :-)
>
> The case at hand seems to be when the wakeup power is in-band or the
> wakeup signal is an in-band interrupt (in which case power needs to be
> provided to the interconnect at least).

Correct!

>
> If they both are out-of-band, the middle layer should know that,
> because as a rule it will be involved in setting up both of them.
> Otherwise, in principle, it should assume that in-band power needs to
> be provided for wakeup to work and avoid turning things off if wakeup
> is enabled.

Agree!

>
>>>
>>> Second, this requirement sort of implies that the device cannot go
>>> into a low-power state during runtime suspend too, so it basically
>>> remains stays at full power even when runtime-suspended.
>>
>> No, not really, because that depends on the current situation.
>>
>> An Ethernet device can surely go into a low power state, at runtime
>> suspend, when the interface is down, for example.
>
> But then it is not expected to generate wakeup signals I suppose.

Correct.

>
> [BTW, I wonder how it detects when the cable is connected again to it.
> I know what happens in PCI NICs, but that clearly is not the case
> here.]
>
> Well, anyway, it looks like the case when the device is
> runtime-suspended right before system suspend and it is going to stay
> suspended is not interesting here, because the state will be retained
> whatever it is then.

Correct.

>
> An interesting case seems to be when the device is not
> runtime-suspended when system suspend triggers.
>

Yes.

The principle is that the driver needs to runtime resume its device,
if not already, during system suspend, as to be able to configure the
in-band interrupt, then instruct the upper layers that the device
needs to stay in its current power state.

>>>
>>> Does it then mean that the middle layer is expected to simply avoid
>>> changing the power state of the device when enabled to wake up the
>>> system, or is there more to that?  In the former case, it may be
>>> better to rename the flag to something along the lines of "don't
>>> change power state if wakeup enabled".
>>
>> Yes, correct.
>>
>> I can try to clarify that in the description and unless you have a
>> suggestion for a better name of the flag, I try to come up with
>> something for that too.
>
> I was thinking about something like DPM_FLAG_IN_BAND_WAKEUP or similar.

Perfect!

>
> But please note that there are cases in which the middle layer has
> information on what power states to put devices into for wakeup to
> work and IMO that should take precedence over the flag as a rule.

Yeah, let me try to clarify that in the doc.

[...]

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
> device during system suspend.
>
> In particular, sometimes the device may need to stay in full power state,
> to have wakeup signals enabled for it. Therefore, define and document a
> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
> exactly about that.
>
> During __device_suspend() in the PM core, let's make sure to also propagate
> the setting of the flag to the parent device, when wakeup signals are
> enabled and unless the parent has the "ignore_children" flag set. This
> makes it also consistent with how the existing "wakeup_path" flag is being
> assigned.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>  drivers/base/power/main.c               |  6 +++++-
>  include/linux/pm.h                      |  5 +++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
> index 53c1b0b..1ca2d0f 100644
> --- a/Documentation/driver-api/pm/devices.rst
> +++ b/Documentation/driver-api/pm/devices.rst
> @@ -414,6 +414,18 @@ 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.
>
> +Moreover, in case wakeup signals are enabled for a device, some bus types and
> +PM domains may manage the device slightly differently during system suspend. For
> +example, sometimes the device needs to stay in full power state, to have wakeup
> +signals enabled for it. In cases when the wakeup settings are mostly managed by
> +the driver, it may not be sufficient for bus types and PM domains to only check
> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
> +:c:member:`power.driver_flags`, by passing the flag to
> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
> +domains to leave the device in full power state, when wakeup signals are enabled
> +for it.
> +
>  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..f64f945 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1432,9 +1432,13 @@ 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;
>
> +               if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
> +                       parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;

No, sorry.

The flag cannot be propagated this way, because that effectively
overrides the choices made by the parent driver and up.

Besides, wakeup_path already had a similar purpose.  What has happened to that?

Thanks,
Rafael

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

[Ulf Hansson]

On 9 November 2017 at 01:41, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
>> device during system suspend.
>>
>> In particular, sometimes the device may need to stay in full power state,
>> to have wakeup signals enabled for it. Therefore, define and document a
>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>> exactly about that.
>>
>> During __device_suspend() in the PM core, let's make sure to also propagate
>> the setting of the flag to the parent device, when wakeup signals are
>> enabled and unless the parent has the "ignore_children" flag set. This
>> makes it also consistent with how the existing "wakeup_path" flag is being
>> assigned.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>>  drivers/base/power/main.c               |  6 +++++-
>>  include/linux/pm.h                      |  5 +++++
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
>> index 53c1b0b..1ca2d0f 100644
>> --- a/Documentation/driver-api/pm/devices.rst
>> +++ b/Documentation/driver-api/pm/devices.rst
>> @@ -414,6 +414,18 @@ 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.
>>
>> +Moreover, in case wakeup signals are enabled for a device, some bus types and
>> +PM domains may manage the device slightly differently during system suspend. For
>> +example, sometimes the device needs to stay in full power state, to have wakeup
>> +signals enabled for it. In cases when the wakeup settings are mostly managed by
>> +the driver, it may not be sufficient for bus types and PM domains to only check
>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>> +:c:member:`power.driver_flags`, by passing the flag to
>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>> +domains to leave the device in full power state, when wakeup signals are enabled
>> +for it.
>> +
>>  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..f64f945 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1432,9 +1432,13 @@ 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;
>>
>> +               if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
>> +                       parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
>
> No, sorry.
>
> The flag cannot be propagated this way, because that effectively
> overrides the choices made by the parent driver and up.

Yes, but that is the hole point.

If a child device needs to stay powered as to deal with wakeup, so is
required by the parent.

>
> Besides, wakeup_path already had a similar purpose.  What has happened to that?

Yes, but wakeup_path is only telling half of what is needed.

Because even if wakeup_path becomes set for a parent device, doesn't
mean that it must stay in full power during system suspend to serve
wakeups for a child. That's why I think the DPM_FLAG_WAKEUP_POWERED
flag needs to be propagated also.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 9:53 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 01:41, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, 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 treat the
>>> device during system suspend.
>>>
>>> In particular, sometimes the device may need to stay in full power state,
>>> to have wakeup signals enabled for it. Therefore, define and document a
>>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>>> exactly about that.
>>>
>>> During __device_suspend() in the PM core, let's make sure to also propagate
>>> the setting of the flag to the parent device, when wakeup signals are
>>> enabled and unless the parent has the "ignore_children" flag set. This
>>> makes it also consistent with how the existing "wakeup_path" flag is being
>>> assigned.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>  Documentation/driver-api/pm/devices.rst | 12 ++++++++++++
>>>  drivers/base/power/main.c               |  6 +++++-
>>>  include/linux/pm.h                      |  5 +++++
>>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
>>> index 53c1b0b..1ca2d0f 100644
>>> --- a/Documentation/driver-api/pm/devices.rst
>>> +++ b/Documentation/driver-api/pm/devices.rst
>>> @@ -414,6 +414,18 @@ 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.
>>>
>>> +Moreover, in case wakeup signals are enabled for a device, some bus types and
>>> +PM domains may manage the device slightly differently during system suspend. For
>>> +example, sometimes the device needs to stay in full power state, to have wakeup
>>> +signals enabled for it. In cases when the wakeup settings are mostly managed by
>>> +the driver, it may not be sufficient for bus types and PM domains to only check
>>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what actions
>>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>>> +:c:member:`power.driver_flags`, by passing the flag to
>>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>>> +domains to leave the device in full power state, when wakeup signals are enabled
>>> +for it.
>>> +
>>>  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..f64f945 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1432,9 +1432,13 @@ 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;
>>>
>>> +               if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
>>> +                       parent->power.driver_flags |= DPM_FLAG_WAKEUP_POWERED;
>>
>> No, sorry.
>>
>> The flag cannot be propagated this way, because that effectively
>> overrides the choices made by the parent driver and up.
>
> Yes, but that is the hole point.
>
> If a child device needs to stay powered as to deal with wakeup, so is
> required by the parent.

So you need a flag and a status bit.

The flag says what the driver wants (and what it wants for a
particular device) and the status bit reflects the current situation
(taking dependencies into account).

Say a device has two children, A and B, and both of them have the new flag set.

If either A or B is configured for system wakeup, power should not be
removed from the parent.  However, if neither A nor B is configured
for system wakeup, power can be removed from the parent on suspend
unless the parent driver itself has set the new flag.
Thus setting the new flag by the child drivers alone doesn't imply the
specific handling of the parent unless additional conditions occur.

That can be represented by a status bit that will be set or unset on
every suspend individually taking the current configuration into
account every time.

>>
>> Besides, wakeup_path already had a similar purpose.  What has happened to that?
>
> Yes, but wakeup_path is only telling half of what is needed.
>
> Because even if wakeup_path becomes set for a parent device, doesn't
> mean that it must stay in full power during system suspend to serve
> wakeups for a child. That's why I think the DPM_FLAG_WAKEUP_POWERED
> flag needs to be propagated also.

OK, so one more status bit is needed, then.

Thanks,
Rafael

[PATCH 3/3] PM / Domains: Take WAKEUP_POWERED driver flag into account

[Ulf Hansson]

Make genpd to take the WAKEUP_POWERED driver flag into account during
system suspend/resume. More precisely, in case the WAKEUP_POWERED flag is
set, 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>
---
 drivers/base/power/domain.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 24e39ce..dc18f88 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1037,8 +1037,11 @@ 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))
-		return 0;
+	if (dev->power.wakeup_path) {
+		if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED) ||
+		    genpd_is_active_wakeup(genpd))
+			return 0;
+	}
 
 	if (poweroff)
 		ret = pm_generic_poweroff_noirq(dev);
@@ -1092,8 +1095,11 @@ static int genpd_resume_noirq(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
-		return 0;
+	if (dev->power.wakeup_path) {
+		if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED) ||
+		    genpd_is_active_wakeup(genpd))
+			return 0;
+	}
 
 	genpd_lock(genpd);
 	genpd_sync_power_on(genpd, true, 0);
-- 
2.7.4

Re: [PATCH 3/3] PM / Domains: Take WAKEUP_POWERED driver flag into account

[Geert Uytterhoeven]

On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Make genpd to take the WAKEUP_POWERED driver flag into account during
> system suspend/resume. More precisely, in case the WAKEUP_POWERED flag is
> set, 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>

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 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
> is enabled for it.

Thanks for your patches!
They look good to me, hence my Reviewed-by.

> 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

While your new WAKEUP_POWERED definitely serves a purpose, I don't think
it's the right solution for the Renesas SoCs.  I can just set the recently
added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
drivers to fix the issue for all Renesas drivers.  After all, all devices in
the clock/power domain must be kept enabled if they're a wakeup source, or
part of the wakeup path.

Not using GENPD_FLAG_ACTIVE_WAKEUP means I would have to add the
WAKEUP_POWERED flag to every single driver that can either be a wakeup
source itself, or be part of the wakeup path.

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 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>> is enabled for it.
>
> Thanks for your patches!
> They look good to me, hence my Reviewed-by.

Hi Geert,

Thanks for reviewing, much appreciated!

>
>> 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
>
> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
> it's the right solution for the Renesas SoCs.  I can just set the recently
> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
> drivers to fix the issue for all Renesas drivers.  After all, all devices in
> the clock/power domain must be kept enabled if they're a wakeup source, or
> part of the wakeup path.

Right, that would work! However, to me, I don't think it's fine grained enough.

Let's take the Ethernet device/driver using WoL as an example, similar
to your cases.

First, let's assume device_may_wakeup() returns true, meaning that the
Ethernet device is wakeup capable and that userspace has requested
wakeup to be enabled.

Then we have three scenarios to consider when the Ethernet driver
becomes suspended (typically when its ->suspend() callback gets
invoked).
1) The Ethernet interface is down.
2) The Ethernet interface is up, but no connection established.
3) The Ethernet interface is up, connection established.

By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
that we can't distinguish between any of the the scenarios above, but
instead always keep the Ethernet device powered on and thus the PM
domain also.

In the more fine grained solution, we can change the Ethernet driver
to consider under what scenario it's being suspended. For 1) and 2),
there is no need to keep the Ethernet device being powered, but
instead only enable WoL in 3) - via also using the WAKEUP_POWERED
flag.

>
> Not using GENPD_FLAG_ACTIVE_WAKEUP means I would have to add the
> WAKEUP_POWERED flag to every single driver that can either be a wakeup
> source itself, or be part of the wakeup path.

Right.

First, is that really that many? Second, nothing prevent us from doing
the migration of each driver in step by step.

Kind regards
Uffe

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

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>>> is enabled for it.

>>> 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
>>
>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>> it's the right solution for the Renesas SoCs.  I can just set the recently
>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>> the clock/power domain must be kept enabled if they're a wakeup source, or
>> part of the wakeup path.
>
> Right, that would work! However, to me, I don't think it's fine grained enough.
>
> Let's take the Ethernet device/driver using WoL as an example, similar
> to your cases.
>
> First, let's assume device_may_wakeup() returns true, meaning that the
> Ethernet device is wakeup capable and that userspace has requested
> wakeup to be enabled.
>
> Then we have three scenarios to consider when the Ethernet driver
> becomes suspended (typically when its ->suspend() callback gets
> invoked).
> 1) The Ethernet interface is down.
> 2) The Ethernet interface is up, but no connection established.
> 3) The Ethernet interface is up, connection established.
>
> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
> that we can't distinguish between any of the the scenarios above, but
> instead always keep the Ethernet device powered on and thus the PM
> domain also.
>
> In the more fine grained solution, we can change the Ethernet driver
> to consider under what scenario it's being suspended. For 1) and 2),
> there is no need to keep the Ethernet device being powered, but
> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
> flag.

The Ethernet driver can still call device_set_wakeup_enable(... , false)
to control this.  If WoL is disabled by the user (or deemed not usable, see
below), it already does so.

In addition, keeping WoL enabled for cases 1 and 2 may be desirable
(e.g allow wake-up if a cable is plugged in during system suspend and
 a magic packet is received afterwards), depending on the hardware.
But the driver can already control that through device_set_wakeup_enable().

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 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>>>> is enabled for it.
>
>>>> 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
>>>
>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>> part of the wakeup path.
>>
>> Right, that would work! However, to me, I don't think it's fine grained enough.
>>
>> Let's take the Ethernet device/driver using WoL as an example, similar
>> to your cases.
>>
>> First, let's assume device_may_wakeup() returns true, meaning that the
>> Ethernet device is wakeup capable and that userspace has requested
>> wakeup to be enabled.
>>
>> Then we have three scenarios to consider when the Ethernet driver
>> becomes suspended (typically when its ->suspend() callback gets
>> invoked).
>> 1) The Ethernet interface is down.
>> 2) The Ethernet interface is up, but no connection established.
>> 3) The Ethernet interface is up, connection established.
>>
>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>> that we can't distinguish between any of the the scenarios above, but
>> instead always keep the Ethernet device powered on and thus the PM
>> domain also.
>>
>> In the more fine grained solution, we can change the Ethernet driver
>> to consider under what scenario it's being suspended. For 1) and 2),
>> there is no need to keep the Ethernet device being powered, but
>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>> flag.
>
> The Ethernet driver can still call device_set_wakeup_enable(... , false)
> to control this.  If WoL is disabled by the user (or deemed not usable, see
> below), it already does so.

I don't think that API is intended to be used like that and I wonder
if it even works as expected.

Quoting the doc:
"If device wakeup mechanisms are enabled or disabled directly by
drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
during a system sleep transition.  Device drivers, however, are not expected to
call :c:func:`device_set_wakeup_enable()` directly in any case."

Rafael, can you comment on this?

>
> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
> (e.g allow wake-up if a cable is plugged in during system suspend and
>  a magic packet is received afterwards), depending on the hardware.
> But the driver can already control that through device_set_wakeup_enable().
>

Ehh, that sounds weird. :-) If the Ethernet interface is down, how
would such packet be received?

Kind regards
Uffe

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

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>>>>> is enabled for it.
>>
>>>>> 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
>>>>
>>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>>> part of the wakeup path.
>>>
>>> Right, that would work! However, to me, I don't think it's fine grained enough.
>>>
>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>> to your cases.
>>>
>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>> Ethernet device is wakeup capable and that userspace has requested
>>> wakeup to be enabled.
>>>
>>> Then we have three scenarios to consider when the Ethernet driver
>>> becomes suspended (typically when its ->suspend() callback gets
>>> invoked).
>>> 1) The Ethernet interface is down.
>>> 2) The Ethernet interface is up, but no connection established.
>>> 3) The Ethernet interface is up, connection established.
>>>
>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>> that we can't distinguish between any of the the scenarios above, but
>>> instead always keep the Ethernet device powered on and thus the PM
>>> domain also.
>>>
>>> In the more fine grained solution, we can change the Ethernet driver
>>> to consider under what scenario it's being suspended. For 1) and 2),
>>> there is no need to keep the Ethernet device being powered, but
>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>> flag.
>>
>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>> below), it already does so.
>
> I don't think that API is intended to be used like that and I wonder
> if it even works as expected.
>
> Quoting the doc:
> "If device wakeup mechanisms are enabled or disabled directly by
> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
> during a system sleep transition.  Device drivers, however, are not expected to
> call :c:func:`device_set_wakeup_enable()` directly in any case."
>
> Rafael, can you comment on this?

There are ca. 100 callers in drivers.

>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>  a magic packet is received afterwards), depending on the hardware.
>> But the driver can already control that through device_set_wakeup_enable().
>
> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
> would such packet be received?

It depends on your meaning of "up".  My interpretation is that "up" means
ready to handle packets between physical media and the Linux networking stack.

So even when "down", the actual Ethernet controller may still be able to
receive a magic packet if WoL is enabled.  The magic packet is really a
magic packet not intended to be transmitted to the networking stack, but
merely serves as a wakeup signal.

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 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

[...]

>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>> below), it already does so.
>>
>> I don't think that API is intended to be used like that and I wonder
>> if it even works as expected.
>>
>> Quoting the doc:
>> "If device wakeup mechanisms are enabled or disabled directly by
>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
>> during a system sleep transition.  Device drivers, however, are not expected to
>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>
>> Rafael, can you comment on this?
>
> There are ca. 100 callers in drivers.

Yeah, but those doesn't normally use it to toggle the setting, but
instead only to set a default value during ->probe() or whatever
initialization code that runs.

I think that makes a big difference, doesn't it?

>
>>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>>  a magic packet is received afterwards), depending on the hardware.
>>> But the driver can already control that through device_set_wakeup_enable().
>>
>> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
>> would such packet be received?
>
> It depends on your meaning of "up".  My interpretation is that "up" means
> ready to handle packets between physical media and the Linux networking stack.
>
> So even when "down", the actual Ethernet controller may still be able to
> receive a magic packet if WoL is enabled.  The magic packet is really a
> magic packet not intended to be transmitted to the networking stack, but
> merely serves as a wakeup signal.

I see! So, in the end this seems like a combination of what the HW
supports and what the user policy is set to.

Out of curiosity, can you tell how those Renesas Ethernet devices
works in this regards?

Anyway, thanks for clarifying!

Kind regards
Uffe

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

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]
>
>>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>>> below), it already does so.
>>>
>>> I don't think that API is intended to be used like that and I wonder
>>> if it even works as expected.
>>>
>>> Quoting the doc:
>>> "If device wakeup mechanisms are enabled or disabled directly by
>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
>>> during a system sleep transition.  Device drivers, however, are not expected to
>>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>>
>>> Rafael, can you comment on this?
>>
>> There are ca. 100 callers in drivers.
>
> Yeah, but those doesn't normally use it to toggle the setting, but
> instead only to set a default value during ->probe() or whatever
> initialization code that runs.
>
> I think that makes a big difference, doesn't it?

The few Ethernet drivers I looked at change the state in their
ethtool_ops.set_wol() callback, not during probe.
This is to be configured from userspace using ethtool.

>>>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>>>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>>>  a magic packet is received afterwards), depending on the hardware.
>>>> But the driver can already control that through device_set_wakeup_enable().
>>>
>>> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
>>> would such packet be received?
>>
>> It depends on your meaning of "up".  My interpretation is that "up" means
>> ready to handle packets between physical media and the Linux networking stack.
>>
>> So even when "down", the actual Ethernet controller may still be able to
>> receive a magic packet if WoL is enabled.  The magic packet is really a
>> magic packet not intended to be transmitted to the networking stack, but
>> merely serves as a wakeup signal.
>
> I see! So, in the end this seems like a combination of what the HW
> supports and what the user policy is set to.
>
> Out of curiosity, can you tell how those Renesas Ethernet devices
> works in this regards?

I don't know, I was just playing the devil's advocate ;-)

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 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> [...]
>>
>>>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>>>> below), it already does so.
>>>>
>>>> I don't think that API is intended to be used like that and I wonder
>>>> if it even works as expected.
>>>>
>>>> Quoting the doc:
>>>> "If device wakeup mechanisms are enabled or disabled directly by
>>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
>>>> during a system sleep transition.  Device drivers, however, are not expected to
>>>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>>>
>>>> Rafael, can you comment on this?
>>>
>>> There are ca. 100 callers in drivers.
>>
>> Yeah, but those doesn't normally use it to toggle the setting, but
>> instead only to set a default value during ->probe() or whatever
>> initialization code that runs.
>>
>> I think that makes a big difference, doesn't it?
>
> The few Ethernet drivers I looked at change the state in their
> ethtool_ops.set_wol() callback, not during probe.
> This is to be configured from userspace using ethtool.

Which is the case I was talking about.

Since changing the WoL setting via ethtool is expected to cause the
sysfs knob to reflect its status, using device_set_wakeup_enable() in
there is not actually confusing.

The same applies to setting the default from ->probe().

It will be confusing in all of the other cases, though.

Thanks,
Rafael

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

[Ulf Hansson]

On 9 November 2017 at 17:31, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Ulf,
>>
>> On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> [...]
>>>
>>>>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>>>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>>>>> below), it already does so.
>>>>>
>>>>> I don't think that API is intended to be used like that and I wonder
>>>>> if it even works as expected.
>>>>>
>>>>> Quoting the doc:
>>>>> "If device wakeup mechanisms are enabled or disabled directly by
>>>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
>>>>> during a system sleep transition.  Device drivers, however, are not expected to
>>>>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>>>>
>>>>> Rafael, can you comment on this?
>>>>
>>>> There are ca. 100 callers in drivers.
>>>
>>> Yeah, but those doesn't normally use it to toggle the setting, but
>>> instead only to set a default value during ->probe() or whatever
>>> initialization code that runs.
>>>
>>> I think that makes a big difference, doesn't it?
>>
>> The few Ethernet drivers I looked at change the state in their
>> ethtool_ops.set_wol() callback, not during probe.
>> This is to be configured from userspace using ethtool.
>
> Which is the case I was talking about.
>
> Since changing the WoL setting via ethtool is expected to cause the
> sysfs knob to reflect its status, using device_set_wakeup_enable() in
> there is not actually confusing.
>
> The same applies to setting the default from ->probe().
>
> It will be confusing in all of the other cases, though.

Agreed, so using it to enable/disable wakeups during suspend is a bad idea.

I will re-spin my series, addressing your review comments.

Kind regards
Uffe

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

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Hi Ulf,
>>
>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>>>>> is enabled for it.
>>
>>>>> 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
>>>>
>>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>>> part of the wakeup path.
>>>
>>> Right, that would work! However, to me, I don't think it's fine grained enough.
>>>
>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>> to your cases.
>>>
>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>> Ethernet device is wakeup capable and that userspace has requested
>>> wakeup to be enabled.
>>>
>>> Then we have three scenarios to consider when the Ethernet driver
>>> becomes suspended (typically when its ->suspend() callback gets
>>> invoked).
>>> 1) The Ethernet interface is down.
>>> 2) The Ethernet interface is up, but no connection established.
>>> 3) The Ethernet interface is up, connection established.
>>>
>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>> that we can't distinguish between any of the the scenarios above, but
>>> instead always keep the Ethernet device powered on and thus the PM
>>> domain also.
>>>
>>> In the more fine grained solution, we can change the Ethernet driver
>>> to consider under what scenario it's being suspended. For 1) and 2),
>>> there is no need to keep the Ethernet device being powered, but
>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>> flag.
>>
>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>> below), it already does so.
>
> I don't think that API is intended to be used like that and I wonder
> if it even works as expected.
>
> Quoting the doc:
> "If device wakeup mechanisms are enabled or disabled directly by
> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
> during a system sleep transition.  Device drivers, however, are not expected to
> call :c:func:`device_set_wakeup_enable()` directly in any case."
>
> Rafael, can you comment on this?

Well, it means what it says.

If you call device_set_wakeup_enable() from a driver, user space will
see a change in sysfs and may be confused by that and that's why doing
so is not recommended.

Not that people listen to recommendations too much, though. :-)

>>
>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>  a magic packet is received afterwards), depending on the hardware.
>> But the driver can already control that through device_set_wakeup_enable().
>>
>
> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
> would such packet be received?

In PCI NICs, if wakeup power is provided, the NIC can detect activity
on the port and generate a PCI PME even if the I/F is down otherwise.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 12:59 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 9 November 2017 at 10:02, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> Hi Ulf,
>>>
>>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>> On 8 November 2017 at 16:41, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>>>> On Wed, Nov 8, 2017 at 4:15 PM, 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 stay powered in case wakeup signals
>>>>>> is enabled for it.
>>>
>>>>>> 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
>>>>>
>>>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>>>> part of the wakeup path.
>>>>
>>>> Right, that would work! However, to me, I don't think it's fine grained enough.
>>>>
>>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>>> to your cases.
>>>>
>>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>>> Ethernet device is wakeup capable and that userspace has requested
>>>> wakeup to be enabled.
>>>>
>>>> Then we have three scenarios to consider when the Ethernet driver
>>>> becomes suspended (typically when its ->suspend() callback gets
>>>> invoked).
>>>> 1) The Ethernet interface is down.
>>>> 2) The Ethernet interface is up, but no connection established.
>>>> 3) The Ethernet interface is up, connection established.
>>>>
>>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>>> that we can't distinguish between any of the the scenarios above, but
>>>> instead always keep the Ethernet device powered on and thus the PM
>>>> domain also.
>>>>
>>>> In the more fine grained solution, we can change the Ethernet driver
>>>> to consider under what scenario it's being suspended. For 1) and 2),
>>>> there is no need to keep the Ethernet device being powered, but
>>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>>> flag.
>>>
>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>> below), it already does so.
>>
>> I don't think that API is intended to be used like that and I wonder
>> if it even works as expected.
>>
>> Quoting the doc:
>> "If device wakeup mechanisms are enabled or disabled directly by
>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
>> during a system sleep transition.  Device drivers, however, are not expected to
>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>
>> Rafael, can you comment on this?
>
> Well, it means what it says.
>
> If you call device_set_wakeup_enable() from a driver, user space will
> see a change in sysfs and may be confused by that and that's why doing
> so is not recommended.
>
> Not that people listen to recommendations too much, though. :-)

But setting up WoL via ethtool from user space is an exception,
because user space actually *does* expect to see a change in sysfs in
this particular case.

It's basically two different ways to change the same setting and both
should result in the same behavior, ideally.

Thanks,
Rafael