Re: [RFC PATCH v6 1/5] ACPI: button: Add a workaround to fix an order issue for old userspace

[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 works fine
> with systemd 229.
> 
> Case 2,4 can be treated as an order issue:
>    If the button driver always evaluates _LID after seeing a LID
>    notification, there shouldn't be such a problem.
> 
> Note that this order issue cannot be fixed by sorting OS drivers' resume
> code:
> 1. When acpi.ec_freeze_events=Y, the order depends on whether PNP0C0D(LID)
>    or PNP0C09(EC) appears first in the namespace (probe order).
> 2. Even when acpi.ec_freeze_events=N, which forces OS to handle EC events
>    as early as possible, the order issue can still be reproduced due to
>    platform delays (reproduce ratio is lower than case 1).
> 3. Some platform simply has a very big delay for LID open events.
> 
> This patch tries to fix this issue for systemd 229 by defer sending initial
> lid state 10 seconds later after resume, which ensures EC events can always
> arrive before button driver evaluates _LID. It finally only fixes case 2
> platforms as fixing case 4 platforms needs additional efforts (see known
> issue below).
> 
> The users can configure wait timeout via button.lid_notify_timeout.
> 
> Known issu:
> 1. systemd/logind 229 still mis-bahaves with case 3,4 platforms
>    After seeing a LID "close" event, systemd 229 will wait several seconds
>    (HoldoffTimeoutSec) before suspending the platform. Thus on case 4
>    platforms, if users close lid, and re-open it during the
>    HoldoffTimeoutSec period, there is still no "open" events seen by the
>    userspace. Thus systemd still considers the last state as "close" and
>    suspends the platform after the HoldoffTimeoutSec times out.
> 
> 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>
> ---

NACK on this one (at least in this current form): You are presenting a
device to user space with an unknown state.
If you want to delay the query of the state, you also need to delay the
call of input_register_device().

Cheers,
Benjamin

>  drivers/acpi/button.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index e19f530..67a0d78 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -28,6 +28,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  #include <linux/input.h>
> +#include <linux/timer.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>  #include <acpi/button.h>
> @@ -79,6 +80,7 @@ MODULE_DEVICE_TABLE(acpi, button_device_ids);
>  static int acpi_button_add(struct acpi_device *device);
>  static int acpi_button_remove(struct acpi_device *device);
>  static void acpi_button_notify(struct acpi_device *device, u32 event);
> +static void acpi_lid_timeout(ulong arg);
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int acpi_button_suspend(struct device *dev);
> @@ -104,6 +106,7 @@ static struct acpi_driver acpi_button_driver = {
>  struct acpi_button {
>  	unsigned int type;
>  	struct input_dev *input;
> +	struct timer_list lid_timer;
>  	char phys[32];			/* for input device */
>  	unsigned long pushed;
>  	int last_state;
> @@ -119,6 +122,10 @@ 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");
>  
> +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");
> +
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
>     -------------------------------------------------------------------------- */
> @@ -371,6 +378,15 @@ static int acpi_lid_update_state(struct acpi_device *device)
>  	return acpi_lid_notify_state(device, state);
>  }
>  
> +static void acpi_lid_start_timer(struct acpi_device *device,
> +				 unsigned long msecs)
> +{
> +	struct acpi_button *button = acpi_driver_data(device);
> +
> +	mod_timer(&button->lid_timer,
> +		  jiffies + msecs_to_jiffies(msecs));
> +}
> +
>  static void acpi_lid_initialize_state(struct acpi_device *device)
>  {
>  	switch (lid_init_state) {
> @@ -386,6 +402,13 @@ static void acpi_lid_initialize_state(struct acpi_device *device)
>  	}
>  }
>  
> +static void acpi_lid_timeout(ulong arg)
> +{
> +	struct acpi_device *device = (struct acpi_device *)arg;
> +
> +	acpi_lid_initialize_state(device);
> +}
> +
>  static void acpi_button_notify(struct acpi_device *device, u32 event)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
> @@ -432,6 +455,8 @@ static int acpi_button_suspend(struct device *dev)
>  	struct acpi_device *device = to_acpi_device(dev);
>  	struct acpi_button *button = acpi_driver_data(device);
>  
> +	if (button->type == ACPI_BUTTON_TYPE_LID)
> +		del_timer(&button->lid_timer);
>  	button->suspended = true;
>  	return 0;
>  }
> @@ -443,7 +468,8 @@ static int acpi_button_resume(struct device *dev)
>  
>  	button->suspended = false;
>  	if (button->type == ACPI_BUTTON_TYPE_LID)
> -		acpi_lid_initialize_state(device);
> +		acpi_lid_start_timer(device,
> +			lid_notify_timeout * MSEC_PER_SEC);
>  	return 0;
>  }
>  #endif
> @@ -490,6 +516,9 @@ static int acpi_button_add(struct acpi_device *device)
>  			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);
>  	} else {
>  		printk(KERN_ERR PREFIX "Unsupported hid [%s]\n", hid);
>  		error = -ENODEV;
> @@ -526,12 +555,13 @@ static int acpi_button_add(struct acpi_device *device)
>  	if (error)
>  		goto err_remove_fs;
>  	if (button->type == ACPI_BUTTON_TYPE_LID) {
> -		acpi_lid_initialize_state(device);
>  		/*
>  		 * This assumes there's only one lid device, or if there are
>  		 * more we only care about the last one...
>  		 */
>  		lid_device = device;
> +		acpi_lid_start_timer(device,
> +			lid_notify_timeout * MSEC_PER_SEC);
>  	}
>  
>  	printk(KERN_INFO PREFIX "%s [%s]\n", name, acpi_device_bid(device));
> @@ -551,6 +581,8 @@ static int acpi_button_remove(struct acpi_device *device)
>  	struct acpi_button *button = acpi_driver_data(device);
>  
>  	acpi_button_remove_fs(device);
> +	if (button->type == ACPI_BUTTON_TYPE_LID)
> +		del_timer_sync(&button->lid_timer);
>  	input_unregister_device(button->input);
>  	kfree(button);
>  	return 0;
> -- 
> 2.7.4
> 
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel