Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1640133 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-12 04:40 +0200
    Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open" "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-05-13 00:00 +0200
      Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-15 09:50 +0200
        Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open" "Rafael J. Wysocki" <rafael@kernel.org> - 2017-05-15 11:20 +0200
          Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-15 11:40 +0200
            Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to lid_init_state=open" "Rafael J. Wysocki" <rafael@kernel.org> - 2017-05-15 13:10 +0200
              Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-15 15:10 +0200
                RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-16 07:40 +0200
                  RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-16 07:50 +0200
                    Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-16 09:20 +0200
                      RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-16 10:40 +0200
                        Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-16 12:20 +0200
                          RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" "Zheng, Lv" <lv.zheng@intel.com> - 2017-05-17 09:40 +0200
                            RE: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Peter Hutterer <peter.hutterer@who-t.net> - 2017-05-17 14:30 +0200
                            Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-17 16:20 +0200
              Re: [PATCH 2/2] Revert "ACPI / button: Change default behavior to  lid_init_state=open" Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-05-24 10:10 +0200

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

Hi, 

> From: Benjamin Tissoires [mailto:benjamin.tissoires@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.tissoires@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 system participants to support their special needs.
   Such participants include but are not limited to the user space tools and the other kernel modules.
   But the default behavior and the timing of tuning button driver behaviors should be determined by the other system participants.
   And as a consequence, the other system participants who change that must be responsible for fixing regressions related to conflicts of the needs.
   As such, we need to know some official bug report window to forward bug reports related to that.
   Otherwise, ACPI Bugzilla can be flooded by wrong bugs.

Cheers,
Lv

> 
> Cheers,
> Benjamin
> 
> >
> > Thanks and best regards
> > Lv
> >
> > >
> > > Link: https://bugzilla.gnome.org/show_bug.cgi?id=782380
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >  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
> >

[toc] | [next] | [standalone]

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

On Friday, May 12, 2017 02:36:20 AM Zheng, Lv wrote:
> Hi, 
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

#1641327

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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

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

On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
<benjamin.tissoires@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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

#1641486

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 9:45 AM, Benjamin Tissoires
> <benjamin.tissoires@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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

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

On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
<benjamin.tissoires@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.tissoires@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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

#1641645

On May 15 2017 or thereabouts, Rafael J. Wysocki wrote:
> On Mon, May 15, 2017 at 11:37 AM, Benjamin Tissoires
> <benjamin.tissoires@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.tissoires@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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

#1642220

Hi, Guys

> From: Benjamin Tissoires [mailto:benjamin.tissoires@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.tissoires@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.tissoires@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.tissoires@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.tissoires@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

[toc] | [prev] | [next] | [standalone]

#1642223

Hi,

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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.tissoires@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.tissoires@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.tissoires@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.tissoires@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.tissoires@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?

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.

Thanks and best regards
Lv

[toc] | [prev] | [next] | [standalone]

#1642259

On May 16 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@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.tissoires@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.tissoires@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.tissoires@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.tissoires@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.tissoires@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?
> 
> 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.

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.

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.

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.

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
- 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
introducing changes in it. I asked you twice, and two times you replied
skipping this part.

It's clear you don't want to revert to the old state, and even if I can
prove to you that you have to, you don't listen.

So please, do not force me to call the maintainers and Linus on this
simple 2 reverts.

Cheers,
Benjamin

[toc] | [prev] | [next] | [standalone]

#1642304

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
> introducing changes in it. I asked you twice, and two times you replied
> skipping this part.

I didn't, you just skipped PATCH 1/2 reply...
Let me copy it from that email:
=====
Actually there is no special maintainership related to the button driver.
All drivers of ACPI spec defined devices (PNPxxx stuffs) are maintained as a whole by Rafael.
And we are helping him by doing triage on kernel Bugzilla.

There isn't a category on kernel Bugzilla related to lid issues.
Probably you can help to create one under ACPI product.
If this can be achieved, you can be the default assignee for such issues.
If this cannot be achieved, we'll just forward some lid reports to you first.
Please tell me which product/component you'd prefer for us to forward.

For modifying acpi/button.c, it's open source, you can send patches and ACPI community can help to review.
=====

> It's clear you don't want to revert to the old state, and even if I can
> prove to you that you have to, you don't listen.
> So please, do not force me to call the maintainers and Linus on this
> simple 2 reverts.

No, I currently cannot persuade myself to revert to "method" mode.
But that doesn't mean I don't want to or I want to.
You just didn't prove that by answering my questions in PATCH 1/2 and in this email.
Especially:
1. After reverting to "method" mode, can libinput work all cases around?
   Are there any timing issues that can prevent libinput from working some sucks around.
   Considering this case, it's likely such problems can happen.
   https://bugzilla.kernel.org/show_bug.cgi?id=106151
2. Who should be responsible for fixing this issue after reverting to "method" mode:
   https://bugzilla.kernel.org/show_bug.cgi?id=106151
   What's the root cause of this issue?
3. How can we generate the quirk for the following possible breakage:
   No lid notification and _LID return cached open after boot/resume

Cheers,
Lv

[toc] | [prev] | [next] | [standalone]

#1642386

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 in kernel space doesn't matter, we need them.

> 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
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.
----

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. 

> 
> > 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 you know there
is a possibility it's actually closed.

> 
> > 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.

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.

> 
> > - 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
> > introducing changes in it. I asked you twice, and two times you replied
> > skipping this part.
> 
> I didn't, you just skipped PATCH 1/2 reply...
> Let me copy it from that email:
> =====
> Actually there is no special maintainership related to the button driver.
> All drivers of ACPI spec defined devices (PNPxxx stuffs) are maintained as a whole by Rafael.

I know, but there is the file MAINTAINER where you can add individual
maintainer per file. Rafael will still be the ACPI maintainer, providing
PR to Linus, but I'll just be the other goto-guy when people run
get_maintainer.pl on the acpi/button file.

> And we are helping him by doing triage on kernel Bugzilla.
> 
> There isn't a category on kernel Bugzilla related to lid issues.
> Probably you can help to create one under ACPI product.

That would be good to have such a category (maybe not LID, but input or
button to reflect the file name)

> If this can be achieved, you can be the default assignee for such issues.
> If this cannot be achieved, we'll just forward some lid reports to you first.

Please do.

> Please tell me which product/component you'd prefer for us to forward.

I'd be happy to be redirected anything input related.

> 
> For modifying acpi/button.c, it's open source, you can send patches and ACPI community can help to review.

Which I already did. My concern is more when there is a change I am not
aware of which introduces regressions and which I could have provided a
valid NACK early enough.

> =====
> 
> > It's clear you don't want to revert to the old state, and even if I can
> > prove to you that you have to, you don't listen.
> > So please, do not force me to call the maintainers and Linus on this
> > simple 2 reverts.
> 
> No, I currently cannot persuade myself to revert to "method" mode.
> But that doesn't mean I don't want to or I want to.
> You just didn't prove that by answering my questions in PATCH 1/2 and in this email.
> Especially:
> 1. After reverting to "method" mode, can libinput work all cases around?

If it doesn't, we'll fix it. So yes, it will eventually.

>    Are there any timing issues that can prevent libinput from working some sucks around.
>    Considering this case, it's likely such problems can happen.
>    https://bugzilla.kernel.org/show_bug.cgi?id=106151

logind has a delay between the time it starts and the time it starts
polling the lid switch state. The default value is a little bit too
short on Fedora considering the faulty devices are running small CPUs.
But this can be changed as wish by the user and by systemd. They told us
they took one arbitrary value by default, and we can change it.

We can ask systemd/logind to change part of the behavior for new devices
when there is a bug. But we can't ask them to change the whole design
because there is a regression in the kernel.

> 2. Who should be responsible for fixing this issue after reverting to "method" mode:
>    https://bugzilla.kernel.org/show_bug.cgi?id=106151

libinput will change the value to open given the heuristics we have.

>    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.

> 3. How can we generate the quirk for the following possible breakage:
>    No lid notification and _LID return cached open after boot/resume

"method" will fetch and report the cached _LID value as open, so there
is no breakage. Unless the lid is actually closed and the EC is plain
wrong, but that is something we can also explain to users or fix by an
other mean. But nothing generic will work. For instance, on the Surface
3, the LID notification for open isn't forwarded, so I wrote a specific
driver to get the correct behavior based on a careful analysis of the
DSDT.

Cheers,
Benjamin

> 
> Cheers,
> Lv

[toc] | [prev] | [next] | [standalone]

#1643043

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 you know there
> is a possibility it's actually closed.

No we are faking events in this mode.
_LID can return cached state, and:
1. it might be "close" while LID is "open" after suspend.
2. it might be "open" while LID is "close" after suspend.
In this case, it explains the side effect of having type 1 fake event:
https://bugzilla.kernel.org/show_bug.cgi?id=106151
If we don't evaluate _LID and send such a "close", the platform can correctly send "open" just with a delay.

> > > 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.
> 
> 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.
Causing some cases not workaroundable.
See below for details.

> > No, I currently cannot persuade myself to revert to "method" mode.
> > But that doesn't mean I don't want to or I want to.
> > You just didn't prove that by answering my questions in PATCH 1/2 and in this email.
> > Especially:
> > 1. After reverting to "method" mode, can libinput work all cases around?
> If it doesn't, we'll fix it. So yes, it will eventually.

That's great.

> >    Are there any timing issues that can prevent libinput from working some sucks around.
> >    Considering this case, it's likely such problems can happen.
> >    https://bugzilla.kernel.org/show_bug.cgi?id=106151
> 
> logind has a delay between the time it starts and the time it starts
> polling the lid switch state. The default value is a little bit too
> short on Fedora considering the faulty devices are running small CPUs.
> But this can be changed as wish by the user and by systemd. They told us
> they took one arbitrary value by default, and we can change it.
> 
> We can ask systemd/logind to change part of the behavior for new devices
> when there is a bug. But we can't ask them to change the whole design
> because there is a regression in the kernel.

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"?
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?

If it can be configured to "INFINITE", with "ignore" mode,
systemd/logind should always be able to see an "open" event before seeing a new BIOS triggered "close" events.

So why do you refuse the possibilities of using "open/ignore" as default modes?

> > 2. Who should be responsible for fixing this issue after reverting to "method" mode:
> >    https://bugzilla.kernel.org/show_bug.cgi?id=106151
> 
> libinput will change the value to open given the heuristics we have.

Yes, I see.
But I also can see one broken case cannot be worked around by libinput if you insist to use "method".

> >    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?
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 mean by using word "poorly", EC driver or EC firmware?
If it's EC driver, can you fix it?

> > 3. How can we generate the quirk for the following possible breakage:
> >    No lid notification and _LID return cached open after boot/resume
> 
> "method" will fetch and report the cached _LID value as open, so there
> is no breakage. Unless the lid is actually closed and the EC is plain
> wrong, but that is something we can also explain to users or fix by an
> other mean. But nothing generic will work. For instance, on the Surface
> 3, the LID notification for open isn't forwarded, so I wrote a specific
> driver to get the correct behavior based on a careful analysis of the
> DSDT.

First, I couldn't see anything related to EC here but you kept on talking EC.
Do you have personal feelings against EC?

In fact, this is just a theoretical issue showing that something cannot be worked around by libinput when "method" is the default mode.
I don't even know any platform acting in this way.
But it is likely there are such platforms as the default "method" mode may cause them invisible to us.

The broken cases related to the timing are:
1. If libinput writes "close" after button driver sends initial notification using _LID return value:
   For a platform that has no lid notification and _LID return cached "open" after boot/resume.
   If such a system connects to an external monitor, and users configure to use external display, then
   "open" will arrive user space programs first, thus internal monitor will be lit on and external one will be lit off.
   Which is not what users want.
2. If libinput writes "close" before button driver sends initial notification using _LID return value:
   For a platform that has no lid notification and _LID return cached "close" after boot/resume.
   If such a system connects to an external monitor, and users configure to use internal display, then
   "close" will arrive userspace programs first, thus internal monitor will be lit off and external one will be lit on.
   Which is not what users want.
   There are even more cases broken if libinput writes "close" before button driver sends initial notification using _LID return value.
So either 1 or 2 will be broken.
What I supposed was 1 would be broken thus I listed such theoretical platform.

Cheers,
Lv

[toc] | [prev] | [next] | [standalone]

#1643309

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 reliably deliver the current state of hardware?

isn't this just a case of: method is less wrong than ignore? 100%
reliability is not possible given the hardware available on the market, but
it seems that 'method' is the least wrong of them, followed by ignore,
trailed by open and close.

> Actually both 'ignore/open/method' modes are trying to be compliant to EV_SW.

"compliant to EV_SW" just means "SW_LID state reflects state of the
hardware", right?

> Among them, "ignore" did the best IMO.

how did ignore do better than method? what devices give you an incorrect
state but send you a correct notification later? And even then it would only
matter for 50% of the cases anyway, because a false state of open first
followed by a close notification later wouldn't matter.

> 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 you know there
> > is a possibility it's actually closed.
> 
> No we are faking events in this mode.
> _LID can return cached state, and:
> 1. it might be "close" while LID is "open" after suspend.
> 2. it might be "open" while LID is "close" after suspend.
> In this case, it explains the side effect of having type 1 fake event:
> https://bugzilla.kernel.org/show_bug.cgi?id=106151
> If we don't evaluate _LID and send such a "close", the platform can
> correctly send "open" just with a delay.

side-note: _LID and LID makes for confusing reading. _LID is ACPI and LID is
SW_LID? 

if acpi/the hw gives you an incorrect cached state, you'll need to quirk
*that* hardware, because it's more broken than others.

> > > > 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.
> >
> > 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.
> Causing some cases not workaroundable.
> See below for details.

see below for answers, but basically:
if libinput writes the correct state before the notification arrives,
*nothing* happens in userspace. The kernel filters duplicate events, so even
if libinput sets the state before the kernel driver does, userspace won't
receive an event.
 
> > > No, I currently cannot persuade myself to revert to "method" mode.
> > > But that doesn't mean I don't want to or I want to.
> > > You just didn't prove that by answering my questions in PATCH 1/2 and in this email.
> > > Especially:
> > > 1. After reverting to "method" mode, can libinput work all cases around?
> > If it doesn't, we'll fix it. So yes, it will eventually.
> 
> That's great.
> 
> > >    Are there any timing issues that can prevent libinput from working some sucks around.
> > >    Considering this case, it's likely such problems can happen.
> > >    https://bugzilla.kernel.org/show_bug.cgi?id=106151
> >
> > logind has a delay between the time it starts and the time it starts
> > polling the lid switch state. The default value is a little bit too
> > short on Fedora considering the faulty devices are running small CPUs.
> > But this can be changed as wish by the user and by systemd. They told us
> > they took one arbitrary value by default, and we can change it.
> >
> > We can ask systemd/logind to change part of the behavior for new devices
> > when there is a bug. But we can't ask them to change the whole design
> > because there is a regression in the kernel.
> 
> 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"?
> 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?
> 
> If it can be configured to "INFINITE", with "ignore" mode,
> systemd/logind should always be able to see an "open" event before seeing a new BIOS triggered "close" events.
> 
> So why do you refuse the possibilities of using "open/ignore" as default modes?
> 
> > > 2. Who should be responsible for fixing this issue after reverting to "method" mode:
> > >    https://bugzilla.kernel.org/show_bug.cgi?id=106151
> >
> > libinput will change the value to open given the heuristics we have.
> 
> Yes, I see.
> But I also can see one broken case cannot be worked around by libinput if you insist to use "method".
> 
> > >    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?
> 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 mean by using word "poorly", EC driver or EC firmware?
> If it's EC driver, can you fix it?
> 
> > > 3. How can we generate the quirk for the following possible breakage:
> > >    No lid notification and _LID return cached open after boot/resume
> >
> > "method" will fetch and report the cached _LID value as open, so there
> > is no breakage. Unless the lid is actually closed and the EC is plain
> > wrong, but that is something we can also explain to users or fix by an
> > other mean. But nothing generic will work. For instance, on the Surface
> > 3, the LID notification for open isn't forwarded, so I wrote a specific
> > driver to get the correct behavior based on a careful analysis of the
> > DSDT.
> 
> First, I couldn't see anything related to EC here but you kept on talking EC.
> Do you have personal feelings against EC?

let's not go down that type of discussion please...

> In fact, this is just a theoretical issue showing that something cannot be
> worked around by libinput when "method" is the default mode.
> I don't even know any platform acting in this way.
> But it is likely there are such platforms as the default "method" mode may
> cause them invisible to us.

note: it may be likely, but it's a *definitive* that existing platforms
break with the current 'open'...

> The broken cases related to the timing are:
> 1. If libinput writes "close" after button driver sends initial notification using _LID return value:

This is a theoretical case only. libinput does not write close, ever.
because there is no heuristics we can use that can guarantee that the lid is
closed (well, short of enabling the camera and checking if it looks really
dark, but let's not do that ;)

>    For a platform that has no lid notification and _LID return cached "open" after boot/resume.
>    If such a system connects to an external monitor, and users configure to use external display, then
>    "open" will arrive user space programs first, thus internal monitor will be lit on and external one will be lit off.
>    Which is not what users want.
> 2. If libinput writes "close" before button driver sends initial notification using _LID return value:

see 1, libinput does not write 'close'.

also, just in case that's not obvious yet: libinput behaviour is a) internal
to libinput and b) tailored to the hardware. The current behaviour works for
the devices we've needed to fix. We can change that at any time and we can
add other behaviours at any time. So even if some theoretical platforms
appear that do the wrong thing - we can deal with it then and figure out.

note also, that the only reason libinput even writes the switch event to the
event node is because we are nice to others. We have that information (user
is typing, therefore lid is not closed) so we share it with anyone else by
changing the device's state. This way other processes can pick it up,
without needing the same heuristics.

Cheers,
   Peter

>    For a platform that has no lid notification and _LID return cached "close" after boot/resume.
>    If such a system connects to an external monitor, and users configure to use internal display, then
>    "close" will arrive userspace programs first, thus internal monitor will be lit off and external one will be lit on.
>    Which is not what users want.
>    There are even more cases broken if libinput writes "close" before button driver sends initial notification using _LID return value.
> So either 1 or 2 will be broken.
> What I supposed was 1 would be broken thus I listed such theoretical platform.
> 
> Cheers,
> Lv

[toc] | [prev] | [next] | [standalone]

#1643394

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 mean by using word "poorly", EC driver or EC firmware?

'Driver' implies in the kernel, and I never intended that there was such
a bug. If there is, then yes, I'd expect the people in charge of it to
fix it. But given the bug also happens in Windows, it's more likely that
it's a firmware issue, and it's the responsibility of Samsung to fix it.

I thought using EC was more precise that when talking about 'ACPI'
because ACPI can also be the project to support it under Linux, not the
FW itself.

> If it's EC driver, can you fix it?

I can't say I can fix anything. I don't have the machine and don't have
much time to dedicate to it. So no I guess.

> 
> > > 3. How can we generate the quirk for the following possible breakage:
> > >    No lid notification and _LID return cached open after boot/resume
> > 
> > "method" will fetch and report the cached _LID value as open, so there
> > is no breakage. Unless the lid is actually closed and the EC is plain
> > wrong, but that is something we can also explain to users or fix by an
> > other mean. But nothing generic will work. For instance, on the Surface
> > 3, the LID notification for open isn't forwarded, so I wrote a specific
> > driver to get the correct behavior based on a careful analysis of the
> > DSDT.
> 
> First, I couldn't see anything related to EC here but you kept on talking EC.
> Do you have personal feelings against EC?

For me EC == Embedded Controller == Firmware written in it that we both not
control (unless Intel writes part of it, which I am not aware of).
Sorry if EC means something else for you.

Cheers,
Benjamin

> 
> In fact, this is just a theoretical issue showing that something cannot be worked around by libinput when "method" is the default mode.
> I don't even know any platform acting in this way.
> But it is likely there are such platforms as the default "method" mode may cause them invisible to us.
> 
> The broken cases related to the timing are:
> 1. If libinput writes "close" after button driver sends initial notification using _LID return value:
>    For a platform that has no lid notification and _LID return cached "open" after boot/resume.
>    If such a system connects to an external monitor, and users configure to use external display, then
>    "open" will arrive user space programs first, thus internal monitor will be lit on and external one will be lit off.
>    Which is not what users want.
> 2. If libinput writes "close" before button driver sends initial notification using _LID return value:
>    For a platform that has no lid notification and _LID return cached "close" after boot/resume.
>    If such a system connects to an external monitor, and users configure to use internal display, then
>    "close" will arrive userspace programs first, thus internal monitor will be lit off and external one will be lit on.
>    Which is not what users want.
>    There are even more cases broken if libinput writes "close" before button driver sends initial notification using _LID return value.
> So either 1 or 2 will be broken.
> What I supposed was 1 would be broken thus I listed such theoretical platform.
> 
> Cheers,
> Lv

[toc] | [prev] | [next] | [standalone]

#1649211

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web