On Wed, Feb 12 2020, Scott Cheloha <[email protected]> wrote:
> On Wed, Feb 12, 2020 at 01:35:22PM +0100, Jeremie Courreges-Anglas wrote:
>> On Wed, Feb 12 2020, Jeremie Courreges-Anglas <[email protected]> wrote:
>> > On Sat, Jan 25 2020, Jeremie Courreges-Anglas <[email protected]> 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
