linux-kernel - RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1AE640813FDE7649BE1B193DEA596E886CEA80B3@SHSMSX101.ccr.corp.intel.com>
Date:   Thu, 25 May 2017 06:17:23 +0000
From:   "Zheng, Lv" <lv.zheng@...el.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>
CC:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Jiri Eischmann <jeischma@...hat.com>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to
 lid_init_state=open"

Hi,

> >> >> >> Benjamin, my understanding is that this is the case, is it correct?
> >> >> >
> >> >> > That is correct. This patch I reverted introduces regression for professional
> >> >> > laptops that expect the LID switch to be reported accurately.
> >> >>
> >> >> And from a user's perspective, what does not work any more?
> >> >
> >> > If you boot or resume your laptop with the lid closed on a docking
> >> > station while using an external monitor connected to it, both internal
> >> > and external displays will light on, while only the external should.
> >> >
> >> > There is a design choice in gdm to only provide the greater on the
> >> > internal display when lit on, so users only see a gray area on the
> >> > external monitor. Also, the cursor will not show up as it's by default
> >> > on the internal display too.
> >> >
> >> > To "fix" that, users have to open the laptop once and close it once
> >> > again to sync the state of the switch with the hardware state.
> >>
> >> OK
> >>
> >> Yeah, that sucks.
> >>
> >> So without the Lv's patch the behavior (on the systems in question) is
> >> as expected, right?
> >
> > Would you agree to take both these reverts without Lv's ACK? We already
> > tried to explain for 2 weeks that they are valuable, but it seems we
> > can't make change his mind.

It's not that difficult to get an agreement.
We just didn't communicate well.

> One of the reverts actually is already in (as a patch from Lv) and
> I'll most probably push the other one for -rc4 next week.

If we really want to go back to "method" mode.
We need one more patch and it is not in Benjamin's series.

There are 3 known broken cases, 2 of them are related to orders.
The last 1 is not order related:
1. Surface Pro 3: open arrives very early to update cached value, not notification
2. Samsung N210+: open arrives very late to update cached value, not notification 
3. Surface Pro 1: no open event, _LID keeps on returning close

The order problem is (considering method mode):

_Qxx <- Invoked due to EC events
  Update _LID return value
  Notify(LID, close)
    input_report(SW_LID 1) -> captured by user space and system starts to suspend
acpi_button_suspend
acpi_ec_suspend
  acpi_ec_disable_event
acpi_button_resume
  if (method)
    input_report(SW_LID, _LID return value, would be 1 for cached value)
acpi_ec_resume
  acpi_ec_enable_event
_Qxx <- Invoked due to EC events, for broken case 3, no such event
  Update _LID return value
  Notify(LID, open) <- for broken case 1, 2, 3, no such notification, thus open cannot be delivered to user space.
    input_report(SW_LID, 0)

The order of acpi_button_resume()/acpi_ec_resume() is determined by the enumeration order.
So it could vary on different platforms.
Considering case 1, for surface pro 3, where acpi_button_resume() is invoked before acpi_ec_resume().
Button driver will send false "close" to user space, and the updated "open" state won't be delivered to user space.
Staying in method mode, we can only suspend the system once, follow-up "close" events won't arrive to user space.

Even we can add many workarounds to make sure acpi_ec_resume() is executed before acpi_button_resume() on such platforms.
We still cannot fix case 2 and case 3.
So finally this order still cannot be ensured, and the solution is still not stable.
I would imagine the order problem is the key reason why MS stops sending "open" on these platforms.

Then given this order issue is not fixable, we need to fix broken case 1 by adding "complement event" in "method" mode.
Why don't we do this first before reverting to "method" mode?

And for broken stuffs like case 2/case 3,
IMO, they can only be solved using "ignore" mode, and changes in user space.
If we don't use this solution, we still need libinput quirks to handle them (may possibly encounter some new unfixable problems).
If you want to stay in "method" mode, will you be responsible for responding end users on kernel Bugzilla using libinput quirks?
We have a Power-Lid category now, we can have you guys as default assignee.

Cheers,
Lv