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

[Dmitry Torokhov]

On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> 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.

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@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?

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