| Message ID | 20231007075433.555715-1-Ken.Xue@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [V5] acpi: trigger wakeup key event from power button | expand |
Hi Ken, kernel test robot noticed the following build errors: [auto build test ERROR on b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4] url: https://github.com/intel-lab-lkp/linux/commits/Ken-Xue/acpi-trigger-wakeup-key-event-from-power-button/20231007-163549 base: b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4 patch link: https://lore.kernel.org/r/20231007075433.555715-1-Ken.Xue%40amd.com patch subject: [PATCH V5] acpi: trigger wakeup key event from power button config: loongarch-randconfig-002-20231011 (https://download.01.org/0day-ci/archive/20231012/202310120000.LkRVKBZN-lkp@intel.com/config) compiler: loongarch64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231012/202310120000.LkRVKBZN-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310120000.LkRVKBZN-lkp@intel.com/ All errors (new ones prefixed by >>): loongarch64-linux-ld: drivers/acpi/button.o: in function `acpi_power_button_wakeup': >> button.c:(.text+0x44): undefined reference to `input_event' >> loongarch64-linux-ld: button.c:(.text+0x58): undefined reference to `input_event' loongarch64-linux-ld: button.c:(.text+0x6c): undefined reference to `input_event' loongarch64-linux-ld: button.c:(.text+0x8c): undefined reference to `input_event' loongarch64-linux-ld: drivers/acpi/button.o: in function `acpi_button_notify': button.c:(.text+0x200): undefined reference to `input_event' loongarch64-linux-ld: drivers/acpi/button.o:button.c:(.text+0x214): more undefined references to `input_event' follow loongarch64-linux-ld: drivers/acpi/button.o: in function `.L42': >> button.c:(.text+0x3ac): undefined reference to `input_allocate_device' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L52': >> button.c:(.text+0x60c): undefined reference to `input_free_device' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L53': >> button.c:(.text+0x67c): undefined reference to `input_set_capability' >> loongarch64-linux-ld: button.c:(.text+0x68c): undefined reference to `input_set_capability' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L58': button.c:(.text+0x6a0): undefined reference to `input_set_capability' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L59': button.c:(.text+0x6b4): undefined reference to `input_set_capability' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L60': >> button.c:(.text+0x6c0): undefined reference to `input_register_device' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L63': >> button.c:(.text+0x6e0): undefined reference to `input_unregister_device' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L87': button.c:(.text+0x7f8): undefined reference to `input_unregister_device' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L99': button.c:(.text+0x89c): undefined reference to `input_event' loongarch64-linux-ld: button.c:(.text+0x8b0): undefined reference to `input_event' loongarch64-linux-ld: drivers/acpi/button.o: in function `.L97': button.c:(.text+0x8c4): undefined reference to `input_event' loongarch64-linux-ld: button.c:(.text+0x8d8): undefined reference to `input_event' Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for ACPI_BUTTON Depends on [m]: ACPI [=y] && INPUT [=m] Selected by [y]: - ACPI_SYSTEM_POWER_STATES_SUPPORT [=y] && ACPI [=y]
On Sat, Oct 7, 2023 at 9:55 AM Ken Xue <Ken.Xue@amd.com> 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 > resuming by default. > > So, send wakeup key event to user space during resume from power button. > > Signed-off-by: Ken Xue <Ken.Xue@amd.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > V1->V2: fix some compile warning/error caused by lack of > "struct acpi_device" declaration by including acpi.h. > V2->V3: use "forward declaration" to fix compile warning/error. > V3->V4: refine coding style and commit message > V4->V5: add "select ACPI_BUTTON" to fix build error if CONFIG_ACPI_BUTTON=m. https://lore.kernel.org/oe-kbuild-all/202309150947.YLjvs2Vv-lkp@intel.com/ > --- > drivers/acpi/Kconfig | 1 + > drivers/acpi/button.c | 17 +++++++++++++++++ > drivers/acpi/sleep.c | 5 +++++ > include/acpi/button.h | 4 ++++ > 4 files changed, 27 insertions(+) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index 00dd309b6682..001da6233fcd 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -53,6 +53,7 @@ config ACPI_GENERIC_GSI > > config ACPI_SYSTEM_POWER_STATES_SUPPORT > bool > + select ACPI_BUTTON > > config ACPI_CCA_REQUIRED > bool > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 1e76a64cce0a..3baddecd66c6 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -363,6 +363,22 @@ static int acpi_button_remove_fs(struct acpi_device *device) > return 0; > } > > +void acpi_power_button_wakeup(struct acpi_device *dev) > +{ > + struct acpi_button *button = acpi_driver_data(dev); > + struct input_dev *input; > + > + if (button->type != ACPI_BUTTON_TYPE_POWER) > + return; > + > + 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 +595,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..f816606abd71 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -20,9 +20,13 @@ > #include <linux/acpi.h> > #include <linux/module.h> > #include <linux/syscore_ops.h> > + > #include <asm/io.h> > + > #include <trace/events/power.h> > > +#include <acpi/button.h> Not really. > + > #include "internal.h" > #include "sleep.h" > > @@ -507,6 +511,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); Why does this need to be done from here? Can't the button driver itself to it? > 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..6126d665aa42 100644 > --- a/include/acpi/button.h > +++ b/include/acpi/button.h > @@ -2,17 +2,21 @@ > #ifndef ACPI_BUTTON_H > #define ACPI_BUTTON_H > > +struct acpi_device; > + > #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 *dev); > #else > static inline int acpi_lid_open(void) > { > return 1; > } > +static inline void acpi_power_button_wakeup(struct acpi_device *dev) {} > #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */ > > #endif /* ACPI_BUTTON_H */ > > base-commit: b483d3b8a54a544ab8854ca6dbb8d99c423b3ba4 > -- > 2.35.1 >
[AMD Official Use Only - General] > > #include "internal.h" > > #include "sleep.h" > > > > @@ -507,6 +511,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); > > Why does this need to be done from here? > > Can't the button driver itself to it? [Ken] Thanks for reminding. It does work in button driver itself too. This patch just tried to make sure "wakeup" event is reported after pwr_btn_event_pending becomes false by adding codes here. But in fact, Reporting "wakeup" event in button driver itself also can support same sequence. I will submit a new patch later.
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 00dd309b6682..001da6233fcd 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -53,6 +53,7 @@ config ACPI_GENERIC_GSI config ACPI_SYSTEM_POWER_STATES_SUPPORT bool + select ACPI_BUTTON config ACPI_CCA_REQUIRED bool diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 1e76a64cce0a..3baddecd66c6 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -363,6 +363,22 @@ static int acpi_button_remove_fs(struct acpi_device *device) return 0; } +void acpi_power_button_wakeup(struct acpi_device *dev) +{ + struct acpi_button *button = acpi_driver_data(dev); + struct input_dev *input; + + if (button->type != ACPI_BUTTON_TYPE_POWER) + return; + + 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 +595,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..f816606abd71 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -20,9 +20,13 @@ #include <linux/acpi.h> #include <linux/module.h> #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 +511,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..6126d665aa42 100644 --- a/include/acpi/button.h +++ b/include/acpi/button.h @@ -2,17 +2,21 @@ #ifndef ACPI_BUTTON_H #define ACPI_BUTTON_H +struct acpi_device; + #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 *dev); #else static inline int acpi_lid_open(void) { return 1; } +static inline void acpi_power_button_wakeup(struct acpi_device *dev) {} #endif /* IS_ENABLED(CONFIG_ACPI_BUTTON) */ #endif /* ACPI_BUTTON_H */