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@xxxxxxxxx> wrote:
> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>>  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