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

[Stefan Seyfried]

On Fri, Oct 06, 2006 at 09:40:38PM +0200, Tim Dijkstra wrote:
> 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 ...

That's true. In powersaved, we did all this sanity-checks first (is the
kernel still there?, think kernel update, are all other preconditions met?)
and only if everything looked ok, we started the suspend process (unloading
modules and stuff.

However, since such fatal conditions should be rather uncommon, we can really
do it in the last second also, and just reload the modules (however, if the
kernel package has been replaced, you probably cannot reload the modules
anymore, but then it is a good time to reboot the machine anyway ;-)

> > +	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?)

Yes, i went for the simple "be compatible with the old in-kernel" variant
and just use the same option for both.
However, i can do this (create s2disk.con) also in a hook that i put into
my suspend package.

> > +		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

Yes, that was my idea with that: if the power user does configure something
in /etc/suspend.conf, then this has higher priority. The autogenerated config
file does work out of the box, but without bells and whistles. It is 
equivalent to the in-kernel suspend.

> > 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. 

Yes. To be honest, i have not looked into splash support yet, since i am
waiting on the management to decide and provide a splash theme with a progress
bar :-)

> > 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 did basically the same in powersave, but with an additional 
"if free_swap < IMAGESIZE then IMAGESIZE=free_swap*0.9"
or something like that.

> > 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.

Well, we can leave that part out totally. I'll just do a hook that generates
the suspend.conf. Since that hook runs before the check this will work just
fine.
 
> Other than that, looks good.

I'll refine it a bit and then we'll see what the Masters Of CVS have to say
about it :-)
-- 
Stefan Seyfried                  \ "I didn't want to write for pay. I
QA / R&D Team Mobile Devices      \ wanted to be paid for what I write."
SUSE LINUX Products GmbH, Nürnberg \                    -- Leonard Cohen