Re: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Jun 05 2017 or thereabouts, Zheng, Lv wrote:
> Hi,
> 
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks
> > 
> > From: Lv Zheng <lv.zheng@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.

Cheers,
Benjamin

> 
> Thanks and best regards
> Lv 
> 
> > 
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@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
>