On Mon, 2006-10-30 at 22:23 +0100, Stefan Seyfried wrote:
[comments inline]
> # default values go here
> HIBERNATE_RESUME_POST_VIDEO=no
> +HIBERNATE_METHOD=""
> SUSPEND_MODULES=""
> RESUME_MODULES=""
Why no SUSPEND_METHOD ?
> +S2DISK_BIN=/usr/sbin/s2disk
> +S2DISK_CONF=/etc/suspend.conf
I don't like this much :/ Can you not configure s2ram with a default
config file at compile time?
Also, I'd rather not do S2DISK_BIN, in favor of what I'll put inline in
a few lines...
> +do_suspend()
> +{
> + if [ -x /usr/sbin/s2ram ]; then
> + /usr/sbin/s2ram $S2RAM_OPTS
> + else
> + echo -n "mem" > /sys/power/state
> + fi
> +}
Why use a hardcoded value here, but not for s2disk? Also, S2RAM_OPTS is
unset here. In general, I'd rather something like:
# these can both be changed by the distro in the default config file
HIBERNATE_METHOD=kernel
SUSPEND_METHOD=kernel
...
add_global HIBERNATE_METHOD
add_global SUSPEND_METHOD
...
do_suspend()
{
S2RAM=$(type -P s2ram)
[ -z "$S2RAM" -o ! -x "$S2RAM" ] && SUSPEND_METHOD=kernel
case "$SUSPEND_METHOD" in
# I'm assuming s2ram knows how to do the pmu
# stuff for the mac
userspace) s2ram ;;
*) pm-pmu --suspend || echo -n "mem" > /sys/power/state ;;
esac
}
And do something vaguely similar for do_hibernate
> +do_hibernate()
> +{
> + if [ -z "$HIBERNATE_METHOD" ]; then
> + if [ -x $S2DISK_BIN -a -c /dev/snapshot ]; then
What sets up /dev/snapshot? Or is that what this call itself does?
> + HIBERNATE_METHOD="userspace"
> + else
> + HIBERNATE_METHOD="kernel"
> + fi
Same comment about _METHOD here as above
> + fi
> + case $HIBERNATE_METHOD in
> + userspace)
> + $S2DISK_BIN -f $S2DISK_CONF
> + ;;
> + kernel)
> + echo -n "platform" > /sys/power/disk
> + echo -n "disk" > /sys/power/state
> + ;;
> + esac
> +}
> +
> pm_main()
> {
> if [ -n "$PM_LOGFILE" ]; then
> @@ -120,17 +157,23 @@ pm_main()
> exec > "$PM_LOGFILE" 2>&1
> fi
> take_suspend_lock || exit 1
> +
> + rm -f $INHIBIT
> +
> run_hooks "$1"
> sync ; sync ; sync
>
> case "$1" in
> suspend)
> - pm-pmu --suspend || echo -n "mem" > /sys/power/state
> + if [ ! -e $INHIBIT ]; then
> + pm-pmu --suspend || do_suspend
> + fi
> run_hooks resume reverse
> ;;
> hibernate)
> - echo -n "platform" > /sys/power/disk
> - echo -n "disk" > /sys/power/state
> + if [ ! -e $INHIBIT ]; then
> + do_hibernate
> + fi
> run_hooks thaw reverse
> ;;
> esac
I like the idea here, but the code here seems a little convoluted... why
not something like:
---- start ----
rm -f "$INHIBIT"
run_hooks "$1"
if [ ! -e "$INHIBIT" ]; then
sync ; sync ; sync
REVERSE_ACTION=""
case "$1" in
suspend)
do_suspend
REVERSE_ACTION=resume
;;
resume)
do_hibernate
REVERSE_ACTION=thaw
;;
esac
fi
[ -n "$REVERSE_ACTION" ] && run_hooks "$REVERSE_ACTION" reverse
---- end ----
Actually, gimme a few minutes to make a different patch for the INHIBIT
part (which we can apply independently of the s2ram bits); there's a
cleaner way than this.
--
Peter
_______________________________________________
Pm-utils mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pm-utils
