Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751624AbdFGJri (ORCPT ); Wed, 7 Jun 2017 05:47:38 -0400 Received: from mga06.intel.com ([134.134.136.31]:26762 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751465AbdFGJrZ (ORCPT ); Wed, 7 Jun 2017 05:47:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,310,1493708400"; d="scan'208";a="96546359" From: "Zheng, Lv" To: Benjamin Tissoires CC: "Wysocki, Rafael J" , "Rafael J . Wysocki" , "Brown, Len" , Lv Zheng , Peter Hutterer , "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: AQHS2weGIXcQHhLK2Uaox0ag7emBIaIVoL7ggAGCjACAAgyTgA== Date: Wed, 7 Jun 2017 09:47:19 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E886CECAB59@SHSMSX101.ccr.corp.intel.com> References: <20170601184632.2980-1-benjamin.tissoires@redhat.com> <20170601184632.2980-5-benjamin.tissoires@redhat.com> <1AE640813FDE7649BE1B193DEA596E886CEC510A@SHSMSX101.ccr.corp.intel.com> <20170606102908.GB14880@mail.corp.redhat.com> In-Reply-To: <20170606102908.GB14880@mail.corp.redhat.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzNiNGU5MTQtNGY4MC00YjczLWE2NGMtZGZkODcwZTZlZmU0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjdRaXh6cThFTGdYQ2J5WU9KZDJ0YUUwMFwvUXJlYTlYeXNWYWRKYmIwVzVvPSJ9 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="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v579m9LV010493 Content-Length: 8071 Lines: 239 Hi, Benjamin > From: Benjamin Tissoires [mailto:benjamin.tissoires@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.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. > > 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 > > > 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 > >