Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Friday, May 26, 2017 05:39:53 PM Peter Hutterer wrote:
> On May 25 2017, Benjamin Tissoires wrote:
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> 
> > > >> >> 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.
> > 
> > I have more that 26 emails in my INBOX (not counting my replies) and I
> > would really like switching to more valuable work than explaining again
> > and again that when a regression is introduced, it needs to be fixed (or
> > reverted in that case).
> 
> Yes please. This should have stopped right after "regression on basically
> every decent laptop out there" and we should be discussing how to fix the
> devices that actually need quirks because they're broken. Instead it 
> turned into a discussion on why we should stick with the regression and
> convince all of userspace to change and implement broken heuristics. I've
> used up my time budget for that.

Appreciated.

Also please note that it actually might help to make the decision.

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Friday, May 26, 2017 05:39:53 PM Peter Hutterer wrote:
> On May 25 2017, Benjamin Tissoires wrote:
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> 
> > > >> >> 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.
> > 
> > I have more that 26 emails in my INBOX (not counting my replies) and I
> > would really like switching to more valuable work than explaining again
> > and again that when a regression is introduced, it needs to be fixed (or
> > reverted in that case).
> 
> Yes please. This should have stopped right after "regression on basically
> every decent laptop out there" and we should be discussing how to fix the
> devices that actually need quirks because they're broken. Instead it 
> turned into a discussion on why we should stick with the regression and
> convince all of userspace to change and implement broken heuristics. I've
> used up my time budget for that.

Appreciated.

Also please note that it actually might help to make the decision.

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Peter Hutterer]

On May 25 2017, Benjamin Tissoires wrote:
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:

> > >> >> 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.
> 
> I have more that 26 emails in my INBOX (not counting my replies) and I
> would really like switching to more valuable work than explaining again
> and again that when a regression is introduced, it needs to be fixed (or
> reverted in that case).

Yes please. This should have stopped right after "regression on basically
every decent laptop out there" and we should be discussing how to fix the
devices that actually need quirks because they're broken. Instead it 
turned into a discussion on why we should stick with the regression and
convince all of userspace to change and implement broken heuristics. I've
used up my time budget for that.

Cheers,
   Peter

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Peter Hutterer]

On May 25 2017, Benjamin Tissoires wrote:
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:

> > >> >> 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.
> 
> I have more that 26 emails in my INBOX (not counting my replies) and I
> would really like switching to more valuable work than explaining again
> and again that when a regression is introduced, it needs to be fixed (or
> reverted in that case).

Yes please. This should have stopped right after "regression on basically
every decent laptop out there" and we should be discussing how to fix the
devices that actually need quirks because they're broken. Instead it 
turned into a discussion on why we should stick with the regression and
convince all of userspace to change and implement broken heuristics. I've
used up my time budget for that.

Cheers,
   Peter

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

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

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

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

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Wed, May 24, 2017 at 10:08 AM, Benjamin Tissoires
 wrote:
> Hi Rafael,
>
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
>> >> >> 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.

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.

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Wed, May 24, 2017 at 10:08 AM, Benjamin Tissoires
 wrote:
> Hi Rafael,
>
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
>> >> >> 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.

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.

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

Hi Rafael,

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> >> 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.

I have more that 26 emails in my INBOX (not counting my replies) and I
would really like switching to more valuable work than explaining again
and again that when a regression is introduced, it needs to be fixed (or
reverted in that case).

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

Hi Rafael,

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> >> 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.

I have more that 26 emails in my INBOX (not counting my replies) and I
would really like switching to more valuable work than explaining again
and again that when a regression is introduced, it needs to be fixed (or
reverted in that case).

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

Hi Lv,

[thank you Peter for jumping in the thread]

Just a few precisions regarding questions you asked:

On May 17 2017 or thereabouts, Zheng, Lv wrote:
[stripped]
> > [User space tools] *are* correct.
> > They are following the exported ACPI documentation
> 
> I doubt. In ACPI world, Windows is the only standard.

I was talking here about the i915 driver that uses the Linux ACPI
documentation in regard to the call of acpi_lid_open().
User space tool shouldn't rely on the _LID call through the sysfs if
acpi/button.c actaully forwards the ACPI call, because it is clear now
that it is not reliable in all situations.

> 
> > and the input node documentation.
> > Quoting the input doc:
> > file Documentation/input/event-codes.rst:
> > EV_SW
> > -
> > 
> > EV_SW events describe stateful binary switches. For example, the SW_LID 
> > code is
> > used to denote when a laptop lid is closed.
> > 
> > Upon binding to a device or resuming from suspend, a driver must report
> > the current switch state. This ensures that the device, kernel, and 
> > userspace
> > state is in sync.
> > 
> > Upon resume, if the switch state is the same as before suspend, then the 
> > input
> > subsystem will filter out the duplicate switch state reports. The driver 
> > does
> > not need to keep the state of the switch at any time.
> > 
> 
> That's really a convenient feature for driver.
> If I'm the driver writers, I would be very appreciated for being able to use 
> such features.
> So you see I don't have objections to having this feature.

It's not a feature on how the system could be improved, it's the
definition of an input node that relay an EV_SW element. acpi/button.c
exports a LID_SW, so it has to conform to the definition. That is not
something we should be discussing.

> 
> I just have concerns related to:
> 1. Is it required to have a timeout in systemd, forcing platform to suspend 
> again, just due to event delays?
> 2. Is it required to use SW_LID to determine whether an internal display 
> should be lit on?
> I don't see any conflicts between the ABI of EV_SW and the 2 questions.

As Peter mentioned, the users of the switch are doing what they want.
But if you don't have the LID_SW input node, you can not know the state
of the laptop (opened or closed). So yes, it's there for a reason.

[stripped]

> > No. We can do whatever we want in libinput. And we can fix hardware when
> > it appears. We don't need to have the correct solution for everybody,
> > ever. libinput is a library and it can be updated, and we can ask users
> > to upgrade. We can even break the API by bumping the soname. We have
> > much more liberty that the kernel has, so the libinput implementation
> > shouldn't be a concern.
> 
> It seems you don't know all the details I was looking at.
> It's about a timing problem, with button.lid_init_state=method and libinput 
> quirk, we actually have 2 quirks.
> And libinput write can appear before/after the faked init notification 
> triggered by "method" mode.

See Peter's answer (nothing will happen in user-space), but I just
wanted to raised the fact that the "method" parameter doesn't trigger
"faked init notifications". The "method" parameter only syncs the state
with the hardware. And given this happens in the .resume() callback,
it means that users will have to wait for the resume to finish before
taking any action. So it's on their side that there is a problem if they
are reading the state before the kernel even had the chance to update
it.

[stripped]

> That sounds great!
> If systemd/logind can be changed, can we ask systemd/logind to change it to 
> be longer enough.
> For example, allowing users to configure this to "INFINITE"?

I think they have a good reason but I never fully understood why they
have to poll on the state.

> That solves many other problems.
> 
> NOTE, EV_SW is a good feature to reduce duplicated events,
> but that doesn't mean users of EV_SW need to be so strict to the timings of 
> SW_LID, right?

When an input event happens over evdev, it's expected that the user
treats it immediately.
In the LID_SW case we can somewhat tell user space that they need to be
extra careful, but they can argue that we should comply to the
definition of an input switch, or not exporting it at all.

[stripped]

> 
> > >What's the root cause of this issue?
> > 
> > Poorly written EC?
> > It doesn't matter. We know the machine is buggy, we'll just need a
> > workaround.
> 
> It seems you know better than the SAMSUNG official supporters?

I wouldn't pretend to know more than Samsung engineers. All I see is
that there is an issue in the notification and/or the acpi _LID call, so
all I can say is that there is a bug in the firmware of the embedded
controller in charge of answering acpi calls.

> The 10-20 seconds lag can be seen even on Windows:
> http://www.samsung.com/uk/support/model/NP-N210-JP02UK
> 
> Since you seem to be able to see something I'm not aware of.
> Let me ask, which do you 

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

Hi Lv,

[thank you Peter for jumping in the thread]

Just a few precisions regarding questions you asked:

On May 17 2017 or thereabouts, Zheng, Lv wrote:
[stripped]
> > [User space tools] *are* correct.
> > They are following the exported ACPI documentation
> 
> I doubt. In ACPI world, Windows is the only standard.

I was talking here about the i915 driver that uses the Linux ACPI
documentation in regard to the call of acpi_lid_open().
User space tool shouldn't rely on the _LID call through the sysfs if
acpi/button.c actaully forwards the ACPI call, because it is clear now
that it is not reliable in all situations.

> 
> > and the input node documentation.
> > Quoting the input doc:
> > file Documentation/input/event-codes.rst:
> > EV_SW
> > -
> > 
> > EV_SW events describe stateful binary switches. For example, the SW_LID 
> > code is
> > used to denote when a laptop lid is closed.
> > 
> > Upon binding to a device or resuming from suspend, a driver must report
> > the current switch state. This ensures that the device, kernel, and 
> > userspace
> > state is in sync.
> > 
> > Upon resume, if the switch state is the same as before suspend, then the 
> > input
> > subsystem will filter out the duplicate switch state reports. The driver 
> > does
> > not need to keep the state of the switch at any time.
> > 
> 
> That's really a convenient feature for driver.
> If I'm the driver writers, I would be very appreciated for being able to use 
> such features.
> So you see I don't have objections to having this feature.

It's not a feature on how the system could be improved, it's the
definition of an input node that relay an EV_SW element. acpi/button.c
exports a LID_SW, so it has to conform to the definition. That is not
something we should be discussing.

> 
> I just have concerns related to:
> 1. Is it required to have a timeout in systemd, forcing platform to suspend 
> again, just due to event delays?
> 2. Is it required to use SW_LID to determine whether an internal display 
> should be lit on?
> I don't see any conflicts between the ABI of EV_SW and the 2 questions.

As Peter mentioned, the users of the switch are doing what they want.
But if you don't have the LID_SW input node, you can not know the state
of the laptop (opened or closed). So yes, it's there for a reason.

[stripped]

> > No. We can do whatever we want in libinput. And we can fix hardware when
> > it appears. We don't need to have the correct solution for everybody,
> > ever. libinput is a library and it can be updated, and we can ask users
> > to upgrade. We can even break the API by bumping the soname. We have
> > much more liberty that the kernel has, so the libinput implementation
> > shouldn't be a concern.
> 
> It seems you don't know all the details I was looking at.
> It's about a timing problem, with button.lid_init_state=method and libinput 
> quirk, we actually have 2 quirks.
> And libinput write can appear before/after the faked init notification 
> triggered by "method" mode.

See Peter's answer (nothing will happen in user-space), but I just
wanted to raised the fact that the "method" parameter doesn't trigger
"faked init notifications". The "method" parameter only syncs the state
with the hardware. And given this happens in the .resume() callback,
it means that users will have to wait for the resume to finish before
taking any action. So it's on their side that there is a problem if they
are reading the state before the kernel even had the chance to update
it.

