Re: [RFC PATCH v6 3/5] ACPI: button: Rework lid_init_state=ignore mode

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Jun 21 2017 or thereabouts, Lv Zheng wrote:
> There are platform variations implementing ACPI lid device in different
> ways:
> 1. Some platforms send "open" events to OS and the events arrive before
>    button driver is resumed;
> 2. Some platforms send "open" events to OS, but the events arrive after
>    button driver is resumed, ex., Samsung N210+;
> 3. Some platforms never send "open" events to OS, but send "open" events to
>    update the cached _LID return value, and the update events arrive before
>    button driver is resumed;
> 4. Some platforms never send "open" events to OS, but send "open" events to
>    update the cached _LID return value, but the update events arrive after
>    button driver is resumed, ex., Surface Pro 3;
> 5. Some platforms never send "open" events, _LID returns value sticks to
>    "close", ex., Surface Pro 1.
> Currently, all cases work find with systemd 233, but only case 1,2,3,4 work
> fine with systemd 229.
> 
> After fixing all the other issues for old userspace programs, case 5 is the
> only case that the exported input node is still not fully compliant to
> SW_LID ABI and thus needs quirks or ABI changes.
> 
> The original button.lid_init_state=ignore ABI change solution is now too
> complicated if its purpose is to only solve this final incompliant use
> case. This patch re-works it by unconditionally prepending "open"
> complement events.

Ouch. I thought the purpose of "ignore" was to not query the state on
boot/resume and only rely on acpi notification to switch the states.

I like the patch, the commit message is IMO too far from what it
actually does:
- it removes the caching update of _LID switch
- it removes the filtering of too close notifications.

One more note inlined:

> 
> Cc: <systemd-devel@lists.freedesktop.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Peter Hutterer <peter.hutterer@who-t.net>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/button.c | 85 +++------------------------------------------------
>  1 file changed, 5 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index a8b119e..1256a8c 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -109,8 +109,6 @@ struct acpi_button {
>  	struct timer_list lid_timer;
>  	char phys[32];			/* for input device */
>  	unsigned long pushed;
> -	int last_state;
> -	ktime_t last_time;
>  	bool suspended;
>  };
>  
> @@ -118,10 +116,6 @@ static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
>  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);
> -MODULE_PARM_DESC(lid_report_interval, "Interval (ms) between lid key events");
> -

You shouldn't do that. It is kernel ABI, and if you remove the module,
people that tuned their kernel with this parameter will have the module
failing to load. You should keep this around for a few kernel releases,
and mark its usage deprecated by issuing a warning in the dmesg.

Cheers,
Benjamin

>  static unsigned long lid_notify_timeout __read_mostly = 10;
>  module_param(lid_notify_timeout, ulong, 0644);
>  MODULE_PARM_DESC(lid_notify_timeout, "Timeout (s) before receiving lid notification");
> @@ -157,79 +151,12 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
>  	int ret;
> -	ktime_t next_report;
> -	bool do_update;
> -
> -	/*
> -	 * In lid_init_state=ignore mode, if user opens/closes lid
> -	 * frequently with "open" missing, and "last_time" is also updated
> -	 * frequently, "close" cannot be delivered to the userspace.
> -	 * So "last_time" is only updated after a timeout or an actual
> -	 * switch.
> -	 */
> -	if (lid_init_state != ACPI_BUTTON_LID_INIT_IGNORE ||
> -	    button->last_state != !!state)
> -		do_update = true;
> -	else
> -		do_update = false;
> -
> -	next_report = ktime_add(button->last_time,
> -				ms_to_ktime(lid_report_interval));
> -	if (button->last_state == !!state &&
> -	    ktime_after(ktime_get(), next_report)) {
> -		/* Complain the buggy firmware */
> -		pr_warn_once("The lid device is not compliant to SW_LID.\n");
>  
> -		/*
> -		 * Send the unreliable complement switch event:
> -		 *
> -		 * On most platforms, the lid device is reliable. However
> -		 * there are exceptions:
> -		 * 1. Platforms returning initial lid state as "close" by
> -		 *    default after booting/resuming:
> -		 *     https://bugzilla.kernel.org/show_bug.cgi?id=89211
> -		 *     https://bugzilla.kernel.org/show_bug.cgi?id=106151
> -		 * 2. Platforms never reporting "open" events:
> -		 *     https://bugzilla.kernel.org/show_bug.cgi?id=106941
> -		 * On these buggy platforms, the usage model of the ACPI
> -		 * lid device actually is:
> -		 * 1. The initial returning value of _LID may not be
> -		 *    reliable.
> -		 * 2. The open event may not be reliable.
> -		 * 3. The close event is reliable.
> -		 *
> -		 * But SW_LID is typed as input switch event, the input
> -		 * layer checks if the event is redundant. Hence if the
> -		 * state is not switched, the userspace cannot see this
> -		 * platform triggered reliable event. By inserting a
> -		 * complement switch event, it then is guaranteed that the
> -		 * platform triggered reliable one can always be seen by
> -		 * the userspace.
> -		 */
> -		if (lid_init_state == ACPI_BUTTON_LID_INIT_IGNORE) {
> -			do_update = true;
> -			/*
> -			 * Do generate complement switch event for "close"
> -			 * as "close" is reliable and wrong "open" won't
> -			 * trigger unexpected behaviors.
> -			 * Do not generate complement switch event for
> -			 * "open" as "open" is not reliable and wrong
> -			 * "close" will trigger unexpected behaviors.
> -			 */
> -			if (!state) {
> -				input_report_switch(button->input,
> -						    SW_LID, state);
> -				input_sync(button->input);
> -			}
> -		}
> -	}
> -	/* Send the platform triggered reliable event */
> -	if (do_update) {
> -		input_report_switch(button->input, SW_LID, !state);
> -		input_sync(button->input);
> -		button->last_state = !!state;
> -		button->last_time = ktime_get();
> -	}
> +	if (lid_init_state == ACPI_BUTTON_LID_INIT_IGNORE)
> +		input_report_switch(button->input, SW_LID, 0);
> +
> +	input_report_switch(button->input, SW_LID, !state);
> +	input_sync(button->input);
>  
>  	if (state)
>  		pm_wakeup_event(&device->dev, 0);
> @@ -524,8 +451,6 @@ static int acpi_button_add(struct acpi_device *device)
>  		strcpy(name, ACPI_BUTTON_DEVICE_NAME_LID);
>  		sprintf(class, "%s/%s",
>  			ACPI_BUTTON_CLASS, ACPI_BUTTON_SUBCLASS_LID);
> -		button->last_state = !!acpi_lid_evaluate_state(device);
> -		button->last_time = ktime_get();
>  		init_timer(&button->lid_timer);
>  		setup_timer(&button->lid_timer,
>  			    acpi_lid_timeout, (ulong)device);
> -- 
> 2.7.4
> 
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel