Hi, Benjamin

From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Benjamin
Tissoires
Subject: Re: [WIP PATCH 2/4] ACPI: button: remove the LID input node when the state is unknown

Hi Lv,

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:

quoted

Hi, Benjamin

quoted

From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
Subject: [WIP PATCH 2/4] ACPI: button: remove the LID input node when the state is unknown

quoted

My dell latitude 6430u test platform sends multiple Notify(lid) before suspend and after resume.

Does this platform requires the not lid_reliable check as per this
series? Because if it doesn't, then we should not care.

No need to mark lid_reliable.

quoted

This is because the aml table puts many Notify(LID, 0x80) in various control methods.
And not one of them but multiple of them will be invoked by various OS drivers during suspend/resume

period.

quoted

I think this is not an isolated platform that will invoke multiple redundant "Notify(lid)".

Fortunately, the lid state for the multiple notify(lid) should be same as the first "Notify(lid)".
I suppose this is why SW_LID is invented, as it can really filter such redundant events.
And user space finally can only see 1 "close" event.

But unconditionally prepending "open" before all "close" events surely can break the logic by
delivering multiple "close" events to the user space.

That doesn't matter. What matters is the state of the switch, not the
event. So if user space receives (in case we marked the switch as not
reliable) several close events, all user space will do is realize that
the state is still closed and will act accordingly.

OK, I tried to address this here:
https://patchwork.kernel.org/patch/9771121/

quoted

Another issue is, for case 5, when we use button.lid_init_state=method.
Unconditionally prepending "open" before driver initiated "close" event
sent due to acpi_lid_initialize_state(), we will see suspend/resume cycles.

Case 5 is broken anyway and needs to be handled specially. It was not
targeted in this WIP series.

It was addressed by button.lid_init_state=open and newer systemd.
It's not broken any more.

quoted

Thus if we consider both cases, we should:
1. put a frequency check to filter possible redundant events.

This doesn't work and should be avoided. The state of the input switch
is known to the input layer only, and given there are spinlocks, you can
not know if the state is actually the one you expected beforehand.

You can however add frequency checks in the input handler, but that
would assume the input layer is not doing its job properly and so should
be avoided.

OK, I dropped frequency check mechanism.

quoted

2. distinguish AML "Notify" call and button driver initiated lid notification.

Again, we don't care if the "event" comes from ACPI, the driver itself or
user space (libinput). All that matters is the current state of the
input node switch, that needs to match the physical world at any time.

That depends on the final test result.
However I managed to get systemd working with case 2,4 using this commit:
https://patchwork.kernel.org/patch/9771121/

quoted

This is another major differences between your proposal and mine.

First of all, I think it should be in a separate patch.

Well, that's already a patch on its own :/

quoted

Second, I have concerns related to such a change:
I can see that, you are trying to address a problem that:
The input layer requires a determined initial SW_LID state while ACPI button driver cannot offer.
So by adding/removing input node, you can introduce a tristate SW_LID input node.

You can put it that way. I prefer putting it: "when we export the LID
switch input node, you are guaranteed to have the proper state".

quoted

However I doubt if this is necessary and can solve real issues, as:
systemd now works fine with button driver for all cases,

I do not care about systemd or the suspend lopps introduced by systemd.
All I care is that the kernel provides correct behavior. If systemd can
work around some issues we see because we are too lazy to fix them in
the kernel (this is not a personal attack, sometimes being lazy is the
right solution), fine. But the current state of this driver doesn't
follow the specification of the input subsystem on some platform, and
this is what this series fixes.

No, we just can see if case 5 is properly addressed (like current systemd 233 does),
we don't need to do anything.
So if you still insist to fix systemd 229, I just post an RFC:
https://patchwork.kernel.org/patch/9771121/

quoted

only desktop managers should be changed to be compliant to case 2/4/5

As long as the kernel lies, we should not even remotely envision asking
user space to change.

By reverting back to "method" mode and fixing "method" mode, there is no regression.
What if we just stay to see what will happen next with MS Surface Pro 1 docking station support.
Maybe it is minor.

quoted

(or if we improve by adding a timer, they should only be changed to be compliant to case 5).
And doing this won't help desktop managers to be able to work fine with case 5.
So this finally may only remove ACPI SW_LID support from Surface Pro 1.

I haven't decided how to solve case 5 completely. So please do not take
this case into account yet.

quoted

While with latest systemd, closing lid on that platform can correctly triggers suspend.
And no suspend/resume cycles should be seen.
So why do we need to remove this feature (ACPI SW_LID) from Surface Pro 1?

Well, with this change, if you mark the surface pro 1 as unreliable, the
event will be forwarded to user space correctly. Just that there is a
systemd bug in which the state is not synced when the device appears.

So yes, it'll expose a new bug in userspace, but given the blacklist of
unreliable LID switches will be handled in userspace, user space can
make sure this will not show up before systemd can handle it.

IMO, we don't need to remove input node, button.lid_init_state=ignore is developed for the same purpose.
And it works perfectly with systemd 233. So we can stay with old solution.
So hope this can replace your aggressive change:
https://patchwork.kernel.org/patch/9771119/

quoted

If you really want to propose an ABI change for user space.
Why don't you do this in input layer by defining SW_LID as tristate?

Because this proposal is not a kABI change, and the kABI change you
propose is just not doable. We have too many users of EV_SW to be able
to say that we change the semantic. A solution would be to add a new
EV_* event, but we don't really need.

Cheers,
Lv
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel