APMD-List:
Archives

  From: Avery Pennarun <apenwarr@worldvisions.ca>
  To  : <craigm@milkyway.gsfc.nasa.gov>
        < apmd-list@worldvisions.ca>
  Date: Sun, 27 Sep 1998 18:48:37 -0400

Re: apm proposed changes

On Sun, Sep 27, 1998 at 12:45:59PM -0400, Craig Markwardt wrote:

>   * ability for user-level code to *reject* a pending standby or
>     suspend event.  Very handy if something nasty is going on.

Sounds interesting, although I don't know what kind of rules you'd use to
decide this.  As far as I know (and I'll be happy if you correct me) the
only reasons you might want to avoid suspending are that the buffer cache
hasn't been flushed, or you haven't finished cleaning up yet... and
preventing those situations should be a kernel concern.

If we get too sneaky, we end up with laptops that refuse to shut down.  Mine
does sometimes, and it's really annoying :(

> I had been working on my own copy of the user-level APM utilities,
> including both apmd and apm (based on Faith's 2.4 version).  It seems
> we may have diverged here.  Hmmm.

Looks that way.  I took Rik's pre-3.0 version, fixed some bugs, made it work
with hwclock<->clock CMOS clock setting programs (with GMT support), and
added a patch by Pavel Machek to make it work better with IBM Thinkpads.  I
don't understand the issues in his patch, but it didn't hurt on my laptop so
I let it pass.

> I like the approach of being able to call a function when there is a
> pending event, and I see you have enabled an external callout
> mechanism for some APM events.  I attempted to make a more general
> approach, using a single "proxy" command which acts as a dispatch
> point for *all* apm events (well, the events are maskable).

I don't clearly understand how your method works (disclaimer: I also haven't
downloaded your snapshot).  Currently, in my version, we use the -s and -r
options to specify arbitrary commands to run on suspend and resume.  This
could be something like:

    apmd -s "cardctl suspend" -r "cardctl resume"
		
or it could be what I do in the Debian package, which is:

    apmd -s "run-parts /etc/apm/suspend.d" -r "run-parts /etc/apm/resume.d"

>From your description, it seems your version has a single dispatch command,
which (I guess) is called with some parameters specifying what just
happened.  It seems to me that you can duplicate this behaviour using the
implementation in my version:

    apmd -s "/sbin/apm-event suspend" -r "/sbin/apm-event resume"
    
Of course, it would be nice to have events for batteries, standby, and so
on, but that's pretty easy to add.  The advantage is that commands like
the first one above will work:  you don't _need_ a dedicated dispatch
command.  Further, the "event masking" is done simply by either supplying or
not supplying command-line options.

And actually, I don't think it's a good idea to run commands when entering
or leaving standby (as opposed to suspend) modes.  If you run a program, the
drive will probably power up and the BIOS/kernel might leave standby mode
because there's system activity!  (This happens regularly on my laptop...)

Am I missing your point?

> Unfortunately, in my hacking on apmd, I have made other serious
> changes.  Mostly, this was in clarifying the logic, but I fear that
> the divergence with your code is severe.  On the plus side, I have
> been running my code stably for several months :-)

It sounds like we've deviated a lot... and I've been running my code stably
for a while too :)

I knew I was in for trouble when I took over maintenance of apmd.  I don't
fully understand all the code -- probably needs some of your logic
clarification.  I'm mainly here to provide web and mailing list resources,
and to collect patches.  But I'm hesitant to start over with a new version
from you, and try to reintegrate all the other changes that have been made.

> apmd-cm.tar.gz    - snapshot of my APMD development track (look for "proxy")
> kernel-apm.tar.gz - what I submitted to Stephen.

I'll look when I have time...

> More work needs to be done to make the user-level routines compatible
> with older kernels.

This worries me too.

>   a user types "apm --standby"
> 
>   APM: calls sleep()
>   APM: calls sync()
>   APM: invokes standby IOCTL
>   APM_BIOS: prepares to enter standby, sends standby event to APMD
>   APMD: calls sleep()
>   APMD: calls sync()
>   APMD: invokes standby IOCTL
>   APM BIOS: enters standby mode.

Yup.  The problem we have to deal with is that kernel APM support can be
present even if the apmd daemon isn't running.  Worse, the apm command might
be executed without apmd being present.

I don't know a really clean way to get around this.  Really, though, it
might be better to have the kernel do this sort of thing:  sync, suspend its
tasks (or whatever) and then suspend.  apmd could be present to log status
messages and run scripts, but flushing disk buffers and suspending activity
sounds like kernel activity to me.

At least then, there will only be one sleep() executed regardless of whether
apm, apmd, or the user tries to suspend the system.

> * The sequence in apm to open the apm device seems a bit wasteful.
>   The sequence is: apm_exists -> apm_open -> apm_read, and each one
>   (I think) opens /proc/apm
> 
>   It has been my experience that APM bios calls (implicitly made when
>   opening /proc/apm) can eat a fair amount of CPU with interrupts off.
>   Would be nice to open /proc/apm only once and cache the results.

True.  Hmm, I think I just found a security hole in apmd, if it really is
creating those devices in /tmp.

Anyway, it looks like apm_open() seems to use apm_read simply to find out
the driver version:  I admit that's pretty wasteful.  apm_exists() seems
just as bad.  I can't see a problem with caching the version number like
that.

On the other hand, if all you want to do is check APM status, there seems to
be no need to call apm_open() or apm_exists() anyway.  apm_read will return
the same value as apm_exists().  Perhaps the easiest change would be to not
call the library when it's not necessary.

But if the kernel calls the APM bios too much, then I can imagine a
denial-of-service (or disruption-of-service) attack like this:

	while :; do cat /proc/apm >/dev/null; done

If that causes problems when run as a user, the kernel needs to be fixed.
(eg. call the APM bios a maximum of once per second or something, caching
results in the meantime)

> * The apm_read functionality could be more flexible.  Currently only
>   the APM bios device can be select()'ed.  It would be nice to be able
>   to separate the select and the read, so that a program could keep
>   track of other file descriptors.  For example, a more intelligent
>   power manager daemon might need this.

I don't see a problem.  Just manually select() on the fd returned from
apm_open (and any others you might want), then call apm_get_events if your
APM fd is ready.  There's nothing wrong with having an extra select() in
apm_get_events(), since the timeout can be zero.

Or have I missed your point again?

Have fun,

Avery