On Thu, Feb 13, 2020 at 02:08:32PM +0100, Jeremie Courreges-Anglas wrote:
> On Wed, Feb 12 2020, Scott Cheloha <scottchel...@gmail.com> wrote:
> > On Wed, Feb 12, 2020 at 01:35:22PM +0100, Jeremie Courreges-Anglas wrote:
> >> On Wed, Feb 12 2020, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
> >> > On Sat, Jan 25 2020, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
> >> >> The diff below improves the way apmd -z/-Z may trigger.
> >> >>
> >> >> I think the current behavior is bogus, incrementing and checking
> >> >> apmtimeout like this doesn't make much sense.
> >> >>
> >> >> Here's a proposal:
> >> >> - on APM_POWER_CHANGE events, check the battery level and trigger
> >> >>   autoaction if needed.  This should be enough to make autoaction just
> >> >>   work with drivers like acpibat(4).
> >> >> - on kevent timeout (10mn by default now, maybe too long), also check
> >> >>   the battery level and suspend if needed.  This should be useful only
> >> >>   if your battery driver doesn't send any APM_POWER_CHANGE event.
> >> >>
> >> >> While here I also tweaked the warning.
> >> >
> >> > This has been committed, thanks Ted.
> >> >
> >> >> Some more context:
> >> >> - a subsequent diff would reorder the code to handle similarly the "!rv"
> >> >>   and "ev->ident == ctl_fd" paths
> >> >
> >> > Diff below.
> >> >
> >> >> - I think we want some throttling mechanism, like wait for 1mn after we
> >> >>   resume before autosuspending again.  But I want to fix obvious
> >> >>   problems first.
> >> 
> >> On top of the previous diff, here's a diff to block autoaction for 60
> >> seconds after:
> >> - autoaction triggers; this prevents apmd from sending multiple suspend
> >> requests before the system goes to sleep
> >> - a resume happens; this gives you 60 seconds to fetch and plug your AC
> >> cable if you notice you're low on power
> >> 
> >> A side effect is that apmd now ignores stale acpiac(4) data at resume
> >> time, so it apmd doesn't suspend the system again when you resume with
> >> a low battery and AC plugged.
> >> 
> >> cc'ing Scott since he has a thing for everything time-related. :)
> >
> > For the first case, is there any way you can detect that a suspend is
> > in-progress but not done yet?
> 
> Well, apmd could record that it asked the kernel for a suspend/hibernate
> and skip autoaction as long as it doesn't get a resume event.

Hmmm.  So what happens if the suspend/hibernate fails?  Could apmd(8)
get stuck waiting for a resume that will never happen?

> > I think that'd be cleaner (in some ways) than an autoaction cooldown
> > timer.
> >
> > Whenever I want to add an arbitrary delay that isn't per se required
> > by an interface I wonder whether I'm working around a deficiency in
> > the state machine instead of addressing the root cause.
> >
> > Sometimes it can't be helped, but I have to ask.
> 
> Initially I only cared about the second case, and then noticed that
> APM_POWER_CHANGE events can happen at any time.  Reusing the 60 seconds
> timer looked appealing (cheap) but please see the updated diff below.
> 
> > For the second case, I thought the design of autoaction was to (a)
> > note that the battery was below a particular threshold and (b) take
> > action to avert data loss.  If you resume from suspend with battery
> > below the threshold and no AC I think you would *want* autoaction to
> > trigger.  Like, it sounds like the state machine is working as
> > designed.
> >
> > If the machine is immediately suspending after resume shouldn't you
> > just plug it in before reattempting resume?  Isn't that better than
> > having the battery die on you?
> 
> We can't know when the battery will fail to feed the system.  I suspect
> that the resume sequence itself may drain more power than 60 seconds
> spent idling (wild guess, no power meter at hand).  So I see no
> convincing reason to prevent any use of the system.
> 
> Regarding the user experience, I think the user should be put into
> control.  60 seconds is enough to plug the power cable or take a quick
> look at a document, or even kill apmd if the laptop is really needed
> like, *right now*.  I know I've been in this kind of situation several
> times.

