RE: [PATCH v4 2/2] ACPI / button: Add document for ACPI control method lid... | linux-input

RE: [PATCH v4 2/2] ACPI / button: Add document for ACPI control method lid device restrictions

From: Zheng, Lv <hidden>
Date: 2016-07-22 09:39:18
Also in: linux-acpi, lkml

Hi, Benjamin

From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control
method lid device restrictions

On Fri, Jul 22, 2016 at 10:47 AM, Zheng, Lv [off-list ref] wrote:

quoted

Hi,

quoted

From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI

control

quoted

method lid device restrictions

On Fri, Jul 22, 2016 at 6:37 AM, Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi Lv,

On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote:

quoted

Hi, Dmitry


Thanks for the review.

quoted

From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI

control

quoted

method lid device restrictions

On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote:

quoted

This patch adds documentation for the usage model of the

control

quoted

method lid

quoted

device.

Signed-off-by: Lv Zheng <redacted>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Benjamin Tissoires <redacted>
Cc: Bastien Nocera: <hadess@hadess.net>
Cc: linux-input@vger.kernel.org
---
 Documentation/acpi/acpi-lid.txt |   89

+++++++++++++++++++++++++++++++++++++++

quoted

 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/acpi/acpi-lid.txt

diff --git a/Documentation/acpi/acpi-lid.txt

b/Documentation/acpi/acpi-

quoted

lid.txt

quoted

new file mode 100644
index 0000000..2addedc

--- /dev/null
+++ b/Documentation/acpi/acpi-lid.txt

@@ -0,0 +1,89 @@
+Usage Model of the ACPI Control Method Lid Device
+
+Copyright (C) 2016, Intel Corporation
+Author: Lv Zheng <lv.zheng@intel.com>
+
+
+Abstract:
+
+Platforms containing lids convey lid state (open/close) to

OSPMs

quoted

quoted
using
quoted
quoted
quoted
a
quoted
+control method lid device. To implement this, the AML tables

issue

quoted

+Notify(lid_device, 0x80) to notify the OSPMs whenever the lid

state has

quoted

+changed. The _LID control method for the lid device must be

implemented to

quoted

+report the "current" state of the lid as either "opened" or

"closed".

quoted

quoted
quoted
quoted
quoted
quoted
+
+This document describes the restrictions and the expections of

the

quoted

Linux

quoted

+ACPI lid device driver.
+
+
+1. Restrictions of the returning value of the _LID control

method

quoted

+
+The _LID control method is described to return the "current" lid

state.

quoted

+However the word of "current" has ambiguity, many AML

tables

quoted

return

quoted

the lid

Can this be fixed in the next ACPI revision?

[Lv Zheng]
Even this is fixed in the ACPI specification, there are platforms

already

quoted

doing this.

quoted

Especially platforms from Microsoft.
So the de-facto standard OS won't care about the change.
And we can still see such platforms.

Here is an example from Surface 3:

DefinitionBlock ("dsdt.aml", "DSDT", 2, "ALASKA", "A M I ",

0x01072009)

quoted

