Re: Allow resuming with closed lid
> What Ted says makes sense. Unfortunately, it's a bit more difficult than > it seems to obtain the wake source. > > 1. Not all machines (very few in fact) implement _SWS > > 2. Instrumenting a few machines to check GPEs on wake, I found that none > of them were asserted in the lid-open case, the power-button case, and a > few others as well. This likely means we are wiping out the status before > we check it. When I checked the status during early init instead, the > GPE status bits were either all on, or all off, or filled with random > data. > > In a nutshell, I haven't yet found a good way to determine the wake > source that works on all machines. A diff that properly does this would > certainly be welcome and go a long way toward fixing a lot of this > nonsense. Yes, that is essentially the problem. We have some on/off events that sometimes work, and we have some true/false state we can probe ... sometimes. The frameworks underneath seem unreliable. It might be bugs in our acpi stack, but quite likely there are also variations (bugs) in the machines themselves... (If these issues are not identified and improved, we could hackishly move the decision making to a timeout-based codepath which will have more information..)
Re: Allow resuming with closed lid
On Fri, Jan 30, 2015 at 05:44:25PM -0500, Ted Unangst wrote: > On Sat, Jan 31, 2015 at 00:24, Ville Valkonen wrote: > > Hello Mike and Max, > > > > my work laptop is running Windows and on there one must press power button > > to wake up the machine. If I connect the dots right, current behaviour was > > implemented to prevent a "hot bag" problem. Mimicking the Windows behaviour > > would also prevent laptop wake ups on a bumpy road. > > > > Would it make any sense to mimic this behaviour in obsd? > > This obviously depends on your windows config. My windows laptop > resumes whenver I open the lid. > > Now what can maybe be improved would be to add a check for which wakeup > event we received. If somebody pushed the power button, the lid > closed check should probably be skipped. If the wakeup event was lid > opening, then making sure the lid really is open makes sense. What Ted says makes sense. Unfortunately, it's a bit more difficult than it seems to obtain the wake source. 1. Not all machines (very few in fact) implement _SWS 2. Instrumenting a few machines to check GPEs on wake, I found that none of them were asserted in the lid-open case, the power-button case, and a few others as well. This likely means we are wiping out the status before we check it. When I checked the status during early init instead, the GPE status bits were either all on, or all off, or filled with random data. In a nutshell, I haven't yet found a good way to determine the wake source that works on all machines. A diff that properly does this would certainly be welcome and go a long way toward fixing a lot of this nonsense. -ml
Re: Allow resuming with closed lid
On Sat, Jan 31, 2015 at 00:24, Ville Valkonen wrote: > Hello Mike and Max, > > my work laptop is running Windows and on there one must press power button > to wake up the machine. If I connect the dots right, current behaviour was > implemented to prevent a "hot bag" problem. Mimicking the Windows behaviour > would also prevent laptop wake ups on a bumpy road. > > Would it make any sense to mimic this behaviour in obsd? This obviously depends on your windows config. My windows laptop resumes whenver I open the lid. Now what can maybe be improved would be to add a check for which wakeup event we received. If somebody pushed the power button, the lid closed check should probably be skipped. If the wakeup event was lid opening, then making sure the lid really is open makes sense.
Re: Allow resuming with closed lid
Hello Mike and Max,
my work laptop is running Windows and on there one must press power button
to wake up the machine. If I connect the dots right, current behaviour was
implemented to prevent a "hot bag" problem. Mimicking the Windows behaviour
would also prevent laptop wake ups on a bumpy road.
Would it make any sense to mimic this behaviour in obsd?
--
Kind regards,
Ville
On 30 January 2015 at 05:27, Mike Larkin wrote:
> On Fri, Jan 30, 2015 at 12:42:04AM +0100, Max Fillinger wrote:
> > Currently, there's code in acpi.c that sends the system back to sleep
> > when resuming with closed lid and machdep.lidsuspend=1. I often use my
> > laptop in a docking station with an external monitor and keep the lid
> > closed, and I'd like to be able to resume just by pushing the power
> > button on the docking station. (Also, I first thought something was
> > broken when pushing the button only made the suspend-indicator light
> > blink for a moment.)
> >
> > If checking for open lids is necessary in some cases, then I can
> > certainly live with lidsuspend=0, but otherwise I'd prefer if it was
> > possible to resume with a closed lid. I removed the check and didn't
> > notice any problems. The diff below removes the check and also the
> > function acpibtn_numopenlids which is not used anywhere else.
> >
>
> This was put in for a reason. I would suggest you go read the commit
> logs and understand why, before proposing reverting functionality
> you obviously have not researched.
>
> -ml
>
> >
> >
> > Index: sys/dev/acpi/acpi.c
> > ===
> > RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> > retrieving revision 1.281
> > diff -u -p -r1.281 acpi.c
> > --- sys/dev/acpi/acpi.c 17 Jan 2015 04:18:49 - 1.281
> > +++ sys/dev/acpi/acpi.c 29 Jan 2015 22:53:42 -
> > @@ -2161,7 +2161,6 @@ int
> > acpi_sleep_state(struct acpi_softc *sc, int state)
> > {
> > extern int perflevel;
> > - extern int lid_suspend;
> > int error = ENXIO;
> > int s;
> >
> > @@ -2305,10 +2304,6 @@ fail_alloc:
> >
> > acpi_record_event(sc, APM_NORMAL_RESUME);
> > acpi_indicator(sc, ACPI_SST_WORKING);
> > -
> > - /* If we woke up but all the lids are closed, go back to sleep */
> > - if (acpibtn_numopenlids() == 0 && lid_suspend != 0)
> > - acpi_addtask(sc, acpi_sleep_task, sc, state);
> >
> > fail_tts:
> > return (error);
> > Index: sys/dev/acpi/acpibtn.c
> > ===
> > RCS file: /cvs/src/sys/dev/acpi/acpibtn.c,v
> > retrieving revision 1.41
> > diff -u -p -r1.41 acpibtn.c
> > --- sys/dev/acpi/acpibtn.c27 Jan 2015 19:40:14 - 1.41
> > +++ sys/dev/acpi/acpibtn.c29 Jan 2015 22:53:42 -
> > @@ -74,37 +74,6 @@ struct cfdriver acpibtn_cd = {
> >
> > const char *acpibtn_hids[] = { ACPI_DEV_LD, ACPI_DEV_PBD, ACPI_DEV_SBD,
> 0 };
> >
> > -/*
> > - * acpibtn_numopenlids
> > - *
> > - * Return the number of _LID devices that are in the "open" state.
> > - * Used to determine if we should go back to sleep/hibernate if we
> > - * woke up with the all the lids still closed for some reason. If
> > - * the machine has no lids, returns -1.
> > - */
> > -int
> > -acpibtn_numopenlids(void)
> > -{
> > - struct acpi_lid *lid;
> > - int64_t val;
> > - int ct = 0;
> > -
> > - /* If we have no lids ... */
> > - if (SLIST_EMPTY(&acpibtn_lids))
> > - return (-1);
> > -
> > - /*
> > - * Determine how many lids are open. Assumes _LID evals to
> > - * non-0 or 0, for on / off (which is what the spec says).
> > - */
> > - SLIST_FOREACH(lid, &acpibtn_lids, abl_link)
> > - if (!aml_evalinteger(lid->abl_softc->sc_acpi,
> > - lid->abl_softc->sc_devnode, "_LID", 0, NULL, &val) &&
> > - val != 0)
> > - ct++;
> > - return (ct);
> > -}
> > -
> > int
> > acpibtn_setpsw(struct acpibtn_softc *sc, int psw)
> > {
> > Index: sys/dev/acpi/acpidev.h
> > ===
> > RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> > retrieving revision 1.36
> > diff -u -p -r1.36 acpidev.h
> > --- sys/dev/acpi/acpidev.h23 Nov 2014 20:33:47 - 1.36
> > +++ sys/dev/acpi/acpidev.h29 Jan 2015 22:53:42 -
> > @@ -337,5 +337,4 @@ struct acpiec_softc {
> >
> > void acpibtn_disable_psw(void);
> > void acpibtn_enable_psw(void);
> > -int acpibtn_numopenlids(void);
> > #endif /* __DEV_ACPI_ACPIDEV_H__ */
> >
>
>
Re: Allow resuming with closed lid
On Fri, Jan 30, 2015 at 12:42:04AM +0100, Max Fillinger wrote:
> Currently, there's code in acpi.c that sends the system back to sleep
> when resuming with closed lid and machdep.lidsuspend=1. I often use my
> laptop in a docking station with an external monitor and keep the lid
> closed, and I'd like to be able to resume just by pushing the power
> button on the docking station. (Also, I first thought something was
> broken when pushing the button only made the suspend-indicator light
> blink for a moment.)
>
> If checking for open lids is necessary in some cases, then I can
> certainly live with lidsuspend=0, but otherwise I'd prefer if it was
> possible to resume with a closed lid. I removed the check and didn't
> notice any problems. The diff below removes the check and also the
> function acpibtn_numopenlids which is not used anywhere else.
>
This was put in for a reason. I would suggest you go read the commit
logs and understand why, before proposing reverting functionality
you obviously have not researched.
-ml
>
>
> Index: sys/dev/acpi/acpi.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.281
> diff -u -p -r1.281 acpi.c
> --- sys/dev/acpi/acpi.c 17 Jan 2015 04:18:49 - 1.281
> +++ sys/dev/acpi/acpi.c 29 Jan 2015 22:53:42 -
> @@ -2161,7 +2161,6 @@ int
> acpi_sleep_state(struct acpi_softc *sc, int state)
> {
> extern int perflevel;
> - extern int lid_suspend;
> int error = ENXIO;
> int s;
>
> @@ -2305,10 +2304,6 @@ fail_alloc:
>
> acpi_record_event(sc, APM_NORMAL_RESUME);
> acpi_indicator(sc, ACPI_SST_WORKING);
> -
> - /* If we woke up but all the lids are closed, go back to sleep */
> - if (acpibtn_numopenlids() == 0 && lid_suspend != 0)
> - acpi_addtask(sc, acpi_sleep_task, sc, state);
>
> fail_tts:
> return (error);
> Index: sys/dev/acpi/acpibtn.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpibtn.c,v
> retrieving revision 1.41
> diff -u -p -r1.41 acpibtn.c
> --- sys/dev/acpi/acpibtn.c27 Jan 2015 19:40:14 - 1.41
> +++ sys/dev/acpi/acpibtn.c29 Jan 2015 22:53:42 -
> @@ -74,37 +74,6 @@ struct cfdriver acpibtn_cd = {
>
> const char *acpibtn_hids[] = { ACPI_DEV_LD, ACPI_DEV_PBD, ACPI_DEV_SBD, 0 };
>
> -/*
> - * acpibtn_numopenlids
> - *
> - * Return the number of _LID devices that are in the "open" state.
> - * Used to determine if we should go back to sleep/hibernate if we
> - * woke up with the all the lids still closed for some reason. If
> - * the machine has no lids, returns -1.
> - */
> -int
> -acpibtn_numopenlids(void)
> -{
> - struct acpi_lid *lid;
> - int64_t val;
> - int ct = 0;
> -
> - /* If we have no lids ... */
> - if (SLIST_EMPTY(&acpibtn_lids))
> - return (-1);
> -
> - /*
> - * Determine how many lids are open. Assumes _LID evals to
> - * non-0 or 0, for on / off (which is what the spec says).
> - */
> - SLIST_FOREACH(lid, &acpibtn_lids, abl_link)
> - if (!aml_evalinteger(lid->abl_softc->sc_acpi,
> - lid->abl_softc->sc_devnode, "_LID", 0, NULL, &val) &&
> - val != 0)
> - ct++;
> - return (ct);
> -}
> -
> int
> acpibtn_setpsw(struct acpibtn_softc *sc, int psw)
> {
> Index: sys/dev/acpi/acpidev.h
> ===
> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 acpidev.h
> --- sys/dev/acpi/acpidev.h23 Nov 2014 20:33:47 - 1.36
> +++ sys/dev/acpi/acpidev.h29 Jan 2015 22:53:42 -
> @@ -337,5 +337,4 @@ struct acpiec_softc {
>
> void acpibtn_disable_psw(void);
> void acpibtn_enable_psw(void);
> -int acpibtn_numopenlids(void);
> #endif /* __DEV_ACPI_ACPIDEV_H__ */
>
Allow resuming with closed lid
Currently, there's code in acpi.c that sends the system back to sleep
when resuming with closed lid and machdep.lidsuspend=1. I often use my
laptop in a docking station with an external monitor and keep the lid
closed, and I'd like to be able to resume just by pushing the power
button on the docking station. (Also, I first thought something was
broken when pushing the button only made the suspend-indicator light
blink for a moment.)
If checking for open lids is necessary in some cases, then I can
certainly live with lidsuspend=0, but otherwise I'd prefer if it was
possible to resume with a closed lid. I removed the check and didn't
notice any problems. The diff below removes the check and also the
function acpibtn_numopenlids which is not used anywhere else.
Index: sys/dev/acpi/acpi.c
===
RCS file: /cvs/src/sys/dev/acpi/acpi.c,v
retrieving revision 1.281
diff -u -p -r1.281 acpi.c
--- sys/dev/acpi/acpi.c 17 Jan 2015 04:18:49 - 1.281
+++ sys/dev/acpi/acpi.c 29 Jan 2015 22:53:42 -
@@ -2161,7 +2161,6 @@ int
acpi_sleep_state(struct acpi_softc *sc, int state)
{
extern int perflevel;
- extern int lid_suspend;
int error = ENXIO;
int s;
@@ -2305,10 +2304,6 @@ fail_alloc:
acpi_record_event(sc, APM_NORMAL_RESUME);
acpi_indicator(sc, ACPI_SST_WORKING);
-
- /* If we woke up but all the lids are closed, go back to sleep */
- if (acpibtn_numopenlids() == 0 && lid_suspend != 0)
- acpi_addtask(sc, acpi_sleep_task, sc, state);
fail_tts:
return (error);
Index: sys/dev/acpi/acpibtn.c
===
RCS file: /cvs/src/sys/dev/acpi/acpibtn.c,v
retrieving revision 1.41
diff -u -p -r1.41 acpibtn.c
--- sys/dev/acpi/acpibtn.c 27 Jan 2015 19:40:14 - 1.41
+++ sys/dev/acpi/acpibtn.c 29 Jan 2015 22:53:42 -
@@ -74,37 +74,6 @@ struct cfdriver acpibtn_cd = {
const char *acpibtn_hids[] = { ACPI_DEV_LD, ACPI_DEV_PBD, ACPI_DEV_SBD, 0 };
-/*
- * acpibtn_numopenlids
- *
- * Return the number of _LID devices that are in the "open" state.
- * Used to determine if we should go back to sleep/hibernate if we
- * woke up with the all the lids still closed for some reason. If
- * the machine has no lids, returns -1.
- */
-int
-acpibtn_numopenlids(void)
-{
- struct acpi_lid *lid;
- int64_t val;
- int ct = 0;
-
- /* If we have no lids ... */
- if (SLIST_EMPTY(&acpibtn_lids))
- return (-1);
-
- /*
-* Determine how many lids are open. Assumes _LID evals to
-* non-0 or 0, for on / off (which is what the spec says).
-*/
- SLIST_FOREACH(lid, &acpibtn_lids, abl_link)
- if (!aml_evalinteger(lid->abl_softc->sc_acpi,
- lid->abl_softc->sc_devnode, "_LID", 0, NULL, &val) &&
- val != 0)
- ct++;
- return (ct);
-}
-
int
acpibtn_setpsw(struct acpibtn_softc *sc, int psw)
{
Index: sys/dev/acpi/acpidev.h
===
RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
retrieving revision 1.36
diff -u -p -r1.36 acpidev.h
--- sys/dev/acpi/acpidev.h 23 Nov 2014 20:33:47 - 1.36
+++ sys/dev/acpi/acpidev.h 29 Jan 2015 22:53:42 -
@@ -337,5 +337,4 @@ struct acpiec_softc {
void acpibtn_disable_psw(void);
void acpibtn_enable_psw(void);
-intacpibtn_numopenlids(void);
#endif /* __DEV_ACPI_ACPIDEV_H__ */
