Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751267AbdFEDdc convert rfc822-to-8bit (ORCPT ); Sun, 4 Jun 2017 23:33:32 -0400 Received: from mga14.intel.com ([192.55.52.115]:53940 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751224AbdFEDdX (ORCPT ); Sun, 4 Jun 2017 23:33:23 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,299,1493708400"; d="scan'208";a="864550465" From: "Zheng, Lv" To: Benjamin Tissoires , "Wysocki, Rafael J" , "Rafael J . Wysocki" , "Brown, Len" , Lv Zheng , "Peter Hutterer" CC: "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "systemd-devel@lists.freedesktop.org" , "linux-input@vger.kernel.org" Subject: RE: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks Thread-Topic: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks Thread-Index: AQHS2weGIXcQHhLK2Uaox0ag7emBIaIVoL7g Date: Mon, 5 Jun 2017 03:33:19 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E886CEC510A@SHSMSX101.ccr.corp.intel.com> References: <20170601184632.2980-1-benjamin.tissoires@redhat.com> <20170601184632.2980-5-benjamin.tissoires@redhat.com> In-Reply-To: <20170601184632.2980-5-benjamin.tissoires@redhat.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZWI3YmFkYmYtOTNhYy00ODM2LWI4NTctNzEyMWM1MzQ1YzM0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlZEbUN1dnl1UTFkcHdrSjd0aHVCZnBOT1VURTdBTlZYRTdiUStod0RpbTQ9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6150 Lines: 203 Hi, > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com] > Subject: [WIP PATCH 4/4] ACPI: button: Fix lid notification locks > > From: Lv Zheng > > 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 > Signed-off-by: Benjamin Tissoires > --- > 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