[PATCH] driver-core: platform: automatically mark wakeup devices

[PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

When probing platform drivers let's check if corresponding devices have
"wakeup-source" property defined (either in device tree, ACPI, or static
platform properties) and automatically enable such devices as wakeup
sources for the system. This will help us standardize on the name for this
property and reduce amount of boilerplate code in the drivers.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/platform.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1dd6d3b..d14071a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
 
 	ret = dev_pm_domain_attach(_dev, true);
 	if (ret != -EPROBE_DEFER && drv->probe) {
+		bool wakeup = device_property_read_bool(_dev, "wakeup-source");
+
+		device_init_wakeup(_dev, wakeup);
 		ret = drv->probe(dev);
-		if (ret)
+		if (ret) {
+			device_init_wakeup(_dev, false);
 			dev_pm_domain_detach(_dev, true);
+		}
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
@@ -540,6 +545,8 @@ static int platform_drv_remove(struct device *_dev)
 
 	if (drv->remove)
 		ret = drv->remove(dev);
+
+	device_init_wakeup(_dev, false);
 	dev_pm_domain_detach(_dev, true);
 
 	return ret;
-- 
2.6.0.rc2.230.g3dd15c0


-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Greg Kroah-Hartman]

On Sun, Jan 17, 2016 at 06:11:38PM -0800, Dmitry Torokhov wrote:
> When probing platform drivers let's check if corresponding devices have
> "wakeup-source" property defined (either in device tree, ACPI, or static
> platform properties) and automatically enable such devices as wakeup
> sources for the system. This will help us standardize on the name for this
> property and reduce amount of boilerplate code in the drivers.

How much boilerplate code can be removed?  Do you have an example patch
of this removal for any drivers if we move this logic into the driver core?

thanks,

greg k-h

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

On Sun, Jan 17, 2016 at 09:11:25PM -0800, Greg Kroah-Hartman wrote:
> On Sun, Jan 17, 2016 at 06:11:38PM -0800, Dmitry Torokhov wrote:
> > When probing platform drivers let's check if corresponding devices have
> > "wakeup-source" property defined (either in device tree, ACPI, or static
> > platform properties) and automatically enable such devices as wakeup
> > sources for the system. This will help us standardize on the name for this
> > property and reduce amount of boilerplate code in the drivers.
> 
> How much boilerplate code can be removed?  Do you have an example patch
> of this removal for any drivers if we move this logic into the driver core?

Admittedly not a lot, a few lines. There is a couple of lines for
checking the property and calling device_init_wakeup() and also
sometimes clearing wakeup flag is the only thing that is left in
remove() method after converting to devm*. I am more interested in
standardizing on the property name and having wakeup flag cleared on
removal or probe failure, similarly how we do it for driver data in
device structure.

I do not have good patches in input at the moment as even though we are
using "wakeup-source" now lots if the drivers did not start with it and
so we have compatibility parsing still that we want to keep around. I
want the new drivers to use only this property though.

FWIW I2C bus code implements automatic parsing of this property as well
and I wonder if we want to do the same for SPI.

Thanks.

-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
> When probing platform drivers let's check if corresponding devices have
> "wakeup-source" property defined (either in device tree, ACPI, or static
> platform properties) and automatically enable such devices as wakeup
> sources for the system. This will help us standardize on the name for this
> property and reduce amount of boilerplate code in the drivers.

ACPI has other ways of telling the OS that the device is wakeup-capable,
but I guess the property in question can be used too (as long as it is
consistent with the other methods).

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/platform.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1dd6d3b..d14071a 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>  
>  	ret = dev_pm_domain_attach(_dev, true);
>  	if (ret != -EPROBE_DEFER && drv->probe) {
> +		bool wakeup = device_property_read_bool(_dev, "wakeup-source");
> +
> +		device_init_wakeup(_dev, wakeup);

But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?

device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
which in principle may unblock spurious wakeups on some systems.

>  		ret = drv->probe(dev);
> -		if (ret)
> +		if (ret) {
> +			device_init_wakeup(_dev, false);
>  			dev_pm_domain_detach(_dev, true);
> +		}
>  	}
>  
>  	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> @@ -540,6 +545,8 @@ static int platform_drv_remove(struct device *_dev)
>  
>  	if (drv->remove)
>  		ret = drv->remove(dev);
> +
> +	device_init_wakeup(_dev, false);
>  	dev_pm_domain_detach(_dev, true);
>  
>  	return ret;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Sudeep Holla]

On Mon, Jan 18, 2016 at 2:47 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>> When probing platform drivers let's check if corresponding devices have
>> "wakeup-source" property defined (either in device tree, ACPI, or static
>> platform properties) and automatically enable such devices as wakeup
>> sources for the system. This will help us standardize on the name for this
>> property and reduce amount of boilerplate code in the drivers.
>
> ACPI has other ways of telling the OS that the device is wakeup-capable,
> but I guess the property in question can be used too (as long as it is
> consistent with the other methods).
>

Just curious to know what you mean when you say this property can also
be used with ACPI. Do you mean we could use "wakeup-source" DSD ?

If so, won't that go against rule for DSD (i.e we *should not* bypass the
existing mechanisms defined by the ACPI, e.g. _SxW in this case)

--
Regards,
Sudeep

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Sudeep Holla]

On Mon, Jan 18, 2016 at 6:14 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Sun, Jan 17, 2016 at 09:11:25PM -0800, Greg Kroah-Hartman wrote:
>> On Sun, Jan 17, 2016 at 06:11:38PM -0800, Dmitry Torokhov wrote:
>> > When probing platform drivers let's check if corresponding devices have
>> > "wakeup-source" property defined (either in device tree, ACPI, or static
>> > platform properties) and automatically enable such devices as wakeup
>> > sources for the system. This will help us standardize on the name for this
>> > property and reduce amount of boilerplate code in the drivers.
>>
>> How much boilerplate code can be removed?  Do you have an example patch
>> of this removal for any drivers if we move this logic into the driver core?
>
> Admittedly not a lot, a few lines. There is a couple of lines for
> checking the property and calling device_init_wakeup() and also
> sometimes clearing wakeup flag is the only thing that is left in
> remove() method after converting to devm*. I am more interested in
> standardizing on the property name and having wakeup flag cleared on
> removal or probe failure, similarly how we do it for driver data in
> device structure.
>
> I do not have good patches in input at the moment as even though we are
> using "wakeup-source" now lots if the drivers did not start with it and
> so we have compatibility parsing still that we want to keep around. I
> want the new drivers to use only this property though.
>

True. We can move the standard property to driver core while legacy compatible
match can be retained in the individual drivers. The list[1] is not
too bad though,
and more over it remain static from now on.

--
Regards,
Sudeep

[1] Documentation/devicetree/bindings/power/wakeup-source.txt

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Monday, January 18, 2016 03:23:18 PM Sudeep Holla wrote:
> On Mon, Jan 18, 2016 at 2:47 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
> >> When probing platform drivers let's check if corresponding devices have
> >> "wakeup-source" property defined (either in device tree, ACPI, or static
> >> platform properties) and automatically enable such devices as wakeup
> >> sources for the system. This will help us standardize on the name for this
> >> property and reduce amount of boilerplate code in the drivers.
> >
> > ACPI has other ways of telling the OS that the device is wakeup-capable,
> > but I guess the property in question can be used too (as long as it is
> > consistent with the other methods).
> >
> 
> Just curious to know what you mean when you say this property can also
> be used with ACPI. Do you mean we could use "wakeup-source" DSD ?

Yes.

> If so, won't that go against rule for DSD (i.e we *should not* bypass the
> existing mechanisms defined by the ACPI, e.g. _SxW in this case)

Not necessarily.

What if the device doesn't use ACPI PM and still can wake up the system?

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Sudeep Holla]

On 18/01/16 15:41, Rafael J. Wysocki wrote:
> On Monday, January 18, 2016 03:23:18 PM Sudeep Holla wrote:
>> On Mon, Jan 18, 2016 at 2:47 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>>>> When probing platform drivers let's check if corresponding devices have
>>>> "wakeup-source" property defined (either in device tree, ACPI, or static
>>>> platform properties) and automatically enable such devices as wakeup
>>>> sources for the system. This will help us standardize on the name for this
>>>> property and reduce amount of boilerplate code in the drivers.
>>>
>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>>> but I guess the property in question can be used too (as long as it is
>>> consistent with the other methods).
>>>
>>
>> Just curious to know what you mean when you say this property can also
>> be used with ACPI. Do you mean we could use "wakeup-source" DSD ?
>
> Yes.
>
>> If so, won't that go against rule for DSD (i.e we *should not* bypass the
>> existing mechanisms defined by the ACPI, e.g. _SxW in this case)
>
> Not necessarily.
>
> What if the device doesn't use ACPI PM and still can wake up the system?
>

May be I don't understand the exact configuration you are referring, but
if you don't use ACPI PM, how will that system enter sleep states ?
IIUC you are referring suspend-to-idle case only here which doesn't
require any ACPI _Sx methods, which make sense.

But still I don't see a strong reason to provide an alternate method to
specify the same information. Firmware responsible for ACPI tables have
to specify this DSD in question, instead it can use the existing
mechanism in ACPI.

Let me know if I am missing some information about the systems you are
referring ?

-- 
Regards,
Sudeep

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>> When probing platform drivers let's check if corresponding devices have
>> "wakeup-source" property defined (either in device tree, ACPI, or static
>> platform properties) and automatically enable such devices as wakeup
>> sources for the system. This will help us standardize on the name for this
>> property and reduce amount of boilerplate code in the drivers.
>
> ACPI has other ways of telling the OS that the device is wakeup-capable,
> but I guess the property in question can be used too (as long as it is
> consistent with the other methods).

