Klemens Nanni writes:
> On Fri, Mar 26, 2021 at 10:38:30PM +0100, Klemens Nanni wrote:
>> apm(8) never knows gets the result of the requested power action carried
>> out by apmd(8), so platforms without suspend/resume support behave like
>> this:
>>
>> $ zzz; echo $?
>> Suspending system...
>> 0
>> $ apm -z; echo $?
>> System will enter suspend mode momentarily.
>> 0
>>
>> Prior to the latest apmd commit there wasn't even a syslog message
>> reporting any failure whatsoever.
>>
>>
>> This diff adds an extra `int error' field to `struct apm_reply' which
>> such that apmd has means to tell apm if something went in simple
>> errno(2) fashion.
>>
>> To do so I need to hoist the power action inside handle_client(),
>> obviously before the reply it sent back to apm -- currently apmd replies
>> *before* carrying out the requested power actions.
>>
>>
>> On arm64 it looks like this then:
>>
>> $ zzz; echo $?
>> Suspending system...
>> zzz: suspend: Operation not supported
>> 1
>> $ apm -z; echo $?
>> System will enter suspend mode momentarily.
>> apm: suspend: Operation not supported
>> 1
>>
>> amd64 notebooks for example keep suspending without error messages and
>> exit zero as before, i.e. expected behaviour stays unchanged for
>> platforms that already work.
>>
Working on my amd64 system. Sadly I don't have other archs (yet) to
test this on. But I don't see any reason it should impact any other
architectures.
>> Feedback? OK?
OK dv@, I do have some feedback/observations below that aren't critical.
> Correct diff without fat fingered sleep(3) this time.
>
> Index: usr.sbin/apm/apm.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apm/apm.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 apm.c
> --- usr.sbin/apm/apm.c 23 Sep 2020 05:50:26 -0000 1.37
> +++ usr.sbin/apm/apm.c 26 Mar 2021 21:48:36 -0000
> @@ -97,6 +97,7 @@ do_zzz(int fd, enum apm_action action)
> struct apm_command command;
> struct apm_reply reply;
> char *msg;
> + int ret;
>
> switch (action) {
> case NONE:
> @@ -117,7 +118,10 @@ do_zzz(int fd, enum apm_action action)
> }
>
> printf("%s...\n", msg);
> - exit(send_command(fd, &command, &reply));
> + ret = send_command(fd, &command, &reply);
> + if (reply.error)
> + errx(1, "%s: %s", apm_state(reply.newstate),
> strerror(reply.error));
> + exit(ret);
> }
>
> static int
> @@ -418,5 +422,7 @@ balony:
> default:
> break;
> }
> + if (reply.error)
> + errx(1, "%s: %s", apm_state(reply.newstate),
> strerror(reply.error));
> return (0);
> }
> Index: usr.sbin/apmd/apm-proto.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apmd/apm-proto.h,v
> retrieving revision 1.10
> diff -u -p -r1.10 apm-proto.h
> --- usr.sbin/apmd/apm-proto.h 23 Sep 2020 05:50:26 -0000 1.10
> +++ usr.sbin/apmd/apm-proto.h 26 Mar 2021 21:48:36 -0000
> @@ -64,6 +64,7 @@ struct apm_reply {
> enum apm_perfmode perfmode;
> int cpuspeed;
> struct apm_power_info batterystate;
> + int error;
> };
>
> #define APMD_VNO 3
> @@ -71,3 +72,4 @@ struct apm_reply {
> extern const char *battstate(int state);
> extern const char *ac_state(int state);
> extern const char *perf_mode(int mode);
> +extern const char *apm_state(int apm_state);
> Index: usr.sbin/apmd/apmd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> retrieving revision 1.102
> diff -u -p -r1.102 apmd.c
> --- usr.sbin/apmd/apmd.c 25 Mar 2021 20:46:55 -0000 1.102
> +++ usr.sbin/apmd/apmd.c 26 Mar 2021 21:48:37 -0000
> @@ -65,9 +65,9 @@ void usage(void);
> int power_status(int fd, int force, struct apm_power_info *pinfo);
> int bind_socket(const char *sn);
> enum apm_state handle_client(int sock_fd, int ctl_fd);
> -void suspend(int ctl_fd);
> -void stand_by(int ctl_fd);
> -void hibernate(int ctl_fd);
> +int suspend(int ctl_fd);
> +int stand_by(int ctl_fd);
> +int hibernate(int ctl_fd);
> void resumed(int ctl_fd);
> void setperfpolicy(char *policy);
> void sigexit(int signo);
> @@ -266,16 +266,23 @@ handle_client(int sock_fd, int ctl_fd)
> return NORMAL;
> }
>
> + reply.error = 0;
I noticed the apm_reply struct isn't explicitly zeroed. Given it's
passed around in whole or in part (.batterystate), is it worth an
explicit zeroing? (It looks like the client apm.c bzero(3)'s its
instance before it uses an APM_IOC_GETPOWER ioctl.)
> power_status(ctl_fd, 0, &reply.batterystate);
> switch (cmd.action) {
> case SUSPEND:
> reply.newstate = SUSPENDING;
> + if (suspend(ctl_fd) == -1)
> + reply.error = errno;
> break;
> case STANDBY:
> reply.newstate = STANDING_BY;
> + if (stand_by(ctl_fd) == -1)
> + reply.error = errno;
> break;
> case HIBERNATE:
> reply.newstate = HIBERNATING;
> + if (hibernate(ctl_fd) == -1)
> + reply.error = errno;
> break;
> case SETPERF_LOW:
> reply.newstate = NORMAL;
> @@ -321,40 +328,49 @@ handle_client(int sock_fd, int ctl_fd)
> return reply.newstate;
> }
>
> -void
> +int
> suspend(int ctl_fd)
> {
> + int ret;
> +
> logmsg(LOG_NOTICE, "system suspending");
> power_status(ctl_fd, 1, NULL);
> do_etc_file(_PATH_APM_ETC_SUSPEND);
> sync();
> sleep(1);
> - if (ioctl(ctl_fd, APM_IOC_SUSPEND, 0) == -1)
> + if ((ret = ioctl(ctl_fd, APM_IOC_SUSPEND, 0)) == -1)
> logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
I wasn't familiar with the apmd(8) stuff until I looked at your diff,
but if I'm reading correctly (on amd64) the suspend/standby/hibernate
ioctls effectively execute the apm task async via a kernel thread
dealing with an acpi task queue.
It looks like there are possibilities for failures (on amd64) that might
not be caught by the ioctl, but I think that's well beyond a reasonable
scope for this change.
> + return (ret);
> }
>
> -void
> +int
> stand_by(int ctl_fd)
> {
> + int ret;
> +
> logmsg(LOG_NOTICE, "system entering standby");
> power_status(ctl_fd, 1, NULL);
> do_etc_file(_PATH_APM_ETC_STANDBY);
> sync();
> sleep(1);
> - if (ioctl(ctl_fd, APM_IOC_STANDBY, 0) == -1)
> + if ((ret = ioctl(ctl_fd, APM_IOC_STANDBY, 0)) == -1)
> logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
> + return (ret);
> }
>
> -void
> +int
> hibernate(int ctl_fd)
> {
> + int ret;
> +
> logmsg(LOG_NOTICE, "system hibernating");
> power_status(ctl_fd, 1, NULL);
> do_etc_file(_PATH_APM_ETC_HIBERNATE);
> sync();
> sleep(1);
> - if (ioctl(ctl_fd, APM_IOC_HIBERNATE, 0) == -1)
> + if ((ret = ioctl(ctl_fd, APM_IOC_HIBERNATE, 0)) == -1)
> logmsg(LOG_WARNING, "%s: %s", __func__, strerror(errno));
> + return (ret);
> }
>
> void
> @@ -512,20 +528,12 @@ main(int argc, char *argv[])
> break;
>
> if (rv == 1 && ev->ident == sock_fd) {
> - switch (handle_client(sock_fd, ctl_fd)) {
> - case NORMAL:
> - break;
> - case SUSPENDING:
> - suspend(ctl_fd);
> - break;
> - case STANDING_BY:
> - stand_by(ctl_fd);
> - break;
> - case HIBERNATING:
> - hibernate(ctl_fd);
> - break;
> - }
> - continue;
> + int state;
> +
> + if ((state = handle_client(sock_fd, ctl_fd)) == -1)
> + logmsg(LOG_WARNING, "%s: %s", apm_state(state),
> strerror(errno));
> + else
> + continue;
> }
>
> suspends = standbys = hibernates = resumes = 0;
> Index: usr.sbin/apmd/apmsubr.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/apmd/apmsubr.c,v
> retrieving revision 1.9
> diff -u -p -r1.9 apmsubr.c
> --- usr.sbin/apmd/apmsubr.c 23 Sep 2020 05:50:26 -0000 1.9
> +++ usr.sbin/apmd/apmsubr.c 26 Mar 2021 21:48:37 -0000
> @@ -83,3 +83,20 @@ perf_mode(int mode)
> return "invalid";
> }
> }
> +
> +const char *
> +apm_state(int apm_state)
> +{
> + switch (apm_state) {
> + case NORMAL:
> + return "normal";
> + case SUSPENDING:
> + return "suspend";
> + case STANDING_BY:
> + return "standby";
> + case HIBERNATING:
> + return "hibenate";
> + default:
> + return "unknown";
> +}
> +}
