[PATCH] ACPI / PM: Propagate KEY_POWER wakeup events to user space

[PATCH] ACPI / PM: Propagate KEY_POWER wakeup events to user space

[Chen, Hu]

When the system is woken from S3 by the ACPI fixed power button, send
KEY_POWER to user space.

I run Android on x86 PC (it's a NUC). Everytime I press the power button
to wake the system, it suspends right away. After some debug, I find
that Android wants to see KEY_POWER at resume. Otherwise, its
opportunistic suspend will kick in shortly.

Signed-off-by: Chen, Hu <hu1.chen@intel.com>
---
 drivers/acpi/button.c | 4 +++-
 drivers/acpi/sleep.c  | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index a19ff3977ac4..117718057938 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -33,6 +33,7 @@
 #include <linux/acpi.h>
 #include <linux/dmi.h>
 #include <acpi/button.h>
+#include <linux/suspend.h>
 
 #define PREFIX "ACPI: "
 
@@ -417,7 +418,8 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 			int keycode;
 
 			acpi_pm_wakeup_event(&device->dev);
-			if (button->suspended)
+			if (button->suspended &&
+			    mem_sleep_current == PM_SUSPEND_TO_IDLE)
 				break;
 
 			keycode = test_bit(KEY_SLEEP, input->keybit) ?
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 403c4ff15349..c5dcee9f5872 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
 	return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
 }
 
+static void pwr_btn_notify(struct device *dev)
+{
+	struct acpi_device *device = to_acpi_device(dev);
+
+	device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
+}
+
 /**
  *	acpi_pm_finish - Instruct the platform to leave a sleep state.
  *
@@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
 				      find_powerf_dev);
 	if (pwr_btn_dev) {
 		pm_wakeup_event(pwr_btn_dev, 0);
+		pwr_btn_notify(pwr_btn_dev);
 		put_device(pwr_btn_dev);
 	}
 }
-- 
2.20.1

Re: [PATCH] ACPI / PM: Propagate KEY_POWER wakeup events to user space

[Rafael J. Wysocki]

On Mon, Mar 4, 2019 at 4:18 AM Chen, Hu <hu1.chen@intel.com> wrote:
>
> When the system is woken from S3 by the ACPI fixed power button, send
> KEY_POWER to user space.
>
> I run Android on x86 PC (it's a NUC). Everytime I press the power button
> to wake the system, it suspends right away. After some debug, I find
> that Android wants to see KEY_POWER at resume. Otherwise, its
> opportunistic suspend will kick in shortly.

Alas that extra button event will cause other (non-Android) systems to
want to power off immediately after resume AFAICS.

Something else needs to be done for the Android case.

> Signed-off-by: Chen, Hu <hu1.chen@intel.com>
> ---
>  drivers/acpi/button.c | 4 +++-
>  drivers/acpi/sleep.c  | 8 ++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index a19ff3977ac4..117718057938 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -33,6 +33,7 @@
>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
>  #include <acpi/button.h>
> +#include <linux/suspend.h>
>
>  #define PREFIX "ACPI: "
>
> @@ -417,7 +418,8 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>                         int keycode;
>
>                         acpi_pm_wakeup_event(&device->dev);
> -                       if (button->suspended)
> +                       if (button->suspended &&
> +                           mem_sleep_current == PM_SUSPEND_TO_IDLE)
>                                 break;
>
>                         keycode = test_bit(KEY_SLEEP, input->keybit) ?
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 403c4ff15349..c5dcee9f5872 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
>         return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
>  }
>
> +static void pwr_btn_notify(struct device *dev)
> +{
> +       struct acpi_device *device = to_acpi_device(dev);
> +
> +       device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
> +}
> +
>  /**
>   *     acpi_pm_finish - Instruct the platform to leave a sleep state.
>   *
> @@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
>                                       find_powerf_dev);
>         if (pwr_btn_dev) {
>                 pm_wakeup_event(pwr_btn_dev, 0);
> +               pwr_btn_notify(pwr_btn_dev);
>                 put_device(pwr_btn_dev);
>         }
>  }
> --
> 2.20.1
>

[PATCH v2] ACPI / PM: Propagate KEY_POWER to user space when resume

[Chen, Hu]

I run Android on x86 PC (it's a NUC). Everytime I press the power button
to wake the system, it suspends right away. After some debug, I find
that Android wants to see KEY_POWER at resume. Otherwise, its
opportunistic suspend will kick in shortly.

However, other OS such as Ubuntu doesn't like KEY_POWER at resume. So
add a knob "/sys/module/button/parameters/key_power_at_resume" for users
to select.

Signed-off-by: Chen, Hu <hu1.chen@intel.com>
---

 drivers/acpi/button.c | 6 +++++-
 drivers/acpi/sleep.c  | 8 ++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index a19ff3977ac4..f98e6d85dd2b 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -129,6 +129,10 @@ struct acpi_button {
 	bool suspended;
 };
 
+/* does userspace want to see KEY_POWER at resume? */
+static bool key_power_at_resume __read_mostly;
+module_param(key_power_at_resume, bool, 0644);
+
 static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
 static struct acpi_device *lid_device;
 static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