{
    Scope (_SB)
    {
        Device (PCI0)
        {
            Name (_HID, EisaId ("PNP0A08"))  // _HID: Hardware ID
            Name (_CID, EisaId ("PNP0A03"))  // _CID: Compatible ID
            Device (SPI1)
            {
                Name (_HID, "8086228E")  // _HID: Hardware ID
                Device (NTRG)
                {
                    Name (_HID, "MSHW0037")  // _HID: Hardware ID
                }
            }
        }

        Device (LID)
        {
            Name (_HID, EisaId ("PNP0C0D"))
            Name (LIDB, Zero)

Start with lid closed? In any case might be wrong.

Actually the initial value doesn't matter if the gpiochip triggers the
_EC4 at boot, which it should
(https://github.com/hadess/fedora-surface3-
kernel/commit/13200f81662c1c0b58137947c3e6c000fe62a2ba,
still unsubmitted)

quoted

            Method (_LID, 0, NotSerialized)
            {
                Return (LIDB)

So "_LID" returns the last state read by "_EC4". "_EC4" is
edge-triggered and will be evaluated every time gpio changes state.

That's assuming the change happens while the system is on. If you go
into suspend by closing the LID. Open it while on suspend and then hit
the power button given that the system doesn't wake up when the lid

is

quoted

opened, the edge change was made while the system is asleep, and we
are screwed (from an ACPI point of view). See my next comment for a
solution.

[Lv Zheng]
I actually not sure if polling can fix all issues.
For example.
If a platform reporting "close" after resuming.
Then polling _LID will always return "close".
And the userspace can still get the "close" not "open".
So it seems polling is not working for such platforms (cached value,

initial close).

quoted

Surface 3 is not the only platform caching an initial close value.
There are 2 traditional platforms listed in the patch description.

quoted

            }
        }

        Device (GPO0)
        {
            Name (_HID, "INT33FF")  // _HID: Hardware ID
            OperationRegion (GPOR, GeneralPurposeIo, Zero, One)
            Field (GPOR, ByteAcc, NoLock, Preserve)
            {
                Connection (
                    GpioIo (Shared, PullNone, 0x0000, 0x0000,

IoRestrictionNone,

quoted

                        "\\_SB.GPO0", 0x00, ResourceConsumer, ,
                        )
                        {   // Pin list
                            0x004C
                        }
                ),
                HELD,   1

Is it possible to read state of this GPIO from userspace on startup to
correct the initial state?

quoted

            }
            Method (_E4C, 0, Serialized)
            {
                If (LEqual(HELD, One))
                {
                    Store(One, ^^LID.LIDB)

There is no "open" event generated by "Surface 3".

Right so we update the state correctly, we just forgot to send the
notification. Nothing that polling can't fix.

Actually, I have a better (though more hackish) way of avoiding polling:
https://github.com/hadess/fedora-surface3-
kernel/blob/5e5775b9bdc308d665064387e0b144ee48e7b243/0002-

WIP-

quoted

add-custom-surface3-platform-device-for-controll.patch

Given that the notification is forwarded to the touchscreen anyway, we
can unregister the generic (and buggy) acpi button driver for the LID
and create our own based on this specific DSDT.
We can also make sure the LID state is also correct because of the WMI
method which allows to read the actual value of the GPIO connected to
the cover without using the cached (and most of the time wrong) acpi
LID.LIDB value.

I still yet have to submit this, but with this patch, but we can
consider the Surface 3 as working and not an issue anymore.

[Lv Zheng]
That could make surface 3 dependent on WMI driver, not ACPI button

driver.

quoted

Will this affect other buttons?
For example, power button/sleep button.

TLDR: no, there is no impact on other buttons.

There are 2 reasons why the impact is limited:
- the patch only removes the input node that contains the LID, and it
is the only one event in the input node
- power/sleep, volume +/- are not handled by ACPI as this is a reduced
platform and these buttons are not notified by ACPI. So we need an
adaptation of the GPIO button array for it to be working (patch
already submitted but I found a non-acpi platform issue, and then not
enough time to fix and send an updated version).

quoted

Our approach is to make ACPI button driver working.
Though this may lead to ABI changes.

Yes, I know you want to fix ACPI button for future non working
tablets/laptops. This is why I gave my rev-by in this series.

quoted

                }
                Else
                {
                    Store(Zero, ^^LID.LIDB)
                    Notify (LID, 0x80)

There is only "close" event generated by "Surface 3".
Thus they are not paired.

                }
                Notify (^^PCI0.SPI1.NTRG, One) // Device Check
            }
        }
    }
}

quoted

+state upon the last lid notification instead of returning the lid

state

quoted

quoted
quoted
quoted
quoted
quoted
+upon the last _LID evaluation. There won't be difference when

the

quoted

quoted
_LID
quoted
quoted
quoted
quoted
+control method is evaluated during the runtime, the problem is

its

quoted

initial

quoted

+returning value. When the AML tables implement this control

method

quoted

with

quoted

+cached value, the initial returning value is likely not reliable.

There

quoted

are

quoted

+simply so many examples always retuning "closed" as initial lid

state.

quoted

+
+2. Restrictions of the lid state change notifications
+
+There are many AML tables never notifying when the lid device

state is

quoted

+changed to "opened". Thus the "opened" notification is not

guaranteed.

quoted

+
+But it is guaranteed that the AML tables always notify "closed"

when

quoted

the

quoted

+lid state is changed to "closed". The "closed" notification is

normally

quoted

+used to trigger some system power saving operations on

Windows.

quoted

Since it is

quoted

+fully tested, the "closed" notification is reliable for all AML

tables.

quoted

quoted
quoted
quoted
quoted
quoted
+
+3. Expections for the userspace users of the ACPI lid device

driver

quoted

quoted
quoted
quoted
quoted
quoted
+
+The ACPI button driver exports the lid state to the userspace

via

quoted

the

quoted

+following file:
+  /proc/acpi/button/lid/LID0/state
+This file actually calls the _LID control method described above.

And

quoted

given

quoted

+the previous explanation, it is not reliable enough on some

platforms.

quoted

So

quoted

+it is advised for the userspace program to not to solely rely on

this

quoted

file

quoted

+to determine the actual lid state.
+
+The ACPI button driver emits 2 kinds of events to the user

space:

quoted

+  SW_LID
+   When the lid state/event is reliable, the userspace can behave
+   according to this input switch event.
+   This is a mode prepared for backward compatiblity.
+  KEY_EVENT_OPEN/KEY_EVENT_CLOSE
+   When the lid state/event is not reliable, the userspace should

behave

quoted

+   according to these 2 input key events.
+   New userspace programs may only be prepared for the input

key

quoted

