[PATCH] ACPI LID: increment wakeup count only when notified.

[PATCH] ACPI LID: increment wakeup count only when notified.

[Ravi Chandra Sadineni]

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

Re: [PATCH] ACPI LID: increment wakeup count only when notified.

[Benson Leung]

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

Re: [PATCH] ACPI LID: increment wakeup count only when notified.

[Rafael J. Wysocki]

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

Re: [PATCH] ACPI LID: increment wakeup count only when notified.

[Benson Leung]

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

Re: [PATCH] ACPI LID: increment wakeup count only when notified.

[Rafael J. Wysocki]

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

[PATCH V2] ACPI LID: increment wakeup count only when notified.

[Ravi Chandra Sadineni]

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

Re: [PATCH V2] ACPI LID: increment wakeup count only when notified.

[Rafael J. Wysocki]

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

Re: [PATCH V2] ACPI LID: increment wakeup count only when notified.

[Rafael J. Wysocki]

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

[PATCH V3] ACPI LID: increment wakeup count only when notified.

[Ravi Chandra Sadineni]

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

Re: [PATCH V3] ACPI LID: increment wakeup count only when notified.

[Rafael J. Wysocki]

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!

Re: [PATCH] ACPI LID: increment wakeup count only when notified.

[Ravi Chandra Sadineni]

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