@@ -417,7 +421,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
 			int keycode;
 
 			acpi_pm_wakeup_event(&device->dev);
-			if (button->suspended)
+			if (button->suspended && !key_power_at_resume)
 				break;
 
 			keycode = test_bit(KEY_SLEEP, input->keybit) ?
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 403c4ff15349..c5dcee9f5872 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
 	return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
 }
 
+static void pwr_btn_notify(struct device *dev)
+{
+	struct acpi_device *device = to_acpi_device(dev);
+
+	device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
+}
+
 /**
  *	acpi_pm_finish - Instruct the platform to leave a sleep state.
  *
@@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
 				      find_powerf_dev);
 	if (pwr_btn_dev) {
 		pm_wakeup_event(pwr_btn_dev, 0);
+		pwr_btn_notify(pwr_btn_dev);
 		put_device(pwr_btn_dev);
 	}
 }
-- 
2.20.1

Re: [PATCH v2] ACPI / PM: Propagate KEY_POWER to user space when resume

[Hans de Goede]

Hi,

On 28-03-19 11:34, Chen, Hu wrote:
> I run Android on x86 PC (it's a NUC). Everytime I press the power button
> to wake the system, it suspends right away. After some debug, I find
> that Android wants to see KEY_POWER at resume. Otherwise, its
> opportunistic suspend will kick in shortly.
> 
> However, other OS such as Ubuntu doesn't like KEY_POWER at resume. So
> add a knob "/sys/module/button/parameters/key_power_at_resume" for users
> to select.

We've had a similar discussion about the power-button on Bay Trail tablets,
which often is connected to a GPIO driver.

There we had the opposite problem, that regular desktop environments
like GNOME (and the related daemons) will immediately re-suspend again
on resume because of a KEY_POWER press event at resume. I submitted patches
to make the gpio-keys code not send a keypress on resume (configurable
per button) but that got nacked by Dmitry, the input subsystem maintainer.

It seems that this mostly is an evdev/input policy decision so that
this patch is going to need an ack from Dmitry, I've added Dmitry
to the Cc.

Note that after my kernel level fix for the Bay Trail devices got
nacked, I worked-around this in userspace:

https://gitlab.gnome.org/GNOME/gnome-settings-daemon/commit/f2ae8a3b9905cde7a9c12f78cb84689e97203380

But for GNOME only, e.g. KDE will likely still have a problem with
a KEY_POWER event being reported after suspend.

It seems the problem is that we have 2 different userspaces here
which have exact opposite expectations wrt KEY_POWER reporting
after a wakeup from suspend with the power-button. Android expects
it to be reported, which is why gpio-keys is reporting it since it
is mostly used with Android, where as classic Linux desktop-environments
expect it to NOT be reported, which is why the acpi-button.c code
so far has not reported it :|

I believe that unconditionally sending KEY_POWER after resume
will indeed causes issues for standard Linux distros, so I believe
that your solution with a parameter for people who want to run
android on plain x86 hardware is the best we can do now, but
I really wish we would remedy this situation one way or the other.

I wanted to give everyone involved the whole story, hence the
long mail :)

Regards,

Hans






> 
> Signed-off-by: Chen, Hu <hu1.chen@intel.com>
> ---
> 
>   drivers/acpi/button.c | 6 +++++-
>   drivers/acpi/sleep.c  | 8 ++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index a19ff3977ac4..f98e6d85dd2b 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -129,6 +129,10 @@ struct acpi_button {
>   	bool suspended;
>   };
>   
> +/* does userspace want to see KEY_POWER at resume? */
> +static bool key_power_at_resume __read_mostly;
> +module_param(key_power_at_resume, bool, 0644);
> +
>   static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>   static struct acpi_device *lid_device;
>   static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> @@ -417,7 +421,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>   			int keycode;
>   
>   			acpi_pm_wakeup_event(&device->dev);
> -			if (button->suspended)
> +			if (button->suspended && !key_power_at_resume)
>   				break;
>   
>   			keycode = test_bit(KEY_SLEEP, input->keybit) ?
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 403c4ff15349..c5dcee9f5872 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
>   	return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
>   }
>   
> +static void pwr_btn_notify(struct device *dev)
> +{
> +	struct acpi_device *device = to_acpi_device(dev);
> +
> +	device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
> +}
> +
>   /**
>    *	acpi_pm_finish - Instruct the platform to leave a sleep state.
>    *
> @@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
>   				      find_powerf_dev);
>   	if (pwr_btn_dev) {
>   		pm_wakeup_event(pwr_btn_dev, 0);
> +		pwr_btn_notify(pwr_btn_dev);
>   		put_device(pwr_btn_dev);
>   	}
>   }
> 

Re: [PATCH v2] ACPI / PM: Propagate KEY_POWER to user space when resume

[Pavel Machek]

> I run Android on x86 PC (it's a NUC). Everytime I press the power button
> to wake the system, it suspends right away. After some debug, I find
> that Android wants to see KEY_POWER at resume. Otherwise, its
> opportunistic suspend will kick in shortly.
> 
> However, other OS such as Ubuntu doesn't like KEY_POWER at resume. So
> add a knob "/sys/module/button/parameters/key_power_at_resume" for users
> to select.
> 
> Signed-off-by: Chen, Hu <hu1.chen@intel.com>

NAK.

Fix android, lets not break kernel.

								Pavel
> ---
> 
>  drivers/acpi/button.c | 6 +++++-
>  drivers/acpi/sleep.c  | 8 ++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index a19ff3977ac4..f98e6d85dd2b 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -129,6 +129,10 @@ struct acpi_button {
>  	bool suspended;
>  };
>  
> +/* does userspace want to see KEY_POWER at resume? */
> +static bool key_power_at_resume __read_mostly;
> +module_param(key_power_at_resume, bool, 0644);
> +
>  static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>  static struct acpi_device *lid_device;
>  static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
> @@ -417,7 +421,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>  			int keycode;
>  
>  			acpi_pm_wakeup_event(&device->dev);
> -			if (button->suspended)
> +			if (button->suspended && !key_power_at_resume)
>  				break;
>  
>  			keycode = test_bit(KEY_SLEEP, input->keybit) ?
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 403c4ff15349..c5dcee9f5872 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
>  	return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
>  }
>  
> +static void pwr_btn_notify(struct device *dev)
> +{
> +	struct acpi_device *device = to_acpi_device(dev);
> +
> +	device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
> +}
> +
>  /**
>   *	acpi_pm_finish - Instruct the platform to leave a sleep state.
>   *
> @@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
>  				      find_powerf_dev);
>  	if (pwr_btn_dev) {
>  		pm_wakeup_event(pwr_btn_dev, 0);
> +		pwr_btn_notify(pwr_btn_dev);
>  		put_device(pwr_btn_dev);
>  	}
>  }

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2] ACPI / PM: Propagate KEY_POWER to user space when resume

[Hans de Goede]

Hi,

On 3/29/19 11:25 AM, Pavel Machek wrote:
>> I run Android on x86 PC (it's a NUC). Everytime I press the power button
>> to wake the system, it suspends right away. After some debug, I find
>> that Android wants to see KEY_POWER at resume. Otherwise, its
>> opportunistic suspend will kick in shortly.
>>
>> However, other OS such as Ubuntu doesn't like KEY_POWER at resume. So
>> add a knob "/sys/module/button/parameters/key_power_at_resume" for users
>> to select.
>>
>> Signed-off-by: Chen, Hu <hu1.chen@intel.com>
> 
> NAK.
> 
> Fix android, lets not break kernel.

It is not that simple, as I explained in my other reply to this
patch, we alreayd have inconsistent behavior here inside the kernel.

When KEY_POWER is handled by the gpio-keys driver it does explicitly
send a KET_POWER press event when the system is woken up through the
power-button.

Arguably that is more consistent, e.g. some systems can also be woken
up through a home-button press and in that case we do want the KEY_HOMEPAGE
to be propagated to userspace after the wakeup so that we not only wake
but also switch to the homescreen (whatever that might be).

Also Android does have a good reason for wanting this, there are
many possible wakeup causes and just staying awake after wakeup is
not always the righ response. So android needs to know what is the
cause of the wakeup and the KEY_POWER event tells it the wakeup was
due to a power-button press, so the user explicitly wants the system
to wakeup.

Note I'm not saying that I'm happy with any of this, but simply NACK-ing
this patch is IMHO not the answer.

Regards,

Hans




> 
> 								Pavel
>> ---
>>
>>   drivers/acpi/button.c | 6 +++++-
>>   drivers/acpi/sleep.c  | 8 ++++++++
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
>> index a19ff3977ac4..f98e6d85dd2b 100644
>> --- a/drivers/acpi/button.c
>> +++ b/drivers/acpi/button.c
>> @@ -129,6 +129,10 @@ struct acpi_button {
>>   	bool suspended;
>>   };
>>   
>> +/* does userspace want to see KEY_POWER at resume? */
>> +static bool key_power_at_resume __read_mostly;
>> +module_param(key_power_at_resume, bool, 0644);
>> +
>>   static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
>>   static struct acpi_device *lid_device;
>>   static u8 lid_init_state = ACPI_BUTTON_LID_INIT_METHOD;
>> @@ -417,7 +421,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>>   			int keycode;
>>   
>>   			acpi_pm_wakeup_event(&device->dev);
>> -			if (button->suspended)
>> +			if (button->suspended && !key_power_at_resume)
>>   				break;
>>   
>>   			keycode = test_bit(KEY_SLEEP, input->keybit) ?
>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>> index 403c4ff15349..c5dcee9f5872 100644
>> --- a/drivers/acpi/sleep.c
>> +++ b/drivers/acpi/sleep.c
>> @@ -462,6 +462,13 @@ static int find_powerf_dev(struct device *dev, void *data)
>>   	return !strcmp(hid, ACPI_BUTTON_HID_POWERF);
>>   }
>>   
>> +static void pwr_btn_notify(struct device *dev)
>> +{
>> +	struct acpi_device *device = to_acpi_device(dev);
>> +
>> +	device->driver->ops.notify(device, ACPI_FIXED_HARDWARE_EVENT);
>> +}
>> +
>>   /**
>>    *	acpi_pm_finish - Instruct the platform to leave a sleep state.
>>    *
>> @@ -505,6 +512,7 @@ static void acpi_pm_finish(void)
>>   				      find_powerf_dev);
>>   	if (pwr_btn_dev) {
>>   		pm_wakeup_event(pwr_btn_dev, 0);
>> +		pwr_btn_notify(pwr_btn_dev);
>>   		put_device(pwr_btn_dev);
>>   	}
>>   }
> 

Re: [PATCH v2] ACPI / PM: Propagate KEY_POWER to user space when resume

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]

On Fri 2019-03-29 12:38:39, Hans de Goede wrote:
> Hi,
> 
> On 3/29/19 11:25 AM, Pavel Machek wrote:
> >>I run Android on x86 PC (it's a NUC). Everytime I press the power button
> >>to wake the system, it suspends right away. After some debug, I find
> >>that Android wants to see KEY_POWER at resume. Otherwise, its
> >>opportunistic suspend will kick in shortly.
> >>
> >>However, other OS such as Ubuntu doesn't like KEY_POWER at resume. So
> >>add a knob "/sys/module/button/parameters/key_power_at_resume" for users
> >>to select.
> >>
> >>Signed-off-by: Chen, Hu <hu1.chen@intel.com>
> >
> >NAK.
> >
> >Fix android, lets not break kernel.
> 
> It is not that simple, as I explained in my other reply to this
> patch, we alreayd have inconsistent behavior here inside the kernel.
> 
> When KEY_POWER is handled by the gpio-keys driver it does explicitly
> send a KET_POWER press event when the system is woken up through the
> power-button.

Which may be okay.

> Arguably that is more consistent, e.g. some systems can also be woken
> up through a home-button press and in that case we do want the KEY_HOMEPAGE
> to be propagated to userspace after the wakeup so that we not only wake
> but also switch to the homescreen (whatever that might be).

Which may also be okay.

> Note I'm not saying that I'm happy with any of this, but simply NACK-ing
> this patch is IMHO not the answer.

Well, to add a knob
"/sys/module/button/parameters/key_power_at_resume" is really not
acceptable. Android does not know that it needs to set it, so it will
not set it, and the problem remains. Plus, we get a great mess in
future.

But yes, we might want userland to know why the system woke up. And it
would be good if everything also worked with wake-on-lan. Hmm.

It is also possible that PC's ACPI power button should generate
something else than KEY_POWER. Power button on my USB keyboard
generates that, and that is really quite different button the one on
the box.

I'm not saying I know what the solution is. But we should have one
solution, not a knob to select between different solutions.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]