[prev in list] [next in list] [prev in thread] [next in thread]
List: linux-input
Subject: RE: [PATCH v4 2/2] ACPI / button: Add document for ACPI control method lid device restrictions
From: "Zheng, Lv" <lv.zheng () intel ! com>
Date: 2016-07-22 9:38:58
Message-ID: 1AE640813FDE7649BE1B193DEA596E883BC05DB7 () SHSMSX101 ! ccr ! corp ! intel ! com
[Download RAW message or body]
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 <lv.zheng@intel.com> wrote:
> > Hi,
> >
> > > 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 6:37 AM, Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > 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@gmail.com]
> > > > > > 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@intel.com>
> > > > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > > > > > Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > > > > > > Cc: Bastien Nocera: <hadess@hadess.net>
> > > > > > > Cc: linux-input@vger.kernel.org
> > > > > > > ---
> > > > > > > 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@intel.com>
> > > > > > > +
> > > > > > > +
> > > > > > > +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.
> > >
> > > 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)
> > >
> > > >
> > > > > 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
> > > 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).
> > Surface 3 is not the only platform caching an initial close value.
> > There are 2 traditional platforms listed in the patch description.
> >
> > > >
> > > > > }
> > > > > }
> > > > >
> > > > > 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?
> > > >
> > > > > }
> > > > > 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-
> > > 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.
> > 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).
>
> >
> > 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.
>
> >
> > > >
> > > > >
> > > > > }
> > > > > 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.
> > > >
> > >
> > > 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.
> > >
> > > > >
> > > > > >
> > > > > > 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.
> > >
> > > 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. :-)
> >
> > > 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,
> > 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
>
> >
> > >
> > > > > [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?
> > > >
> > >
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic