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

[Zheng, Lv]

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
> >