events.

No, absolutely not. If some x86 vendors managed to mess up their
firmware implementations that does not mean that everyone now

has to

quoted

abandon working perfectly well for them SW_LID events and rush

to

quoted

switch
to a brand new event.

[Lv Zheng]
However there is no clear wording in the ACPI specification asking

the

quoted

vendors to achieve paired lid events.

quoted

Apparently were are a few issues, main is that some systems not

reporting

quoted

"open" event. This can be dealt with by userspace "writing" to the
lid's evdev device EV_SW/SW_LID/0 event upon system resume

(and

quoted

startup)
for selected systems. This will mean that if system wakes up not

because

quoted

LID is open we'll incorrectly assume that it is, but we can either

add

quoted

more smarts to the process emitting SW_LID event or simply say

"well,

quoted

tough, the hardware is crappy" and bug vendor to see if they can

fix

quoted

the

quoted

issue (if not for current firmware them for next).

[Lv Zheng]
The problem is there is no vendor actually caring about fixing this

"issue".

quoted

Because Windows works well with their firmware.
Then finally becomes a big table customization business for our

team.

quoted

Well, OK. But you do not expect that we will redo up and down the

stack

quoted

lid handling just because MS messed up DSDT on Surface 3? No, let

them

quoted

know (they now care about Linux, right?) so Surface 4 works and

quirk

quoted

the behavior for Surface 3.

From what I understood, it was more than just the Surface 3. Other
laptops were having issues and Lv's team gave up on fixing those
machines.

quoted

As an additional workaround, we can toggle the LID switch off and

on

quoted

when we get notification, much like your proposed patch does for

the

quoted

key

quoted

quoted
quoted
events.

I really don't like this approach. The problem being that we will fix
the notifications to user space, but nothing will tell userspace that
the LID state is known to be wrong.
OTOH, I already agreed for a hwdb in userspace so I guess this point is
moot.

Having both events (one SW for reliable HW, always correct, and one
KEY for unreliable HW) allows userspace to make a clear distinction
between the working and non working events and they can continue to
keep using the polling of the SW node without extra addition.

[Lv Zheng]
I think this solution is good and fair for all of the vendors. :-)

quoted

Anyway, if the kernel doesn't want to (or can't) fix the actual issue
(by making sure the DSDT is reliable), userspace needs to be changed
so any solution will be acceptable.

[Lv Zheng]
I think the answer is "can't".
If we introduced too many workarounds into acpi button driver,
in order to make something working while the platform firmware

doesn't expect it to be working,

quoted

then we'll start to worry about breaking good laptops.

Then you just need to amend the documentation to say that the fallback
of the KEY events is not the "future" but a way to get events on some
reduced platforms and it will not be the default.

[Lv Zheng] 
OK.

Please make sure userspace knows that the default is the good SW_LID,
and some particular cases will need to be handled through the KEY
events, not the other way around.

[Lv Zheng] 
However, we were thinking that user space should just switch to use the key events when the lid events are from ACPI button driver.
So you mean I need to change this to say that the key events should only be used for special hardware.
Right?

[few thoughts later]

How about:
- you send only one patch with the SW_LID ON/OFF or OFF/ON when we
receive the notification on buggy platform
- in the same patch, you add the documentation saying that on most
platforms, LID is reliable but some don't provide a reliable LID
state, but you guarantee to send an event when the state changes

[Lv Zheng] 
If I understand correctly, you mean I should improve the documentation.
Making the SW_LID the expected Linux behavior.
But allowing KEY_LID_XXX as a quirk mechanism to handle old platforms.

If so, I think I only need to refresh the document.
Right?

Cheers,
Lv

- in userspace, we add the hwdb which says "on this particular
platform, don't rely on the actual state, but wait for events" -> this
basically removes the polling on these platforms.

Bastien, Dmitry?

I still don't like relying on userspace to actually set the SW_LID
back to open on resume, as we should not rely on some userspace
program to set the value (but if logind really wants it, it's up to
them).

Cheers,
Benjamin

quoted

[Lv Zheng]
I think this is doable, I'll refresh my patchset to address your this

comment.

quoted

By inserting open/close events when next close/open event arrives

after a certain period,

quoted

this may fix some issues for the old programs.
Where user may be required to open/close lid twice to trigger 2nd

suspend.

quoted

However, this still cannot fix the problems like "Surface 3".
We'll still need a new usage model for such platforms (no open

event).

quoted

No, for surface 3 you simply need to add polling of "_LID" method to

the

quoted

button driver.

What are the other devices that mess up lid handling?

I also would be interested in knowing how much issues you are facing
compared to the average number of "good" laptops. IIRC, you talked
about 3 (counting the Surface 3), but I believe you had more in mind.

[Lv Zheng]
Yes.
However they happened before I started to look at the lid issues.
I think Rui has several such experiences.
+Rui.

Thanks and best regards
-Lv

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help