[stripped]

> That sounds great!
> If systemd/logind can be changed, can we ask systemd/logind to change it to 
> be longer enough.
> For example, allowing users to configure this to "INFINITE"?

I think they have a good reason but I never fully understood why they
have to poll on the state.

> That solves many other problems.
> 
> NOTE, EV_SW is a good feature to reduce duplicated events,
> but that doesn't mean users of EV_SW need to be so strict to the timings of 
> SW_LID, right?

When an input event happens over evdev, it's expected that the user
treats it immediately.
In the LID_SW case we can somewhat tell user space that they need to be
extra careful, but they can argue that we should comply to the
definition of an input switch, or not exporting it at all.

[stripped]

> 
> > >What's the root cause of this issue?
> > 
> > Poorly written EC?
> > It doesn't matter. We know the machine is buggy, we'll just need a
> > workaround.
> 
> It seems you know better than the SAMSUNG official supporters?

I wouldn't pretend to know more than Samsung engineers. All I see is
that there is an issue in the notification and/or the acpi _LID call, so
all I can say is that there is a bug in the firmware of the embedded
controller in charge of answering acpi calls.

> The 10-20 seconds lag can be seen even on Windows:
> http://www.samsung.com/uk/support/model/NP-N210-JP02UK
> 
> Since you seem to be able to see something I'm not aware of.
> Let me ask, which do you 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Peter Hutterer]

Hi Lv

> > Yes, it's called a quirk. And the good practice is to register those
> > quirks and make them available to everybody. Being in hwdb in user space
> > or in acpi/button in kernel space doesn't matter, we need them.
> 
> I have no objections but concerns related to the combination of "default 
> mode" and "quirk responsibles".
> From my point of view, these are my conclusions:
> 1. If you want to use libinput to generate quirks, you should use "ignore"
> rather than "method" mode as default mode;

libinput does not replace the kernel. libinput can help with some
heuristics, but that should be the exception, not the default. And
heuristics cannot detect some cases, ignore suffers from that problem (see
below).

> 2. If you want to use button driver to generate quirks, we need "close" mode;
> 3. If GDM can change or users are ok use command lines, we can remain to use 
> "open" as the default behavior.
> (I'll send technical details in private about these conclusions)
> But you seem to always:
> 1. Say no to "ignore" which makes 1 impossible;

as an outsider, 'ignore' makes me wonder: Why wouldn't you query the state
of the hardware if it lets you? That's what we do in userspace with EV_SW.
We don't just rely on the notification, we look at the state of it too.

sure, some hardware is buggy and we can somehow work around this but that's
not a reason to throw everyone else under the bus too.

> 2. Say no to "close" which makes 2 impossible;

'close' forces userspace to fix up the kernel in every case rather than for
those devices where method doesn't work correctly. That's just effectively
deciding to be always the least wrong just to avoid a few outliers.
(also, if the kernel is always wrong, what purpose does it serve? :)

> 3. Say no to "open" which makes 3 impossible.

as mentioned above, some things we cannot detect.
we cannot detect a false-open with heuristics, this makes it impossible to
fix with heuristics in userspace. for 'close' and 'method' we can take user
input as indication that the lid isn't as closed as it pretends to be. That
doesn't work the other way round, lack of user input does not imply the
lid is closed.

> > > We haven't asked user space to change.
> > > We are just discussing the correctness of some user space behaviors.
> >
> > They *are* correct.
> > They are following the exported ACPI documentation
> 
> I doubt. In ACPI world, Windows is the only standard.

ok, here I need to note something: ACPI doesn't matter to userspace.
the kernel has EV_SW/SW_LID, which is *not* ACPI. that promises that a
userspace can assume that if SW_LID is 1, then the lid is closed,
otherwise it's open. Some leeway can be given because, well, reality, 
but a userspace behaviour to assume a lid is closed when the lid switch is
set to that state should not be up for discussion.
 
> > and the input node documentation.
> > Quoting the input doc:
> > file Documentation/input/event-codes.rst:
> > EV_SW
> > -
> >
> > EV_SW events describe stateful binary switches. For example, the SW_LID 
> > code is
> > used to denote when a laptop lid is closed.
> >
> > Upon binding to a device or resuming from suspend, a driver must report
> > the current switch state. This ensures that the device, kernel, and 
> > userspace
> > state is in sync.
> >
> > Upon resume, if the switch state is the same as before suspend, then the 
> > input
> > subsystem will filter out the duplicate switch state reports. The driver 
> > does
> > not need to keep the state of the switch at any time.
> > 
> 
> That's really a convenient feature for driver.
> If I'm the driver writers, I would be very appreciated for being able to use 
> such features.
> So you see I don't have objections to having this feature.
> 
> I just have concerns related to:
> 1. Is it required to have a timeout in systemd, forcing platform to suspend 
> again, just due to event delays?
> 2. Is it required to use SW_LID to determine whether an internal display 
> should be lit on?
> I don't see any conflicts between the ABI of EV_SW and the 2 questions.

the ABI of EV_SW? that's just evdev. It seems you're discussing three, four
things simultaneously, ACPI, the button driver, the evdev switch API and
userspace behaviour in response to this whole thread. The simple answer is:
the last bit doesn't matter - if EV_SW/SW_LID is on, the lid can be assumed
to be closed.

How you get to this point is your side of the problem :)
And I'm willing to accommodate for some issues (see the heuristics benjamin
already mentioned) but they should be exception, not the rule.

but whether e.g. displays are lit or not has nothing to do with this
discussion, it just doesn't matter what userspace does with the information.

> > So no, you can't have 'ignore' or 'open' to be the default, because user
> > space expects the switch to reflect the current state of the hardware.
> 
> Then what's the benefit of having 'method' to be the default,
> Given it is still not able to 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Peter Hutterer]

Hi Lv

> > Yes, it's called a quirk. And the good practice is to register those
> > quirks and make them available to everybody. Being in hwdb in user space
> > or in acpi/button in kernel space doesn't matter, we need them.
> 
> I have no objections but concerns related to the combination of "default 
> mode" and "quirk responsibles".
> From my point of view, these are my conclusions:
> 1. If you want to use libinput to generate quirks, you should use "ignore"
> rather than "method" mode as default mode;

libinput does not replace the kernel. libinput can help with some
heuristics, but that should be the exception, not the default. And
heuristics cannot detect some cases, ignore suffers from that problem (see
below).

> 2. If you want to use button driver to generate quirks, we need "close" mode;
> 3. If GDM can change or users are ok use command lines, we can remain to use 
> "open" as the default behavior.
> (I'll send technical details in private about these conclusions)
> But you seem to always:
> 1. Say no to "ignore" which makes 1 impossible;

as an outsider, 'ignore' makes me wonder: Why wouldn't you query the state
of the hardware if it lets you? That's what we do in userspace with EV_SW.
We don't just rely on the notification, we look at the state of it too.

sure, some hardware is buggy and we can somehow work around this but that's
not a reason to throw everyone else under the bus too.

> 2. Say no to "close" which makes 2 impossible;

'close' forces userspace to fix up the kernel in every case rather than for
those devices where method doesn't work correctly. That's just effectively
deciding to be always the least wrong just to avoid a few outliers.
(also, if the kernel is always wrong, what purpose does it serve? :)

> 3. Say no to "open" which makes 3 impossible.

as mentioned above, some things we cannot detect.
we cannot detect a false-open with heuristics, this makes it impossible to
fix with heuristics in userspace. for 'close' and 'method' we can take user
input as indication that the lid isn't as closed as it pretends to be. That
doesn't work the other way round, lack of user input does not imply the
lid is closed.

> > > We haven't asked user space to change.
> > > We are just discussing the correctness of some user space behaviors.
> >
> > They *are* correct.
> > They are following the exported ACPI documentation
> 
> I doubt. In ACPI world, Windows is the only standard.

ok, here I need to note something: ACPI doesn't matter to userspace.
the kernel has EV_SW/SW_LID, which is *not* ACPI. that promises that a
userspace can assume that if SW_LID is 1, then the lid is closed,
otherwise it's open. Some leeway can be given because, well, reality, 
but a userspace behaviour to assume a lid is closed when the lid switch is
set to that state should not be up for discussion.
 
> > and the input node documentation.
> > Quoting the input doc:
> > file Documentation/input/event-codes.rst:
> > EV_SW
> > -
> >
> > EV_SW events describe stateful binary switches. For example, the SW_LID 
> > code is
> > used to denote when a laptop lid is closed.
> >
> > Upon binding to a device or resuming from suspend, a driver must report
> > the current switch state. This ensures that the device, kernel, and 
> > userspace
> > state is in sync.
> >
> > Upon resume, if the switch state is the same as before suspend, then the 
> > input
> > subsystem will filter out the duplicate switch state reports. The driver 
> > does
> > not need to keep the state of the switch at any time.
> > 
> 
> That's really a convenient feature for driver.
> If I'm the driver writers, I would be very appreciated for being able to use 
> such features.
> So you see I don't have objections to having this feature.
> 
> I just have concerns related to:
> 1. Is it required to have a timeout in systemd, forcing platform to suspend 
> again, just due to event delays?
> 2. Is it required to use SW_LID to determine whether an internal display 
> should be lit on?
> I don't see any conflicts between the ABI of EV_SW and the 2 questions.

the ABI of EV_SW? that's just evdev. It seems you're discussing three, four
things simultaneously, ACPI, the button driver, the evdev switch API and
userspace behaviour in response to this whole thread. The simple answer is:
the last bit doesn't matter - if EV_SW/SW_LID is on, the lid can be assumed
to be closed.

How you get to this point is your side of the problem :)
And I'm willing to accommodate for some issues (see the heuristics benjamin
already mentioned) but they should be exception, not the rule.

but whether e.g. displays are lit or not has nothing to do with this
discussion, it just doesn't matter what userspace does with the information.

> > So no, you can't have 'ignore' or 'open' to be the default, because user
> > space expects the switch to reflect the current state of the hardware.
> 
> Then what's the benefit of having 'method' to be the default,
> Given it is still not able to 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Benjamin

> > What's that?
> > I mean, the bad faith?
> I already explained 4 times why we need to revert these two patches and
> why we need to keep 'method'. And you keep answering with long emails
> that you would rather not. I call it bad faith, sorry.

The 4 times explanations didn't answer my questions.
But that's OK, let's clarify it again.

> > > This is a REGRESSION. It used to work on thousands of devices, it
> > > doesn't anymore. So any regression has to be chased down and no good
> > > reason can justify such a regression.
> > I triggered many such kind of layered regressions and did fix them 1 by 1 
> > in different places.
> > However, this might be different.
> No. It is a regression. It used to work for thousands of devices befor
> v4.11, and now it's broken for those devices. It's a regression.
> Some new devices are broken with "method", it's a bug, and we can't fix
> them by regressing on all the others.
...
> I call this "fixing by users", and this is wrong. It used to work for
> years for almost everybody, you can not ask users to fix this one by
> one.

