Message-ID: <1AE640813FDE7649BE1B193DEA596E883BBB5C64@SHSMSX101.ccr.corp.intel.com>
Date:	Tue, 31 May 2016 02:55:33 +0000
From:	"Zheng, Lv" <lv.zheng@...el.com>
To:	Benjamin Tissoires <benjamin.tissoires@...il.com>
CC:	"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"Brown, Len" <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	"Bastien Nocera:" <hadess@...ess.net>,
	"Valdis.Kletnieks@...edu" <Valdis.Kletnieks@...edu>
Subject: RE: [PATCH v2 3/3] ACPI / button: Send "open" state after
 boot/resume

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires@...il.com]
> Subject: Re: [PATCH v2 3/3] ACPI / button: Send "open" state after
> boot/resume
> 
> On Fri, May 27, 2016 at 9:16 AM, Lv Zheng <lv.zheng@...el.com> wrote:
> > Linux userspace (systemd-logind) keeps on rechecking lid state when the
> > lid state is closed. If it failed to update the lid state to open after
> > boot/resume, the system suspending right after the boot/resume could
> be
> > resulted.
> > Graphics drivers also uses the lid notifications to implment
> > MODESET_ON_LID_OPEN option.
> >
> > Before the situation is improved from the userspace and from the
> graphics
> > driver, simply send initial "open" lid state to avoid issues. After this is
> > improved from the userspace and from the graphics driver, Linux kernel
> > could simply revert this minimal commit.
> >
> > Link 1: https://lkml.org/2016/3/7/460
> > Link 2: https://github.com/systemd/systemd/issues/2087
> > Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> > Cc: Bastien Nocera: <hadess@...ess.net>
> > ---
> >  drivers/acpi/button.c |    3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index e706e4b..6e77312 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -342,6 +342,8 @@ static int acpi_button_resume(struct device
> *dev)
> >         struct acpi_button *button = acpi_driver_data(device);
> >
> >         button->suspended = false;
> > +       if (button->type == ACPI_BUTTON_TYPE_LID)
> > +               return acpi_lid_notify_state(device, 1);
> 
> As Valdis replied on 0/3, I don't think this is a good solution (even
> temporary). Linux should not assume the current state of a input
> device, and sending unconditionally 1 here is wrong. If the device is
> on a docking station, you will wake up the wrong monitor and screw the
> user session (and this will be a regression).
[Lv Zheng] 
We are doing the test to see how this behaves on several different platforms.

> 
> How about we simply send the current LID state stored in the ACPI?
> something like calling acpi_lid_send_state() directly?
[Lv Zheng] 
This is what we are going to eliminate in [PATCH 01].
We have several real bugs related to sending a wrong state to the userspace.
Userspace will suspend right after resume because of the 'close' state.


> 
> [15 min later, after writing a long email]
> 
> Well, it looks like we already have that in the kernel and for a long
> time apparently.
> 
> So, I think the issue is that Microsoft does not wake up the system by
> lid opening (seen in one of the comments in the mentioned bugs and by
> looking at the behavior on the surface devices). It must be just
> querying it's state on resume or might even not care at all until it
> receives a close event.
> 
> If I read correctly, we managed to get the 3 bogus devices to a
> correct state at the ACPI level (/proc/acpi/button/lid/LID0/state),
> but we did not get the notification. Given that the LID state is
> triggered by an ACPI operation region, there is no guarantee that the
> resume of the acpi/button.c driver will be called after the region has
> been updated/called.
[Lv Zheng] 
The understanding here is incorrect.
We have 3 bogus devices.
1 of them is surface 3 which is a hardware reduced platform.
The others are all traditional platforms.

=====
The facts are:

Both the platforms return cached lid state from _LID.
The cached value will be updated by lid irq (via GPIO IRQ, GPE, or EC event).
AML tables will send lid notification in the irq handler.

Some AML tables will update the cached value in _WAK (I'll describe why it is necessary below).
But updating the cached value in _WAK is not guaranteed by all AML tables.

For the 'close' state irq, all tables will send lid close notification.
For the 'open' state irq, it seems there are tables never sending lid open notification (sounds like Windows do not care about lid open).
=====

Surface 3 is entirely a different case.
It is a runtime idle system and hardware reduced.
On that kind of system, lid open is handled by OS not by BIOS.
Surface 3 is exactly the platform that doesn't send lid open notification.
I guess the AML is intentionally written in this way to be compliant to the traditional platforms.

While on the traditional platforms:
When lid is opened, BIOS handles the lid irq and wakes the system from the FACS waking vector.
So it is likely that there is no lid open irq after the system is resumed.
BIOS may forget to update the cached lid value in the _WAK or some other control methods that could be executed after resuming.
Then if we send _LID result to the user space, the cached value could apparently be 'close'.

That explains why there is no "lid open" configuration in the "Windows Device Manager".

> 
> I propose as a workaround to enable a kthread that will monitor the
> lid state and update the correct value to userspace (5 sec of polling
> time should be enough given that systemd checks every 20 sec).
> We should probably have this workaround only for a set of known
> devices, as it might just be temporary for those until the actual
> underlying problem is fixed (wrong DSDT in the Surface 3 case that
> doesn't notify at all, issue in the EC for the Surface Pro 1 and the
> Samsung N210).
[Lv Zheng] 
That cannot help to solve the issue/gap.

The problem is Linux userspace has a facility re-checking lid state when lid state is "close".
If it failed to update the lid state to "open" for a timeout period, it would suspend the platform.

We actually are recommending Linus userspace to introduce a new lid handling mode to only handle "lid close" event and ignore "lid open" event.
During the period this is not changed from the userspace, we have to send "open" after resume/boot in order not to trigger the timeout mechanism.

Thanks and best regards
-Lv

> 
> Cheers,
> Benjamin
> 
> 
> >         return 0;
> >  }
> >  #endif
> > @@ -422,6 +424,7 @@ static int acpi_button_add(struct acpi_device
> *device)
> >         if (error)
> >                 goto err_remove_fs;
> >         if (button->type == ACPI_BUTTON_TYPE_LID) {
> > +               (void)acpi_lid_notify_state(device, 1);
> >                 /*
> >                  * This assumes there's only one lid device, or if there are
> >                  * more we only care about the last one...
> > --
> > 1.7.10
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists