[Pm-utils] Some thoughts about some of the hooks

Fri Oct 6 12:40:38 PDT 2006

On Fri, 6 Oct 2006 11:39:13 +0200
Stefan Seyfried <seife at suse.de> wrote:

> On Fri, Oct 06, 2006 at 10:01:19AM +0200, Tim Dijkstra wrote:
> > On Fri, 6 Oct 2006 07:56:11 +0200
> > Stefan Seyfried <seife at suse.de> wrote:
> > 
> > > Ok. I am going to try to move all our powersaved scripts to pm-utils,
> > > and making it work with uswsusp (which is our default now) would have
> > > been one of the tasks anyway :-)
> > 
> > Very good. Takes something from my todo list;)
> 
> how about this:
> 
> Index: functions
> ===================================================================
> RCS file: /cvs/pm-utils/pm-utils/pm/functions,v
> retrieving revision 1.20
> diff -u -p -r1.20 functions
> --- functions	28 Sep 2006 21:25:36 -0000	1.20
> +++ functions	6 Oct 2006 09:27:49 -0000
> @@ -96,14 +102,32 @@ pm_main()
>  	run_hooks "$1"
>  	sync ; sync ; sync
>  
> +	[ -e /etc/pm/config.d/$1 ] && . /etc/pm/config.d/$1
> +
>  	case "$1" in
>  		suspend)
>  			pm-pmu --suspend || echo -n "mem" > /sys/power/state
>  			run_hooks resume reverse
>  			;;
>  		hibernate)
> -			echo -n "platform" > /sys/power/disk
> -			echo -n "disk" > /sys/power/state
> +			if [ -z "$HIBERNATE" ]; then
> +				if [ -x /usr/sbin/s2disk -a -c /dev/snapshot ]; then
> +					HIBERNATE="userspace"
> +				else
> +					HIBERNATE="kernel"
> +				fi
> +			fi
> +			if checkhibernate; then
> +				case $HIBERNATE in
> +					userspace)
> +						/usr/sbin/s2disk -f /var/lib/s2disk.conf
> +						;;
> +					kernel)

Also the VT switching is only necessary for 'kernel', you should factor that out the locking functions
and move it here, IMO.

> +						echo -n "platform" > /sys/power/disk
> +						echo -n "disk" > /sys/power/state
> +						;;
> +				esac
> +			fi
>  			run_hooks thaw reverse
>  			;;
>  	esac
> @@ -155,3 +213,52 @@ restorestate()
>  {
>  	eval echo \$${1}_STATE
>  }
> +
> +# sanity check the environment if resume will be possible after hibernate
> +checkhibernate()
> +{
> +	if [ "$HIBERNATE" = "kernel" ]; then
> +		read RDEV < /sys/power/resume
> +		if [ "$RDEV" = "0:0" ]; then
> +			# DEBUG "no resume partition set"
> +			# maybe "resume=..." was given, but initrd did not set up
> +			# /sys/power/resume correctly.
> +			return 1
> +		fi
> +		if [ -n "$IMAGE_SIZE" -a -w /sys/power/image_size ]; then
> +			echo "$IMAGE_SIZE" > /sys/power/image_size 2>/dev/null
> +		fi
> +	fi
> +	RDEV=""
> +	read CMDLINE < /proc/cmdline
> +	for CMD in $CMDLINE; do
> +		case $CMD in
> +			resume=*) RDEV=${CMD#*=}
> +			break ;;
> +		esac
> +	done
> +	if [ -z "$RDEV" ]; then
> +		# DEBUG "no resume parameter"
> +		return 1
> +	fi

For uswsusp you don't need a resume  parameter, the resume binary
doesn't do anything with it anyway... 

> +	while read SDEV STYPE DUMMY; do
> +		[ "$STYPE" != "partition" ] && continue
> +		[ "$SDEV" = "$RDEV" ] && break
> +		SDEV=""
> +	done < /proc/swaps
> +	if [ -z "$SDEV" ]; then
> +		# DEBUG "resume partition '$RDEV' not swapon'ed"
> +		return 1
> +	fi

All these checks can go in the if [ kernel ] part IMO, 
the s2disk binary will do all these checks also ...

> +	if [ "$HIBERNATE" = "userspace" ]; then
> +		rm -f /var/lib/s2disk.conf
> +		echo "resume device = $RDEV" >> /var/lib/s2disk.conf

Ah, now I get it. You're creating a config file on the fly... hmm. On
debian I create the /etc/suspend.conf at installation time, asking some
questions (want encryption? want splash?)

> +		if [ -n "$IMAGE_SIZE" ]; then
> +			echo "image size = $IMAGE_SIZE" >> /var/lib/s2disk.conf
> +		# add the parameters from /etc/suspend.conf to /var/lib/s2disk.conf
> +		if [ -e /etc/suspend.conf ]; then
> +			sed '/^[[:space:]]*\(#\|$\)/d;' /etc/suspend.conf >> /var/lib/s2disk.conf
> +		fi
IIRC, the config parser in s2disk will in the case of multiple lines for the same 
parameter give precedence to the latter, so that seems OK

> +	fi
> +	return 0
> +}

> 
> 
> This is basically how i did it until now in powersaved.
> Precondition:
> - your initrd does invoke resume with the device from "resume=". I think it
>   is a good idea to use the well known resume parameter also for userspace
>   suspend. Generally, the user should not need to care about which method he
>   uses.
>   Right now i do not even copy /etc/suspend.conf into the initrd anymore
>   since it is not needed for resume.

Only for splash you do, I think. You could however use a cmdline parameter 
for that too. 

> 
> Then i generate a temporary suspend config file in /var/lib/s2disk.conf from
> the resume= parameter (read from /proc/cmdline) and from IMAGE_SIZE (which
> might be set in /etc/pm/config.d/hibernate or calculated intelligently from
> the free swap space and the total amount of system memory, this is not yet
> in this patch).

I do this in the install script for debian.
IMAGESIZE=`awk '$1 == "MemTotal:" {print int($2*1024*0.46)}' /proc/meminfo `

> I append any options from /etc/suspend.conf to this temporary config file.
> Since s2disk just takes the latest incarnation of an option, the settings
> from /etc/suspend.conf override our automatic determined setting.
> I ship an empty (all options commented out) config file, and it just works
> out of the box, if the resume= parameter is correctly set (done by the
> installer).
>
> What do you think about that?

I think, I should learn to read an e-mail before starting to write the reply:)

What I don't like about the resume= parameter is that it is a bit hard
(not allowed in debian even) to automatically change grub/menu.list or lilo.conf.

So I would want to change the above a bit, such that it will first
check if there's a 'resume device' value in /etc/suspend.conf, if that's
not there than fall back to /proc/cmdline.

Other than that, looks good.

grts Tim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/pm-utils/attachments/20061006/8f51bbab/signature.pgp