What about regressions triggered by this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=23de5d9ef2a4
Before that (year 2007), "ignore" is the default mode.

Other than this, I just had concerns related to fixing things back and forth, 
but you didn't reply properly.
Again, that's OK, let's just clarify it.

> Yes, it's called a quirk. And the good practice is to register those
> quirks and make them available to everybody. Being in hwdb in user space
> or in acpi/button in kernel space doesn't matter, we need them.

I have no objections but concerns related to the combination of "default mode" 
and "quirk responsibles".
From my point of view, these are my conclusions:
1. If you want to use libinput to generate quirks, you should use "ignore" 
rather than "method" mode as default mode;
2. If you want to use button driver to generate quirks, we need "close" mode;
3. If GDM can change or users are ok use command lines, we can remain to use 
"open" as the default behavior.
(I'll send technical details in private about these conclusions)
But you seem to always:
1. Say no to "ignore" which makes 1 impossible;
2. Say no to "close" which makes 2 impossible;
3. Say no to "open" which makes 3 impossible.

> > We haven't asked user space to change.
> > We are just discussing the correctness of some user space behaviors.
> 
> They *are* correct.
> They are following the exported ACPI documentation

I doubt. In ACPI world, Windows is the only standard.

> and the input node documentation.
> Quoting the input doc:
> file Documentation/input/event-codes.rst:
> EV_SW
> -
> 
> EV_SW events describe stateful binary switches. For example, the SW_LID code 
> is
> used to denote when a laptop lid is closed.
> 
> Upon binding to a device or resuming from suspend, a driver must report
> the current switch state. This ensures that the device, kernel, and userspace
> state is in sync.
> 
> Upon resume, if the switch state is the same as before suspend, then the input
> subsystem will filter out the duplicate switch state reports. The driver does
> not need to keep the state of the switch at any time.
> 

That's really a convenient feature for driver.
If I'm the driver writers, I would be very appreciated for being able to use 
such features.
So you see I don't have objections to having this feature.

I just have concerns related to:
1. Is it required to have a timeout in systemd, forcing platform to suspend 
again, just due to event delays?
2. Is it required to use SW_LID to determine whether an internal display should 
be lit on?
I don't see any conflicts between the ABI of EV_SW and the 2 questions.

> So no, you can't have 'ignore' or 'open' to be the default, because user
> space expects the switch to reflect the current state of the hardware.

Then what's the benefit of having 'method' to be the default,
Given it is still not able to reliably deliver the current state of hardware?
Actually both 'ignore/open/method' modes are trying to be compliant to EV_SW.
Among them, "ignore" did the best IMO.
And cases broken in "ignore" mode but not broken in "method" mode are all 
issues:
 - Platform doesn't send notification after boot/resume.
   IMO, we should also collect them and indicate them to desktop managers.

So in the end, we just have differences related to picking which default mode.

> > > You can not also change the semantic of an input switch. An input
> > > switch, as per the input subsystem is supposed to forward an actual
> > > state of the underlying hardware. Any fake information is bad and has to
> > > be avoided.
> > Since fake events are harmful, why do we fake an event after boot/resume?
> > button.lid_init_state=method seems can fake such an event.
> We don't fake an event, we are syncing the input switch state with the
> hardware.
> Faking an event is when you send "switch is open" while 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Benjamin

> > What's that?
> > I mean, the bad faith?
> I already explained 4 times why we need to revert these two patches and
> why we need to keep 'method'. And you keep answering with long emails
> that you would rather not. I call it bad faith, sorry.

The 4 times explanations didn't answer my questions.
But that's OK, let's clarify it again.

> > > This is a REGRESSION. It used to work on thousands of devices, it
> > > doesn't anymore. So any regression has to be chased down and no good
> > > reason can justify such a regression.
> > I triggered many such kind of layered regressions and did fix them 1 by 1 
> > in different places.
> > However, this might be different.
> No. It is a regression. It used to work for thousands of devices befor
> v4.11, and now it's broken for those devices. It's a regression.
> Some new devices are broken with "method", it's a bug, and we can't fix
> them by regressing on all the others.
...
> I call this "fixing by users", and this is wrong. It used to work for
> years for almost everybody, you can not ask users to fix this one by
> one.

What about regressions triggered by this commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=23de5d9ef2a4
Before that (year 2007), "ignore" is the default mode.

Other than this, I just had concerns related to fixing things back and forth, 
but you didn't reply properly.
Again, that's OK, let's just clarify it.

> Yes, it's called a quirk. And the good practice is to register those
> quirks and make them available to everybody. Being in hwdb in user space
> or in acpi/button in kernel space doesn't matter, we need them.

I have no objections but concerns related to the combination of "default mode" 
and "quirk responsibles".
From my point of view, these are my conclusions:
1. If you want to use libinput to generate quirks, you should use "ignore" 
rather than "method" mode as default mode;
2. If you want to use button driver to generate quirks, we need "close" mode;
3. If GDM can change or users are ok use command lines, we can remain to use 
"open" as the default behavior.
(I'll send technical details in private about these conclusions)
But you seem to always:
1. Say no to "ignore" which makes 1 impossible;
2. Say no to "close" which makes 2 impossible;
3. Say no to "open" which makes 3 impossible.

> > We haven't asked user space to change.
> > We are just discussing the correctness of some user space behaviors.
> 
> They *are* correct.
> They are following the exported ACPI documentation

I doubt. In ACPI world, Windows is the only standard.

> and the input node documentation.
> Quoting the input doc:
> file Documentation/input/event-codes.rst:
> EV_SW
> -
> 
> EV_SW events describe stateful binary switches. For example, the SW_LID code 
> is
> used to denote when a laptop lid is closed.
> 
> Upon binding to a device or resuming from suspend, a driver must report
> the current switch state. This ensures that the device, kernel, and userspace
> state is in sync.
> 
> Upon resume, if the switch state is the same as before suspend, then the input
> subsystem will filter out the duplicate switch state reports. The driver does
> not need to keep the state of the switch at any time.
> 

That's really a convenient feature for driver.
If I'm the driver writers, I would be very appreciated for being able to use 
such features.
So you see I don't have objections to having this feature.

I just have concerns related to:
1. Is it required to have a timeout in systemd, forcing platform to suspend 
again, just due to event delays?
2. Is it required to use SW_LID to determine whether an internal display should 
be lit on?
I don't see any conflicts between the ABI of EV_SW and the 2 questions.

> So no, you can't have 'ignore' or 'open' to be the default, because user
> space expects the switch to reflect the current state of the hardware.

Then what's the benefit of having 'method' to be the default,
Given it is still not able to reliably deliver the current state of hardware?
Actually both 'ignore/open/method' modes are trying to be compliant to EV_SW.
Among them, "ignore" did the best IMO.
And cases broken in "ignore" mode but not broken in "method" mode are all 
issues:
 - Platform doesn't send notification after boot/resume.
   IMO, we should also collect them and indicate them to desktop managers.

So in the end, we just have differences related to picking which default mode.

> > > You can not also change the semantic of an input switch. An input
> > > switch, as per the input subsystem is supposed to forward an actual
> > > state of the underlying hardware. Any fake information is bad and has to
> > > be avoided.
> > Since fake events are harmful, why do we fake an event after boot/resume?
> > button.lid_init_state=method seems can fake such an event.
> We don't fake an event, we are syncing the input switch state with the
> hardware.
> Faking an event is when you send "switch is open" while 

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 16 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Benjamin
> 
> > > > > > >> >> > > > > For example, such a hwdb entry is:
> > > > > > >> >> > > > > libinput:name:*Lid 
> > > > > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > > > > >> >> Well, if it worked in a specific way that users depended on 
> > > > > > >> >> before the commit in
> > > > > > >> >> question and now it works differently, then it does break 
> > > > > > >> >> things.
> > > > > > >> >> 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?
> > > > > Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.
> > > > I would make an argument that:
> > > > A. Is this necessarily a button driver regression?
> > > > 1. Users already configured to not using internal display, why gdm need 
> > > > to determine it again
> > instead
> > > > of users choice?
> > > > 2. Can gdm/graphics driver saves state before suspend, and restores 
> > > > saved state after resume?
> > > >If users didn't change state during suspend, then everything should 
> > > > be correct.
> > > >If users changed state during suspend, it should be acceptable for 
> > > > users to change it again to
> > make
> > > > the state correct.
> > > > See, this is obviously a case that is not so strictly related to ACPI 
> > > > button driver.
> > > > Why do we need to force button driver to marry external monitors.
> > > > B. Bug reporters are all ok with using quirk modes as boot parameters 
> > > > to work this around.
> > > > Why should we change our default behavior aimlessly?
> > >
> > > I have one more concern:
> > > In button.lid_init_state=method mode,
> > > Is that possible for libinput to work things around if _LID return value 
> > > is not correct?
> > > How libinput ensures correct timing of overwriting the input node value?
> > > Will button driver faked event value overwrites what libinput has written?
> > >
> > > From this point of view, button.lid_init_state=ignore might be a better 
> > > choice than
> > button.lid_init_state=method to allow libinput to deal with all kind of 
> > cases.
> > >
> > 
> > This is my last email on this topic, I don't even want to fully read/answer
> > the one in 1/2 given the amount of bad faith you put in that.
> 
> What's that?
> I mean, the bad faith?

I already explained 4 times why we need to revert these two patches and
why we need to keep 'method'. And you keep answering with long emails
that you would rather not. I call it bad faith, sorry.

> 
> > This is a REGRESSION. It used to work on thousands of devices, it
> > doesn't anymore. So any regression has to be chased down and no good
> > reason can justify such a regression.
> 
> I triggered many such kind of layered regressions and did fix them 1 by 1 in 
> different places.
> However, this might be different.

No. It is a regression. It used to work for thousands of devices befor
v4.11, and now it's broken for those devices. It's a regression.

Some new devices are broken with "method", it's a bug, and we can't fix
them by regressing on all the others.

> Which depends on our agreement.
> 
> > The only solution is to revert both these changes. We can not ask user
> > space to fix a kernel regression, it's not how it works.
> 
> Yes, I know.
> We just asked users to use quirk modes of button driver.

I call this "fixing by users", and this is wrong. It used to work for
years for almost everybody, you can not ask users to fix this one by
one.

> And there is in fact always one of them working.

Yes, it's called a quirk. And the good practice is to register those
quirks and make them available to everybody. Being in hwdb in user space
or in acpi/button 

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 16 2017 or thereabouts, Zheng, Lv wrote:
> Hi, Benjamin
> 
> > > > > > >> >> > > > > For example, such a hwdb entry is:
> > > > > > >> >> > > > > libinput:name:*Lid 
> > > > > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > > > > >> >> Well, if it worked in a specific way that users depended on 
> > > > > > >> >> before the commit in
> > > > > > >> >> question and now it works differently, then it does break 
> > > > > > >> >> things.
> > > > > > >> >> 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?
> > > > > Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.
> > > > I would make an argument that:
> > > > A. Is this necessarily a button driver regression?
> > > > 1. Users already configured to not using internal display, why gdm need 
> > > > to determine it again
> > instead
> > > > of users choice?
> > > > 2. Can gdm/graphics driver saves state before suspend, and restores 
> > > > saved state after resume?
> > > >If users didn't change state during suspend, then everything should 
> > > > be correct.
> > > >If users changed state during suspend, it should be acceptable for 
> > > > users to change it again to
> > make
> > > > the state correct.
> > > > See, this is obviously a case that is not so strictly related to ACPI 
> > > > button driver.
> > > > Why do we need to force button driver to marry external monitors.
> > > > B. Bug reporters are all ok with using quirk modes as boot parameters 
> > > > to work this around.
> > > > Why should we change our default behavior aimlessly?
> > >
> > > I have one more concern:
> > > In button.lid_init_state=method mode,
> > > Is that possible for libinput to work things around if _LID return value 
> > > is not correct?
> > > How libinput ensures correct timing of overwriting the input node value?
> > > Will button driver faked event value overwrites what libinput has written?
> > >
> > > From this point of view, button.lid_init_state=ignore might be a better 
> > > choice than
> > button.lid_init_state=method to allow libinput to deal with all kind of 
> > cases.
> > >
> > 
> > This is my last email on this topic, I don't even want to fully read/answer
> > the one in 1/2 given the amount of bad faith you put in that.
> 
> What's that?
> I mean, the bad faith?

I already explained 4 times why we need to revert these two patches and
why we need to keep 'method'. And you keep answering with long emails
that you would rather not. I call it bad faith, sorry.

> 
> > This is a REGRESSION. It used to work on thousands of devices, it
> > doesn't anymore. So any regression has to be chased down and no good
> > reason can justify such a regression.
> 
> I triggered many such kind of layered regressions and did fix them 1 by 1 in 
> different places.
> However, this might be different.

No. It is a regression. It used to work for thousands of devices befor
v4.11, and now it's broken for those devices. It's a regression.

Some new devices are broken with "method", it's a bug, and we can't fix
them by regressing on all the others.

> Which depends on our agreement.
> 
> > The only solution is to revert both these changes. We can not ask user
> > space to fix a kernel regression, it's not how it works.
> 
> Yes, I know.
> We just asked users to use quirk modes of button driver.

I call this "fixing by users", and this is wrong. It used to work for
years for almost everybody, you can not ask users to fix this one by
one.

> And there is in fact always one of them working.

Yes, it's called a quirk. And the good practice is to register those
quirks and make them available to everybody. Being in hwdb in user space
or in acpi/button 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Benjamin

> > > > > >> >> > > > > For example, such a hwdb entry is:
> > > > > >> >> > > > > libinput:name:*Lid 
> > > > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > > > >> >> Well, if it worked in a specific way that users depended on 
> > > > > >> >> before the commit in
> > > > > >> >> question and now it works differently, then it does break 
> > > > > >> >> things.
> > > > > >> >> 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?
> > > > Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.
> > > I would make an argument that:
> > > A. Is this necessarily a button driver regression?
> > > 1. Users already configured to not using internal display, why gdm need 
> > > to determine it again
> instead
> > > of users choice?
> > > 2. Can gdm/graphics driver saves state before suspend, and restores saved 
> > > state after resume?
> > >If users didn't change state during suspend, then everything should be 
> > > correct.
> > >If users changed state during suspend, it should be acceptable for 
> > > users to change it again to
> make
> > > the state correct.
> > > See, this is obviously a case that is not so strictly related to ACPI 
> > > button driver.
> > > Why do we need to force button driver to marry external monitors.
> > > B. Bug reporters are all ok with using quirk modes as boot parameters to 
> > > work this around.
> > > Why should we change our default behavior aimlessly?
> >
> > I have one more concern:
> > In button.lid_init_state=method mode,
> > Is that possible for libinput to work things around if _LID return value is 
> > not correct?
> > How libinput ensures correct timing of overwriting the input node value?
> > Will button driver faked event value overwrites what libinput has written?
> >
> > From this point of view, button.lid_init_state=ignore might be a better 
> > choice than
> button.lid_init_state=method to allow libinput to deal with all kind of cases.
> >
> 
> This is my last email on this topic, I don't even want to fully read/answer
> the one in 1/2 given the amount of bad faith you put in that.

What's that?
I mean, the bad faith?

> This is a REGRESSION. It used to work on thousands of devices, it
> doesn't anymore. So any regression has to be chased down and no good
> reason can justify such a regression.

I triggered many such kind of layered regressions and did fix them 1 by 1 in 
different places.
However, this might be different.
Which depends on our agreement.

> The only solution is to revert both these changes. We can not ask user
> space to fix a kernel regression, it's not how it works.

Yes, I know.
We just asked users to use quirk modes of button driver.
And there is in fact always one of them working.
We haven't asked user space to change.
We are just discussing the correctness of some user space behaviors.

> You can not also change the semantic of an input switch. An input
> switch, as per the input subsystem is supposed to forward an actual
> state of the underlying hardware. Any fake information is bad and has to
> be avoided.

Since fake events are harmful, why do we fake an event after boot/resume?
button.lid_init_state=method seems can fake such an event.

> I already gave you 2 solutions to fix the 7 machines you see that are
> problematic, and you just seem to ignore them:
> - revert to the v4.10 behavior and let libinput fix that for you

I already chose this.
But I just raised a concern that button.lid_init_state=method could bring 
troubles to libinput quirks.

> - revert to the v4.10 behavior and have a quirk database in acpi/button
> 
> I also proposed to take maintainership on this particular module because
> you said you were assigned this by default because you were the last
> 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Benjamin

> > > > > >> >> > > > > For example, such a hwdb entry is:
> > > > > >> >> > > > > libinput:name:*Lid 
> > > > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > > > >> >> Well, if it worked in a specific way that users depended on 
> > > > > >> >> before the commit in
> > > > > >> >> question and now it works differently, then it does break 
> > > > > >> >> things.
> > > > > >> >> 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?
> > > > Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.
> > > I would make an argument that:
> > > A. Is this necessarily a button driver regression?
> > > 1. Users already configured to not using internal display, why gdm need 
> > > to determine it again
> instead
> > > of users choice?
> > > 2. Can gdm/graphics driver saves state before suspend, and restores saved 
> > > state after resume?
> > >If users didn't change state during suspend, then everything should be 
> > > correct.
> > >If users changed state during suspend, it should be acceptable for 
> > > users to change it again to
> make
> > > the state correct.
> > > See, this is obviously a case that is not so strictly related to ACPI 
> > > button driver.
> > > Why do we need to force button driver to marry external monitors.
> > > B. Bug reporters are all ok with using quirk modes as boot parameters to 
> > > work this around.
> > > Why should we change our default behavior aimlessly?
> >
> > I have one more concern:
> > In button.lid_init_state=method mode,
> > Is that possible for libinput to work things around if _LID return value is 
> > not correct?
> > How libinput ensures correct timing of overwriting the input node value?
> > Will button driver faked event value overwrites what libinput has written?
> >
> > From this point of view, button.lid_init_state=ignore might be a better 
> > choice than
> button.lid_init_state=method to allow libinput to deal with all kind of cases.
> >
> 
> This is my last email on this topic, I don't even want to fully read/answer
> the one in 1/2 given the amount of bad faith you put in that.

What's that?
I mean, the bad faith?

> This is a REGRESSION. It used to work on thousands of devices, it
> doesn't anymore. So any regression has to be chased down and no good
> reason can justify such a regression.

I triggered many such kind of layered regressions and did fix them 1 by 1 in 
different places.
However, this might be different.
Which depends on our agreement.

> The only solution is to revert both these changes. We can not ask user
> space to fix a kernel regression, it's not how it works.

Yes, I know.
We just asked users to use quirk modes of button driver.
And there is in fact always one of them working.
We haven't asked user space to change.
We are just discussing the correctness of some user space behaviors.

> You can not also change the semantic of an input switch. An input
> switch, as per the input subsystem is supposed to forward an actual
> state of the underlying hardware. Any fake information is bad and has to
> be avoided.

Since fake events are harmful, why do we fake an event after boot/resume?
button.lid_init_state=method seems can fake such an event.

> I already gave you 2 solutions to fix the 7 machines you see that are
> problematic, and you just seem to ignore them:
> - revert to the v4.10 behavior and let libinput fix that for you

I already chose this.
But I just raised a concern that button.lid_init_state=method could bring 
troubles to libinput quirks.

> - revert to the v4.10 behavior and have a quirk database in acpi/button
> 
> I also proposed to take maintainership on this particular module because
> you said you were assigned this by default because you were the last
> 

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 16 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: linux-acpi-ow...@vger.kernel.org 
> > [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng,
> > Lv
> > Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > Hi, Guys
> > 
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > to lid_init_state=open"
> > >
> > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > > > <benjamin.tissoi...@redhat.com> wrote:
> > > > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > > > >> <benjamin.tissoi...@redhat.com> wrote:
> > > > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > > > >> >> > Hi,
> > > > >> >> >
> > > > >> >> > > From: Benjamin Tissoires 
> > > > >> >> > > [mailto:benjamin.tissoi...@redhat.com]
> > > > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change 
> > > > >> >> > > default behavior to
> > > lid_init_state=open"
> > > > >> >> > >
> > > > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > > >> >> > > > Hi,
> > > > >> >> > > >
> > > > >> >> > > > > From: Benjamin Tissoires 
> > > > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > > > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change 
> > > > >> >> > > > > default behavior to
> > > lid_init_state=open"
> > > > >> >> > > > >
> > > > >> >> > > > > This reverts commit 
> > > > >> >> > > > > 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > > >> >> > > > >
> > > > >> >> > > > > Even if the method implementation can be buggy on some 
> > > > >> >> > > > > platform,
> > > > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > > > >> >> > > > > basically
> > > > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > > > >> >> > > > >
> > > > >> >> > > > > On the contrary, it's rather easy in user-space to have a 
> > > > >> >> > > > > hwdb
> > > > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) 
> > > > >> >> > > > > can fix
> > > > >> >> > > > > the state of the LID switch for us: you need to set the 
> > > > >> >> > > > > udev
> > > > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 
> > > > >> >> > > > > 'write_open'.
> > > > >> >> > > > >
> > > > >> >> > > > > When libinput detects internal keyboard events, it will
> > > > >> >> > > > > overwrite the state of the switch to open, making it 
> > > > >> >> > > > > reliable
> > > > >> >> > > > > again. Given that logind only checks the LID switch value 
> > > > >> >> > > > > after
> > > > >> >> > > > > a timeout, we can assume the user will use the internal 
> > > > >> >> > > > > keyboard
> > > > >> >> > > > > before this timeout expires.
> > > > >> >> > > > >
> > > > >> >> > > > > For example, such a hwdb entry is:
> > > > >> >> > > > >
> > > > >> >> > > > > lib

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 16 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: linux-acpi-ow...@vger.kernel.org 
> > [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng,
> > Lv
> > Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > Hi, Guys
> > 
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > to lid_init_state=open"
> > >
> > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > > >  wrote:
> > > > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > > > >>  wrote:
> > > > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > > > >> >> > Hi,
> > > > >> >> >
> > > > >> >> > > From: Benjamin Tissoires 
> > > > >> >> > > [mailto:benjamin.tissoi...@redhat.com]
> > > > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change 
> > > > >> >> > > default behavior to
> > > lid_init_state=open"
> > > > >> >> > >
> > > > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > > >> >> > > > Hi,
> > > > >> >> > > >
> > > > >> >> > > > > From: Benjamin Tissoires 
> > > > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > > > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change 
> > > > >> >> > > > > default behavior to
> > > lid_init_state=open"
> > > > >> >> > > > >
> > > > >> >> > > > > This reverts commit 
> > > > >> >> > > > > 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > > >> >> > > > >
> > > > >> >> > > > > Even if the method implementation can be buggy on some 
> > > > >> >> > > > > platform,
> > > > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > > > >> >> > > > > basically
> > > > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > > > >> >> > > > >
> > > > >> >> > > > > On the contrary, it's rather easy in user-space to have a 
> > > > >> >> > > > > hwdb
> > > > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) 
> > > > >> >> > > > > can fix
> > > > >> >> > > > > the state of the LID switch for us: you need to set the 
> > > > >> >> > > > > udev
> > > > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 
> > > > >> >> > > > > 'write_open'.
> > > > >> >> > > > >
> > > > >> >> > > > > When libinput detects internal keyboard events, it will
> > > > >> >> > > > > overwrite the state of the switch to open, making it 
> > > > >> >> > > > > reliable
> > > > >> >> > > > > again. Given that logind only checks the LID switch value 
> > > > >> >> > > > > after
> > > > >> >> > > > > a timeout, we can assume the user will use the internal 
> > > > >> >> > > > > keyboard
> > > > >> >> > > > > before this timeout expires.
> > > > >> >> > > > >
> > > > >> >> > > > > For example, such a hwdb entry is:
> > > > >> >> > > > >
> > > > >> >> > > > > libinput:name:*Lid 
> > > > >> >> > > > > 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi,

> From: linux-acpi-ow...@vger.kernel.org 
> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng,
> Lv
> Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> Hi, Guys
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> >
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > > <benjamin.tissoi...@redhat.com> wrote:
> > > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > > >> <benjamin.tissoi...@redhat.com> wrote:
> > > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > > >> >> > Hi,
> > > >> >> >
> > > >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> > > >> >> > > behavior to
> > lid_init_state=open"
> > > >> >> > >
> > > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > >> >> > > > Hi,
> > > >> >> > > >
> > > >> >> > > > > From: Benjamin Tissoires 
> > > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> > > >> >> > > > > behavior to
> > lid_init_state=open"
> > > >> >> > > > >
> > > >> >> > > > > This reverts commit 
> > > >> >> > > > > 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > >> >> > > > >
> > > >> >> > > > > Even if the method implementation can be buggy on some 
> > > >> >> > > > > platform,
> > > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > > >> >> > > > > basically
> > > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > > >> >> > > > >
> > > >> >> > > > > On the contrary, it's rather easy in user-space to have a 
> > > >> >> > > > > hwdb
> > > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can 
> > > >> >> > > > > fix
> > > >> >> > > > > the state of the LID switch for us: you need to set the udev
> > > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 
> > > >> >> > > > > 'write_open'.
> > > >> >> > > > >
> > > >> >> > > > > When libinput detects internal keyboard events, it will
> > > >> >> > > > > overwrite the state of the switch to open, making it 
> > > >> >> > > > > reliable
> > > >> >> > > > > again. Given that logind only checks the LID switch value 
> > > >> >> > > > > after
> > > >> >> > > > > a timeout, we can assume the user will use the internal 
> > > >> >> > > > > keyboard
> > > >> >> > > > > before this timeout expires.
> > > >> >> > > > >
> > > >> >> > > > > For example, such a hwdb entry is:
> > > >> >> > > > >
> > > >> >> > > > > libinput:name:*Lid 
> > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > >> >> > > >
> > > >> >
> > > >> > [...]
> > > >> >
> > > >> >>
> > > >> >> Well, if it worked in a specific way that users depended on befor

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi,

> From: linux-acpi-ow...@vger.kernel.org 
> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng,
> Lv
> Subject: RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> Hi, Guys
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> >
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > >  wrote:
> > > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > > >>  wrote:
> > > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > > >> >> > Hi,
> > > >> >> >
> > > >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> > > >> >> > > behavior to
> > lid_init_state=open"
> > > >> >> > >
> > > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > >> >> > > > Hi,
> > > >> >> > > >
> > > >> >> > > > > From: Benjamin Tissoires 
> > > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> > > >> >> > > > > behavior to
> > lid_init_state=open"
> > > >> >> > > > >
> > > >> >> > > > > This reverts commit 
> > > >> >> > > > > 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > >> >> > > > >
> > > >> >> > > > > Even if the method implementation can be buggy on some 
> > > >> >> > > > > platform,
> > > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > > >> >> > > > > basically
> > > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > > >> >> > > > >
> > > >> >> > > > > On the contrary, it's rather easy in user-space to have a 
> > > >> >> > > > > hwdb
> > > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can 
> > > >> >> > > > > fix
> > > >> >> > > > > the state of the LID switch for us: you need to set the udev
> > > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 
> > > >> >> > > > > 'write_open'.
> > > >> >> > > > >
> > > >> >> > > > > When libinput detects internal keyboard events, it will
> > > >> >> > > > > overwrite the state of the switch to open, making it 
> > > >> >> > > > > reliable
> > > >> >> > > > > again. Given that logind only checks the LID switch value 
> > > >> >> > > > > after
> > > >> >> > > > > a timeout, we can assume the user will use the internal 
> > > >> >> > > > > keyboard
> > > >> >> > > > > before this timeout expires.
> > > >> >> > > > >
> > > >> >> > > > > For example, such a hwdb entry is:
> > > >> >> > > > >
> > > >> >> > > > > libinput:name:*Lid 
> > > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > >> >> > > >
> > > >> >
> > > >> > [...]
> > > >> >
> > > >> >>
> > > >> >> Well, if it worked in a specific way that users depended on before 
> > > >> >> the commit in
> > > &

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Guys

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> > <benjamin.tissoi...@redhat.com> wrote:
> > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > >> <benjamin.tissoi...@redhat.com> wrote:
> > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > >> >> > Hi,
> > >> >> >
> > >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> > >> >> > > behavior to
> lid_init_state=open"
> > >> >> > >
> > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > >> >> > > > Hi,
> > >> >> > > >
> > >> >> > > > > From: Benjamin Tissoires 
> > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> > >> >> > > > > behavior to
> lid_init_state=open"
> > >> >> > > > >
> > >> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > >> >> > > > >
> > >> >> > > > > Even if the method implementation can be buggy on some 
> > >> >> > > > > platform,
> > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > >> >> > > > > basically
> > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > >> >> > > > >
> > >> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can 
> > >> >> > > > > fix
> > >> >> > > > > the state of the LID switch for us: you need to set the udev
> > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > >> >> > > > >
> > >> >> > > > > When libinput detects internal keyboard events, it will
> > >> >> > > > > overwrite the state of the switch to open, making it reliable
> > >> >> > > > > again. Given that logind only checks the LID switch value 
> > >> >> > > > > after
> > >> >> > > > > a timeout, we can assume the user will use the internal 
> > >> >> > > > > keyboard
> > >> >> > > > > before this timeout expires.
> > >> >> > > > >
> > >> >> > > > > For example, such a hwdb entry is:
> > >> >> > > > >
> > >> >> > > > > libinput:name:*Lid 
> > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > >> >> > > >
> > >> >
> > >> > [...]
> > >> >
> > >> >>
> > >> >> Well, if it worked in a specific way that users depended on before 
> > >> >> the commit in
> > >> >> question and now it works differently, then it does break things.
> > >> >>
> > >> >> 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?
> >
> 
> Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.

I would make an argument that:
A. Is this necessarily a button driver regression?
1. Users already configured to not using internal display, why gdm need to 
determine it again instead of users choice?
2. Can gdm/graphics driver saves state before suspend, and restores saved state 
after resume?
   If users didn't change state during suspend, then everything should be 
correct.
   If users changed state during suspend, it should be acceptable for users to 
change it again to make the state correct.
See, this is obviously a case that is not so strictly related to ACPI button 
driver.
Why do we need to force button driver to marry external monitors.
B. Bug reporters are all ok with using quirk modes as boot parameters to work 
this around.
Why should we change our default behavior aimlessly?

Thanks and best regards
Lv

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, Guys

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> >  wrote:
> > > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> > >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> > >>  wrote:
> > >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> > >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > >> >> > Hi,
> > >> >> >
> > >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> > >> >> > > behavior to
> lid_init_state=open"
> > >> >> > >
> > >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > >> >> > > > Hi,
> > >> >> > > >
> > >> >> > > > > From: Benjamin Tissoires 
> > >> >> > > > > [mailto:benjamin.tissoi...@redhat.com]
> > >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> > >> >> > > > > behavior to
> lid_init_state=open"
> > >> >> > > > >
> > >> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > >> >> > > > >
> > >> >> > > > > Even if the method implementation can be buggy on some 
> > >> >> > > > > platform,
> > >> >> > > > > the "open" choice is worse. It breaks docking stations 
> > >> >> > > > > basically
> > >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> > >> >> > > > >
> > >> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> > >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can 
> > >> >> > > > > fix
> > >> >> > > > > the state of the LID switch for us: you need to set the udev
> > >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > >> >> > > > >
> > >> >> > > > > When libinput detects internal keyboard events, it will
> > >> >> > > > > overwrite the state of the switch to open, making it reliable
> > >> >> > > > > again. Given that logind only checks the LID switch value 
> > >> >> > > > > after
> > >> >> > > > > a timeout, we can assume the user will use the internal 
> > >> >> > > > > keyboard
> > >> >> > > > > before this timeout expires.
> > >> >> > > > >
> > >> >> > > > > For example, such a hwdb entry is:
> > >> >> > > > >
> > >> >> > > > > libinput:name:*Lid 
> > >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > >> >> > > >
> > >> >
> > >> > [...]
> > >> >
> > >> >>
> > >> >> Well, if it worked in a specific way that users depended on before 
> > >> >> the commit in
> > >> >> question and now it works differently, then it does break things.
> > >> >>
> > >> >> 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?
> >
> 
> Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.

I would make an argument that:
A. Is this necessarily a button driver regression?
1. Users already configured to not using internal display, why gdm need to 
determine it again instead of users choice?
2. Can gdm/graphics driver saves state before suspend, and restores saved state 
after resume?
   If users didn't change state during suspend, then everything should be 
correct.
   If users changed state during suspend, it should be acceptable for users to 
change it again to make the state correct.
See, this is obviously a case that is not so strictly related to ACPI button 
driver.
Why do we need to force button driver to marry external monitors.
B. Bug reporters are all ok with using quirk modes as boot parameters to work 
this around.
Why should we change our default behavior aimlessly?

Thanks and best regards
Lv

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> <benjamin.tissoi...@redhat.com> wrote:
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> >> <benjamin.tissoi...@redhat.com> wrote:
> >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> >> >> > Hi,
> >> >> >
> >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> >> > > behavior to lid_init_state=open"
> >> >> > >
> >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> >> >> > > > Hi,
> >> >> > > >
> >> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> >> > > > > behavior to lid_init_state=open"
> >> >> > > > >
> >> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> >> >> > > > >
> >> >> > > > > Even if the method implementation can be buggy on some platform,
> >> >> > > > > the "open" choice is worse. It breaks docking stations basically
> >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> >> >> > > > >
> >> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> >> >> > > > > the state of the LID switch for us: you need to set the udev
> >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> >> >> > > > >
> >> >> > > > > When libinput detects internal keyboard events, it will
> >> >> > > > > overwrite the state of the switch to open, making it reliable
> >> >> > > > > again. Given that logind only checks the LID switch value after
> >> >> > > > > a timeout, we can assume the user will use the internal keyboard
> >> >> > > > > before this timeout expires.
> >> >> > > > >
> >> >> > > > > For example, such a hwdb entry is:
> >> >> > > > >
> >> >> > > > > libinput:name:*Lid 
> >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >> >> > > >
> >> >
> >> > [...]
> >> >
> >> >>
> >> >> Well, if it worked in a specific way that users depended on before the 
> >> >> commit in
> >> >> question and now it works differently, then it does break things.
> >> >>
> >> >> 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?
> 

Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
>  wrote:
> > On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> >>  wrote:
> >> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> >> >> > Hi,
> >> >> >
> >> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> >> > > behavior to lid_init_state=open"
> >> >> > >
> >> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> >> >> > > > Hi,
> >> >> > > >
> >> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> >> > > > > behavior to lid_init_state=open"
> >> >> > > > >
> >> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> >> >> > > > >
> >> >> > > > > Even if the method implementation can be buggy on some platform,
> >> >> > > > > the "open" choice is worse. It breaks docking stations basically
> >> >> > > > > and there is no way to have a user-space hwdb to fix that.
> >> >> > > > >
> >> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> >> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> >> >> > > > > the state of the LID switch for us: you need to set the udev
> >> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> >> >> > > > >
> >> >> > > > > When libinput detects internal keyboard events, it will
> >> >> > > > > overwrite the state of the switch to open, making it reliable
> >> >> > > > > again. Given that logind only checks the LID switch value after
> >> >> > > > > a timeout, we can assume the user will use the internal keyboard
> >> >> > > > > before this timeout expires.
> >> >> > > > >
> >> >> > > > > For example, such a hwdb entry is:
> >> >> > > > >
> >> >> > > > > libinput:name:*Lid 
> >> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >> >> > > >
> >> >
> >> > [...]
> >> >
> >> >>
> >> >> Well, if it worked in a specific way that users depended on before the 
> >> >> commit in
> >> >> question and now it works differently, then it does break things.
> >> >>
> >> >> 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?
> 

Yes, reverting these 2 patches restores the pre v4.11 kernel behavior.

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
>> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
>> <benjamin.tissoi...@redhat.com> wrote:
>> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
>> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
>> >> > Hi,
>> >> >
>> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
>> >> > > behavior to lid_init_state=open"
>> >> > >
>> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
>> >> > > > Hi,
>> >> > > >
>> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
>> >> > > > > behavior to lid_init_state=open"
>> >> > > > >
>> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
>> >> > > > >
>> >> > > > > Even if the method implementation can be buggy on some platform,
>> >> > > > > the "open" choice is worse. It breaks docking stations basically
>> >> > > > > and there is no way to have a user-space hwdb to fix that.
>> >> > > > >
>> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
>> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
>> >> > > > > the state of the LID switch for us: you need to set the udev
>> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
>> >> > > > >
>> >> > > > > When libinput detects internal keyboard events, it will
>> >> > > > > overwrite the state of the switch to open, making it reliable
>> >> > > > > again. Given that logind only checks the LID switch value after
>> >> > > > > a timeout, we can assume the user will use the internal keyboard
>> >> > > > > before this timeout expires.
>> >> > > > >
>> >> > > > > For example, such a hwdb entry is:
>> >> > > > >
>> >> > > > > libinput:name:*Lid 
>> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
>> >> > > >
>> >
>> > [...]
>> >
>> >>
>> >> Well, if it worked in a specific way that users depended on before the 
>> >> commit in
>> >> question and now it works differently, then it does break things.
>> >>
>> >> 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?

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
 wrote:
> On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
>> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
>>  wrote:
>> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
>> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
>> >> > Hi,
>> >> >
>> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
>> >> > > behavior to lid_init_state=open"
>> >> > >
>> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
>> >> > > > Hi,
>> >> > > >
>> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
>> >> > > > > behavior to lid_init_state=open"
>> >> > > > >
>> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
>> >> > > > >
>> >> > > > > Even if the method implementation can be buggy on some platform,
>> >> > > > > the "open" choice is worse. It breaks docking stations basically
>> >> > > > > and there is no way to have a user-space hwdb to fix that.
>> >> > > > >
>> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
>> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
>> >> > > > > the state of the LID switch for us: you need to set the udev
>> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
>> >> > > > >
>> >> > > > > When libinput detects internal keyboard events, it will
>> >> > > > > overwrite the state of the switch to open, making it reliable
>> >> > > > > again. Given that logind only checks the LID switch value after
>> >> > > > > a timeout, we can assume the user will use the internal keyboard
>> >> > > > > before this timeout expires.
>> >> > > > >
>> >> > > > > For example, such a hwdb entry is:
>> >> > > > >
>> >> > > > > libinput:name:*Lid 
>> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
>> >> > > >
>> >
>> > [...]
>> >
>> >>
>> >> Well, if it worked in a specific way that users depended on before the 
>> >> commit in
>> >> question and now it works differently, then it does break things.
>> >>
>> >> 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?

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> <benjamin.tissoi...@redhat.com> wrote:
> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> >> > Hi,
> >> >
> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> > > behavior to lid_init_state=open"
> >> > >
> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> >> > > > Hi,
> >> > > >
> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> > > > > behavior to lid_init_state=open"
> >> > > > >
> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> >> > > > >
> >> > > > > Even if the method implementation can be buggy on some platform,
> >> > > > > the "open" choice is worse. It breaks docking stations basically
> >> > > > > and there is no way to have a user-space hwdb to fix that.
> >> > > > >
> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> >> > > > > the state of the LID switch for us: you need to set the udev
> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> >> > > > >
> >> > > > > When libinput detects internal keyboard events, it will
> >> > > > > overwrite the state of the switch to open, making it reliable
> >> > > > > again. Given that logind only checks the LID switch value after
> >> > > > > a timeout, we can assume the user will use the internal keyboard
> >> > > > > before this timeout expires.
> >> > > > >
> >> > > > > For example, such a hwdb entry is:
> >> > > > >
> >> > > > > libinput:name:*Lid 
> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >> > > >
> >
> > [...]
> >
> >>
> >> Well, if it worked in a specific way that users depended on before the 
> >> commit in
> >> question and now it works differently, then it does break things.
> >>
> >> 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.

Cheers,
Benjamin

> 
> Thanks,
> Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
>  wrote:
> > On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> >> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> >> > Hi,
> >> >
> >> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> > > behavior to lid_init_state=open"
> >> > >
> >> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> >> > > > Hi,
> >> > > >
> >> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> >> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default 
> >> > > > > behavior to lid_init_state=open"
> >> > > > >
> >> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> >> > > > >
> >> > > > > Even if the method implementation can be buggy on some platform,
> >> > > > > the "open" choice is worse. It breaks docking stations basically
> >> > > > > and there is no way to have a user-space hwdb to fix that.
> >> > > > >
> >> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> >> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> >> > > > > the state of the LID switch for us: you need to set the udev
> >> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> >> > > > >
> >> > > > > When libinput detects internal keyboard events, it will
> >> > > > > overwrite the state of the switch to open, making it reliable
> >> > > > > again. Given that logind only checks the LID switch value after
> >> > > > > a timeout, we can assume the user will use the internal keyboard
> >> > > > > before this timeout expires.
> >> > > > >
> >> > > > > For example, such a hwdb entry is:
> >> > > > >
> >> > > > > libinput:name:*Lid 
> >> > > > > Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >> > > >
> >
> > [...]
> >
> >>
> >> Well, if it worked in a specific way that users depended on before the 
> >> commit in
> >> question and now it works differently, then it does break things.
> >>
> >> 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.

Cheers,
Benjamin

> 
> Thanks,
> Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
>> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
>> > Hi,
>> >
>> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
>> > > to lid_init_state=open"
>> > >
>> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
>> > > > Hi,
>> > > >
>> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
>> > > > > to lid_init_state=open"
>> > > > >
>> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
>> > > > >
>> > > > > Even if the method implementation can be buggy on some platform,
>> > > > > the "open" choice is worse. It breaks docking stations basically
>> > > > > and there is no way to have a user-space hwdb to fix that.
>> > > > >
>> > > > > On the contrary, it's rather easy in user-space to have a hwdb
>> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
>> > > > > the state of the LID switch for us: you need to set the udev
>> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
>> > > > >
>> > > > > When libinput detects internal keyboard events, it will
>> > > > > overwrite the state of the switch to open, making it reliable
>> > > > > again. Given that logind only checks the LID switch value after
>> > > > > a timeout, we can assume the user will use the internal keyboard
>> > > > > before this timeout expires.
>> > > > >
>> > > > > For example, such a hwdb entry is:
>> > > > >
>> > > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
>> > > >
>
> [...]
>
>>
>> Well, if it worked in a specific way that users depended on before the 
>> commit in
>> question and now it works differently, then it does break things.
>>
>> 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?

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
 wrote:
> On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
>> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
>> > Hi,
>> >
>> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
>> > > to lid_init_state=open"
>> > >
>> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
>> > > > Hi,
>> > > >
>> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
>> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
>> > > > > to lid_init_state=open"
>> > > > >
>> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
>> > > > >
>> > > > > Even if the method implementation can be buggy on some platform,
>> > > > > the "open" choice is worse. It breaks docking stations basically
>> > > > > and there is no way to have a user-space hwdb to fix that.
>> > > > >
>> > > > > On the contrary, it's rather easy in user-space to have a hwdb
>> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
>> > > > > the state of the LID switch for us: you need to set the udev
>> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
>> > > > >
>> > > > > When libinput detects internal keyboard events, it will
>> > > > > overwrite the state of the switch to open, making it reliable
>> > > > > again. Given that logind only checks the LID switch value after
>> > > > > a timeout, we can assume the user will use the internal keyboard
>> > > > > before this timeout expires.
>> > > > >
>> > > > > For example, such a hwdb entry is:
>> > > > >
>> > > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
>> > > >
>
> [...]
>
>>
>> Well, if it worked in a specific way that users depended on before the 
>> commit in
>> question and now it works differently, then it does break things.
>>
>> 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?

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > Hi, 
> > 
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > to lid_init_state=open"
> > > 
> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > > Hi,
> > > >
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > > > to lid_init_state=open"
> > > > >
> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > > >
> > > > > Even if the method implementation can be buggy on some platform,
> > > > > the "open" choice is worse. It breaks docking stations basically
> > > > > and there is no way to have a user-space hwdb to fix that.
> > > > >
> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > > > the state of the LID switch for us: you need to set the udev
> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > > > >
> > > > > When libinput detects internal keyboard events, it will
> > > > > overwrite the state of the switch to open, making it reliable
> > > > > again. Given that logind only checks the LID switch value after
> > > > > a timeout, we can assume the user will use the internal keyboard
> > > > > before this timeout expires.
> > > > >
> > > > > For example, such a hwdb entry is:
> > > > >
> > > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > >

[...]

> 
> Well, if it worked in a specific way that users depended on before the commit 
> in
> question and now it works differently, then it does break things.
> 
> 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.

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 12 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> > Hi, 
> > 
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > to lid_init_state=open"
> > > 
> > > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > > Hi,
> > > >
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior 
> > > > > to lid_init_state=open"
> > > > >
> > > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > > >
> > > > > Even if the method implementation can be buggy on some platform,
> > > > > the "open" choice is worse. It breaks docking stations basically
> > > > > and there is no way to have a user-space hwdb to fix that.
> > > > >
> > > > > On the contrary, it's rather easy in user-space to have a hwdb
> > > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > > > the state of the LID switch for us: you need to set the udev
> > > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > > > >
> > > > > When libinput detects internal keyboard events, it will
> > > > > overwrite the state of the switch to open, making it reliable
> > > > > again. Given that logind only checks the LID switch value after
> > > > > a timeout, we can assume the user will use the internal keyboard
> > > > > before this timeout expires.
> > > > >
> > > > > For example, such a hwdb entry is:
> > > > >
> > > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > > >

[...]

> 
> Well, if it worked in a specific way that users depended on before the commit 
> in
> question and now it works differently, then it does break things.
> 
> 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.

Cheers,
Benjamin

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> Hi, 
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > Hi,
> > >
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > > > lid_init_state=open"
> > > >
> > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > >
> > > > Even if the method implementation can be buggy on some platform,
> > > > the "open" choice is worse. It breaks docking stations basically
> > > > and there is no way to have a user-space hwdb to fix that.
> > > >
> > > > On the contrary, it's rather easy in user-space to have a hwdb
> > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > > the state of the LID switch for us: you need to set the udev
> > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > > >
> > > > When libinput detects internal keyboard events, it will
> > > > overwrite the state of the switch to open, making it reliable
> > > > again. Given that logind only checks the LID switch value after
> > > > a timeout, we can assume the user will use the internal keyboard
> > > > before this timeout expires.
> > > >
> > > > For example, such a hwdb entry is:
> > > >
> > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > >
> > > For the reason mentioned previously and proofs here (see patch 
> > > descriptions):
> > > https://patchwork.kernel.org/patch/9717111/
> > 
> > I am not sure you can call this proofs. The "close" state is IMO the
> > exact same as the "method" one, it's just that the intel driver
> > triggers the evaluation of the _LID method, not acpi/button.c.
> 
> I should correct you that:
> 1. intel_lvds driver only evaluates _LID method for its own purpose,
>See my reply to PATCH 4-5;
> 2. acpi/button.c is still responsible for generating the lid input event (to 
> input layer and thus the userspace)
>And the input event generated by button.c differs for the 2 modes.
>See my another reply to PATCH 02.
> 
> > 
> > And remember that this new default prevents userspace to fix it because
> > it's rather easy to detect that the device is actually opened (input
> > events coming from interanl keyboard, touchscreen, or touchpad), while
> > reporting the lid switch as open means we can't know if the user is
> > simply not using the internal devices, or if we have just a wrong state.
> 
> That depends on what the meaning of "fix" is IMO.
> I saw you wrote a lot in another message, let's discuss this there.
> 
> > 
> > Given that this patch breaks all Lenovos with a dock (I can guess that 6
> > lines this year are using docks, and within each line you have 2-5
> > variants), I'd suggest we do not break those existing laptops and just
> > revert this patch.
> > 
> > I would think other OEMs have a docking station with an actual power
> > button but I can't be sure by looking at the product pages.
> 
> I'm not sure if it breaks the external monitor usage model.

Well, if it worked in a specific way that users depended on before the commit in
question and now it works differently, then it does break things.

Benjamin, my understanding is that this is the case, is it correct?

Thanks,
Rafael

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Rafael J. Wysocki]

