* [PATCH] ACPI LID: increment wakeup count only when notified. @ 2018-06-04 18:26 Ravi Chandra Sadineni 2018-06-04 19:19 ` Benson Leung 2018-06-06 7:00 ` Rafael J. Wysocki 0 siblings, 2 replies; 11+ messages in thread From: Ravi Chandra Sadineni @ 2018-06-04 18:26 UTC (permalink / raw) To: rjw, lenb, ravisadineni, ravisadineni Cc: dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, bleung, furquan Currently ACPI LID increments wakeup count irrespective of the wake source. This is because we call acpi_lid_initialize_state on every resume. Userland deamons using wakeup_count to identify the potential wake source for the last wake will be thrown off by this. Instead increment the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> --- drivers/acpi/button.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index f1cc4f9d31cd9..d40fef7241f08 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) button->last_time = ktime_get(); } - if (state) - acpi_pm_wakeup_event(&device->dev); - ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); if (ret == NOTIFY_DONE) ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) /* fall through */ case ACPI_BUTTON_NOTIFY_STATUS: input = button->input; + acpi_pm_wakeup_event(&device->dev); if (button->type == ACPI_BUTTON_TYPE_LID) { mutex_lock(&button->input->mutex); users = button->input->users; @@ -426,7 +424,6 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) } else { int keycode; - acpi_pm_wakeup_event(&device->dev); if (button->suspended) break; -- 2.17.1.1185.g55be947832-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ACPI LID: increment wakeup count only when notified. 2018-06-04 18:26 [PATCH] ACPI LID: increment wakeup count only when notified Ravi Chandra Sadineni @ 2018-06-04 19:19 ` Benson Leung 2018-06-06 7:00 ` Rafael J. Wysocki 1 sibling, 0 replies; 11+ messages in thread From: Benson Leung @ 2018-06-04 19:19 UTC (permalink / raw) To: Ravi Chandra Sadineni Cc: rjw, lenb, ravisadineni, dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, furquan, bleung [-- Attachment #1: Type: text/plain, Size: 703 bytes --] On Mon, Jun 04, 2018 at 11:26:12AM -0700, Ravi Chandra Sadineni wrote: > Currently ACPI LID increments wakeup count irrespective of the wake source. > This is because we call acpi_lid_initialize_state on every resume. > Userland deamons using wakeup_count to identify the potential wake > source for the last wake will be thrown off by this. Instead increment > the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> Reviewed-by: Benson Leung <bleung@chromium.org> -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ACPI LID: increment wakeup count only when notified. 2018-06-04 18:26 [PATCH] ACPI LID: increment wakeup count only when notified Ravi Chandra Sadineni 2018-06-04 19:19 ` Benson Leung @ 2018-06-06 7:00 ` Rafael J. Wysocki 2018-06-06 23:11 ` Benson Leung 1 sibling, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-06 7:00 UTC (permalink / raw) To: Ravi Chandra Sadineni Cc: Rafael J. Wysocki, Len Brown, Ravi Chandra Sadineni, Dmitry Torokhov, Todd Broch, Linux Kernel Mailing List, ACPI Devel Maling List, Rajat Jain, Benson Leung, Furquan Shaikh On Mon, Jun 4, 2018 at 8:26 PM, Ravi Chandra Sadineni <ravisadineni@chromium.org> wrote: > Currently ACPI LID increments wakeup count irrespective of the wake source. > This is because we call acpi_lid_initialize_state on every resume. I don't quite understand the connection between the two sentences above. Care to clarify? > Userland deamons using wakeup_count to identify the potential wake > source for the last wake will be thrown off by this. Instead increment > the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > --- > drivers/acpi/button.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index f1cc4f9d31cd9..d40fef7241f08 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) > button->last_time = ktime_get(); > } > > - if (state) > - acpi_pm_wakeup_event(&device->dev); > - > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); > if (ret == NOTIFY_DONE) > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > /* fall through */ > case ACPI_BUTTON_NOTIFY_STATUS: > input = button->input; > + acpi_pm_wakeup_event(&device->dev); Not really. There already is an acpi_pm_wakeup_event() call in the else branch below. > if (button->type == ACPI_BUTTON_TYPE_LID) { > mutex_lock(&button->input->mutex); > users = button->input->users; > @@ -426,7 +424,6 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > } else { > int keycode; > > - acpi_pm_wakeup_event(&device->dev); > if (button->suspended) > break; > > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ACPI LID: increment wakeup count only when notified. 2018-06-06 7:00 ` Rafael J. Wysocki @ 2018-06-06 23:11 ` Benson Leung 2018-06-06 23:21 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Benson Leung @ 2018-06-06 23:11 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Ravi Chandra Sadineni, Rafael J. Wysocki, Len Brown, Ravi Chandra Sadineni, Dmitry Torokhov, Todd Broch, Linux Kernel Mailing List, ACPI Devel Maling List, Rajat Jain, Furquan Shaikh, bleung [-- Attachment #1: Type: text/plain, Size: 1358 bytes --] Hi Rafael, On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote: > > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > > /* fall through */ > > case ACPI_BUTTON_NOTIFY_STATUS: > > input = button->input; > > + acpi_pm_wakeup_event(&device->dev); > > Not really. > > There already is an acpi_pm_wakeup_event() call in the else branch below. > Ravi removes that other call below. The intent for this is to call acpi_pm_wakeup_event() regardless if the button->type is ACPI_BUTTON_TYPE_LID, in case that event is ACPI_BUTTON_NOTIFY_STATUS. > > if (button->type == ACPI_BUTTON_TYPE_LID) { > > mutex_lock(&button->input->mutex); > > users = button->input->users; > > @@ -426,7 +424,6 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > > } else { > > int keycode; > > > > - acpi_pm_wakeup_event(&device->dev); > > if (button->suspended) > > break; > > > > -- Thanks! Benson -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ACPI LID: increment wakeup count only when notified. 2018-06-06 23:11 ` Benson Leung @ 2018-06-06 23:21 ` Rafael J. Wysocki 2018-06-11 17:57 ` [PATCH V2] " Ravi Chandra Sadineni 2018-06-11 17:59 ` [PATCH] " Ravi Chandra Sadineni 0 siblings, 2 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-06 23:21 UTC (permalink / raw) To: Benson Leung Cc: Rafael J. Wysocki, Ravi Chandra Sadineni, Rafael J. Wysocki, Len Brown, Ravi Chandra Sadineni, Dmitry Torokhov, Todd Broch, Linux Kernel Mailing List, ACPI Devel Maling List, Rajat Jain, Furquan Shaikh, Benson Leung On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung <bleung@google.com> wrote: > Hi Rafael, > > On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote: >> > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) >> > /* fall through */ >> > case ACPI_BUTTON_NOTIFY_STATUS: >> > input = button->input; >> > + acpi_pm_wakeup_event(&device->dev); >> >> Not really. >> >> There already is an acpi_pm_wakeup_event() call in the else branch below. >> > > Ravi removes that other call below. OK, I missed that, not sure why, sorry. > The intent for this is to call > acpi_pm_wakeup_event() regardless if the button->type is ACPI_BUTTON_TYPE_LID, > in case that event is ACPI_BUTTON_NOTIFY_STATUS. Well, the patch really drops the acpi_pm_wakeup_event() call from acpi_lid_notify_state() and so it has to ensure that this function will be called here for ACPI_BUTTON_NOTIFY_STATUS regardless of the button->type value. That's fine, but still the changelog doesn't make it clear why the acpi_pm_wakeup_event() call in acpi_lid_notify_state() is not necessary in general. Thanks, Rafael ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V2] ACPI LID: increment wakeup count only when notified. 2018-06-06 23:21 ` Rafael J. Wysocki @ 2018-06-11 17:57 ` Ravi Chandra Sadineni [not found] ` <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com> 2018-06-26 9:55 ` Rafael J. Wysocki 2018-06-11 17:59 ` [PATCH] " Ravi Chandra Sadineni 1 sibling, 2 replies; 11+ messages in thread From: Ravi Chandra Sadineni @ 2018-06-11 17:57 UTC (permalink / raw) To: rjw, lenb, ravisadineni, ravisadineni Cc: dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, bleung, furquan Currently ACPI LID increments wakeup count irrespective of the wake source. This is because we call acpi_lid_initialize_state on every resume. Userland deamons using wakeup_count to identify the potential wake source for the last wake will be thrown off by this. Instead increment the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> --- V2: Increment the wakeup count only when the lid is open. drivers/acpi/button.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 2345a5ee2dbbc..d2fe03e4faf05 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) button->last_time = ktime_get(); } - if (state) - acpi_pm_wakeup_event(&device->dev); - ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); if (ret == NOTIFY_DONE) ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, @@ -366,7 +363,8 @@ int acpi_lid_open(void) } EXPORT_SYMBOL(acpi_lid_open); -static int acpi_lid_update_state(struct acpi_device *device) +static int acpi_lid_update_state(struct acpi_device *device, + bool is_notification) { int state; @@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device) if (state < 0) return state; + if (state && is_notification) + acpi_pm_wakeup_event(&device->dev); + return acpi_lid_notify_state(device, state); } @@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device *device) (void)acpi_lid_notify_state(device, 1); break; case ACPI_BUTTON_LID_INIT_METHOD: - (void)acpi_lid_update_state(device); + (void)acpi_lid_update_state(device, false); break; case ACPI_BUTTON_LID_INIT_IGNORE: default: @@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) users = button->input->users; mutex_unlock(&button->input->mutex); if (users) - acpi_lid_update_state(device); + acpi_lid_update_state(device, true); } else { int keycode; -- 2.18.0.rc1.242.g61856ae69a-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com>]
* Re: [PATCH V2] ACPI LID: increment wakeup count only when notified. [not found] ` <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com> @ 2018-06-21 13:11 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-21 13:11 UTC (permalink / raw) To: Ravi Chandra Sadineni Cc: Ravi Chandra Sadineni, Len Brown, Dmitry Torokhov, Todd Broch, Linux Kernel Mailing List, ACPI Devel Maling List, Rajat Jain, Benson Leung, Furquan Shaikh On Wednesday, June 20, 2018 5:45:50 PM CEST Ravi Chandra Sadineni wrote: > Hi Rafael, > > Is this good to be merged ? Not for 4.18, so please give me some time here. > If you see any problem with the patch, can you > please let me know. I will. Thanks, Rafael > On Mon, Jun 11, 2018 at 10:57 AM, Ravi Chandra Sadineni < > ravisadineni@chromium.org> wrote: > > > Currently ACPI LID increments wakeup count irrespective of the wake source. > > This is because we call acpi_lid_initialize_state on every resume. > > Userland deamons using wakeup_count to identify the potential wake > > source for the last wake will be thrown off by this. Instead increment > > the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. > > > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > --- > > V2: Increment the wakeup count only when the lid is open. > > > > drivers/acpi/button.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > > index 2345a5ee2dbbc..d2fe03e4faf05 100644 > > --- a/drivers/acpi/button.c > > +++ b/drivers/acpi/button.c > > @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device > > *device, int state) > > button->last_time = ktime_get(); > > } > > > > - if (state) > > - acpi_pm_wakeup_event(&device->dev); > > - > > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, > > device); > > if (ret == NOTIFY_DONE) > > ret = blocking_notifier_call_chain(&acpi_lid_notifier, > > state, > > @@ -366,7 +363,8 @@ int acpi_lid_open(void) > > } > > EXPORT_SYMBOL(acpi_lid_open); > > > > -static int acpi_lid_update_state(struct acpi_device *device) > > +static int acpi_lid_update_state(struct acpi_device *device, > > + bool is_notification) > > { > > int state; > > > > @@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device > > *device) > > if (state < 0) > > return state; > > > > + if (state && is_notification) > > + acpi_pm_wakeup_event(&device->dev); > > + > > return acpi_lid_notify_state(device, state); > > } > > > > @@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct > > acpi_device *device) > > (void)acpi_lid_notify_state(device, 1); > > break; > > case ACPI_BUTTON_LID_INIT_METHOD: > > - (void)acpi_lid_update_state(device); > > + (void)acpi_lid_update_state(device, false); > > break; > > case ACPI_BUTTON_LID_INIT_IGNORE: > > default: > > @@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device > > *device, u32 event) > > users = button->input->users; > > mutex_unlock(&button->input->mutex); > > if (users) > > - acpi_lid_update_state(device); > > + acpi_lid_update_state(device, true); > > } else { > > int keycode; > > > > -- > > 2.18.0.rc1.242.g61856ae69a-goog > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] ACPI LID: increment wakeup count only when notified. 2018-06-11 17:57 ` [PATCH V2] " Ravi Chandra Sadineni [not found] ` <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com> @ 2018-06-26 9:55 ` Rafael J. Wysocki 2018-06-27 17:55 ` [PATCH V3] " Ravi Chandra Sadineni 1 sibling, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-06-26 9:55 UTC (permalink / raw) To: Ravi Chandra Sadineni Cc: lenb, ravisadineni, dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, bleung, furquan On Monday, June 11, 2018 7:57:59 PM CEST Ravi Chandra Sadineni wrote: > Currently ACPI LID increments wakeup count irrespective of the wake source. What exactly does the above mean? > This is because we call acpi_lid_initialize_state on every resume. I guess "because acpi_lid_initialize_state() is called on every system resume and it triggers acpi_lid_notify_state() which invokes acpi_pm_wakeup_event() for the lid device, the lid's wakeup count is incremented even if the lid was not the source of the event that woke up the system", right? > Userland deamons using wakeup_count to identify the potential wake > source for the last wake will be thrown off by this. Something like: "That behavior confuses user space deamons using wakeup_count to identify the potential system wakeup source." > Instead increment > the wakeup count only when there is a FIXED_HARDWARE/NOTFIY_STATUS event. So "to avoid that confusion, only trigger acpi_pm_wakeup_event() in the acpi_button_notify() path and don't do that in the acpi_lid_initialize_state() path." > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > --- > V2: Increment the wakeup count only when the lid is open. > > drivers/acpi/button.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 2345a5ee2dbbc..d2fe03e4faf05 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) > button->last_time = ktime_get(); > } > > - if (state) > - acpi_pm_wakeup_event(&device->dev); > - > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); > if (ret == NOTIFY_DONE) > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, > @@ -366,7 +363,8 @@ int acpi_lid_open(void) > } > EXPORT_SYMBOL(acpi_lid_open); > > -static int acpi_lid_update_state(struct acpi_device *device) > +static int acpi_lid_update_state(struct acpi_device *device, > + bool is_notification) Please rename "is_notification" to "signal_wakeup" or similar. > { > int state; > > @@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device) > if (state < 0) > return state; > > + if (state && is_notification) > + acpi_pm_wakeup_event(&device->dev); > + > return acpi_lid_notify_state(device, state); > } > > @@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > (void)acpi_lid_notify_state(device, 1); > break; > case ACPI_BUTTON_LID_INIT_METHOD: > - (void)acpi_lid_update_state(device); > + (void)acpi_lid_update_state(device, false); > break; > case ACPI_BUTTON_LID_INIT_IGNORE: > default: > @@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > users = button->input->users; > mutex_unlock(&button->input->mutex); > if (users) > - acpi_lid_update_state(device); > + acpi_lid_update_state(device, true); > } else { > int keycode; > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH V3] ACPI LID: increment wakeup count only when notified. 2018-06-26 9:55 ` Rafael J. Wysocki @ 2018-06-27 17:55 ` Ravi Chandra Sadineni 2018-07-04 10:39 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Ravi Chandra Sadineni @ 2018-06-27 17:55 UTC (permalink / raw) To: rjw, lenb, ravisadineni, ravisadineni Cc: dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, bleung, furquan Because acpi_lid_initialize_state() is called on every system resume and it triggers acpi_lid_notify_state() which invokes acpi_pm_wakeup_event() for the lid device, the lid's wakeup count is incremented even if the lid was not the source of the event that woke up the system. That behavior confuses user space deamons using wakeup_count to identify the potential system wakeup source. To avoid the confusion, only trigger acpi_pm_wakeup_event() in the acpi_button_notify() path and don't do that in the acpi_lid_initialize_state() path. Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> --- v3: Change is_notification to signal_wakeup V2: Increment the wakeup count only when the lid is open. drivers/acpi/button.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 2345a5ee2dbbc..40ed3ec9fc94c 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) button->last_time = ktime_get(); } - if (state) - acpi_pm_wakeup_event(&device->dev); - ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); if (ret == NOTIFY_DONE) ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, @@ -366,7 +363,8 @@ int acpi_lid_open(void) } EXPORT_SYMBOL(acpi_lid_open); -static int acpi_lid_update_state(struct acpi_device *device) +static int acpi_lid_update_state(struct acpi_device *device, + bool signal_wakeup) { int state; @@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device) if (state < 0) return state; + if (state && signal_wakeup) + acpi_pm_wakeup_event(&device->dev); + return acpi_lid_notify_state(device, state); } @@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device *device) (void)acpi_lid_notify_state(device, 1); break; case ACPI_BUTTON_LID_INIT_METHOD: - (void)acpi_lid_update_state(device); + (void)acpi_lid_update_state(device, false); break; case ACPI_BUTTON_LID_INIT_IGNORE: default: @@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) users = button->input->users; mutex_unlock(&button->input->mutex); if (users) - acpi_lid_update_state(device); + acpi_lid_update_state(device, true); } else { int keycode; -- 2.18.0.rc2.346.g013aa6912e-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V3] ACPI LID: increment wakeup count only when notified. 2018-06-27 17:55 ` [PATCH V3] " Ravi Chandra Sadineni @ 2018-07-04 10:39 ` Rafael J. Wysocki 0 siblings, 0 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-07-04 10:39 UTC (permalink / raw) To: Ravi Chandra Sadineni Cc: lenb, ravisadineni, dmitry.torokhov, tbroch, linux-kernel, linux-acpi, rajatja, bleung, furquan On Wednesday, June 27, 2018 7:55:02 PM CEST Ravi Chandra Sadineni wrote: > Because acpi_lid_initialize_state() is called on every system > resume and it triggers acpi_lid_notify_state() which invokes > acpi_pm_wakeup_event() for the lid device, the lid's wakeup count is > incremented even if the lid was not the source of the event that woke up > the system. That behavior confuses user space deamons using > wakeup_count to identify the potential system wakeup source. To avoid > the confusion, only trigger acpi_pm_wakeup_event() in the > acpi_button_notify() path and don't do that in the > acpi_lid_initialize_state() path. > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > --- > v3: Change is_notification to signal_wakeup > V2: Increment the wakeup count only when the lid is open. > > drivers/acpi/button.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 2345a5ee2dbbc..40ed3ec9fc94c 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -235,9 +235,6 @@ static int acpi_lid_notify_state(struct acpi_device *device, int state) > button->last_time = ktime_get(); > } > > - if (state) > - acpi_pm_wakeup_event(&device->dev); > - > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device); > if (ret == NOTIFY_DONE) > ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, > @@ -366,7 +363,8 @@ int acpi_lid_open(void) > } > EXPORT_SYMBOL(acpi_lid_open); > > -static int acpi_lid_update_state(struct acpi_device *device) > +static int acpi_lid_update_state(struct acpi_device *device, > + bool signal_wakeup) > { > int state; > > @@ -374,6 +372,9 @@ static int acpi_lid_update_state(struct acpi_device *device) > if (state < 0) > return state; > > + if (state && signal_wakeup) > + acpi_pm_wakeup_event(&device->dev); > + > return acpi_lid_notify_state(device, state); > } > > @@ -384,7 +385,7 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > (void)acpi_lid_notify_state(device, 1); > break; > case ACPI_BUTTON_LID_INIT_METHOD: > - (void)acpi_lid_update_state(device); > + (void)acpi_lid_update_state(device, false); > break; > case ACPI_BUTTON_LID_INIT_IGNORE: > default: > @@ -409,7 +410,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > users = button->input->users; > mutex_unlock(&button->input->mutex); > if (users) > - acpi_lid_update_state(device); > + acpi_lid_update_state(device, true); > } else { > int keycode; > > Applied, thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ACPI LID: increment wakeup count only when notified. 2018-06-06 23:21 ` Rafael J. Wysocki 2018-06-11 17:57 ` [PATCH V2] " Ravi Chandra Sadineni @ 2018-06-11 17:59 ` Ravi Chandra Sadineni 1 sibling, 0 replies; 11+ messages in thread From: Ravi Chandra Sadineni @ 2018-06-11 17:59 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Benson Leung, Ravi Chandra Sadineni, Rafael J. Wysocki, Len Brown, Dmitry Torokhov, Todd Broch, Linux Kernel Mailing List, ACPI Devel Maling List, Rajat Jain, Furquan Shaikh, Benson Leung Hi Rafael, Hopefully this will clear things a bit. 1. Why is this patch needed ? Consider the following scenario. 1. User left the device idle for some time. 2. A deamon in the userland that controls suspend policy might suspend the device. The lid is still open. 3. Now the user returns and wakes the system up by pressing a key. The system resumes. 4. In the current implementation we call pm_wakeup_event() (if the lid is open) irrespective of whether we have received a notify signal. 5. The deamon in the userland tries to identify what might woken the system up based on the wakeup_count. 6. The deamon sees a wakeup_count increment for both keyboard and lid and is confused. This patch is an attempt to address the same. 2. Will it break any existing logic ? AFAIK pm_wakeup_event() serves 2 purposes. 1. Helps preventing a race between system wide suspend and wakeup event. 2. Helps identifying the device that woke the system up. As far as preventing races during suspend is concerned, in the existing implementation, we increment the wakeup_count only when there is a lid open. Thus only lid open can prevent the system from suspending (if lid is a wake enabled device). But with my previous change, we will end up incrementing the wakeup_count for both lid close and lid open. Fixed this in V2, so that we do not increment the wakeup_count when there is a lid close. And currently we cannot identify if the lid is the reason for wake anyway. So this patch can only make things better here. Thanks, Ravi On Wed, Jun 6, 2018 at 4:21 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung <bleung@google.com> wrote: >> Hi Rafael, >> >> On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote: >>> > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) >>> > /* fall through */ >>> > case ACPI_BUTTON_NOTIFY_STATUS: >>> > input = button->input; >>> > + acpi_pm_wakeup_event(&device->dev); >>> >>> Not really. >>> >>> There already is an acpi_pm_wakeup_event() call in the else branch below. >>> >> >> Ravi removes that other call below. > > OK, I missed that, not sure why, sorry. > >> The intent for this is to call >> acpi_pm_wakeup_event() regardless if the button->type is ACPI_BUTTON_TYPE_LID, >> in case that event is ACPI_BUTTON_NOTIFY_STATUS. > > Well, the patch really drops the acpi_pm_wakeup_event() call from > acpi_lid_notify_state() and so it has to ensure that this function > will be called here for ACPI_BUTTON_NOTIFY_STATUS regardless of the > button->type value. > > That's fine, but still the changelog doesn't make it clear why the > acpi_pm_wakeup_event() call in acpi_lid_notify_state() is not > necessary in general. > > Thanks, > Rafael ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-07-04 10:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-04 18:26 [PATCH] ACPI LID: increment wakeup count only when notified Ravi Chandra Sadineni 2018-06-04 19:19 ` Benson Leung 2018-06-06 7:00 ` Rafael J. Wysocki 2018-06-06 23:11 ` Benson Leung 2018-06-06 23:21 ` Rafael J. Wysocki 2018-06-11 17:57 ` [PATCH V2] " Ravi Chandra Sadineni [not found] ` <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com> 2018-06-21 13:11 ` Rafael J. Wysocki 2018-06-26 9:55 ` Rafael J. Wysocki 2018-06-27 17:55 ` [PATCH V3] " Ravi Chandra Sadineni 2018-07-04 10:39 ` Rafael J. Wysocki 2018-06-11 17:59 ` [PATCH] " Ravi Chandra Sadineni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).