Date:	Mon, 18 Jan 2016 23:18:12 +0100
From:	"Rafael J. Wysocki" <rafael@...nel.org>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	"Rafael J. Wysocki" <rafael@...nel.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Grant Likely <grant.likely@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Thierry Reding <treding@...dia.com>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] driver-core: platform: automatically mark wakeup devices

On Mon, Jan 18, 2016 at 6:55 PM, Dmitry Torokhov
<dmitry.torokhov@...il.com> wrote:
> On Mon, Jan 18, 2016 at 9:19 AM, Rafael J. Wysocki <rafael@...nel.org> wrote:
>> On Mon, Jan 18, 2016 at 5:22 PM, Dmitry Torokhov
>> <dmitry.torokhov@...il.com> wrote:
>>> On Mon, Jan 18, 2016 at 6:47 AM, Rafael J. Wysocki <rjw@...ysocki.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@...il.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

Powered by blists - more mailing lists