[Pm-utils] [PATCH] userspace suspend (s2disk/s2ram) support

[Peter Jones]

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