Fair enough.

> So here's an updated diff that:
> - disables autoaction for 60 seconds after resume.  This is still done
>   in a naive way, autoaction won't kick in exactly after 60 seconds
>   after resume.  Good enough for now, I think.

I think this part is fine.

You could use an alarm(3) to pull you out of kevent(2) prematurely.
That could be done in a separate change.

> - prevents autoaction to kick in several times before suspend/hibernate
> - improves naming (suggestions welcome)
> - logs why autoaction doesn't kick in
> - documents the 60 seconds grace period in the manpage
> 
> This still works around the issue with stale acpiac(4) data at resume
> time.
> 
> Thanks for your input so far, I hope I have addressed your concerns.
> 
> Comments / oks?

Just the question above.

> Index: apmd.c
> ===================================================================
> --- apmd.c.orig
> +++ apmd.c
> @@ -368,18 +368,20 @@ resumed(int ctl_fd)
>  }
>  
>  #define TIMO (10*60)                 /* 10 minutes */
> +#define AUTOACTION_GRACE_PERIOD (60) /* 1mn after resume */
>  
>  int
>  main(int argc, char *argv[])
>  {
>       const char *fname = _PATH_APM_CTLDEV;
>       int ctl_fd, sock_fd, ch, suspends, standbys, hibernates, resumes;
> -     int autoaction = 0;
> +     int autoaction = 0, autoaction_inflight = 0;
>       int autolimit = 0;
>       int statonly = 0;
>       int powerstatus = 0, powerbak = 0, powerchange = 0;
>       int noacsleep = 0;
>       struct timespec ts = {TIMO, 0}, sts = {0, 0};
> +     struct timespec last_resume = { 0, 0 };
>       struct apm_power_info pinfo;
>       const char *sockname = _PATH_APM_SOCKET;
>       const char *errstr;
> @@ -566,6 +568,8 @@ main(int argc, char *argv[])
>                               powerstatus = powerbak;
>                               powerchange = 1;
>                       }
> +                     clock_gettime(CLOCK_MONOTONIC, &last_resume);
> +                     autoaction_inflight = 0;
>                       resumes++;
>                       break;
>               case APM_POWER_CHANGE:
> @@ -577,17 +581,30 @@ main(int argc, char *argv[])
>  
>                       if (!powerstatus && autoaction &&
>                           autolimit > (int)pinfo.battery_life) {
> +                             struct timespec graceperiod, now;
> +
> +                             graceperiod = last_resume;
> +                             graceperiod.tv_sec += AUTOACTION_GRACE_PERIOD;
> +                             clock_gettime(CLOCK_MONOTONIC, &now);
> +
>                               logmsg(LOG_NOTICE,
>                                   "estimated battery life %d%%"
> -                                 " below configured limit %d%%",
> -                                 pinfo.battery_life,
> -                                 autolimit
> +                                 " below configured limit %d%%%s%s",
> +                                 pinfo.battery_life, autolimit,
> +                                 !autoaction_inflight ? "" : ", in flight",
> +                                 timespeccmp(&now, &graceperiod, >) ?
> +                                     "" : ", grace period"
>                               );
>  
> -                             if (autoaction == AUTO_SUSPEND)
> -                                     suspends++;
> -                             else
> -                                     hibernates++;
> +                             if (!autoaction_inflight &&
> +                                 timespeccmp(&now, &graceperiod, >)) {
> +                                     if (autoaction == AUTO_SUSPEND)
> +                                             suspends++;
> +                                     else
> +                                             hibernates++;
> +                                     /* Block autoaction until next resume */
> +                                     autoaction_inflight = 1;
> +                             }
>                       }
>                       break;
>               default:
> Index: apmd.8
> ===================================================================
> --- apmd.8.orig
> +++ apmd.8
> @@ -128,6 +128,8 @@ If both
>  and
>  .Fl z
>  are specified, the last one will supersede the other.
> +After a resume, AC state and estimated battery life are ignored for 60
> +seconds.
>  .El
>  .Pp
>  When a client requests a suspend or stand-by state,
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply via email to