Hi, Benjamin
> From: Benjamin Tissoires [mailto:benjamin.tissoi...@redhat.com]
> 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.tissoi...@redhat.com]
> > > Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
> > >
> > > From: Lv Zheng <lv.zh...@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.
>
> 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.zh...@intel.com>
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@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
> >
