[systemd-devel] [WIP PATCH 4/4] ACPI: button: Fix lid notification locks

[Zheng, Lv]

Hi,

> From: Benjamin Tissoires [mailto:benjamin.tissoires at redhat.com]
> Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
> 
> From: Lv Zheng <lv.zheng at intel.com>
> 
> acpi/button.c now contains the logic to avoid frequently replayed events
> which originally was ensured by using blocking notifier.
> On the contrary, using a blocking notifier is wrong as it could keep on
> returning NOTIFY_DONE, causing events lost.
> 
> This patch thus changes lid notification to raw notifier in order not to
> have any events lost.

This patch is on top of the following:
https://patchwork.kernel.org/patch/9756467/
where button driver implements a frequency check and
thus is capable of filtering redundant events itself:
I saw you have deleted it from PATCH 02.
So this patch is not applicable now.

Is input layer capable of filtering redundant events.
I saw you unconditionally prepend "open" before "close",
which may make input layer incapable of filtering redundant close events.

If input layer is capable of filtering redundant events,
why don't you:
1. drop this commit;
2. remove all ACPI lid notifier APIs;
3. change lid notifier callers to register notification via input layer?

Thanks and best regards
Lv 

> 
> Signed-off-by: Lv Zheng <lv.zheng at intel.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires at redhat.com>
> ---
>  drivers/acpi/button.c | 68 ++++++++++++++++++++-------------------------------
>  1 file changed, 27 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 03e5981..1927b08 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -114,7 +114,7 @@ struct acpi_button {
> 
>  static DEFINE_MUTEX(button_input_lock);
> 
> -static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> +static RAW_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
>  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> 
> @@ -179,14 +179,12 @@ static int acpi_lid_evaluate_state(struct acpi_device *device)
>  	return lid_state ? 1 : 0;
>  }
> 
> -static int acpi_lid_notify_state(struct acpi_device *device, int state)
> +static void acpi_lid_notify_state(struct acpi_device *device, int state)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
> 
> -	/* button_input_lock must be held */
> -
>  	if (!button->input)
> -		return 0;
> +		return;
> 
>  	/*
>  	 * If the lid is unreliable, always send an "open" event before any
> @@ -201,8 +199,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state)
> 
>  	if (state)
>  		pm_wakeup_hard_event(&device->dev);
> -
> -	return 0;
>  }
> 
>  /*
> @@ -214,28 +210,14 @@ static void acpi_button_lid_events(struct input_handle *handle,
>  {
>  	const struct input_value *v;
>  	int state = -1;
> -	int ret;
> 
>  	for (v = vals; v != vals + count; v++) {
>  		switch (v->type) {
>  		case EV_SYN:
> -			if (v->code == SYN_REPORT && state >= 0) {
> -				ret = blocking_notifier_call_chain(&acpi_lid_notifier,
> +			if (v->code == SYN_REPORT && state >= 0)
> +				(void)raw_notifier_call_chain(&acpi_lid_notifier,
>  								state,
>  								lid_device);
> -				if (ret == NOTIFY_DONE)
> -					ret = blocking_notifier_call_chain(&acpi_lid_notifier,
> -								state,
> -								lid_device);
> -				if (ret == NOTIFY_DONE || ret == NOTIFY_OK) {
> -					/*
> -					 * It is also regarded as success if
> -					 * the notifier_chain returns NOTIFY_OK
> -					 * or NOTIFY_DONE.
> -					 */
> -					ret = 0;
> -				}
> -			}
>  			break;
>  		case EV_SW:
>  			if (v->code == SW_LID)
> @@ -433,13 +415,25 @@ static int acpi_button_remove_fs(struct acpi_device *device)
>     -------------------------------------------------------------------------- */
>  int acpi_lid_notifier_register(struct notifier_block *nb)
>  {
> -	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
> +	return raw_notifier_chain_register(&acpi_lid_notifier, nb);
>  }
>  EXPORT_SYMBOL(acpi_lid_notifier_register);
> 
> +static inline int __acpi_lid_notifier_unregister(struct notifier_block *nb,
> +						 bool sync)
> +{
> +	int ret;
> +
> +	ret = raw_notifier_chain_unregister(&acpi_lid_notifier, nb);
> +	if (sync)
> +		synchronize_rcu();
> +
> +	return ret;
> +}
> +
>  int acpi_lid_notifier_unregister(struct notifier_block *nb)
>  {
> -	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
> +	return __acpi_lid_notifier_unregister(nb, false);
>  }
>  EXPORT_SYMBOL(acpi_lid_notifier_unregister);
> 
> @@ -452,40 +446,36 @@ int acpi_lid_open(void)
>  }
>  EXPORT_SYMBOL(acpi_lid_open);
> 
> -static int acpi_lid_update_state(struct acpi_device *device)
> +static void acpi_lid_update_state(struct acpi_device *device)
>  {
>  	int state;
> 
>  	state = acpi_lid_evaluate_state(device);
>  	if (state < 0)
> -		return state;
> +		return;
> 
> -	return acpi_lid_notify_state(device, state);
> +	acpi_lid_notify_state(device, state);
>  }
> 
> -static int acpi_lid_notify(struct acpi_device *device)
> +static void acpi_lid_notify(struct acpi_device *device)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
> -	int ret;
> 
>  	mutex_lock(&button_input_lock);
>  	if (!button->input)
>  		acpi_button_add_input(device);
> -	ret = acpi_lid_update_state(device);
> +	acpi_lid_update_state(device);
>  	mutex_unlock(&button_input_lock);
> -
> -
> -	return ret;
>  }
> 
>  static void acpi_lid_initialize_state(struct acpi_device *device)
>  {
>  	switch (lid_init_state) {
>  	case ACPI_BUTTON_LID_INIT_OPEN:
> -		(void)acpi_lid_notify_state(device, 1);
> +		acpi_lid_notify_state(device, 1);
>  		break;
>  	case ACPI_BUTTON_LID_INIT_METHOD:
> -		(void)acpi_lid_update_state(device);
> +		acpi_lid_update_state(device);
>  		break;
>  	case ACPI_BUTTON_LID_INIT_IGNORE:
>  	default:
> @@ -641,11 +631,7 @@ static int acpi_lid_update_reliable(struct acpi_device *device)
>  		if (error)
>  			return error;
> 
> -		error = acpi_lid_update_state(device);
> -		if (error) {
> -			acpi_button_remove_input(device);
> -			return error;
> -		}
> +		acpi_lid_update_state(device);
>  	}
> 
>  	if (!lid_reliable && button->input)
> --
> 2.9.4