From: Mike Larkin Subject: Re: [PATCH] Don't shut down after power button wakes To: Mark Kettenis Cc: James Cook , tech@openbsd.org Date: Fri, 10 May 2024 20:06:49 -0700 On Thu, May 09, 2024 at 06:29:42PM +0200, Mark Kettenis wrote: > > Date: Sun, 21 Apr 2024 17:04:21 +0000 > > From: James Cook > > > > This patch is adapted from one deraadt sent me in February. > > > > Without this patch, if I wake my laptop by pressing the power button, > > the laptop shuts down immediately after waking. The patch seems to > > solve the problem. > > > > I spent some time trying to be clever and re-enable the power button > > right after resuming was done, without using gettime(), but I don't > > understand how ACPI events work and could not push the re-enable-ing > > late enough. So I ended up sticking with this approach. > > > > Originally reported at > > https://marc.info/?l=openbsd-bugs&m=161038121430416&w=2 , but other > > bugs I mentioned on that thread have already disappeared. > > > > -- > > James > > > > > > diff refs/heads/master 16b4fd4f9a94db0f64c04f33b222b076e5808b3c > > commit - b809db8c59b9d17241cabc19d997f1eceddcff41 > > commit + 16b4fd4f9a94db0f64c04f33b222b076e5808b3c > > blob - 70b67bb8fa7fb283c773098abb0d31dc01c323d1 > > blob + da1703340257c980fe491db6323ef439dca4a9ed > > --- sys/dev/acpi/acpi.c > > +++ sys/dev/acpi/acpi.c > > @@ -2014,6 +2014,17 @@ acpi_gpe_task(void *arg0, int gpe) > > } > > } > > > > +/* Ignore the power button for a short time after a resume */ > > +int > > +acpi_buttonvalid(void) > > +{ > > + extern time_t waketime; > > + > > + if (gettime() < waketime + 10) > > + return 0; > > + return 1; > > +} > > + > > void > > acpi_pbtn_task(void *arg0, int dummy) > > { > > @@ -2035,7 +2046,8 @@ acpi_pbtn_task(void *arg0, int dummy) > > case 0: > > break; > > case 1: > > - acpi_addtask(sc, acpi_powerdown_task, sc, 0); > > + if (acpi_buttonvalid()) > > + acpi_addtask(sc, acpi_powerdown_task, sc, 0); > > break; > > #ifndef SMALL_KERNEL > > case 2: > > blob - 373f05e1377fad67f7435b3ba238fdc6fae4fec4 > > blob + cbced59a97fbb257f3290e769c1eb8ba837553d8 > > --- sys/dev/acpi/acpibtn.c > > +++ sys/dev/acpi/acpibtn.c > > @@ -286,8 +286,9 @@ sleep: > > case 0: > > break; > > case 1: > > - acpi_addtask(sc->sc_acpi, acpi_powerdown_task, > > - sc->sc_acpi, 0); > > + if (acpi_buttonvalid()) > > + acpi_addtask(sc->sc_acpi, acpi_powerdown_task, > > + sc->sc_acpi, 0); > > break; > > #ifndef SMALL_KERNEL > > case 2: > > blob - 20521653bc8ff99e82a0f7ccb4e498b6a92bf0c7 > > blob + 2bddceaa6689a1250b10b256a8c24657cd15d2bb > > --- sys/dev/acpi/acpivar.h > > +++ sys/dev/acpi/acpivar.h > > @@ -371,6 +371,7 @@ void acpi_write_pmreg(struct acpi_softc *, int, int, i > > > > void acpi_poll(void *); > > void acpi_sleep(int, char *); > > +int acpi_buttonvalid(void); > > > > int acpi_matchcls(struct acpi_attach_args *, int, int, int); > > int acpi_matchhids(struct acpi_attach_args *, const char *[], const char *); > > blob - 1b4f406291812c59af2d4fc8e3910e618c063f40 > > blob + ab1fffb6099d40fcf2db9c5cb090729c20d40cd5 > > --- sys/kern/subr_suspend.c > > +++ sys/kern/subr_suspend.c > > @@ -45,6 +45,8 @@ device_register_wakeup(struct device *dev) > > wakeup_devices++; > > } > > > > +time_t waketime; > > + > > int > > sleep_state(void *v, int sleepmode) > > { > > @@ -180,6 +182,8 @@ fail_suspend: > > clockintr_cpu_init(NULL); > > clockintr_trigger(); > > > > + waketime = gettime(); > > + > > sleep_resume(v); > > resume_randomness(rndbuf, rndbuflen); > > #ifdef MULTIPROCESSOR > > > > A ran into a similar issue while working on non-S3 suspend. I > initially used a timeout, but using a timestamp is simpler. But I > think this should use getuptime() instead of gettime(). I also don't > think we need this for non-ACPI code, so I reworked it a bit to keep > the changes restricted to the ACPI code. > > Does this still work for you? > if this works for OP, it looks fine to me. ok mlarkin if/when you're ready. > > Index: dev/acpi/acpi.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpi.c,v > retrieving revision 1.426 > diff -u -p -r1.426 acpi.c > --- dev/acpi/acpi.c 8 Jan 2024 19:52:29 -0000 1.426 > +++ dev/acpi/acpi.c 9 May 2024 16:21:31 -0000 > @@ -1965,6 +1965,12 @@ acpi_sleep_task(void *arg0, int sleepmod > > #endif /* SMALL_KERNEL */ > > +int > +acpi_resuming(struct acpi_softc *sc) > +{ > + return (getuptime() < sc->sc_resume_time + 10); > +} > + > void > acpi_reset(void) > { > @@ -2030,6 +2036,10 @@ acpi_pbtn_task(void *arg0, int dummy) > acpi_write_pmreg(sc, ACPIREG_PM1_EN, 0, > en | ACPI_PM1_PWRBTN_EN); > splx(s); > + > + /* Ignore button events if we're resuming. */ > + if (acpi_resuming(sc)) > + return; > > switch (pwr_action) { > case 0: > Index: dev/acpi/acpi_x86.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpi_x86.c,v > retrieving revision 1.17 > diff -u -p -r1.17 acpi_x86.c > --- dev/acpi/acpi_x86.c 6 Jul 2023 06:58:07 -0000 1.17 > +++ dev/acpi/acpi_x86.c 9 May 2024 16:21:31 -0000 > @@ -114,6 +114,8 @@ sleep_resume(void *v) > { > struct acpi_softc *sc = v; > > + sc->sc_resume_time = getuptime(); > + > acpibtn_disable_psw(); /* disable _LID for wakeup */ > > /* 3rd resume AML step: _TTS(runstate) */ > Index: dev/acpi/acpibtn.c > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpibtn.c,v > retrieving revision 1.51 > diff -u -p -r1.51 acpibtn.c > --- dev/acpi/acpibtn.c 29 Jun 2023 20:58:08 -0000 1.51 > +++ dev/acpi/acpibtn.c 9 May 2024 16:21:31 -0000 > @@ -229,6 +229,10 @@ acpibtn_notify(struct aml_node *node, in > dnprintf(10, "acpibtn_notify: %.2x %s\n", notify_type, > sc->sc_devnode->name); > > + /* Ignore button events if we're resuming. */ > + if (acpi_resuming(sc->sc_acpi)) > + return (0); > + > switch (sc->sc_btn_type) { > case ACPIBTN_LID: > #ifndef SMALL_KERNEL > Index: dev/acpi/acpivar.h > =================================================================== > RCS file: /cvs/src/sys/dev/acpi/acpivar.h,v > retrieving revision 1.125 > diff -u -p -r1.125 acpivar.h > --- dev/acpi/acpivar.h 29 Nov 2023 03:41:31 -0000 1.125 > +++ dev/acpi/acpivar.h 9 May 2024 16:21:31 -0000 > @@ -269,6 +269,7 @@ struct acpi_softc { > struct aml_node *sc_sst; > struct aml_node *sc_wak; > int sc_state; > + time_t sc_resume_time; > struct acpiec_softc *sc_ec; /* XXX assume single EC */ > > struct acpi_ac_head sc_ac; > @@ -341,6 +342,7 @@ void acpi_sleep_pm(struct acpi_softc *, > void acpi_resume_pm(struct acpi_softc *, int); > void acpi_resume_cpu(struct acpi_softc *, int); > void acpi_sleep_walk(struct acpi_softc *, int); > +int acpi_resuming(struct acpi_softc *); > > #define ACPI_IOREAD 0 > #define ACPI_IOWRITE 1 >