RE: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
From: Zheng, Lv
Date: Sun Jun 04 2017 - 23:33:38 EST
Hi,
> From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxxx]
> Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
>
> From: Lv Zheng <lv.zheng@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> ---
> 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