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. > 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. 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. - 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? 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