On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> Hi, 
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > > Hi,
> > >
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > > > lid_init_state=open"
> > > >
> > > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > > >
> > > > Even if the method implementation can be buggy on some platform,
> > > > the "open" choice is worse. It breaks docking stations basically
> > > > and there is no way to have a user-space hwdb to fix that.
> > > >
> > > > On the contrary, it's rather easy in user-space to have a hwdb
> > > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > > the state of the LID switch for us: you need to set the udev
> > > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > > >
> > > > When libinput detects internal keyboard events, it will
> > > > overwrite the state of the switch to open, making it reliable
> > > > again. Given that logind only checks the LID switch value after
> > > > a timeout, we can assume the user will use the internal keyboard
> > > > before this timeout expires.
> > > >
> > > > For example, such a hwdb entry is:
> > > >
> > > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> > >
> > > For the reason mentioned previously and proofs here (see patch 
> > > descriptions):
> > > https://patchwork.kernel.org/patch/9717111/
> > 
> > I am not sure you can call this proofs. The "close" state is IMO the
> > exact same as the "method" one, it's just that the intel driver
> > triggers the evaluation of the _LID method, not acpi/button.c.
> 
> I should correct you that:
> 1. intel_lvds driver only evaluates _LID method for its own purpose,
>See my reply to PATCH 4-5;
> 2. acpi/button.c is still responsible for generating the lid input event (to 
> input layer and thus the userspace)
>And the input event generated by button.c differs for the 2 modes.
>See my another reply to PATCH 02.
> 
> > 
> > And remember that this new default prevents userspace to fix it because
> > it's rather easy to detect that the device is actually opened (input
> > events coming from interanl keyboard, touchscreen, or touchpad), while
> > reporting the lid switch as open means we can't know if the user is
> > simply not using the internal devices, or if we have just a wrong state.
> 
> That depends on what the meaning of "fix" is IMO.
> I saw you wrote a lot in another message, let's discuss this there.
> 
> > 
> > Given that this patch breaks all Lenovos with a dock (I can guess that 6
> > lines this year are using docks, and within each line you have 2-5
> > variants), I'd suggest we do not break those existing laptops and just
> > revert this patch.
> > 
> > I would think other OEMs have a docking station with an actual power
> > button but I can't be sure by looking at the product pages.
> 
> I'm not sure if it breaks the external monitor usage model.

