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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > 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.
>>
>> 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@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.
>>
>> 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