I was thinking that down the road ACPI can wire its internal wakeup
knowledge into generic device property. Unfortunately at the moment
most drivers go like this:

- if it is device tree platform: good, we'll take the data from device
tree property and set up driver behavior accordingly;
- if it is legacy board setup: OK, take for driver-specific platform data;
- ACPI... hmm... dunno... enable wakeup and if it is not correct the
system will just not wake up, not the best but not terribly wrong
either.

>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/base/platform.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 1dd6d3b..d14071a 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>>
>>       ret = dev_pm_domain_attach(_dev, true);
>>       if (ret != -EPROBE_DEFER && drv->probe) {
>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>> +
>> +             device_init_wakeup(_dev, wakeup);
>
> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>
> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
> which in principle may unblock spurious wakeups on some systems.

Why would we not want enable wakeup for devices where system
(platform) tells us that they are wakeup sources? This is how most of
the drivers I am involved in behave. All of them use
device_init_wakeup() and then we adjust the behavior depending on
runtime state, i.e. do not wake up from touchpad when lid is closed
(which is useful if lid is flexible and may interact with touchpad if
slammed down hard enough).

>
>>               ret = drv->probe(dev);
>> -             if (ret)
>> +             if (ret) {
>> +                     device_init_wakeup(_dev, false);
>>                       dev_pm_domain_detach(_dev, true);
>> +             }
>>       }
>>
>>       if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
>> @@ -540,6 +545,8 @@ static int platform_drv_remove(struct device *_dev)
>>
>>       if (drv->remove)
>>               ret = drv->remove(dev);
>> +
>> +     device_init_wakeup(_dev, false);
>>       dev_pm_domain_detach(_dev, true);
>>
>>       return ret;
>>
>

Thanks.

-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Mon, Jan 18, 2016 at 4:58 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 18/01/16 15:41, Rafael J. Wysocki wrote:
>>
>> On Monday, January 18, 2016 03:23:18 PM Sudeep Holla wrote:
>>>
>>> On Mon, Jan 18, 2016 at 2:47 PM, Rafael J. Wysocki <rjw@rjwysocki.net>
>>> wrote:
>>>>
>>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>>>>>
>>>>> When probing platform drivers let's check if corresponding devices have
>>>>> "wakeup-source" property defined (either in device tree, ACPI, or
>>>>> static
>>>>> platform properties) and automatically enable such devices as wakeup
>>>>> sources for the system. This will help us standardize on the name for
>>>>> this
>>>>> property and reduce amount of boilerplate code in the drivers.
>>>>
>>>>
>>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>>>> but I guess the property in question can be used too (as long as it is
>>>> consistent with the other methods).
>>>>
>>>
>>> Just curious to know what you mean when you say this property can also
>>> be used with ACPI. Do you mean we could use "wakeup-source" DSD ?
>>
>>
>> Yes.
>>
>>> If so, won't that go against rule for DSD (i.e we *should not* bypass the
>>> existing mechanisms defined by the ACPI, e.g. _SxW in this case)
>>
>>
>> Not necessarily.
>>
>> What if the device doesn't use ACPI PM and still can wake up the system?
>>
>
> May be I don't understand the exact configuration you are referring, but
> if you don't use ACPI PM, how will that system enter sleep states ?
> IIUC you are referring suspend-to-idle case only here which doesn't
> require any ACPI _Sx methods, which make sense.

That plus PM on HW-reduced ACPI platforms where GPIOs can be used for
system wakeup in principle (although admittedly I have no experience
with such platforms, so this is just a theory).

> But still I don't see a strong reason to provide an alternate method to
> specify the same information. Firmware responsible for ACPI tables have
> to specify this DSD in question, instead it can use the existing
> mechanism in ACPI.

We actually don't use _SxW to determine whether or not devices are
wakeup-capable.  We look at _PRW and we set "wakeup-capable" if it is
present and the contents is valid.

> Let me know if I am missing some information about the systems you are
> referring ?

That said I'm actually quite unsure about the property suggested by
Dmitry, but let me reply to him on that. :-)

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>>> When probing platform drivers let's check if corresponding devices have
>>> "wakeup-source" property defined (either in device tree, ACPI, or static
>>> platform properties) and automatically enable such devices as wakeup
>>> sources for the system. This will help us standardize on the name for this
>>> property and reduce amount of boilerplate code in the drivers.
>>
>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>> but I guess the property in question can be used too (as long as it is
>> consistent with the other methods).
>
> I was thinking that down the road ACPI can wire its internal wakeup
> knowledge into generic device property. Unfortunately at the moment
> most drivers go like this:

That is not really going to happen any time soon AFAICS.

> - if it is device tree platform: good, we'll take the data from device
> tree property and set up driver behavior accordingly;
> - if it is legacy board setup: OK, take for driver-specific platform data;
> - ACPI... hmm... dunno... enable wakeup and if it is not correct the
> system will just not wake up, not the best but not terribly wrong
> either.

First, they shouldn't need to do the last part on ACPI systems (where
they should be belong to the ACPI PM domain), or can look up the
relevant information in the devices' ACPI companions.

Second, I'm quite unsure about the "wakeup-capable" property, because
I'm not sure what it is really supposed to mean.

For the suspend-to-idle case all devices that can generate interrupts
can potentially wake up the system if set up for that during suspend.

For deeper system sleep modes (in which power is removed from
interrupt controllers) there needs to be a special way to set up the
device for system wakeup that should be described by the firmware.
Without that, saying that the device is "wakeup-capable" alone is
pretty much meaningless.