Well, if it worked in a specific way that users depended on before the commit in
question and now it works differently, then it does break things.

Benjamin, my understanding is that this is the case, is it correct?

Thanks,
Rafael

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, 

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > Hi,
> >
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > > lid_init_state=open"
> > >
> > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > >
> > > Even if the method implementation can be buggy on some platform,
> > > the "open" choice is worse. It breaks docking stations basically
> > > and there is no way to have a user-space hwdb to fix that.
> > >
> > > On the contrary, it's rather easy in user-space to have a hwdb
> > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > the state of the LID switch for us: you need to set the udev
> > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > >
> > > When libinput detects internal keyboard events, it will
> > > overwrite the state of the switch to open, making it reliable
> > > again. Given that logind only checks the LID switch value after
> > > a timeout, we can assume the user will use the internal keyboard
> > > before this timeout expires.
> > >
> > > For example, such a hwdb entry is:
> > >
> > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >
> > For the reason mentioned previously and proofs here (see patch 
> > descriptions):
> > https://patchwork.kernel.org/patch/9717111/
> 
> I am not sure you can call this proofs. The "close" state is IMO the
> exact same as the "method" one, it's just that the intel driver
> triggers the evaluation of the _LID method, not acpi/button.c.

I should correct you that:
1. intel_lvds driver only evaluates _LID method for its own purpose,
   See my reply to PATCH 4-5;
