RE: [PATCH v4 2/2] ACPI / button: Add document for ACPI control method lid device restrictions
From: Zheng, Lv
Date: Sat Jul 23 2016 - 07:57:55 EST
Hi, Dmitry
> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Dmitry Torokhov
> Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI control
> method lid device restrictions
>
> On Fri, Jul 22, 2016 at 08:37:50AM +0000, Zheng, Lv wrote:
> > Hi, Dmitry
> >
> > Thanks for the review.
> >
> > > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI
> control
> > > method lid device restrictions
> > >
> > > Hi Lv,
> > >
> > > On Fri, Jul 22, 2016 at 12:24:50AM +0000, Zheng, Lv wrote:
> > > > Hi, Dmitry
> > > >
> > > >
> > > > Thanks for the review.
> > > >
> > > > > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx]
> > > > > Subject: Re: [PATCH v4 2/2] ACPI / button: Add document for ACPI
> > > control
> > > > > method lid device restrictions
> > > > >
> > > > > On Tue, Jul 19, 2016 at 04:11:21PM +0800, Lv Zheng wrote:
> > > > > > This patch adds documentation for the usage model of the
> control
> > > > > method lid
> > > > > > device.
> > > > > >
> > > > > > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
> > > > > > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > > > > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
> > > > > > Cc: Bastien Nocera: <hadess@xxxxxxxxxx>
> > > > > > Cc: linux-input@xxxxxxxxxxxxxxx
> > > > > > ---
> > > > > > Documentation/acpi/acpi-lid.txt | 89
> > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > > 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-
> > > > > lid.txt
> > > > > > 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@xxxxxxxxx>
> > > > > > +
> > > > > > +
> > > > > > +Abstract:
> > > > > > +
> > > > > > +Platforms containing lids convey lid state (open/close) to
> OSPMs
> > > using
> > > > > a
> > > > > > +control method lid device. To implement this, the AML tables
> issue
> > > > > > +Notify(lid_device, 0x80) to notify the OSPMs whenever the lid
> state
> > > has
> > > > > > +changed. The _LID control method for the lid device must be
> > > > > implemented to
> > > > > > +report the "current" state of the lid as either "opened" or
> "closed".
> > > > > > +
> > > > > > +This document describes the restrictions and the expections of
> the
> > > > > Linux
> > > > > > +ACPI lid device driver.
> > > > > > +
> > > > > > +
> > > > > > +1. Restrictions of the returning value of the _LID control method
> > > > > > +
> > > > > > +The _LID control method is described to return the "current" lid
> > > state.
> > > > > > +However the word of "current" has ambiguity, many AML tables
> > > return
> > > > > 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
> > > doing this.
> > > > 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)
> > > > {
> > > > 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.
> > [Lv Zheng]
> > And we validated with qemu that during boot, Windows7 evaluates _LID
> once but doesn't get suspended because of this value.
> > So we think Windows only suspends against "notification" not _LID
> evaluation result.
> >
> > >
> > > > 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.
> > [Lv Zheng]
> > Right.
> >
> > >
> > > > }
> > > > }
> > > >
> > > > 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,
> > > > "\\_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?
> > [Lv Zheng]
> > I think Benjamin has a proposal of fixing this in GPIO driver.
> >
> > >
> > > > }
> > > > 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.
> > >
> > [Lv Zheng]
> > However, polling is not efficient, and not power efficient.
>
> We would not need to do this by default, and polling on a relaxed
> schedule (so that wakeups for polling coincide with other wakeups)
> should not be too bad (as in fractions of percent of power spent).
[Lv Zheng]
Yes, this feature is on my radar.
Because:
1. There seem to be some gaps in Linux, Linux cannot make some platforms reporting LID notifications and userspace has to poll exported lid state to work it around.
2. Since we've decided that the LID driver should be responsible for sending SW_LID, we should implement the kernel polling quirk instead of the userspace quirk.
However, this feature cannot fix the issue related to this patchset.
When the platform reports cached "close" after resuming, it seems the polling can still result in "close" to be sent.
And the userspace can still suspend the platform right after resume.
Or if we start to use lid_init_state=open, we cannot achieve "dark resume" usage model.
And after a long run, the polling code may still erroneously decide to send a "close" to the userspace.
And the userspace can still suspend the platform erroneously.
So we still need the userspace to change to be compliant to this documented new usage model (at least we need such a quirk mechanism).
>
> > OTOH, according to the validation result, Windows never poll _LID.
> >
> > > >
> > > > }
> > > > 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
> > > > }
> > > > }
> > > > }
> > > > }
> > > >
> > > > >
> > > > > > +state upon the last lid notification instead of returning the lid
> state
> > > > > > +upon the last _LID evaluation. There won't be difference when
> the
> > > _LID
> > > > > > +control method is evaluated during the runtime, the problem is
> its
> > > > > initial
> > > > > > +returning value. When the AML tables implement this control
> > > method
> > > > > with
> > > > > > +cached value, the initial returning value is likely not reliable.
> There
> > > are
> > > > > > +simply so many examples always retuning "closed" as initial lid
> > > state.
> > > > > > +
> > > > > > +2. Restrictions of the lid state change notifications
> > > > > > +
> > > > > > +There are many AML tables never notifying when the lid device
> > > state is
> > > > > > +changed to "opened". Thus the "opened" notification is not
> > > guaranteed.
> > > > > > +
> > > > > > +But it is guaranteed that the AML tables always notify "closed"
> > > when
> > > > > the
> > > > > > +lid state is changed to "closed". The "closed" notification is
> normally
> > > > > > +used to trigger some system power saving operations on
> Windows.
> > > > > Since it is
> > > > > > +fully tested, the "closed" notification is reliable for all AML
> tables.
> > > > > > +
> > > > > > +3. Expections for the userspace users of the ACPI lid device
> driver
> > > > > > +
> > > > > > +The ACPI button driver exports the lid state to the userspace via
> the
> > > > > > +following file:
> > > > > > + /proc/acpi/button/lid/LID0/state
> > > > > > +This file actually calls the _LID control method described above.
> And
> > > > > given
> > > > > > +the previous explanation, it is not reliable enough on some
> > > platforms.
> > > > > So
> > > > > > +it is advised for the userspace program to not to solely rely on
> this
> > > file
> > > > > > +to determine the actual lid state.
> > > > > > +
> > > > > > +The ACPI button driver emits 2 kinds of events to the user space:
> > > > > > + 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
> > > > > > + according to these 2 input key events.
> > > > > > + New userspace programs may only be prepared for the input
> key
> > > > > events.
> > > > >
> > > > > No, absolutely not. If some x86 vendors managed to mess up their
> > > > > firmware implementations that does not mean that everyone now
> has
> > > to
> > > > > abandon working perfectly well for them SW_LID events and rush
> to
> > > > > switch
> > > > > to a brand new event.
> > > > [Lv Zheng]
> > > > However there is no clear wording in the ACPI specification asking
> the
> > > vendors to achieve paired lid events.
> > > >
> > > > >
> > > > > Apparently were are a few issues, main is that some systems not
> > > reporting
> > > > > "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
> > > > > startup)
> > > > > for selected systems. This will mean that if system wakes up not
> > > because
> > > > > LID is open we'll incorrectly assume that it is, but we can either add
> > > > > more smarts to the process emitting SW_LID event or simply say
> "well,
> > > > > tough, the hardware is crappy" and bug vendor to see if they can fix
> > > the
> > > > > issue (if not for current firmware them for next).
> > > > [Lv Zheng]
> > > > The problem is there is no vendor actually caring about fixing this
> "issue".
> > > > Because Windows works well with their firmware.
> > > > Then finally becomes a big table customization business for our team.
> > >
> > > Well, OK. But you do not expect that we will redo up and down the
> stack
> > > lid handling just because MS messed up DSDT on Surface 3? No, let
> them
> > > know (they now care about Linux, right?) so Surface 4 works and quirk
> > > the behavior for Surface 3.
> > [Lv Zheng]
> > I think there are other platforms broken.
>
> Probably. I think we should deal with them as they come.
[Lv Zheng]
This seems to be what the ACPI team has been doing for years.
However, if the userspace has been changed to be compliant to the new documented usage model.
I think we needn't do that for the users, users can just specify a userspace option to work it around themselves.
>
> >
> > >
> > > >
> > > > >
> > > > > As an additional workaround, we can toggle the LID switch off and
> on
> > > > > when we get notification, much like your proposed patch does for
> the
> > > key
> > > > > events.
> > > > [Lv Zheng]
> > > > I think this is doable, I'll refresh my patchset to address your this
> > > comment.
> > > > By inserting open/close events when next close/open event arrives
> after
> > > a certain period,
> > > > this may fix some issues for the old programs.
> > > > Where user may be required to open/close lid twice to trigger 2nd
> > > suspend.
> > > >
> > > > However, this still cannot fix the problems like "Surface 3".
> > > > We'll still need a new usage model for such platforms (no open
> event).
> > >
> > > No, for surface 3 you simply need to add polling of "_LID" method to
> the
> > > button driver.
> > >
> > > What are the other devices that mess up lid handling?
> > [Lv Zheng]
> > The patch lists 3 of them.
> > Which are known to me because they all occurred after I started to look
> at the lid issues.
> >
> > According to my teammates.
> > They've been fixing such wrong "DSDT" using customization for years.
> > For example, by adding _LID(); Notify(lid) into _WAK() or EC._REG().
>
> What did they do with them once they did the fix? Were they submitting
> fixes to manufacturers? What happened to them?
[Lv Zheng]
I really don't know.
However, I think the ending of the story is likely:
The users who report such breakage happily get their platforms working.
And they'll never report it again.
Users using the same platforms can find the quirk via web search.
Or the best ending is:
The reporters have reported this to the Linux contribution vendors.
And the knowledge is documented in their published laptop knowledge base or included in the distribution.
I think no manufacturer really cares about such fixes.
But I'm really not the correct one to answer this question.
I'm new to ACPI bug fixing work.
IMO, if the userspace can have an option to implement a mode compliant to this documented usage model.
And always automatically enable this option when PNP0C0D is detected.
No one will need any kind of quirks.
But as Benjamin suggested, we may use the hwdb to enable this option for those buggy platforms.
So that all other platforms are compliant to the unified Linux lid model.
Cheers
-Lv