>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>  drivers/base/platform.c | 9 ++++++++-
>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>>> index 1dd6d3b..d14071a 100644
>>> --- a/drivers/base/platform.c
>>> +++ b/drivers/base/platform.c
>>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>>>
>>>       ret = dev_pm_domain_attach(_dev, true);
>>>       if (ret != -EPROBE_DEFER && drv->probe) {
>>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>>> +
>>> +             device_init_wakeup(_dev, wakeup);
>>
>> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>>
>> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
>> which in principle may unblock spurious wakeups on some systems.
>
> Why would we not want enable wakeup for devices where system
> (platform) tells us that they are wakeup sources?

Because user space may not want to use them for system wakeup?

> This is how most of the drivers I am involved in behave. All of them use
> device_init_wakeup() and then we adjust the behavior depending on
> runtime state, i.e. do not wake up from touchpad when lid is closed
> (which is useful if lid is flexible and may interact with touchpad if
> slammed down hard enough).

Plus there is the sysfs attribute that needs to be taken into account
(so user space can disable wakeup devices it doesn't want to be used).

But it is fine if device_init_wakeup() is used by a driver from the
start (which only means that its default value for the above-mentioned
sysfs attribute is "enabled"), but using that in the core may change
the default for some drivers and lead to problems.

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>>>> When probing platform drivers let's check if corresponding devices have
>>>> "wakeup-source" property defined (either in device tree, ACPI, or static
>>>> platform properties) and automatically enable such devices as wakeup
>>>> sources for the system. This will help us standardize on the name for this
>>>> property and reduce amount of boilerplate code in the drivers.
>>>
>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>>> but I guess the property in question can be used too (as long as it is
>>> consistent with the other methods).
>>
>> I was thinking that down the road ACPI can wire its internal wakeup
>> knowledge into generic device property. Unfortunately at the moment
>> most drivers go like this:
>
> That is not really going to happen any time soon AFAICS.
>
>> - if it is device tree platform: good, we'll take the data from device
>> tree property and set up driver behavior accordingly;
>> - if it is legacy board setup: OK, take for driver-specific platform data;
>> - ACPI... hmm... dunno... enable wakeup and if it is not correct the
>> system will just not wake up, not the best but not terribly wrong
>> either.
>
> First, they shouldn't need to do the last part on ACPI systems (where
> they should be belong to the ACPI PM domain), or can look up the
> relevant information in the devices' ACPI companions.

That is the point. We do not want to look into ACPI components, or OF,
or platform data. We want to have unified way of setting the device as
wakeup capable and enabled, preferably by the bus or device core.

>
> Second, I'm quite unsure about the "wakeup-capable" property, because
> I'm not sure what it is really supposed to mean.

On OF system it means that platform integrator decided that this
device is supposed to be waking up the particular system and the
platform and device driver should perform whatever steps are necessary
to wake up the system. The same behavior we also have with drivers
supporting legacy platform data (usually expressed as pdata->wakeup).
ACPI is an outlier here.

>
> For the suspend-to-idle case all devices that can generate interrupts
> can potentially wake up the system if set up for that during suspend.

You need to tell the driver to not put the device in "too deep" sleep
mode though so the interrupts can still be generated.

>
> For deeper system sleep modes (in which power is removed from
> interrupt controllers) there needs to be a special way to set up the
> device for system wakeup that should be described by the firmware.
> Without that, saying that the device is "wakeup-capable" alone is
> pretty much meaningless.

Right, and most of the drivers have the following in their PM code paths:

if (device_wakeup_enabled(dev)) {
    ...
    enable driver's "light" sleep
    ...
    enable_irq_wake();
} else {
   ...
   enable deep sleep
   ...
}

or something similar. The point is that at this time we do not check
for ACPI, or DT, or whatever but rely on single call to
device_may_wakeup().

>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> ---
>>>>  drivers/base/platform.c | 9 ++++++++-
>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>>>> index 1dd6d3b..d14071a 100644
>>>> --- a/drivers/base/platform.c
>>>> +++ b/drivers/base/platform.c
>>>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>>>>
>>>>       ret = dev_pm_domain_attach(_dev, true);
>>>>       if (ret != -EPROBE_DEFER && drv->probe) {
>>>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>>>> +
>>>> +             device_init_wakeup(_dev, wakeup);
>>>
>>> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>>>
>>> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
>>> which in principle may unblock spurious wakeups on some systems.
>>
>> Why would we not want enable wakeup for devices where system
>> (platform) tells us that they are wakeup sources?
>
> Because user space may not want to use them for system wakeup?

Then userspace will disable it. The point is that platform integrator
intended for the device to be wakeup source and so it should be
enabled by default for such devices.

>
>> This is how most of the drivers I am involved in behave. All of them use
>> device_init_wakeup() and then we adjust the behavior depending on
>> runtime state, i.e. do not wake up from touchpad when lid is closed
>> (which is useful if lid is flexible and may interact with touchpad if
>> slammed down hard enough).
>
> Plus there is the sysfs attribute that needs to be taken into account
> (so user space can disable wakeup devices it doesn't want to be used).

It is already taken into account as drivers are supposed to be using
device_may_wakeup() to check if wakeup is enabled at time of power
state transition.

> But it is fine if device_init_wakeup() is used by a driver from the
> start (which only means that its default value for the above-mentioned
> sysfs attribute is "enabled"), but using that in the core may change
> the default for some drivers and lead to problems.

I am not aware of any drivers denoting itself as being wakeup sources
and not enabling wakeup. Do you have examples?

Thanks.

-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Mon, Jan 18, 2016 at 6:55 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>>>>> When probing platform drivers let's check if corresponding devices have
>>>>> "wakeup-source" property defined (either in device tree, ACPI, or static
>>>>> platform properties) and automatically enable such devices as wakeup
>>>>> sources for the system. This will help us standardize on the name for this
>>>>> property and reduce amount of boilerplate code in the drivers.
>>>>
>>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>>>> but I guess the property in question can be used too (as long as it is
>>>> consistent with the other methods).
>>>
>>> I was thinking that down the road ACPI can wire its internal wakeup
>>> knowledge into generic device property. Unfortunately at the moment
>>> most drivers go like this:
>>
>> That is not really going to happen any time soon AFAICS.
>>
>>> - if it is device tree platform: good, we'll take the data from device
>>> tree property and set up driver behavior accordingly;
>>> - if it is legacy board setup: OK, take for driver-specific platform data;
>>> - ACPI... hmm... dunno... enable wakeup and if it is not correct the
>>> system will just not wake up, not the best but not terribly wrong
>>> either.
>>
>> First, they shouldn't need to do the last part on ACPI systems (where
>> they should be belong to the ACPI PM domain), or can look up the
>> relevant information in the devices' ACPI companions.
>
> That is the point. We do not want to look into ACPI components, or OF,
> or platform data. We want to have unified way of setting the device as
> wakeup capable and enabled, preferably by the bus or device core.

On a properly set up ACPI system, the ACPI layer should take care of this.

The driver only has to look at device_may_wakeup() and do some
device-specific setup (if necessary) for system wakeup during system
suspend if that returns 'true'.

IMO, on a non-ACPI system it should work exactly the same way.  That
is, the platform/board code should take care of whatever needs to be
done except for the device-specific setup.

>> Second, I'm quite unsure about the "wakeup-capable" property, because
>> I'm not sure what it is really supposed to mean.
>
> On OF system it means that platform integrator decided that this
> device is supposed to be waking up the particular system and the
> platform and device driver should perform whatever steps are necessary
> to wake up the system.

This really sounds like "I wish the device woke up the system and you
go figure out how to make that happen".  Not very useful to me, to be
honest.

Let me repeat what I said below: Without specifying how to set up the
device for system wakeup, using the "wakeup-capable" property is
meaningless.  However, if it is specified how to do that, the property
is simply redundant, because the wakeup capability obviously follows
from the fact that there is a way to set up the device for system
wakeup.

> The same behavior we also have with drivers
> supporting legacy platform data (usually expressed as pdata->wakeup).
> ACPI is an outlier here.

Which doesn't mean that it is wrong and everybody else is right.

>> For the suspend-to-idle case all devices that can generate interrupts
>> can potentially wake up the system if set up for that during suspend.
>
> You need to tell the driver to not put the device in "too deep" sleep
> mode though so the interrupts can still be generated.

Checking device_may_wakeup() should help here, though.

>>
>> For deeper system sleep modes (in which power is removed from
>> interrupt controllers) there needs to be a special way to set up the
>> device for system wakeup that should be described by the firmware.
>> Without that, saying that the device is "wakeup-capable" alone is
>> pretty much meaningless.
>
> Right, and most of the drivers have the following in their PM code paths:
>
> if (device_wakeup_enabled(dev)) {
>     ...
>     enable driver's "light" sleep
>     ...
>     enable_irq_wake();
> } else {
>    ...
>    enable deep sleep
>    ...
> }

OK, so this appears to indicate that the "wakeup-capable" property
really is supposed to mean "the in-band interrupt of this device can
wake up the system if set up properly", which is far more precise than
the interpretation of it mentioned so far.  Is that the case?

>
> or something similar. The point is that at this time we do not check
> for ACPI, or DT, or whatever but rely on single call to
> device_may_wakeup().

Well, device_may_wakeup() need not imply that you're supposed to set
up the in-band interrupt of the device for system wakeup, really.

>>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>>> ---
>>>>>  drivers/base/platform.c | 9 ++++++++-
>>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>>>>> index 1dd6d3b..d14071a 100644
>>>>> --- a/drivers/base/platform.c
>>>>> +++ b/drivers/base/platform.c
>>>>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>>>>>
>>>>>       ret = dev_pm_domain_attach(_dev, true);
>>>>>       if (ret != -EPROBE_DEFER && drv->probe) {
>>>>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>>>>> +
>>>>> +             device_init_wakeup(_dev, wakeup);
>>>>
>>>> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>>>>
>>>> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
>>>> which in principle may unblock spurious wakeups on some systems.
>>>
>>> Why would we not want enable wakeup for devices where system
>>> (platform) tells us that they are wakeup sources?
>>
>> Because user space may not want to use them for system wakeup?
>
> Then userspace will disable it. The point is that platform integrator
> intended for the device to be wakeup source and so it should be
> enabled by default for such devices.

Whatever the system integrator intended need not matter to the driver
core and can you actually guarantee that "wakeup-capable" will always
be used correctly by all platform firmware?

And what about devices that generate spurious wakeups that will be
magically enabled by this change?

>>> This is how most of the drivers I am involved in behave. All of them use
>>> device_init_wakeup() and then we adjust the behavior depending on
>>> runtime state, i.e. do not wake up from touchpad when lid is closed
>>> (which is useful if lid is flexible and may interact with touchpad if
>>> slammed down hard enough).
>>
>> Plus there is the sysfs attribute that needs to be taken into account
>> (so user space can disable wakeup devices it doesn't want to be used).
>
> It is already taken into account as drivers are supposed to be using
> device_may_wakeup() to check if wakeup is enabled at time of power
> state transition.
>
>> But it is fine if device_init_wakeup() is used by a driver from the
>> start (which only means that its default value for the above-mentioned
>> sysfs attribute is "enabled"), but using that in the core may change
>> the default for some drivers and lead to problems.
>
> I am not aware of any drivers denoting itself as being wakeup sources
> and not enabling wakeup. Do you have examples?

Yes, I do.

i8042 is one of them, exactly to avoid enabling those devices to do
system wakeup on systems where they were not enabled to do that in the
past.

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

On Mon, Jan 18, 2016 at 11:18:12PM +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 18, 2016 at 6:55 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >>> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
> >>>>> When probing platform drivers let's check if corresponding devices have
> >>>>> "wakeup-source" property defined (either in device tree, ACPI, or static
> >>>>> platform properties) and automatically enable such devices as wakeup
> >>>>> sources for the system. This will help us standardize on the name for this
> >>>>> property and reduce amount of boilerplate code in the drivers.
> >>>>
> >>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
> >>>> but I guess the property in question can be used too (as long as it is
> >>>> consistent with the other methods).
> >>>
> >>> I was thinking that down the road ACPI can wire its internal wakeup
> >>> knowledge into generic device property. Unfortunately at the moment
> >>> most drivers go like this:
> >>
> >> That is not really going to happen any time soon AFAICS.
> >>
> >>> - if it is device tree platform: good, we'll take the data from device
> >>> tree property and set up driver behavior accordingly;
> >>> - if it is legacy board setup: OK, take for driver-specific platform data;
> >>> - ACPI... hmm... dunno... enable wakeup and if it is not correct the
> >>> system will just not wake up, not the best but not terribly wrong
> >>> either.
> >>
> >> First, they shouldn't need to do the last part on ACPI systems (where
> >> they should be belong to the ACPI PM domain), or can look up the
> >> relevant information in the devices' ACPI companions.
> >
> > That is the point. We do not want to look into ACPI components, or OF,
> > or platform data. We want to have unified way of setting the device as
> > wakeup capable and enabled, preferably by the bus or device core.
> 
> On a properly set up ACPI system, the ACPI layer should take care of this.

You mean marking device as wakeup-capable? Good, then this patch will
likely be a noop on ACPI systems. However can you please point me how we
do that, for example, for I2C devices instantiated via ACPI?

> 
> The driver only has to look at device_may_wakeup() and do some
> device-specific setup (if necessary) for system wakeup during system
> suspend if that returns 'true'.

Right.

> 
> IMO, on a non-ACPI system it should work exactly the same way.  That
> is, the platform/board code should take care of whatever needs to be
> done except for the device-specific setup.

Yes and no. Like with PM domains, we now have generic property API (that
is implemented by platform code in platform-specific way) so we can just
query necessary properties in needed places, such as various buses code.

> 
> >> Second, I'm quite unsure about the "wakeup-capable" property, because
> >> I'm not sure what it is really supposed to mean.
> >
> > On OF system it means that platform integrator decided that this
> > device is supposed to be waking up the particular system and the
> > platform and device driver should perform whatever steps are necessary
> > to wake up the system.
> 
> This really sounds like "I wish the device woke up the system and you
> go figure out how to make that happen".  Not very useful to me, to be
> honest.

No, not really. It is like "I want this device to wake up system and
driver is supposed to know how to configure this device for wakeup".
Right now drivers parse this attribute themselves, I'd like to
centralize it.

> 
> Let me repeat what I said below: Without specifying how to set up the
> device for system wakeup, using the "wakeup-capable" property is
> meaningless.  However, if it is specified how to do that, the property
> is simply redundant, because the wakeup capability obviously follows
> from the fact that there is a way to set up the device for system
> wakeup.

It is not. You need to tell the driver that on given board:

1. Wakeup by given device can be enabled (hardware is wired properly)
2. Wakeup by given device should be enabled

On both OF and static boards there usually no separate annotations for
the above, they all rolled into single "wakeup-source" property.

> 
> > The same behavior we also have with drivers
> > supporting legacy platform data (usually expressed as pdata->wakeup).
> > ACPI is an outlier here.
> 
> Which doesn't mean that it is wrong and everybody else is right.

>From the driver's point of view it is wrong in the sense that it makes
them implement ACPI-specific behavior and code paths.

> 
> >> For the suspend-to-idle case all devices that can generate interrupts
> >> can potentially wake up the system if set up for that during suspend.
> >
> > You need to tell the driver to not put the device in "too deep" sleep
> > mode though so the interrupts can still be generated.
> 
> Checking device_may_wakeup() should help here, though.

And that's what everyone does. But somebody *has to set* this flag.

> 
> >>
> >> For deeper system sleep modes (in which power is removed from
> >> interrupt controllers) there needs to be a special way to set up the
> >> device for system wakeup that should be described by the firmware.
> >> Without that, saying that the device is "wakeup-capable" alone is
> >> pretty much meaningless.
> >
> > Right, and most of the drivers have the following in their PM code paths:
> >
> > if (device_wakeup_enabled(dev)) {
> >     ...
> >     enable driver's "light" sleep
> >     ...
> >     enable_irq_wake();
> > } else {
> >    ...
> >    enable deep sleep
> >    ...
> > }
> 
> OK, so this appears to indicate that the "wakeup-capable" property
> really is supposed to mean "the in-band interrupt of this device can
> wake up the system if set up properly", which is far more precise than
> the interpretation of it mentioned so far.  Is that the case?

No, not really. It does not say anything about in band interrupt or
anything else. It depends on the driver.

> 
> >
> > or something similar. The point is that at this time we do not check
> > for ACPI, or DT, or whatever but rely on single call to
> > device_may_wakeup().
> 
> Well, device_may_wakeup() need not imply that you're supposed to set
> up the in-band interrupt of the device for system wakeup, really.

I never said it did imply that.

> 
> >>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>>> ---
> >>>>>  drivers/base/platform.c | 9 ++++++++-
> >>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >>>>> index 1dd6d3b..d14071a 100644
> >>>>> --- a/drivers/base/platform.c
> >>>>> +++ b/drivers/base/platform.c
> >>>>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
> >>>>>
> >>>>>       ret = dev_pm_domain_attach(_dev, true);
> >>>>>       if (ret != -EPROBE_DEFER && drv->probe) {
> >>>>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
> >>>>> +
> >>>>> +             device_init_wakeup(_dev, wakeup);
> >>>>
> >>>> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
> >>>>
> >>>> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
> >>>> which in principle may unblock spurious wakeups on some systems.
> >>>
> >>> Why would we not want enable wakeup for devices where system
> >>> (platform) tells us that they are wakeup sources?
> >>
> >> Because user space may not want to use them for system wakeup?
> >
> > Then userspace will disable it. The point is that platform integrator
> > intended for the device to be wakeup source and so it should be
> > enabled by default for such devices.
> 
> Whatever the system integrator intended need not matter to the driver
> core

Huh? Should we ignore ACPI-provided interrupt and address assignments?
Because they are set by platform integrators.

>  and can you actually guarantee that "wakeup-capable" will always
> be used correctly by all platform firmware?

Can you guarantee that all ACPI code ever produced is perfect and doe
snot do stupid things?

> 
> And what about devices that generate spurious wakeups that will be
> magically enabled by this change?

Maybe that could happen on ACPI systems, but see below.

> 
> >>> This is how most of the drivers I am involved in behave. All of them use
> >>> device_init_wakeup() and then we adjust the behavior depending on
> >>> runtime state, i.e. do not wake up from touchpad when lid is closed
> >>> (which is useful if lid is flexible and may interact with touchpad if
> >>> slammed down hard enough).
> >>
> >> Plus there is the sysfs attribute that needs to be taken into account
> >> (so user space can disable wakeup devices it doesn't want to be used).
> >
> > It is already taken into account as drivers are supposed to be using
> > device_may_wakeup() to check if wakeup is enabled at time of power
> > state transition.
> >
> >> But it is fine if device_init_wakeup() is used by a driver from the
> >> start (which only means that its default value for the above-mentioned
> >> sysfs attribute is "enabled"), but using that in the core may change
> >> the default for some drivers and lead to problems.
> >
> > I am not aware of any drivers denoting itself as being wakeup sources
> > and not enabling wakeup. Do you have examples?
> 
> Yes, I do.
> 
> i8042 is one of them, exactly to avoid enabling those devices to do
> system wakeup on systems where they were not enabled to do that in the
> past.

Ah, yes, you added it to i8042. Well, maybe that is an argument for not
automatically enabling wakeup on ACPI systems because that was the
default behavior for them. OF/board platforms have different default
behavior. I guess if we do not do anything besides what the patch is
doing, then ACPI will not have the property I propose, so they will be
capable but not enabled, and OF/boards will have the property and will
be capable and enabled.

Thanks.

-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 11:18:12PM +0100, Rafael J. Wysocki wrote:
>> On Mon, Jan 18, 2016 at 6:55 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >>> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> >>>> On Sunday, January 17, 2016 06:11:38 PM Dmitry Torokhov wrote:
>> >>>>> When probing platform drivers let's check if corresponding devices have
>> >>>>> "wakeup-source" property defined (either in device tree, ACPI, or static
>> >>>>> platform properties) and automatically enable such devices as wakeup
>> >>>>> sources for the system. This will help us standardize on the name for this
>> >>>>> property and reduce amount of boilerplate code in the drivers.
>> >>>>
>> >>>> ACPI has other ways of telling the OS that the device is wakeup-capable,
>> >>>> but I guess the property in question can be used too (as long as it is
>> >>>> consistent with the other methods).
>> >>>
>> >>> I was thinking that down the road ACPI can wire its internal wakeup
>> >>> knowledge into generic device property. Unfortunately at the moment
>> >>> most drivers go like this:
>> >>
>> >> That is not really going to happen any time soon AFAICS.
>> >>
>> >>> - if it is device tree platform: good, we'll take the data from device
>> >>> tree property and set up driver behavior accordingly;
>> >>> - if it is legacy board setup: OK, take for driver-specific platform data;
>> >>> - ACPI... hmm... dunno... enable wakeup and if it is not correct the
>> >>> system will just not wake up, not the best but not terribly wrong
>> >>> either.
>> >>
>> >> First, they shouldn't need to do the last part on ACPI systems (where
>> >> they should be belong to the ACPI PM domain), or can look up the
>> >> relevant information in the devices' ACPI companions.
>> >
>> > That is the point. We do not want to look into ACPI components, or OF,
>> > or platform data. We want to have unified way of setting the device as
>> > wakeup capable and enabled, preferably by the bus or device core.
>>
>> On a properly set up ACPI system, the ACPI layer should take care of this.
>
> You mean marking device as wakeup-capable? Good, then this patch will
> likely be a noop on ACPI systems. However can you please point me how we
> do that, for example, for I2C devices instantiated via ACPI?

First, acpi_bind_one() associates the device with its ACPI companion
(this happens during its registration via the ->platform_notify
callback).  In the last step it looks at the ACPI companion's flags
and calls device_set_wakeup_capable(dev, true) if wakeup support is
indicated.

Next, i2c_device_probe() -> dev_pm_domain_attach() ->
acpi_dev_pm_attach() adds the device to acpi_general_pm_domain which
has appropriate PM callbacks that take care of setting up wakeup among
other things.

Caveat: this generally won't work on HW-reduced ACPI systems where
there are no GPEs.

>>
>> The driver only has to look at device_may_wakeup() and do some
>> device-specific setup (if necessary) for system wakeup during system
>> suspend if that returns 'true'.
>
> Right.
>
>>
>> IMO, on a non-ACPI system it should work exactly the same way.  That
>> is, the platform/board code should take care of whatever needs to be
>> done except for the device-specific setup.
>
> Yes and no. Like with PM domains, we now have generic property API (that
> is implemented by platform code in platform-specific way) so we can just
> query necessary properties in needed places, such as various buses code.

Well, say platform A has a separate dedicated wakeup IRQ line for each
device and platform B uses in-band interrupts for system wakeup with
all devices.

What you're saying seems to mean that the exact interpretation of
"wakeup-capable" would depend on the platform in question.

>> >> Second, I'm quite unsure about the "wakeup-capable" property, because
>> >> I'm not sure what it is really supposed to mean.
>> >
>> > On OF system it means that platform integrator decided that this
>> > device is supposed to be waking up the particular system and the
>> > platform and device driver should perform whatever steps are necessary
>> > to wake up the system.
>>
>> This really sounds like "I wish the device woke up the system and you
>> go figure out how to make that happen".  Not very useful to me, to be
>> honest.
>
> No, not really. It is like "I want this device to wake up system and
> driver is supposed to know how to configure this device for wakeup".

How is the driver supposed to know that in practice?  Is every driver
supposed to contain some platform knowledge about every platform it is
intended to work with?

> Right now drivers parse this attribute themselves, I'd like to centralize it.

Oh I'm all for unification of things, but in a way I can understand. :-)

Device wakeup setup necessarily consists of two parts, device-specific
and platform-specific or bus-specific (for buses whose protocols
define that).  On ACPI platforms (full HW ACPI at least) the last bit
is taken care of by ACPI.  On non-ACPI platforms there are two ways
this may be addressed.  First, there may be a wakeup setup method
described by platform firmware or equivalent, like "in-band device
interrupt should be used for system wakeup".  Second, there may be
platform-specific code in the kernel that will know how to carry out
the thing.

In the second case "wakeup-capable" may be interpreted as "the kernel
has some code which knows how to set up that thing and it should work
for this device".  In the first case it won't be sufficient, though,
so I'm guessing that we're talking about the second case?

>> Let me repeat what I said below: Without specifying how to set up the
>> device for system wakeup, using the "wakeup-capable" property is
>> meaningless.  However, if it is specified how to do that, the property
>> is simply redundant, because the wakeup capability obviously follows
>> from the fact that there is a way to set up the device for system
>> wakeup.
>
> It is not. You need to tell the driver that on given board:
>
> 1. Wakeup by given device can be enabled (hardware is wired properly)

The information that the hardware is wired up properly is something
that's good to know, but insufficient if you need to know *how* to
make the wakeup actually happen.  However, if you are told "this is
how to make the wakeup happen", then you know that the hardware is
wired up properly too.  Which is exactly my point.

> 2. Wakeup by given device should be enabled

I'm reading this as "If you're unsure what the default should be,
system wakeup enabled is the right choice".  Fair enough.

> On both OF and static boards there usually no separate annotations for
> the above, they all rolled into single "wakeup-source" property.
>
>> > The same behavior we also have with drivers
>> > supporting legacy platform data (usually expressed as pdata->wakeup).
>> > ACPI is an outlier here.
>>
>> Which doesn't mean that it is wrong and everybody else is right.
>
> From the driver's point of view it is wrong in the sense that it makes
> them implement ACPI-specific behavior and code paths.

Whereas they don't have to that.  All of it is supposed to be transparent.

>> >> For the suspend-to-idle case all devices that can generate interrupts
>> >> can potentially wake up the system if set up for that during suspend.
>> >
>> > You need to tell the driver to not put the device in "too deep" sleep
>> > mode though so the interrupts can still be generated.
>>
>> Checking device_may_wakeup() should help here, though.
>
> And that's what everyone does. But somebody *has to set* this flag.

On what basis?

If the kernel has some platform-specific code that knows how to carry
out platform-specific parts of wakuep preparation, the property should
really be parsed by that code and not by the core, because the core a
priori doesn't know whether or not that code is present and really
working.

If the kernel doesn't have any platform-specific code like that, the
property also should specify how the platform-specific parts of wakeup
preparation are supposed to be carried out.

>> >> For deeper system sleep modes (in which power is removed from
>> >> interrupt controllers) there needs to be a special way to set up the
>> >> device for system wakeup that should be described by the firmware.
>> >> Without that, saying that the device is "wakeup-capable" alone is
>> >> pretty much meaningless.
>> >
>> > Right, and most of the drivers have the following in their PM code paths:
>> >
>> > if (device_wakeup_enabled(dev)) {
>> >     ...
>> >     enable driver's "light" sleep
>> >     ...
>> >     enable_irq_wake();
>> > } else {
>> >    ...
>> >    enable deep sleep
>> >    ...
>> > }
>>
>> OK, so this appears to indicate that the "wakeup-capable" property
>> really is supposed to mean "the in-band interrupt of this device can
>> wake up the system if set up properly", which is far more precise than
>> the interpretation of it mentioned so far.  Is that the case?
>
> No, not really. It does not say anything about in band interrupt or
> anything else. It depends on the driver.

How so?  The driver should be able to run on multiple platforms with
different ways to set up system wakeup.

>> >
>> > or something similar. The point is that at this time we do not check
>> > for ACPI, or DT, or whatever but rely on single call to
>> > device_may_wakeup().
>>
>> Well, device_may_wakeup() need not imply that you're supposed to set
>> up the in-band interrupt of the device for system wakeup, really.
>
> I never said it did imply that.

The piece of pseudo code above indicates that.  Or that there is a
dedicated wakeup interrupt for the device which the driver knows about
in some magical way.

>> >>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >>>>> ---
>> >>>>>  drivers/base/platform.c | 9 ++++++++-
>> >>>>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> >>>>>
>> >>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> >>>>> index 1dd6d3b..d14071a 100644
>> >>>>> --- a/drivers/base/platform.c
>> >>>>> +++ b/drivers/base/platform.c
>> >>>>> @@ -514,9 +514,14 @@ static int platform_drv_probe(struct device *_dev)
>> >>>>>
>> >>>>>       ret = dev_pm_domain_attach(_dev, true);
>> >>>>>       if (ret != -EPROBE_DEFER && drv->probe) {
>> >>>>> +             bool wakeup = device_property_read_bool(_dev, "wakeup-source");
>> >>>>> +
>> >>>>> +             device_init_wakeup(_dev, wakeup);
>> >>>>
>> >>>> But I'm wondering if this should be device_set_wakeup_capable(dev, true) rather?
>> >>>>
>> >>>> device_init_wakeup() additionally sets the default in sysfs to "do wakeup"
>> >>>> which in principle may unblock spurious wakeups on some systems.
>> >>>
>> >>> Why would we not want enable wakeup for devices where system
>> >>> (platform) tells us that they are wakeup sources?
>> >>
>> >> Because user space may not want to use them for system wakeup?
>> >
>> > Then userspace will disable it. The point is that platform integrator
>> > intended for the device to be wakeup source and so it should be
>> > enabled by default for such devices.
>>
>> Whatever the system integrator intended need not matter to the driver
>> core
>
> Huh? Should we ignore ACPI-provided interrupt and address assignments?
> Because they are set by platform integrators.

They are configuration data, not information on what defaults to use
for certain features.

Also we sometimes ignore them too if we have better ways to figure things out.

>>  and can you actually guarantee that "wakeup-capable" will always
>> be used correctly by all platform firmware?
>
> Can you guarantee that all ACPI code ever produced is perfect and doe
> snot do stupid things?

Obviously not and I really only want it to be used where there are no
other means to gather the necessary information, exactly for this
reason.

>> And what about devices that generate spurious wakeups that will be
>> magically enabled by this change?
>
> Maybe that could happen on ACPI systems, but see below.

Oh it does happen there.

>> >>> This is how most of the drivers I am involved in behave. All of them use
>> >>> device_init_wakeup() and then we adjust the behavior depending on
>> >>> runtime state, i.e. do not wake up from touchpad when lid is closed
>> >>> (which is useful if lid is flexible and may interact with touchpad if
>> >>> slammed down hard enough).
>> >>
>> >> Plus there is the sysfs attribute that needs to be taken into account
>> >> (so user space can disable wakeup devices it doesn't want to be used).
>> >
>> > It is already taken into account as drivers are supposed to be using
>> > device_may_wakeup() to check if wakeup is enabled at time of power
>> > state transition.
>> >
>> >> But it is fine if device_init_wakeup() is used by a driver from the
>> >> start (which only means that its default value for the above-mentioned
>> >> sysfs attribute is "enabled"), but using that in the core may change
>> >> the default for some drivers and lead to problems.
>> >
>> > I am not aware of any drivers denoting itself as being wakeup sources
>> > and not enabling wakeup. Do you have examples?
>>
>> Yes, I do.
>>
>> i8042 is one of them, exactly to avoid enabling those devices to do
>> system wakeup on systems where they were not enabled to do that in the
>> past.
>
> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
> automatically enabling wakeup on ACPI systems because that was the
> default behavior for them. OF/board platforms have different default
> behavior. I guess if we do not do anything besides what the patch is
> doing, then ACPI will not have the property I propose, so they will be
> capable but not enabled, and OF/boards will have the property and will
> be capable and enabled.

The defaults to use should generally depend on two things IMO: On what
the firmware tells us to use (given the argument that the firmware
people probably have a good reason to tell us what they are telling
us) and on what we were doing for this class of devices in the past.
Ignoring the last bit generally leads to regressions in the cases when
the firmware people are wrong after all, so I'm always cautious about
this.  And this rule applies to DT as well as to ACPI, although the DT
people usually won't admit it. :-)


Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Wed, Jan 20, 2016 at 3:40 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:

[cut]

>>> > I am not aware of any drivers denoting itself as being wakeup sources
>>> > and not enabling wakeup. Do you have examples?
>>>
>>> Yes, I do.
>>>
>>> i8042 is one of them, exactly to avoid enabling those devices to do
>>> system wakeup on systems where they were not enabled to do that in the
>>> past.
>>
>> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
>> automatically enabling wakeup on ACPI systems because that was the
>> default behavior for them. OF/board platforms have different default
>> behavior. I guess if we do not do anything besides what the patch is
>> doing, then ACPI will not have the property I propose, so they will be
>> capable but not enabled, and OF/boards will have the property and will
>> be capable and enabled.
>
> The defaults to use should generally depend on two things IMO: On what
> the firmware tells us to use (given the argument that the firmware
> people probably have a good reason to tell us what they are telling
> us) and on what we were doing for this class of devices in the past.
> Ignoring the last bit generally leads to regressions in the cases when
> the firmware people are wrong after all, so I'm always cautious about
> this.  And this rule applies to DT as well as to ACPI, although the DT
> people usually won't admit it. :-)

OK, I realize that the example I gave was not really a good one,
because the wakeup part was added to i8042 to support wakeup from
suspend-to-idle which is really special and the implementation is
based on what the driver was doing previously.

But this also makes me think that the problem is really more
complicated than it may seem to be, so what about starting over and
looking at the wakeup problem in general?

To that end, there are two categories of wakeup support in devices.
The first one is support for something called "remote wakeup" in USB
and which means that the device is able to generate wakeup signals
when it (the device) is suspended and the rest of the system can
generally be considered as working.  I'll use the "remote wakeup" term
in general for the lack of a better one.

Remote wakeup support is used in runtime PM and suspend-to-idle (which
essentially uses the same kind of hardware mechanics, but in a
different way).

Note, though, that "device is suspended" need not map 1:1 onto a
particular hardware state.  What it really means is that either it has
been suspended using the runtime PM framework, or all devices have
gone through the device suspend sequence during suspend-to-idle.  The
hardware state the device ends up in depends on the driver/bus type/PM
domain combination and is generally expected to be low-power.

It is easy in PCI or USB and on ACPI systems where low-power states of
devices are well defined and the connection between them and the
ability to generate wakeup signals is known.  It is not so easy in the
other cases, though.  My personal opinion is that if wakeup support is
required, the device should be put into the lowest-power state from
which it can generate wakeup signals.  That very well may be the
full-power state of it if that's the only suitable one.

If that point of view is adopted, then any device that (a) can take
input and (b) uses interrupts can do remote wakeup.  We don't really
have a good way to express that in the driver core.

The second category of wakeup support is for platform-assisted
low-power states of the whole system where there's a big switch (or a
bunch of them) allowing multiple things to be powered off in one go
from the OS perspective.

I'll write about that in the next message, as I need to take care of
some urgent stuff now.

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

On Wed, Jan 20, 2016 at 2:51 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jan 20, 2016 at 3:40 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>
> [cut]
>
>>>> > I am not aware of any drivers denoting itself as being wakeup sources
>>>> > and not enabling wakeup. Do you have examples?
>>>>
>>>> Yes, I do.
>>>>
>>>> i8042 is one of them, exactly to avoid enabling those devices to do
>>>> system wakeup on systems where they were not enabled to do that in the
>>>> past.
>>>
>>> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
>>> automatically enabling wakeup on ACPI systems because that was the
>>> default behavior for them. OF/board platforms have different default
>>> behavior. I guess if we do not do anything besides what the patch is
>>> doing, then ACPI will not have the property I propose, so they will be
>>> capable but not enabled, and OF/boards will have the property and will
>>> be capable and enabled.
>>
>> The defaults to use should generally depend on two things IMO: On what
>> the firmware tells us to use (given the argument that the firmware
>> people probably have a good reason to tell us what they are telling
>> us) and on what we were doing for this class of devices in the past.
>> Ignoring the last bit generally leads to regressions in the cases when
>> the firmware people are wrong after all, so I'm always cautious about
>> this.  And this rule applies to DT as well as to ACPI, although the DT
>> people usually won't admit it. :-)
>
> OK, I realize that the example I gave was not really a good one,
> because the wakeup part was added to i8042 to support wakeup from
> suspend-to-idle which is really special and the implementation is
> based on what the driver was doing previously.

Which is not to say that there are no good (or at least better) examples.

USB HID devices pretty much universally support remote wakeup
officially, but enough of them cause problems to happen while using it
(spurious wakeups etc) that we don't enable them for system wakeup by
default.  Plus some of them are cordless mice and such and you really
need to be careful to avoid waking up a sleeping system by accident
with them (when enabled).

Ethernet NICs support wakeup signaling, but at least some of them are
not enabled for system wakeup by default.  Moreover, there are
multiple ways to trigger the wakeup there that the user needs to
choose from in the first place.

Some wireless adapters support WoW, but enabling them to wake up the
system from sleep by default would be asking for troubles.

All in all, combining the information that the device can signal
wakeup with the requirement to enable it to wake up the system from
sleep states by default doesn't look like a good idea to me.

> But this also makes me think that the problem is really more
> complicated than it may seem to be, so what about starting over and
> looking at the wakeup problem in general?
>
> To that end, there are two categories of wakeup support in devices.
> The first one is support for something called "remote wakeup" in USB
> and which means that the device is able to generate wakeup signals
> when it (the device) is suspended and the rest of the system can
> generally be considered as working.  I'll use the "remote wakeup" term
> in general for the lack of a better one.
>
> Remote wakeup support is used in runtime PM and suspend-to-idle (which
> essentially uses the same kind of hardware mechanics, but in a
> different way).
>
> Note, though, that "device is suspended" need not map 1:1 onto a
> particular hardware state.  What it really means is that either it has
> been suspended using the runtime PM framework, or all devices have
> gone through the device suspend sequence during suspend-to-idle.  The
> hardware state the device ends up in depends on the driver/bus type/PM
> domain combination and is generally expected to be low-power.
>
> It is easy in PCI or USB and on ACPI systems where low-power states of
> devices are well defined and the connection between them and the
> ability to generate wakeup signals is known.  It is not so easy in the
> other cases, though.  My personal opinion is that if wakeup support is
> required, the device should be put into the lowest-power state from
> which it can generate wakeup signals.  That very well may be the
> full-power state of it if that's the only suitable one.
>
> If that point of view is adopted, then any device that (a) can take
> input and (b) uses interrupts can do remote wakeup.  We don't really
> have a good way to express that in the driver core.
>
> The second category of wakeup support is for platform-assisted
> low-power states of the whole system where there's a big switch (or a
> bunch of them) allowing multiple things to be powered off in one go
> from the OS perspective.
>
> I'll write about that in the next message, as I need to take care of
> some urgent stuff now.

Continuing.

If the device in question belongs to the part of the system powered
off by the big switch, it has to be provided with some special
"wakeup" power source so it can generate signals.  There needs to be a
physical line (or bus or similar) that will be activated when wakeup
is signaled by the device.  The activation of it needs to be noticed
by something and turned into a signal that eventually will wake up the
CPU (or make it start executing a power-up sequence or equivalent).
All of that generally requires specific setup.

The device has to be left in a state in which it can use the "wakeup"
power source in the first place.  The "wakeup" power source for it has
to be turned on.  The signal "line" needs to be prepared for
activation by the device's wakeup signals.  The part of the system
responsible for waking up the CPU when that "line" is active has to be
prepared too.  All this means that generally it is not enough to say
"things are wired up properly" for the right stuff to happen.

Traditionally, we set the can_wakeup bit if (a) there is an interface
we can use to set up those things (either by accessing some registers
available to us or by invoking the platform firmware to do it) and (b)
the device is hooked up to that interface in a known way.  IOW, that
bit is inferred from what we can find out about the device's
configuration rather that just taken from the firmware for granted.

Now, this generally may not be the right approach.

Maybe we just don't need the can_wakeup bit at all.  Maybe we should
just create the "wakeup" sysfs attribute for all devices, let user
space set it the way it wants and handle it on the best effort basis.
That is, it will wake up from the sleep states it can wake up from,
but it may not wake up from any of them if there's no support.
Granted, we really can't guarantee that it will work anyway (even if
the firmware exposes the interface to us, it may not be correctly
implemented etc and there's no way to verify that upfront).  And we
may allow the same attribute to be used for disabling remote wakeup
for runtime PM if someone wants to.

In that case, though, we really won't need the firmware to tell us
whether or not the device is "wakeup-capable".  We will always treat
it this way and if it turns out that something is missing, wakeup will
just not work.

Thanks,
Rafael

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Dmitry Torokhov]

On Thu, Jan 21, 2016 at 12:01:59AM +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 20, 2016 at 2:51 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jan 20, 2016 at 3:40 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >> On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
> >> <dmitry.torokhov@gmail.com> wrote:
> >
> > [cut]
> >
> >>>> > I am not aware of any drivers denoting itself as being wakeup sources
> >>>> > and not enabling wakeup. Do you have examples?
> >>>>
> >>>> Yes, I do.
> >>>>
> >>>> i8042 is one of them, exactly to avoid enabling those devices to do
> >>>> system wakeup on systems where they were not enabled to do that in the
> >>>> past.
> >>>
> >>> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
> >>> automatically enabling wakeup on ACPI systems because that was the
> >>> default behavior for them. OF/board platforms have different default
> >>> behavior. I guess if we do not do anything besides what the patch is
> >>> doing, then ACPI will not have the property I propose, so they will be
> >>> capable but not enabled, and OF/boards will have the property and will
> >>> be capable and enabled.
> >>
> >> The defaults to use should generally depend on two things IMO: On what
> >> the firmware tells us to use (given the argument that the firmware
> >> people probably have a good reason to tell us what they are telling
> >> us) and on what we were doing for this class of devices in the past.
> >> Ignoring the last bit generally leads to regressions in the cases when
> >> the firmware people are wrong after all, so I'm always cautious about
> >> this.  And this rule applies to DT as well as to ACPI, although the DT
> >> people usually won't admit it. :-)
> >
> > OK, I realize that the example I gave was not really a good one,
> > because the wakeup part was added to i8042 to support wakeup from
> > suspend-to-idle which is really special and the implementation is
> > based on what the driver was doing previously.
> 
> Which is not to say that there are no good (or at least better) examples.
> 
> USB HID devices pretty much universally support remote wakeup
> officially, but enough of them cause problems to happen while using it
> (spurious wakeups etc) that we don't enable them for system wakeup by
> default.  Plus some of them are cordless mice and such and you really
> need to be careful to avoid waking up a sleeping system by accident
> with them (when enabled).
> 
> Ethernet NICs support wakeup signaling, but at least some of them are
> not enabled for system wakeup by default.  Moreover, there are
> multiple ways to trigger the wakeup there that the user needs to
> choose from in the first place.
> 
> Some wireless adapters support WoW, but enabling them to wake up the
> system from sleep by default would be asking for troubles.
> 
> All in all, combining the information that the device can signal
> wakeup with the requirement to enable it to wake up the system from
> sleep states by default doesn't look like a good idea to me.

You are making a convincing argument here for having an option to allow
specifying that device is just wakeup capable, but not enabled by
default. For ACPI you have your own ways of defining whether the device
is wakeup capable (and you also have some rules to mark certain devices
as wakeup enabled by default - button GPEs for example), so that leaves
DT and static board files where historically we enabled wakeup for
platform/I2C/SPI devices that board believes are wakeup-capable.

I wonder if I should do that property parsing in genpd code, similarly
to what you are doing in ACPI power domain. What does not help is that
we have different notions of power domain at once - one where device is
under control of ACPI and another that denotes grouping of devices
together.

> 
> > But this also makes me think that the problem is really more
> > complicated than it may seem to be, so what about starting over and
> > looking at the wakeup problem in general?
> >
> > To that end, there are two categories of wakeup support in devices.
> > The first one is support for something called "remote wakeup" in USB
> > and which means that the device is able to generate wakeup signals
> > when it (the device) is suspended and the rest of the system can
> > generally be considered as working.  I'll use the "remote wakeup" term
> > in general for the lack of a better one.
> >
> > Remote wakeup support is used in runtime PM and suspend-to-idle (which
> > essentially uses the same kind of hardware mechanics, but in a
> > different way).
> >
> > Note, though, that "device is suspended" need not map 1:1 onto a
> > particular hardware state.  What it really means is that either it has
> > been suspended using the runtime PM framework, or all devices have
> > gone through the device suspend sequence during suspend-to-idle.  The
> > hardware state the device ends up in depends on the driver/bus type/PM
> > domain combination and is generally expected to be low-power.
> >
> > It is easy in PCI or USB and on ACPI systems where low-power states of
> > devices are well defined and the connection between them and the
> > ability to generate wakeup signals is known.  It is not so easy in the
> > other cases, though.  My personal opinion is that if wakeup support is
> > required, the device should be put into the lowest-power state from
> > which it can generate wakeup signals.  That very well may be the
> > full-power state of it if that's the only suitable one.
> >
> > If that point of view is adopted, then any device that (a) can take
> > input and (b) uses interrupts can do remote wakeup.  We don't really
> > have a good way to express that in the driver core.
> >
> > The second category of wakeup support is for platform-assisted
> > low-power states of the whole system where there's a big switch (or a
> > bunch of them) allowing multiple things to be powered off in one go
> > from the OS perspective.
> >
> > I'll write about that in the next message, as I need to take care of
> > some urgent stuff now.
> 
> Continuing.
> 
> If the device in question belongs to the part of the system powered
> off by the big switch, it has to be provided with some special
> "wakeup" power source so it can generate signals.  There needs to be a
> physical line (or bus or similar) that will be activated when wakeup
> is signaled by the device.  The activation of it needs to be noticed
> by something and turned into a signal that eventually will wake up the
> CPU (or make it start executing a power-up sequence or equivalent).
> All of that generally requires specific setup.
> 
> The device has to be left in a state in which it can use the "wakeup"
> power source in the first place.  The "wakeup" power source for it has
> to be turned on.  The signal "line" needs to be prepared for
> activation by the device's wakeup signals.  The part of the system
> responsible for waking up the CPU when that "line" is active has to be
> prepared too.  All this means that generally it is not enough to say
> "things are wired up properly" for the right stuff to happen.

There are usually 2 components to this, one from device/driver
perspective and another from system perspective. From the device
perspective there is not difference between "remote wakeup" and "big
switch" wakeup. In both cases the driver should put the device into
lowest power state that still allows it to react to external inputs.
Putting the device into that power state may also affect other devices
it is connected to, for example it may request to power down some
regulators and/or stop some clocks if they are not needed in that low
power mode. When there is activity the device sends signal (could be
dedicated interrupt pin or in-bound interrupt pin - this is device
specific) and how they are wired is board-specific.

>From the system perspective it provides notion of power domains as in
grouping (so that we know when we can turn off certain supplies and what
parts of the system should stay powered up even when system transitions
into lower power state), performs PMU set up (the bit that actually sets
up the system to process wake sources and it is PMU specific and
board-specific and is usually expressed via various DT properties, both
on PMU node and individual device nodes) and it does know how wakeup
signals are set up. In the past most of the drivers assumed that in-band
IRQ is the one that should wake up the system, now in I2C bus we added
OF "wakeup-interrupts" that can be used to set up a dedicated interrupt
as wakeup irq, otherwise we set client's in-band irq (client->irq) as
wakeup source (provided that device is annotated as "wakeup-source").
For platform devices this does not quite work as we do not know how many
interrupts they are using.

I think we are talking basically about the same thing, just in different
terms. I do not expect that by simply marking device as "wakeup-source"
it will magically wake up my system, but there is normally support for
wakeup support in both driver and platform code that actually does the
right thing when requested via this property. The property _has_ to be
documented in binding for the driver, which ensures driver-side support
is present, and making sure that system part (power domain, PMIC, PMU)
are set up appropriately is system integrators task, no different from
the task of providing appropriate BIOS/ACPI tables for system
integrators creating X86 boxes.

>
> Traditionally, we set the can_wakeup bit if (a) there is an interface
> we can use to set up those things (either by accessing some registers
> available to us or by invoking the platform firmware to do it) and (b)
> the device is hooked up to that interface in a known way.  IOW, that
> bit is inferred from what we can find out about the device's
> configuration rather that just taken from the firmware for granted.

It _really_ depends on the device. Some of them are non-discoverable and
thus you need to trust firmware. For example for I2C device you need to
know interrupt number, you can't normally query device for it.

> 
> Now, this generally may not be the right approach.
> 
> Maybe we just don't need the can_wakeup bit at all.  Maybe we should
> just create the "wakeup" sysfs attribute for all devices, let user
> space set it the way it wants and handle it on the best effort basis.
> That is, it will wake up from the sleep states it can wake up from,
> but it may not wake up from any of them if there's no support.
> Granted, we really can't guarantee that it will work anyway (even if
> the firmware exposes the interface to us, it may not be correctly
> implemented etc and there's no way to verify that upfront).  And we
> may allow the same attribute to be used for disabling remote wakeup
> for runtime PM if someone wants to.
> 
> In that case, though, we really won't need the firmware to tell us
> whether or not the device is "wakeup-capable".  We will always treat
> it this way and if it turns out that something is missing, wakeup will
> just not work.

This is a non-starter. Quite often there is difference in power
consumption between deep sleep/powered off state and state in which
device can signal activity so that the system (or subsystem) can wake
up. So if firmware knows that device is not wired to wake up the system
we better communicate it to kernel/userspace instead of them having to
cross their fingers and hope for the best.

Thanks.

-- 
Dmitry

Re: [PATCH] driver-core: platform: automatically mark wakeup devices

[Rafael J. Wysocki]

Sorry about the delay, I'm traveling ATM.

On Thu, Jan 21, 2016 at 1:23 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jan 21, 2016 at 12:01:59AM +0100, Rafael J. Wysocki wrote:
>> On Wed, Jan 20, 2016 at 2:51 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> > On Wed, Jan 20, 2016 at 3:40 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> >> On Wed, Jan 20, 2016 at 1:45 AM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >
>> > [cut]
>> >
>> >>>> > I am not aware of any drivers denoting itself as being wakeup sources
>> >>>> > and not enabling wakeup. Do you have examples?
>> >>>>
>> >>>> Yes, I do.
>> >>>>
>> >>>> i8042 is one of them, exactly to avoid enabling those devices to do
>> >>>> system wakeup on systems where they were not enabled to do that in the
>> >>>> past.
>> >>>
>> >>> Ah, yes, you added it to i8042. Well, maybe that is an argument for not
>> >>> automatically enabling wakeup on ACPI systems because that was the
>> >>> default behavior for them. OF/board platforms have different default
>> >>> behavior. I guess if we do not do anything besides what the patch is
>> >>> doing, then ACPI will not have the property I propose, so they will be
>> >>> capable but not enabled, and OF/boards will have the property and will
>> >>> be capable and enabled.
>> >>
>> >> The defaults to use should generally depend on two things IMO: On what
>> >> the firmware tells us to use (given the argument that the firmware
>> >> people probably have a good reason to tell us what they are telling
>> >> us) and on what we were doing for this class of devices in the past.
>> >> Ignoring the last bit generally leads to regressions in the cases when
>> >> the firmware people are wrong after all, so I'm always cautious about
>> >> this.  And this rule applies to DT as well as to ACPI, although the DT
>> >> people usually won't admit it. :-)
>> >
>> > OK, I realize that the example I gave was not really a good one,
>> > because the wakeup part was added to i8042 to support wakeup from
>> > suspend-to-idle which is really special and the implementation is
>> > based on what the driver was doing previously.
>>
>> Which is not to say that there are no good (or at least better) examples.
>>
>> USB HID devices pretty much universally support remote wakeup
>> officially, but enough of them cause problems to happen while using it
>> (spurious wakeups etc) that we don't enable them for system wakeup by
>> default.  Plus some of them are cordless mice and such and you really
>> need to be careful to avoid waking up a sleeping system by accident
>> with them (when enabled).
>>
>> Ethernet NICs support wakeup signaling, but at least some of them are
>> not enabled for system wakeup by default.  Moreover, there are
>> multiple ways to trigger the wakeup there that the user needs to
>> choose from in the first place.
>>
>> Some wireless adapters support WoW, but enabling them to wake up the
>> system from sleep by default would be asking for troubles.
>>
>> All in all, combining the information that the device can signal
>> wakeup with the requirement to enable it to wake up the system from
>> sleep states by default doesn't look like a good idea to me.
>
> You are making a convincing argument here for having an option to allow
> specifying that device is just wakeup capable, but not enabled by
> default. For ACPI you have your own ways of defining whether the device
> is wakeup capable (and you also have some rules to mark certain devices
> as wakeup enabled by default - button GPEs for example), so that leaves
> DT and static board files where historically we enabled wakeup for
> platform/I2C/SPI devices that board believes are wakeup-capable.
>
> I wonder if I should do that property parsing in genpd code, similarly
> to what you are doing in ACPI power domain.

That would be more suitable IMO.

> What does not help is that
> we have different notions of power domain at once - one where device is
> under control of ACPI and another that denotes grouping of devices
> together.

That's why a "PM domain" really means "a group of devices with
analogous PM handling" rather than "a group of devices with the same
clock or VR" etc.

>>
>> > But this also makes me think that the problem is really more
>> > complicated than it may seem to be, so what about starting over and
>> > looking at the wakeup problem in general?
>> >
>> > To that end, there are two categories of wakeup support in devices.
>> > The first one is support for something called "remote wakeup" in USB
>> > and which means that the device is able to generate wakeup signals
>> > when it (the device) is suspended and the rest of the system can
>> > generally be considered as working.  I'll use the "remote wakeup" term
>> > in general for the lack of a better one.
>> >
>> > Remote wakeup support is used in runtime PM and suspend-to-idle (which
>> > essentially uses the same kind of hardware mechanics, but in a
>> > different way).
>> >
>> > Note, though, that "device is suspended" need not map 1:1 onto a
>> > particular hardware state.  What it really means is that either it has
>> > been suspended using the runtime PM framework, or all devices have
>> > gone through the device suspend sequence during suspend-to-idle.  The
>> > hardware state the device ends up in depends on the driver/bus type/PM
>> > domain combination and is generally expected to be low-power.
>> >
>> > It is easy in PCI or USB and on ACPI systems where low-power states of
>> > devices are well defined and the connection between them and the
>> > ability to generate wakeup signals is known.  It is not so easy in the
>> > other cases, though.  My personal opinion is that if wakeup support is
>> > required, the device should be put into the lowest-power state from
>> > which it can generate wakeup signals.  That very well may be the
>> > full-power state of it if that's the only suitable one.
>> >
>> > If that point of view is adopted, then any device that (a) can take
>> > input and (b) uses interrupts can do remote wakeup.  We don't really
>> > have a good way to express that in the driver core.
>> >
>> > The second category of wakeup support is for platform-assisted
>> > low-power states of the whole system where there's a big switch (or a
>> > bunch of them) allowing multiple things to be powered off in one go
>> > from the OS perspective.
>> >
>> > I'll write about that in the next message, as I need to take care of
>> > some urgent stuff now.
>>
>> Continuing.
>>
>> If the device in question belongs to the part of the system powered
>> off by the big switch, it has to be provided with some special
>> "wakeup" power source so it can generate signals.  There needs to be a
>> physical line (or bus or similar) that will be activated when wakeup
>> is signaled by the device.  The activation of it needs to be noticed
>> by something and turned into a signal that eventually will wake up the
>> CPU (or make it start executing a power-up sequence or equivalent).
>> All of that generally requires specific setup.
>>
>> The device has to be left in a state in which it can use the "wakeup"
>> power source in the first place.  The "wakeup" power source for it has
>> to be turned on.  The signal "line" needs to be prepared for
>> activation by the device's wakeup signals.  The part of the system
>> responsible for waking up the CPU when that "line" is active has to be
>> prepared too.  All this means that generally it is not enough to say
>> "things are wired up properly" for the right stuff to happen.
>
> There are usually 2 components to this, one from device/driver
> perspective and another from system perspective. From the device
> perspective there is not difference between "remote wakeup" and "big
> switch" wakeup. In both cases the driver should put the device into
> lowest power state that still allows it to react to external inputs.

Moreover, in cases like ACPI and PCI devices are not put into
low-power states by drivers even.  Rather, bus types and/or PM domains
do that then, so there really is no difference from the perspective of
what the driver is expected to do.

> Putting the device into that power state may also affect other devices
> it is connected to, for example it may request to power down some
> regulators and/or stop some clocks if they are not needed in that low
> power mode. When there is activity the device sends signal (could be
> dedicated interrupt pin or in-bound interrupt pin - this is device
> specific) and how they are wired is board-specific.
>
> From the system perspective it provides notion of power domains as in
> grouping (so that we know when we can turn off certain supplies and what
> parts of the system should stay powered up even when system transitions
> into lower power state), performs PMU set up (the bit that actually sets
> up the system to process wake sources and it is PMU specific and
> board-specific and is usually expressed via various DT properties, both
> on PMU node and individual device nodes) and it does know how wakeup
> signals are set up. In the past most of the drivers assumed that in-band
> IRQ is the one that should wake up the system, now in I2C bus we added
> OF "wakeup-interrupts" that can be used to set up a dedicated interrupt
> as wakeup irq, otherwise we set client's in-band irq (client->irq) as
> wakeup source (provided that device is annotated as "wakeup-source").
> For platform devices this does not quite work as we do not know how many
> interrupts they are using.

Right.

> I think we are talking basically about the same thing, just in different
> terms. I do not expect that by simply marking device as "wakeup-source"
> it will magically wake up my system, but there is normally support for
> wakeup support in both driver and platform code that actually does the
> right thing when requested via this property. The property _has_ to be
> documented in binding for the driver, which ensures driver-side support
> is present, and making sure that system part (power domain, PMIC, PMU)
> are set up appropriately is system integrators task, no different from
> the task of providing appropriate BIOS/ACPI tables for system
> integrators creating X86 boxes.

OK, so that answers my question about what "wakeup-source" is supposed
to mean. :-)