2. acpi/button.c is still responsible for generating the lid input event (to 
input layer and thus the userspace)
   And the input event generated by button.c differs for the 2 modes.
   See my another reply to PATCH 02.

> 
> And remember that this new default prevents userspace to fix it because
> it's rather easy to detect that the device is actually opened (input
> events coming from interanl keyboard, touchscreen, or touchpad), while
> reporting the lid switch as open means we can't know if the user is
> simply not using the internal devices, or if we have just a wrong state.

That depends on what the meaning of "fix" is IMO.
I saw you wrote a lot in another message, let's discuss this there.

> 
> Given that this patch breaks all Lenovos with a dock (I can guess that 6
> lines this year are using docks, and within each line you have 2-5
> variants), I'd suggest we do not break those existing laptops and just
> revert this patch.
> 
> I would think other OEMs have a docking station with an actual power
> button but I can't be sure by looking at the product pages.

I'm not sure if it breaks the external monitor usage model.
Why don't the userspace just
1. always light on the external display and keep the internal display not lit 
on after boot/resume, or
2. don't do anything and let the BIOS to light on the right display, or
3. don't do anything and let the graphics drivers to light on the right display 
(using saved state when suspends and resumes to that saved state).

Why such decision have to be made based on ACPI control method lid device?
I cannot see a strong reason that ACPI control method lid device must 
participate in this usage model.

