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

[Stefan Seyfried]

On Tue, Oct 31, 2006 at 11:19:26AM -0500, Peter Jones wrote:
> 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 ?

It was not needed IMO, since calling s2ram should always be "better" than
just doing "echo mem > state". I'm not against adding it, i just did not
see the benefit.
 
> > +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?

Yes, s2disk is using /etc/suspend.conf by default. I probably need to
explain my goal a bit more long winded :-)

The /etc/suspend.conf file needs to be edited by the user. Users are
accustomed to use the "resume=" kernel parameter for in-kernel suspend.
They should not learn something new for userspace suspend, in fact they
should not care at all. So right now, in our initrd, i am using the resume=
commandline option to determine the device to resume from with uswsusp (and
if that fails, i retry with in-kernel swsusp).
During suspend, in my s2disk-hook, i generate a /var/lib/s2disk.conf from the
(normally empty, but potentially "enhanced" by the user) /etc/suspend.conf
and from the resume= parameter, and use that in $S2DISK_CONF. This will,
however, probably be a suse-specific patch for some time to come anyway,
(to change /etc/suspend.conf to /var/lib/s2disk.conf or at least to the
default confi) so i can also patch the whole $S2DISK_CONF stuff in when
doing this.

> Also, I'd rather not do S2DISK_BIN, in favor of what I'll put inline in
> a few lines...

S2DISK_BIN is a different matter, since it lets you choose easily between
s2disk and s2both in /etc/pm/config.

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

there is no alternative i know of for s2ram, but there is one for
s2disk, namely s2both.

> Also, S2RAM_OPTS is unset here.

Yes, my bad, should be added to the globals and be configurable via 
/etc/pm/config for machines that are not yet in the whitelist.

> 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

no, it doesn't (i think it wouldn't even build nor make sense there since all
the video workarounds are rather ix86 specific AFAICT).

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

/dev/snapshot is setup by udev. If you don't have /dev/snapshot, your kernel
probably does not support uswsusp. That's why i test if s2disk is executable
and /dev/snapshot is a character device.

> I like the idea here, but the code here seems a little convoluted... why
> not something like:
> 
> ---- start ----

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

Yes, that was very good.
-- 
Stefan Seyfried
QA / R&D Team Mobile Devices        |              "Any ideas, John?"
SUSE LINUX Products GmbH, Nürnberg  | "Well, surrounding them's out."