The case in question seems to be when the same driver works with
platforms that may or may not wire up the device for wakeup and that
attribute tells it what the case is on the given platform/board.

>>
>> Traditionally, we set the can_wakeup bit if (a) there is an interface
>> we can use to set up those things (either by accessing some registers
>> available to us or by invoking the platform firmware to do it) and (b)
>> the device is hooked up to that interface in a known way.  IOW, that
>> bit is inferred from what we can find out about the device's
>> configuration rather that just taken from the firmware for granted.
>
> It _really_ depends on the device. Some of them are non-discoverable and
> thus you need to trust firmware. For example for I2C device you need to
> know interrupt number, you can't normally query device for it.
>
>>
>> Now, this generally may not be the right approach.
>>
>> Maybe we just don't need the can_wakeup bit at all.  Maybe we should
>> just create the "wakeup" sysfs attribute for all devices, let user
>> space set it the way it wants and handle it on the best effort basis.
>> That is, it will wake up from the sleep states it can wake up from,
>> but it may not wake up from any of them if there's no support.
>> Granted, we really can't guarantee that it will work anyway (even if
>> the firmware exposes the interface to us, it may not be correctly
>> implemented etc and there's no way to verify that upfront).  And we
>> may allow the same attribute to be used for disabling remote wakeup
>> for runtime PM if someone wants to.
>>
>> In that case, though, we really won't need the firmware to tell us
>> whether or not the device is "wakeup-capable".  We will always treat
>> it this way and if it turns out that something is missing, wakeup will
>> just not work.
>
> This is a non-starter. Quite often there is difference in power
> consumption between deep sleep/powered off state and state in which
> device can signal activity so that the system (or subsystem) can wake
> up. So if firmware knows that device is not wired to wake up the system
> we better communicate it to kernel/userspace instead of them having to
> cross their fingers and hope for the best.

OK, I see.

Thanks,
Rafael