Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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 wrote: > On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung 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(>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
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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 wrote: > On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung 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(>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
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung 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(>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
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung 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(>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
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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(>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(>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(>dev);
> > if (button->suspended)
> > break;
> >
> > --
Thanks!
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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(>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(>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(>dev);
> > if (button->suspended)
> > break;
> >
> > --
Thanks!
Benson
--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
ble...@google.com
Chromium OS Project
ble...@chromium.org
signature.asc
Description: PGP signature
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
On Mon, Jun 4, 2018 at 8:26 PM, 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.
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
> ---
> 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(>dev);
> -
> ret = blocking_notifier_call_chain(_lid_notifier, state, device);
> if (ret == NOTIFY_DONE)
> ret = blocking_notifier_call_chain(_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(>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(>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(>dev);
> if (button->suspended)
> break;
>
> --
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
On Mon, Jun 4, 2018 at 8:26 PM, 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.
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
> ---
> 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(>dev);
> -
> ret = blocking_notifier_call_chain(_lid_notifier, state, device);
> if (ret == NOTIFY_DONE)
> ret = blocking_notifier_call_chain(_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(>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(>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(>dev);
> if (button->suspended)
> break;
>
> --
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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 Reviewed-by: Benson Leung -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. ble...@google.com Chromium OS Project ble...@chromium.org signature.asc Description: PGP signature
Re: [PATCH] ACPI LID: increment wakeup count only when notified.
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 Reviewed-by: Benson Leung -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. ble...@google.com Chromium OS Project ble...@chromium.org signature.asc Description: PGP signature
[PATCH] ACPI LID: increment wakeup count only when notified.
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
---
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(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_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(>dev);
if (button->type == ACPI_BUTTON_TYPE_LID) {
mutex_lock(>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(>dev);
if (button->suspended)
break;
--
2.17.1.1185.g55be947832-goog
[PATCH] ACPI LID: increment wakeup count only when notified.
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
---
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(>dev);
-
ret = blocking_notifier_call_chain(_lid_notifier, state, device);
if (ret == NOTIFY_DONE)
ret = blocking_notifier_call_chain(_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(>dev);
if (button->type == ACPI_BUTTON_TYPE_LID) {
mutex_lock(>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(>dev);
if (button->suspended)
break;
--
2.17.1.1185.g55be947832-goog