See drivers/gpu/drm/nouveau/nouveau_connector.c,
the ignorelid parameter proves that the graphics drivers can do this on their 
own and do this perfectly.
 
> 
> > We shouldn't do this.
> 
> I strongly disagree.
> 
> I am fine if you don't want to have a blacklist in the kernel for the
> devices that are problematic (the ones that require open), but please
> don't prevent user space to have this blacklist and please do not make
> false assumptions in the kernel on the state of a switch. This is worse
> than it helps and in the end, user space won't be able to solve this if
> you change the default behavior at each release.

We are actually also fine with any default value.

But be aware of that, ACPI subsystem just provides a button driver:
1. Allows users to suspend the system upon receiving "close" lid event;
2. Stops to support any other usage models but is willing to provide tunable 
behavior for the other s

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi, 

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> On May 11 2017 or thereabouts, Zheng, Lv wrote:
> > Hi,
> >
> > > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > > lid_init_state=open"
> > >
> > > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > >
> > > Even if the method implementation can be buggy on some platform,
> > > the "open" choice is worse. It breaks docking stations basically
> > > and there is no way to have a user-space hwdb to fix that.
> > >
> > > On the contrary, it's rather easy in user-space to have a hwdb
> > > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > > the state of the LID switch for us: you need to set the udev
> > > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > >
> > > When libinput detects internal keyboard events, it will
> > > overwrite the state of the switch to open, making it reliable
> > > again. Given that logind only checks the LID switch value after
> > > a timeout, we can assume the user will use the internal keyboard
> > > before this timeout expires.
> > >
> > > For example, such a hwdb entry is:
> > >
> > > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> > >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> >
> > For the reason mentioned previously and proofs here (see patch 
> > descriptions):
> > https://patchwork.kernel.org/patch/9717111/
> 
> I am not sure you can call this proofs. The "close" state is IMO the
> exact same as the "method" one, it's just that the intel driver
> triggers the evaluation of the _LID method, not acpi/button.c.

