| Message ID | 20230908095747.446389-1-Ken.Xue@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi: trigger wakeup key event from power button | expand |
On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote: > Andorid can wakeup from various wakeup sources, > but only several wakeup sources can wake up screen > with right events(POWER, WAKEUP) from input device. > > Regarding pressing acpi power button, it can resume system and > ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS > are set in pm1a_sts, but kernel does not report any key > event to user space during resume by default. > > So, trigger wakeup key event to user space during resume > from power button. > Reported-by: kernel test robot <lkp@intel.com> Are you sure? > Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/ Are you sure? > > Blank lines are not allowed in the tag block. > Signed-off-by: Ken Xue <Ken.Xue@amd.com> > --- How this change is different to the previous patch you sent? Do you forgot versioning? Do you forgot changelog? Please, read Submitting Patches documentation before trying again. It will help to make your contribution nice and understandable. ... > + if (button->type == ACPI_BUTTON_TYPE_POWER) { if (... != ) return; ? > + input = button->input; > + input_report_key(input, KEY_WAKEUP, 1); > + input_sync(input); > + input_report_key(input, KEY_WAKEUP, 0); > + input_sync(input); > + } ... > +#include <linux/acpi.h> There is no users of this header. Check how forward declaration can be used (as it's done in many other headers). > +extern void acpi_power_button_wakeup(struct acpi_device *device); ... > +static inline void acpi_power_button_wakeup(struct acpi_device *device) > +{ > +} This can be done on a single line.
On 2023/9/11 17:42, Andy Shevchenko wrote: > On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote: >> Andorid can wakeup from various wakeup sources, >> but only several wakeup sources can wake up screen >> with right events(POWER, WAKEUP) from input device. >> >> Regarding pressing acpi power button, it can resume system and >> ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS >> are set in pm1a_sts, but kernel does not report any key >> event to user space during resume by default. >> >> So, trigger wakeup key event to user space during resume >> from power button. >> Reported-by: kernel test robot <lkp@intel.com> > Are you sure? Thanks for review. Sorry for confusion. 1)The patch is trying to fix the android wakeup from power button issue I can see Android can resume from S3 and wake up screen from USB keyboard. but Android only can resume from S3 and fail to wake up screen from power button. opensource android-x86 tried to fix this issue with a patch (https://git.osdn.net/view?p=android-x86/system-hardware-interfaces.git;a=commit;h=78688b149314ec16cb2d90507a5908e5f2ba0fda) in upper layer system_suspend service as a WA. It emulates wakeup key during resume in a user space thread. And it is still not part of upstream Android. I believe the root cause is no wakeup key event being reported in kernel during the resume from acpi power button. And I have verified this patch on linux also, it seems no side effect for Linux resume with this patch. 2) test robot reported some compile warnings and errors detected by test robot which is fixed in V2. > >> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/ >> Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/ >> Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/ > Are you sure? Just some errors/warnings from the v1 patch. > >> > Blank lines are not allowed in the tag block. > >> Signed-off-by: Ken Xue <Ken.Xue@amd.com> >> --- > How this change is different to the previous patch you sent? > Do you forgot versioning? > Do you forgot changelog? > > Please, read Submitting Patches documentation before trying again. > It will help to make your contribution nice and understandable. > > ... sorry. I will add V3 and change log later. > >> + if (button->type == ACPI_BUTTON_TYPE_POWER) { > if (... != ) > return; > > ? > >> + input = button->input; >> + input_report_key(input, KEY_WAKEUP, 1); >> + input_sync(input); >> + input_report_key(input, KEY_WAKEUP, 0); >> + input_sync(input); >> + } > ... > >> +#include <linux/acpi.h> > There is no users of this header. > > Check how forward declaration can be used (as it's done in many other headers). > Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but include acpi_bus.h alone will lead to more compile issues. Regarding "forward declaration", how about typedef struct acpi_device *acpi_device; >> +extern void acpi_power_button_wakeup(struct acpi_device *device); > ... > >> +static inline void acpi_power_button_wakeup(struct acpi_device *device) >> +{ >> +} > This can be done on a single line. > Ack.
On Tue, Sep 12, 2023 at 01:32:02PM +0800, Ken Xue wrote: > On 2023/9/11 17:42, Andy Shevchenko wrote: > > On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote: ... > > > Reported-by: kernel test robot <lkp@intel.com> > > Are you sure? > > Thanks for review. Sorry for confusion. > 2) test robot reported some compile warnings and errors detected by test > robot which is fixed in V2. Yes and that's what I'm asking about. You are not supposed to add it as the initial problem, the patch is trying to solve, has _not_ been reported by LKP, hasn't it? ... > > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/ > > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/ > > > Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/ > > Are you sure? > > Just some errors/warnings from the v1 patch. Same as above. ... > > > +#include <linux/acpi.h> > > There are no users of this header. > > > > Check how forward declaration can be used (as it's done in many other headers). > > > Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but > include acpi_bus.h alone will lead to more compile issues. > > Regarding "forward declaration", how about > > typedef struct acpi_device *acpi_device; Is it a forward declaration?
On 2023/9/12 17:30, Andy Shevchenko wrote: > On Tue, Sep 12, 2023 at 01:32:02PM +0800, Ken Xue wrote: >> On 2023/9/11 17:42, Andy Shevchenko wrote: >>> On Fri, Sep 08, 2023 at 05:57:49PM +0800, Ken Xue wrote: ... >> 2) test robot reported some compile warnings and errors detected by test >> robot which is fixed in V2. > Yes and that's what I'm asking about. You are not supposed to add it as the > initial problem, the patch is trying to solve, has _not_ been reported by LKP, > hasn't it? > > ... > >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/ >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/ >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/ >>> Are you sure? >> Just some errors/warnings from the v1 patch. > Same as above. Get it. Those info will not be included in commit message. > > ... > >>>> +#include <linux/acpi.h> >>> There are no users of this header. >>> >>> Check how forward declaration can be used (as it's done in many other headers). >>> >> Yes, "struct acpi_device" is defined in "include/acpi/acpi_bus.h", but >> include acpi_bus.h alone will lead to more compile issues. >> >> Regarding "forward declaration", how about >> >> typedef struct acpi_device *acpi_device; > Is it a forward declaration? Ok, I will use "struct acpi_device;"
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 1e76a64cce0a..0e0f30286c22 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -363,6 +363,21 @@ static int acpi_button_remove_fs(struct acpi_device *device) return 0; } +void acpi_power_button_wakeup(struct acpi_device *device) +{ + struct acpi_button *button = acpi_driver_data(device); + struct input_dev *input; + + if (button->type == ACPI_BUTTON_TYPE_POWER) { + input = button->input; + input_report_key(input, KEY_WAKEUP, 1); + input_sync(input); + input_report_key(input, KEY_WAKEUP, 0); + input_sync(input); + } +} +EXPORT_SYMBOL(acpi_power_button_wakeup); + /* Driver Interface */ int acpi_lid_open(void) { @@ -579,6 +594,7 @@ static int acpi_button_add(struct acpi_device *device) switch (button->type) { case ACPI_BUTTON_TYPE_POWER: input_set_capability(input, EV_KEY, KEY_POWER); + input_set_capability(input, EV_KEY, KEY_WAKEUP); break; case ACPI_BUTTON_TYPE_SLEEP: diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index 808484d11209..dcd5d0237eeb 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -22,6 +22,7 @@ #include <linux/syscore_ops.h> #include <asm/io.h> #include <trace/events/power.h> +#include <acpi/button.h> #include "internal.h" #include "sleep.h" @@ -507,6 +508,7 @@ static void acpi_pm_finish(void) pwr_btn_adev = acpi_dev_get_first_match_dev(ACPI_BUTTON_HID_POWERF, NULL, -1); if (pwr_btn_adev) { + acpi_power_button_wakeup(pwr_btn_adev); pm_wakeup_event(&pwr_btn_adev->dev, 0); acpi_dev_put(pwr_btn_adev); } diff --git a/include/acpi/button.h b/include/acpi/button.h index af2fce5d2ee3..fa0fb170cfb5 100644 --- a/include/acpi/button.h +++ b/include/acpi/button.h @@ -2,17 +2,23 @@ #ifndef ACPI_BUTTON_H #define ACPI_BUTTON_H +#include <linux/acpi.h> + #define ACPI_BUTTON_HID_POWER "PNP0C0C" #define ACPI_BUTTON_HID_LID "PNP0C0D" #define ACPI_BUTTON_HID_SLEEP "PNP0C0E" #if IS_ENABLED(CONFIG_ACPI_BUTTON) extern int acpi_lid_open(void); +extern void acpi_power_button_wakeup(struct acpi_device *device); #else static inline int acpi_lid_open(void) { return 1; } +static inline void acpi_power_button_wakeup(struct acpi_device *device) +{ +} #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */ #endif /* ACPI_BUTTON_H */
Andorid can wakeup from various wakeup sources, but only several wakeup sources can wake up screen with right events(POWER, WAKEUP) from input device. Regarding pressing acpi power button, it can resume system and ACPI_BITMASK_WAKE_STATUS and ACPI_BITMASK_POWER_BUTTON_STATUS are set in pm1a_sts, but kernel does not report any key event to user space during resume by default. So, trigger wakeup key event to user space during resume from power button. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202309080315.txQUEyHQ-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202309080239.IiC7uLpW-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202309080351.xHt2qhP2-lkp@intel.com/ Signed-off-by: Ken Xue <Ken.Xue@amd.com> --- drivers/acpi/button.c | 16 ++++++++++++++++ drivers/acpi/sleep.c | 2 ++ include/acpi/button.h | 6 ++++++ 3 files changed, 24 insertions(+) base-commit: b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4