Message-ID: <20160120004546.GD31589@dtor-ws>
Date:	Tue, 19 Jan 2016 16:45:46 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	"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 11:18:12PM +0100, Rafael J. Wysocki wrote:
> 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.

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

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

Powered by blists - more mailing lists