I should correct you that:
1. intel_lvds driver only evaluates _LID method for its own purpose,
   See my reply to PATCH 4-5;
2. acpi/button.c is still responsible for generating the lid input event (to 
input layer and thus the userspace)
   And the input event generated by button.c differs for the 2 modes.
   See my another reply to PATCH 02.

> 
> And remember that this new default prevents userspace to fix it because
> it's rather easy to detect that the device is actually opened (input
> events coming from interanl keyboard, touchscreen, or touchpad), while
> reporting the lid switch as open means we can't know if the user is
> simply not using the internal devices, or if we have just a wrong state.

That depends on what the meaning of "fix" is IMO.
I saw you wrote a lot in another message, let's discuss this there.

> 
> Given that this patch breaks all Lenovos with a dock (I can guess that 6
> lines this year are using docks, and within each line you have 2-5
> variants), I'd suggest we do not break those existing laptops and just
> revert this patch.
> 
> I would think other OEMs have a docking station with an actual power
> button but I can't be sure by looking at the product pages.

I'm not sure if it breaks the external monitor usage model.
Why don't the userspace just
1. always light on the external display and keep the internal display not lit 
on after boot/resume, or
2. don't do anything and let the BIOS to light on the right display, or
3. don't do anything and let the graphics drivers to light on the right display 
(using saved state when suspends and resumes to that saved state).

Why such decision have to be made based on ACPI control method lid device?
I cannot see a strong reason that ACPI control method lid device must 
participate in this usage model.

See drivers/gpu/drm/nouveau/nouveau_connector.c,
the ignorelid parameter proves that the graphics drivers can do this on their 
own and do this perfectly.
 
> 
> > We shouldn't do this.
> 
> I strongly disagree.
> 
> I am fine if you don't want to have a blacklist in the kernel for the
> devices that are problematic (the ones that require open), but please
> don't prevent user space to have this blacklist and please do not make
> false assumptions in the kernel on the state of a switch. This is worse
> than it helps and in the end, user space won't be able to solve this if
> you change the default behavior at each release.

We are actually also fine with any default value.

But be aware of that, ACPI subsystem just provides a button driver:
1. Allows users to suspend the system upon receiving "close" lid event;
2. Stops to support any other usage models but is willing to provide tunable 
behavior for the other s

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 11 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > 
> > Even if the method implementation can be buggy on some platform,
> > the "open" choice is worse. It breaks docking stations basically
> > and there is no way to have a user-space hwdb to fix that.
> > 
> > On the contrary, it's rather easy in user-space to have a hwdb
> > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > the state of the LID switch for us: you need to set the udev
> > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > 
> > When libinput detects internal keyboard events, it will
> > overwrite the state of the switch to open, making it reliable
> > again. Given that logind only checks the LID switch value after
> > a timeout, we can assume the user will use the internal keyboard
> > before this timeout expires.
> > 
> > For example, such a hwdb entry is:
> > 
> > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> 
> For the reason mentioned previously and proofs here (see patch descriptions):
> https://patchwork.kernel.org/patch/9717111/

I am not sure you can call this proofs. The "close" state is IMO the
exact same as the "method" one, it's just that the intel driver
triggers the evaluation of the _LID method, not acpi/button.c.

And remember that this new default prevents userspace to fix it because
it's rather easy to detect that the device is actually opened (input
events coming from interanl keyboard, touchscreen, or touchpad), while
reporting the lid switch as open means we can't know if the user is
simply not using the internal devices, or if we have just a wrong state.

Given that this patch breaks all Lenovos with a dock (I can guess that 6
lines this year are using docks, and within each line you have 2-5
variants), I'd suggest we do not break those existing laptops and just
revert this patch.

I would think other OEMs have a docking station with an actual power
button but I can't be sure by looking at the product pages.

> We shouldn't do this.

I strongly disagree.

I am fine if you don't want to have a blacklist in the kernel for the
devices that are problematic (the ones that require open), but please
don't prevent user space to have this blacklist and please do not make
false assumptions in the kernel on the state of a switch. This is worse
than it helps and in the end, user space won't be able to solve this if
you change the default behavior at each release.

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv
> 
> > 
> > Link: https://bugzilla.gnome.org/show_bug.cgi?id=782380
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires 
> > ---
> >  drivers/acpi/button.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 6d5a8c1..e19f530 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -113,7 +113,7 @@ struct acpi_button {
> > 
> >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> > -static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > 
> >  static unsigned long lid_report_interval __read_mostly = 500;
> >  module_param(lid_report_interval, ulong, 0644);
> > --
> > 2.9.3
> 

Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Benjamin Tissoires]

On May 11 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> > Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> > lid_init_state=open"
> > 
> > This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> > 
> > Even if the method implementation can be buggy on some platform,
> > the "open" choice is worse. It breaks docking stations basically
> > and there is no way to have a user-space hwdb to fix that.
> > 
> > On the contrary, it's rather easy in user-space to have a hwdb
> > with the problematic platforms. Then, libinput (1.7.0+) can fix
> > the state of the LID switch for us: you need to set the udev
> > property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> > 
> > When libinput detects internal keyboard events, it will
> > overwrite the state of the switch to open, making it reliable
> > again. Given that logind only checks the LID switch value after
> > a timeout, we can assume the user will use the internal keyboard
> > before this timeout expires.
> > 
> > For example, such a hwdb entry is:
> > 
> > libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
> >  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open
> 
> For the reason mentioned previously and proofs here (see patch descriptions):
> https://patchwork.kernel.org/patch/9717111/

I am not sure you can call this proofs. The "close" state is IMO the
exact same as the "method" one, it's just that the intel driver
triggers the evaluation of the _LID method, not acpi/button.c.

And remember that this new default prevents userspace to fix it because
it's rather easy to detect that the device is actually opened (input
events coming from interanl keyboard, touchscreen, or touchpad), while
reporting the lid switch as open means we can't know if the user is
simply not using the internal devices, or if we have just a wrong state.

Given that this patch breaks all Lenovos with a dock (I can guess that 6
lines this year are using docks, and within each line you have 2-5
variants), I'd suggest we do not break those existing laptops and just
revert this patch.

I would think other OEMs have a docking station with an actual power
button but I can't be sure by looking at the product pages.

> We shouldn't do this.

I strongly disagree.

I am fine if you don't want to have a blacklist in the kernel for the
devices that are problematic (the ones that require open), but please
don't prevent user space to have this blacklist and please do not make
false assumptions in the kernel on the state of a switch. This is worse
than it helps and in the end, user space won't be able to solve this if
you change the default behavior at each release.

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv
> 
> > 
> > Link: https://bugzilla.gnome.org/show_bug.cgi?id=782380
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Benjamin Tissoires 
> > ---
> >  drivers/acpi/button.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 6d5a8c1..e19f530 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -113,7 +113,7 @@ struct acpi_button {
> > 
> >  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> >  static struct acpi_device *lid_device;
> > -static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> > +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> > 
> >  static unsigned long lid_report_interval __read_mostly = 500;
> >  module_param(lid_report_interval, ulong, 0644);
> > --
> > 2.9.3
> 

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> 
> Even if the method implementation can be buggy on some platform,
> the "open" choice is worse. It breaks docking stations basically
> and there is no way to have a user-space hwdb to fix that.
> 
> On the contrary, it's rather easy in user-space to have a hwdb
> with the problematic platforms. Then, libinput (1.7.0+) can fix
> the state of the LID switch for us: you need to set the udev
> property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> 
> When libinput detects internal keyboard events, it will
> overwrite the state of the switch to open, making it reliable
> again. Given that logind only checks the LID switch value after
> a timeout, we can assume the user will use the internal keyboard
> before this timeout expires.
> 
> For example, such a hwdb entry is:
> 
> libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open

For the reason mentioned previously and proofs here (see patch descriptions):
https://patchwork.kernel.org/patch/9717111/
We shouldn't do this.

Thanks and best regards
Lv

> 
> Link: https://bugzilla.gnome.org/show_bug.cgi?id=782380
> Cc: sta...@vger.kernel.org
> Signed-off-by: Benjamin Tissoires 
> ---
>  drivers/acpi/button.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 6d5a8c1..e19f530 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -113,7 +113,7 @@ struct acpi_button {
> 
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
> -static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> 
>  static unsigned long lid_report_interval __read_mostly = 500;
>  module_param(lid_report_interval, ulong, 0644);
> --
> 2.9.3

RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open"

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> Subject: [PATCH 2/2] Revert "ACPI / button: Change default behavior to 
> lid_init_state=open"
> 
> This reverts commit 77e9a4aa9de10cc1418bf9a892366988802a8025.
> 
> Even if the method implementation can be buggy on some platform,
> the "open" choice is worse. It breaks docking stations basically
> and there is no way to have a user-space hwdb to fix that.
> 
> On the contrary, it's rather easy in user-space to have a hwdb
> with the problematic platforms. Then, libinput (1.7.0+) can fix
> the state of the LID switch for us: you need to set the udev
> property LIBINPUT_ATTR_LID_SWITCH_RELIABILITY to 'write_open'.
> 
> When libinput detects internal keyboard events, it will
> overwrite the state of the switch to open, making it reliable
> again. Given that logind only checks the LID switch value after
> a timeout, we can assume the user will use the internal keyboard
> before this timeout expires.
> 
> For example, such a hwdb entry is:
> 
> libinput:name:*Lid Switch*:dmi:*svnMicrosoftCorporation:pnSurface3:*
>  LIBINPUT_ATTR_LID_SWITCH_RELIABILITY=write_open

For the reason mentioned previously and proofs here (see patch descriptions):
https://patchwork.kernel.org/patch/9717111/
We shouldn't do this.

Thanks and best regards
Lv

> 
> Link: https://bugzilla.gnome.org/show_bug.cgi?id=782380
> Cc: sta...@vger.kernel.org
> Signed-off-by: Benjamin Tissoires 
> ---
>  drivers/acpi/button.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 6d5a8c1..e19f530 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -113,7 +113,7 @@ struct acpi_button {
> 
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
> -static u8 lid_init_state = ACPI_BUTTON_LID_INIT_OPEN;
> +static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> 
>  static unsigned long lid_report_interval __read_mostly = 500;
>  module_param(lid_report_interval, ulong, 0644);
> --
> 2.9.3