linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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

* 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

* 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

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