Hi, Benjamin > From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxxx] > Subject: Re: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks > > On Jun 05 2017 or thereabouts, Zheng, Lv wrote: > > 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. > > I actually rebased it in this series. I kept your SoB line given that > the idea came from you and the resulting patch was rather similar (only > one hunk differs, but the meaning is the same). > > > > > Is input layer capable of filtering redundant events. > > I don't think it does, and it should not. If an event is emitted, it has > to be forwarded. However, the logic of the protocol makes that the only > state that matters is when an EV_SYN is emitted. So if a SW_LID 0 then 1 > is sent between the 2 EV_SYN, and the state was 1 before, from a > protocol point of view it's a no-op. > > > I saw you unconditionally prepend "open" before "close", > > which may make input layer incapable of filtering redundant close events. > > Again, we don't care about events. We care about states, and those are > only emitted when the lid is marked as non reliable. > > > > > 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? > > Having the i915 driver listening to the input events is actually a good > solution. Let me think about it a little bit more and I'll come back. OK, then I'll drop the frequency check mechanism and drop patch 4/5. Cheers, Lv > > Cheers, > Benjamin > > > > > 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 > > ��.n��������+%������w��{.n�����{��)��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
