Linux suspend

Hi!

> (sent to pavel already, but now i subscribed suspend-devel, i'll
>  resend it here).
> 
> Hi,
> 
> this implements some more fixes / features, against CVS from today.

Thanks, applied. (I deleted the code you #if 0-ed).

> @@ -293,27 +297,88 @@ void s2ram_resume(void)
>  		chvt(active_console);
>  }
>  
> +void usage(void)
> +{
> +	printf("Usage: s2ram [-nh] [-fsra]\n"
> +	       "\n"
> +	       "Options:\n"
> +	       "    -h, --help:       this text.\n"
> +	       "    -n, --test:       test if the machine is in the database.\n"
> +	       "                      returns 0 if known and supported\n"
> +	       "    -f, --force:      force suspending, even on unknown "
> +	       "machines.\n"
> +	       "\n"
> +	       "the following options are only available with --force:\n"
> +	       "    -s, --vbe_save:   save VBE state before suspending and "
> +	       "restore after resume.\n"
> +	       "    -r, --radeontool: turn off the backlight on radeons "
> +	       "before suspending.\n"
> +	       "    -a, --acpi_sleep: set the acpi_sleep parameter before "
> +	       "suspend\n"
> +	       "                      1=s3_bios, 2=s3_mode, 3=both\n"
> +	       "\n");
> +	exit(1);

Seems sane. *Maybe* I'd add --strict-test, that would reject machines
from acpi-support whitelist, because we have not actually tested
those, but perhaps it is better to wait for first problems.

> the radeontool ifdefs are to make it a bit nicer wrt. usage() etc.
> the "if (!active_console)" is needed for the case "vbe_save" && "radeontool",
> otherwise it will not switch back after resume.
> 
> Something that is somewhat important is the "user interface", which means
> the name of the options. I'd like to integrate this into a maintained
> product and one purpose is that i can easily update s2ram to support new
> machines and do not have to touch other packages.
> If the options change often, this will be much harder :-)

Option names seem sane to me. I do not see why we'd want to change them.

> I'll also start working on the whitelist with machine updates and maybe
> even nicer code.

								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

More stuff.

--identify identifies the machine, so it can be easily reported.
(this usually gets only printed on unsupported machines, but not on
supported machines. We might encounter machines that are "supported"
but in fact do not work, so we need to identify them).

What i plan to do next:

refactor the machine identification code. Put the stuff into a header
file, some table or struct and loop over its contents until it matches.
Then return the flags for what workaround is needed on this machine.
To be honest: the code as it is yet is really hurting my eyes :-))
This would also enable a --identify-like option that tells us that
the machine has matched the internal database and what options will
be applied.
An optimal solution would include reading the table from a file, so
you do not need to recompile the binary just to add a single machine,
but this should be easy to implement once i have implemented the other
stuff.

If there are objections, please speak up now ;-)


Patch: add --identify option that does not suspend the machine but print
       the relevant options from the dmi scan.

Index: s2ram.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvsroot/suspend/suspend/s2ram.c,v
retrieving revision 1.13
diff -u -p -r1.13 s2ram.c
--- s2ram.c	24 Feb 2006 11:54:13 -0000	1.13
+++ s2ram.c	24 Feb 2006 15:03:51 -0000
@@ -306,12 +306,13 @@ void s2ram_resume(void)
=20
 void usage(void)
 {
-	printf("Usage: s2ram [-nh] [-fsra]\n"
+	printf("Usage: s2ram [-nhi] [-fsra]\n"
 	       "\n"
 	       "Options:\n"
 	       "    -h, --help:       this text.\n"
 	       "    -n, --test:       test if the machine is in the database.\n=
"
 	       "                      returns 0 if known and supported\n"
+	       "    -i, --identify:   prints a string that identifies the machi=
ne.\n"
 	       "    -f, --force:      force suspending, even on unknown "
 	       "machines.\n"
 	       "\n"
@@ -327,6 +328,16 @@ void usage(void)
 	exit(1);
 }
=20
+void identify_machine(void)
+{
+	dmi_scan();
+	printf("bios_version =3D \"%s\", sys_vendor =3D \"%s\", sys_product =3D=
 \"%s\", sys_version =3D \"%s\"\n",
+	       bios_version, sys_vendor, sys_product, sys_version);
+	printf("See /usr/src/linux/Doc*/power/video.txt for details,\n"
+	       "then reimplement neccessary steps here and mail patch to\n"
+	       "pavel@.... Good luck!\n");
+}
+
 int main(int argc, char *argv[])
 {
 	int i, force =3D 0, vbe_save =3D 0, radeontool =3D 0, acpi_sleep =3D -1=
;
@@ -336,15 +347,19 @@ int main(int argc, char *argv[])
 		{ "force",	no_argument,		NULL, 'f'},
 		{ "vbe_save",	no_argument,		NULL, 's'},
 		{ "radeontool",	no_argument,		NULL, 'r'},
+		{ "identify",	no_argument,		NULL, 'i'},
 		{ "acpi_sleep",	required_argument,	NULL, 'a'},
 		{ NULL,		0,			NULL,  0 }
 	};
=20
-	while ((i =3D getopt_long(argc, argv, "nhfsra:", options, NULL)) !=3D -=
1) {
+	while ((i =3D getopt_long(argc, argv, "nhfsria:", options, NULL)) !=3D =
-1) {
 		switch (i) {
 		case 'h':
 			usage();
 			break;
+		case 'i':
+			identify_machine();
+			exit(0);
 		case 'n':
 			test_mode =3D 1;
 			break;
@@ -360,6 +375,10 @@ int main(int argc, char *argv[])
 		case 'a':
 			acpi_sleep =3D atoi(optarg);
 			break;
+		default:
+			usage();
+			exit(1);
+			break;
 		}
 	}
 	if (((acpi_sleep > -1) || vbe_save || radeontool) && !force) {
--=20
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=FCrnberg \                    -- Leonard Cohe=
n

Hi!


> More stuff.
> 
> --identify identifies the machine, so it can be easily reported.
> (this usually gets only printed on unsupported machines, but not on
> supported machines. We might encounter machines that are "supported"
> but in fact do not work, so we need to identify them).

Commited (I killed some code duplication).

> What i plan to do next:
> 
> refactor the machine identification code. Put the stuff into a header
> file, some table or struct and loop over its contents until it matches.
> Then return the flags for what workaround is needed on this machine.
> To be honest: the code as it is yet is really hurting my eyes :-))
> This would also enable a --identify-like option that tells us that
> the machine has matched the internal database and what options will
> be applied.
> An optimal solution would include reading the table from a file, so
> you do not need to recompile the binary just to add a single machine,
> but this should be easy to implement once i have implemented the other
> stuff.
> 
> If there are objections, please speak up now ;-)

External file is probably okay. But I'd wait a bit. It is possible
that we'll get some weird entries (so far everything is from
acpi_support package), and only with weird entries we'll know what the
requirements for for config file are.

(I also wanted to keep it in C so that new things -- like have to
switch consoles, may not call vbetool_post, ... -- are easy to add).

I understand hurting your eyes. If you want to move it to whitelist.c
or something like that, that would be okay.
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On Fri, Feb 24, 2006 at 05:41:57PM +0100, Pavel Machek wrote:
> > If there are objections, please speak up now ;-)
>=20
> External file is probably okay. But I'd wait a bit. It is possible
> that we'll get some weird entries (so far everything is from
> acpi_support package), and only with weird entries we'll know what the
> requirements for for config file are.

Yes, external file will come once we have nothing else to do ;-)

I am moving the whitelist into a struct in whitelist.h, currently looks l=
ike:

struct machine_entry
{
	char sys_vendor[1024];
	char sys_product[1024];
	char sys_version[1024];
	char bios_version[1024];
	int  vbe_save;
	int  radeon;
	int  acpi_sleep;
	int  halfknown;
};

const struct machine_entry whitelist[] =3D {
	{ "IBM",			"",		"Thinkpad X32",	"", 0, 1, 3, 0 },
	{ "Hewlett Packard",	"",	"HP OmniBook XE3 GF           ","", 1, 0, 0, 0 =
},
	{ "ASUSTEK ",			"L3000D",		"",	"", 1, 0, 0, 0 },
	{ "TOSHIBA",			"Libretto L5/TNK",	"",	"", 0, 0, 0, 0 },
	{ "TOSHIBA",			"Libretto L5/TNKW",	"",	"", 0, 0, 0, 0 },
	{ "ASUSTeK Computer Inc.",	"L7000G series Notebook PC", "","", 1, 0, 0, =
1 },
.....
}

you guess it. Then i'll do a loop to match and take the "flags" to decide=
 on
actions.

> (I also wanted to keep it in C so that new things -- like have to
> switch consoles, may not call vbetool_post, ... -- are easy to add).

Yes, i already guessed that C++ would not be popular here (although it
would make things much more easy... ;-))
=20
> I understand hurting your eyes. If you want to move it to whitelist.c
> or something like that, that would be okay.

I hope the above does not cause pain to anybody else. I don't want to rep=
lace
one ugly solution with another one.

BTW: i know, that this will increase the size of the executable tremendou=
sly,
especially since some entried will be duplicates (from what we know now, =
all
Dells are currently listed twice) but honestly: i don't care. Once we swi=
tch
to a configuration file, everything will be fine again (as long as we don=
't
need some multi-megabyte library to parse it :-)
--=20
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=FCrnberg \                    -- Leonard Cohe=
n

On P=E1 24-02-06 18:12:41, Stefan Seyfried wrote:
> On Fri, Feb 24, 2006 at 05:41:57PM +0100, Pavel Machek wrote:
> > > If there are objections, please speak up now ;-)
> >=20
> > External file is probably okay. But I'd wait a bit. It is possible
> > that we'll get some weird entries (so far everything is from
> > acpi_support package), and only with weird entries we'll know what th=
e
> > requirements for for config file are.
>=20
> Yes, external file will come once we have nothing else to do ;-)
>=20
> I am moving the whitelist into a struct in whitelist.h, currently looks=
 like:
>=20
> struct machine_entry
> {
> 	char sys_vendor[1024];
> 	char sys_product[1024];
> 	char sys_version[1024];
> 	char bios_version[1024];
> 	int  vbe_save;
> 	int  radeon;
> 	int  acpi_sleep;
> 	int  halfknown;
> };
>=20
> const struct machine_entry whitelist[] =3D {
> 	{ "IBM",			"",		"Thinkpad X32",	"", 0, 1, 3, 0 },
> 	{ "Hewlett Packard",	"",	"HP OmniBook XE3 GF           ","", 1, 0, 0, =
0 },
> 	{ "ASUSTEK ",			"L3000D",		"",	"", 1, 0, 0, 0 },
> 	{ "TOSHIBA",			"Libretto L5/TNK",	"",	"", 0, 0, 0, 0 },
> 	{ "TOSHIBA",			"Libretto L5/TNKW",	"",	"", 0, 0, 0, 0 },
> 	{ "ASUSTeK Computer Inc.",	"L7000G series Notebook PC", "","", 1, 0, 0=
, 1 },
> .....
> }
>=20
> you guess it. Then i'll do a loop to match and take the "flags" to deci=
de on
> actions.

Does not look good. If we'll want to add another flag, you'd have to
add ", 0" , everywhere. Also selection of sys_* is rather arbitrary,
and we may want to change it in future.

> I hope the above does not cause pain to anybody else. I don't want to r=
eplace
> one ugly solution with another one.
>=20
> BTW: i know, that this will increase the size of the executable tremend=
ously,
> especially since some entried will be duplicates (from what we know now=
, all
> Dells are currently listed twice) but honestly: i don't care. Once we s=
witch
> to a configuration file, everything will be fine again (as long as we d=
on't
> need some multi-megabyte library to parse it :-)

It also makes it very easy to typo in manufacturer name... I'd say
that current code is not _that_ bad.
								Pavel

--=20
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On Fri, Feb 24, 2006 at 06:28:47PM +0100, Pavel Machek wrote:
=20
> Does not look good. If we'll want to add another flag, you'd have to
> add ", 0" , everywhere. Also selection of sys_* is rather arbitrary,
> and we may want to change it in future.

Could you suggest an alternative, one that you would like?
One solution (one that i do not like) would be to start using a
config file _now_ and make it XML. Bah.

> It also makes it very easy to typo in manufacturer name... I'd say

This is why i'd like the --identify switch to basically put out a line
that can be cut'n'pasted into whitelist.h.

> that current code is not _that_ bad.

have you looked at it? IMO it only gets beaten by radeontool.c ;-))
And with the current code, i cannot imagine how we could kludge a config
file onto it.
--=20
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=FCrnberg \                    -- Leonard Cohe=
n

On P=E1 24-02-06 18:43:37, Stefan Seyfried wrote:
> On Fri, Feb 24, 2006 at 06:28:47PM +0100, Pavel Machek wrote:
> =20
> > Does not look good. If we'll want to add another flag, you'd have to
> > add ", 0" , everywhere. Also selection of sys_* is rather arbitrary,
> > and we may want to change it in future.
>=20
> Could you suggest an alternative, one that you would like?
> One solution (one that i do not like) would be to start using a
> config file _now_ and make it XML. Bah.

No, please don't go XML way.

> > It also makes it very easy to typo in manufacturer name... I'd say
>=20
> This is why i'd like the --identify switch to basically put out a line
> that can be cut'n'pasted into whitelist.h.
>=20
> > that current code is not _that_ bad.
>=20
> have you looked at it? IMO it only gets beaten by radeontool.c ;-))
> And with the current code, i cannot imagine how we could kludge a confi=
g
> file onto it.

We could use something like kernel's DMI code, and c99 initializers to
address worst problems. (Also we should use bitflags, not separate
variables, to help extensibility)

I see that current code is ugly.... Just think it is big and ugly
table written in C. I'd leave it alone for a while; when we know what
features are neccessary. Current code is at least hierarchical, and
has no limits where to extend it.

								Pavel
--=20
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On 2/24/06, Pavel Machek <pavel@...> wrote:
> On P=E1 24-02-06 18:12:41, Stefan Seyfried wrote:
> > BTW: i know, that this will increase the size of the executable tremend=
ously,
> > especially since some entried will be duplicates (from what we know now=
, all
> > Dells are currently listed twice) but honestly: i don't care. Once we s=
witch
> > to a configuration file, everything will be fine again (as long as we d=
on't
> > need some multi-megabyte library to parse it :-)
>
> It also makes it very easy to typo in manufacturer name...

This code is valid C99, and removes the need to duplicate manufacturer name=
:

struct machine {
=09char *sys_product;
=09/* all the other stuff */
};

struct whitelist {
=09char *sys_vendor;
=09struct machine *machines;
};

struct whitelist wlist[] =3D {
=09{
=09=09.sys_vendor =3D "whatever",
=09=09.machines =3D (struct machine[]){
=09=09=09{ "foo" },
=09=09=09{ "bar" },
=09=09}
=09},
};

Luca

On P=E1 24-02-06 19:32:02, Luca Tettamanti wrote:
> On 2/24/06, Pavel Machek <pavel@...> wrote:
> > On P=E1 24-02-06 18:12:41, Stefan Seyfried wrote:
> > > BTW: i know, that this will increase the size of the executable tre=
mendously,
> > > especially since some entried will be duplicates (from what we know=
 now, all
> > > Dells are currently listed twice) but honestly: i don't care. Once =
we switch
> > > to a configuration file, everything will be fine again (as long as =
we don't
> > > need some multi-megabyte library to parse it :-)
> >
> > It also makes it very easy to typo in manufacturer name...
>=20
> This code is valid C99, and removes the need to duplicate manufacturer =
name:
>=20
> struct machine {
> 	char *sys_product;
> 	/* all the other stuff */
> };
>=20
> struct whitelist {
> 	char *sys_vendor;
> 	struct machine *machines;
> };
>=20
> struct whitelist wlist[] =3D {
> 	{
> 		.sys_vendor =3D "whatever",
> 		.machines =3D (struct machine[]){
> 			{ "foo" },
> 			{ "bar" },
> 		}
> 	},
> };

Yep, but perhaps C-code is better than this?

Or maybe some macros can be invented to make list less ugly?
								Pavel
--=20
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On Friday 24 February 2006 19:45, Pavel Machek wrote:
> On P=E1 24-02-06 19:32:02, Luca Tettamanti wrote:
> > On 2/24/06, Pavel Machek <pavel@...> wrote:
> > > On P=E1 24-02-06 18:12:41, Stefan Seyfried wrote:
> > > > BTW: i know, that this will increase the size of the executable tre=
mendously,
> > > > especially since some entried will be duplicates (from what we know=
 now, all
> > > > Dells are currently listed twice) but honestly: i don't care. Once =
we switch
> > > > to a configuration file, everything will be fine again (as long as =
we don't
> > > > need some multi-megabyte library to parse it :-)
> > >
> > > It also makes it very easy to typo in manufacturer name...
> >=20
> > This code is valid C99, and removes the need to duplicate manufacturer =
name:
> >=20
> > struct machine {
> > 	char *sys_product;
> > 	/* all the other stuff */
> > };
> >=20
> > struct whitelist {
> > 	char *sys_vendor;
> > 	struct machine *machines;
> > };
> >=20
> > struct whitelist wlist[] =3D {
> > 	{
> > 		.sys_vendor =3D "whatever",
> > 		.machines =3D (struct machine[]){
> > 			{ "foo" },
> > 			{ "bar" },
> > 		}
> > 	},
> > };
>=20
> Yep, but perhaps C-code is better than this?
>=20
> Or maybe some macros can be invented to make list less ugly?

I think we can create a whitelist file having the

[<vendor_name_1>]
	<machine_name_1> =3D <foo>, <bar> ...
	<machine_name_2> =3D ...
	...
[<vendor_name_2>]
	...

structure and a function to load that into memory.

Greetings,
Rafael

On P=E1 24-02-06 16:14:41, Luca wrote:
> Il Fri, Feb 24, 2006 at 12:47:30AM +0100, Rafael J. Wysocki ha scritto:=
=20
> > Hi,
> >=20
> > On Friday 24 February 2006 00:20, Luca wrote:
> > > Hi,
> > > I'm testing the userspace suspend. The first time I tried I forgot =
to
> > > activate the swap space and of course suspend failed; the problem i=
s
> > > that I didn't see any error message on the console: very confusing.=
..
> > > After looking at the code I think that this is what happened:
> > >=20
> > > ./suspend on tty1
> > > prepare_console -> switch to tty2
> > > set_swap_file fails, prints error message on tty2
> > > restore_console -> switch back to tty1
> > >=20
> > > I think that prepare_console can be delayed until the chroot, in th=
is
> > > way error messages go the same console where suspend is started.
> >=20
> > I think you're right.  I'd be grateful if you could verify whether th=
e appended
> > (untested) patch makes things better.
>=20
> Works as expected, but gotos must be fixed:
>=20
> Index: suspend.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> RCS file: /cvsroot/suspend/suspend/suspend.c,v
> retrieving revision 1.18
> diff -u -r1.18 suspend.c
> --- suspend.c	20 Feb 2006 00:48:09 -0000	1.18
> +++ suspend.c	24 Feb 2006 15:08:11 -0000
> @@ -661,28 +661,21 @@
>  	}
>  	resume_dev =3D stat_buf.st_rdev;
> =20
> -	vt_fd =3D prepare_console(&orig_vc, &suspend_vc);
> -	if (vt_fd < 0) {
> -		fprintf(stderr, "suspend: Could not open a virtual terminal\n");
> -		ret =3D errno;
> -		goto Close_resume_fd;
> -	}
> -
>  	if (stat(snapshot_dev_name, &stat_buf)) {
>  		fprintf(stderr, "suspend: Could not stat the snapshot device file\n"=
);
>  		ret =3D ENODEV;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
>  	if (!S_ISCHR(stat_buf.st_mode)) {
>  		fprintf(stderr, "suspend: Invalid snapshot device\n");
>  		ret =3D EINVAL;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
>  	snapshot_fd =3D open(snapshot_dev_name, O_RDONLY);
>  	if (snapshot_fd < 0) {
>  		fprintf(stderr, "suspend: Could not open the snapshot device\n");
>  		ret =3D errno;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
> =20
>  	if (set_swap_file(snapshot_fd, resume_dev)) {
> @@ -692,11 +685,18 @@
>  		goto Close_snapshot_fd;
>  	}
> =20
> +	vt_fd =3D prepare_console(&orig_vc, &suspend_vc);
> +	if (vt_fd < 0) {
> +		fprintf(stderr, "suspend: Could not open a virtual terminal\n");
> +		ret =3D errno;
> +		goto Close_snapshot_fd;
> +	}
> +
>  	sprintf(chroot_path, "/proc/%d", getpid());
>  	if (chroot(chroot_path)) {
>  		fprintf(stderr, "suspend: Could not chroot to %s\n", chroot_path);
>  		ret =3D errno;
> -		goto Close_snapshot_fd;
> +		goto Restore_console;
>  	}
>  	chdir("/");
> =20
> @@ -710,10 +710,10 @@
>  	if (orig_loglevel >=3D 0)
>  		set_kernel_console_loglevel(orig_loglevel);
> =20
> -Close_snapshot_fd:
> -	close(snapshot_fd);
>  Restore_console:
>  	restore_console(vt_fd, orig_vc);
> +Close_snapshot_fd:
> +	close(snapshot_fd);
>  Close_resume_fd:
>  	close(resume_fd);
> =20
>=20
> This patch improves the situation, but doesn't cover all cases. For
> example if suspend_system fails with ENOSPC the user won't see the
> error message. It may be a good idea to use a buffer to store the
> messages, as Pavel suggested. Should I write some code for that?

I think I'd slightly prefer the "save string" approach. Changing
consoles does open(), and I'd prefer that before freezing system.

								Pavel
--=20
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On 2/24/06, Pavel Machek <pavel@...> wrote:
> On P=E1 24-02-06 16:14:41, Luca wrote:
> > --- suspend.c 20 Feb 2006 00:48:09 -0000      1.18
> > +++ suspend.c 24 Feb 2006 15:08:11 -0000
> > @@ -692,11 +685,18 @@
> >               goto Close_snapshot_fd;
> >       }
> >
> > +     vt_fd =3D prepare_console(&orig_vc, &suspend_vc);
> > +     if (vt_fd < 0) {
> > +             fprintf(stderr, "suspend: Could not open a virtual termin=
al\n");
> > +             ret =3D errno;
> > +             goto Close_snapshot_fd;
> > +     }
> > +
> >       sprintf(chroot_path, "/proc/%d", getpid());
> >       if (chroot(chroot_path)) {
> >               fprintf(stderr, "suspend: Could not chroot to %s\n", chro=
ot_path);
> >               ret =3D errno;
> > -             goto Close_snapshot_fd;
> > +             goto Restore_console;
> >       }
> >       chdir("/");
>
> I think I'd slightly prefer the "save string" approach. Changing
> consoles does open(), and I'd prefer that before freezing system.

Unless I didn't understand the code, freezing is done in
suspend_system; with my patch prepare_console is still called before
suspend_system.

Luca

On P=E1 24-02-06 18:06:43, Luca Tettamanti wrote:
> On 2/24/06, Pavel Machek <pavel@...> wrote:
> > On P=E1 24-02-06 16:14:41, Luca wrote:
> > > --- suspend.c 20 Feb 2006 00:48:09 -0000      1.18
> > > +++ suspend.c 24 Feb 2006 15:08:11 -0000
> > > @@ -692,11 +685,18 @@
> > >               goto Close_snapshot_fd;
> > >       }
> > >
> > > +     vt_fd =3D prepare_console(&orig_vc, &suspend_vc);
> > > +     if (vt_fd < 0) {
> > > +             fprintf(stderr, "suspend: Could not open a virtual te=
rminal\n");
> > > +             ret =3D errno;
> > > +             goto Close_snapshot_fd;
> > > +     }
> > > +
> > >       sprintf(chroot_path, "/proc/%d", getpid());
> > >       if (chroot(chroot_path)) {
> > >               fprintf(stderr, "suspend: Could not chroot to %s\n", =
chroot_path);
> > >               ret =3D errno;
> > > -             goto Close_snapshot_fd;
> > > +             goto Restore_console;
> > >       }
> > >       chdir("/");
> >
> > I think I'd slightly prefer the "save string" approach. Changing
> > consoles does open(), and I'd prefer that before freezing system.
>=20
> Unless I didn't understand the code, freezing is done in
> suspend_system; with my patch prepare_console is still called before
> suspend_system.

Ok, you are right.
								Pavel

--=20
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

Hi,

On Friday 24 February 2006 16:14, Luca wrote:
> Il Fri, Feb 24, 2006 at 12:47:30AM +0100, Rafael J. Wysocki ha scritto: 
> > On Friday 24 February 2006 00:20, Luca wrote:
> > > I'm testing the userspace suspend. The first time I tried I forgot to
> > > activate the swap space and of course suspend failed; the problem is
> > > that I didn't see any error message on the console: very confusing...
> > > After looking at the code I think that this is what happened:
> > > 
> > > ./suspend on tty1
> > > prepare_console -> switch to tty2
> > > set_swap_file fails, prints error message on tty2
> > > restore_console -> switch back to tty1
> > > 
> > > I think that prepare_console can be delayed until the chroot, in this
> > > way error messages go the same console where suspend is started.
> > 
> > I think you're right.  I'd be grateful if you could verify whether the appended
> > (untested) patch makes things better.
> 
> Works as expected, but gotos must be fixed:

Sure, thanks.

> Index: suspend.c
> ===================================================================
> RCS file: /cvsroot/suspend/suspend/suspend.c,v
> retrieving revision 1.18
> diff -u -r1.18 suspend.c
> --- suspend.c	20 Feb 2006 00:48:09 -0000	1.18
> +++ suspend.c	24 Feb 2006 15:08:11 -0000
> @@ -661,28 +661,21 @@
>  	}
>  	resume_dev = stat_buf.st_rdev;
>  
> -	vt_fd = prepare_console(&orig_vc, &suspend_vc);
> -	if (vt_fd < 0) {
> -		fprintf(stderr, "suspend: Could not open a virtual terminal\n");
> -		ret = errno;
> -		goto Close_resume_fd;
> -	}
> -
>  	if (stat(snapshot_dev_name, &stat_buf)) {
>  		fprintf(stderr, "suspend: Could not stat the snapshot device file\n");
>  		ret = ENODEV;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
>  	if (!S_ISCHR(stat_buf.st_mode)) {
>  		fprintf(stderr, "suspend: Invalid snapshot device\n");
>  		ret = EINVAL;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
>  	snapshot_fd = open(snapshot_dev_name, O_RDONLY);
>  	if (snapshot_fd < 0) {
>  		fprintf(stderr, "suspend: Could not open the snapshot device\n");
>  		ret = errno;
> -		goto Restore_console;
> +		goto Close_resume_fd;
>  	}
>  
>  	if (set_swap_file(snapshot_fd, resume_dev)) {
> @@ -692,11 +685,18 @@
>  		goto Close_snapshot_fd;
>  	}
>  
> +	vt_fd = prepare_console(&orig_vc, &suspend_vc);
> +	if (vt_fd < 0) {
> +		fprintf(stderr, "suspend: Could not open a virtual terminal\n");
> +		ret = errno;
> +		goto Close_snapshot_fd;
> +	}
> +
>  	sprintf(chroot_path, "/proc/%d", getpid());
>  	if (chroot(chroot_path)) {
>  		fprintf(stderr, "suspend: Could not chroot to %s\n", chroot_path);
>  		ret = errno;
> -		goto Close_snapshot_fd;
> +		goto Restore_console;
>  	}
>  	chdir("/");
>  
> @@ -710,10 +710,10 @@
>  	if (orig_loglevel >= 0)
>  		set_kernel_console_loglevel(orig_loglevel);
>  
> -Close_snapshot_fd:
> -	close(snapshot_fd);
>  Restore_console:
>  	restore_console(vt_fd, orig_vc);
> +Close_snapshot_fd:
> +	close(snapshot_fd);
>  Close_resume_fd:
>  	close(resume_fd);
>  
> 
> This patch improves the situation, but doesn't cover all cases. For
> example if suspend_system fails with ENOSPC the user won't see the
> error message. It may be a good idea to use a buffer to store the
> messages, as Pavel suggested. Should I write some code for that?

That would have to be a static buffer or it would have to be allocated before
calling suspend_system().

Still I think it would be easier to just examine the value that
suspend_system() returns and print a message according to that after
the console has been restored.

Greetings,
Rafael

On Fri, 24 Feb 2006 12:02:26 +0100, Pavel Machek wrote:
> On ?t 23-02-06 21:27:47, Ben Slusky wrote:
> > Patch attached.
> 
> Applied, thanks.
> 
> > Looking over my script again I noticed two more things I still have to do
> > for a safe and happy suspend-to-ram experience:
> > 
> > * Before suspending, touch a file so that acpid won't power down laptop
> >   as soon as it comes back online (acpid will think I just pressed the
> >   power button).
> 
> Do vanilla versions of acpid understand that? If so we definitely what
> it added to s2ram.c.

No, the action scripts that ship with acpid do not check for that file.
Maybe they should tho'; Google indicates that many users have encountered
this problem and re-invented the wheel in solving it.

> 
> > * After resuming, rerun the laptop_mode script, since the hard disk
> >   standby timeout is reset over suspend.
> 
> This one is harder...

But probably just as widespread.

> > These would seem to be out of scope for s2ram.c, and make me wonder if a
> > C program is really the way to go...
> 
> Well, it has to be in C if we want to do suspend-to-disk&ram... How to
> solve laptopmode... well wrapper script around s2ram is probably not
> *that* bad.

Right, I'd forgotten about suspend-to-both. 

-
-Ben


-- 
Ben Slusky                      | I hope you don't regret today
sluskyb@...          | for the rest of your lives
sluskyb@...              |                       -Oasis
PGP keyID ADA44B3B

Hi!

> > > Patch attached.
> > 
> > Applied, thanks.
> > 
> > > Looking over my script again I noticed two more things I still have to do
> > > for a safe and happy suspend-to-ram experience:
> > > 
> > > * Before suspending, touch a file so that acpid won't power down laptop
> > >   as soon as it comes back online (acpid will think I just pressed the
> > >   power button).
> > 
> > Do vanilla versions of acpid understand that? If so we definitely what
> > it added to s2ram.c.
> 
> No, the action scripts that ship with acpid do not check for that file.
> Maybe they should tho'; Google indicates that many users have encountered
> this problem and re-invented the wheel in solving it.

Yes, it is very widespread. If you convince acpi people to add the
check, I can add the touch. Alternatively, if it helps you, I can add
the touch now, and you may have easier dealing with acpi people.

(Well... It is not completely trivial. We want not to create the file
in case something fails, else power button will ignore one event).

> > > * After resuming, rerun the laptop_mode script, since the hard disk
> > >   standby timeout is reset over suspend.
> > 
> > This one is harder...
> 
> But probably just as widespread.

You could argue that this is kernel problem (should preserve hdd
state), and see what IDE people say...
								Pavel
-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

On Fri, Feb 24, 2006 at 05:50:39PM +0100, Pavel Machek wrote:
> Hi!
> > this problem and re-invented the wheel in solving it.
>=20
> Yes, it is very widespread. If you convince acpi people to add the
> check, I can add the touch. Alternatively, if it helps you, I can add
> the touch now, and you may have easier dealing with acpi people.

Why not just use a wrapper script? The powersave daemon (for example)
correctly handles the problem, that button events have to be ignored
on some machines after resume and does not need this unnecessary file.

Mind you: not all machines throw a button/power event when resuming,
so on those machines that don't, you'll not be able to shut down after
suspend using the power button. (Yes, you can of course but have to
push the button twice, but it is ugly).
=20
> (Well... It is not completely trivial. We want not to create the file
> in case something fails, else power button will ignore one event).
>=20
> > > > * After resuming, rerun the laptop_mode script, since the hard di=
sk
> > > >   standby timeout is reset over suspend.
> > >=20
> > > This one is harder...
> >=20
> > But probably just as widespread.
>=20
> You could argue that this is kernel problem (should preserve hdd
> state), and see what IDE people say...

No it isn't. The standby timeout cannot be queried from the disk, so
the kernel cannot save it before suspend. The kernel could of course
"filter" the ide commands and if he sees a "set spindown timeout" command
remember this for the next suspend/resume cycle. I am pretty sure you
do not want that.

Again, this is something a wrapper script or a daemon should take care of
and it is very easy to do (just use powersaved ;-). I think we should=20
concentrate on the "hard" stuff in s2ram, the stuff that really is not
easy and is needed on all machines. The rest can be easily glued around.

(Man, where have i put that hot-glue gun..... :-)
--=20
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=FCrnberg \                    -- Leonard Cohe=
n

Hi!

> > Hi!
> > > this problem and re-invented the wheel in solving it.
> > 
> > Yes, it is very widespread. If you convince acpi people to add the
> > check, I can add the touch. Alternatively, if it helps you, I can add
> > the touch now, and you may have easier dealing with acpi people.
> 
> Why not just use a wrapper script? The powersave daemon (for example)
> correctly handles the problem, that button events have to be ignored
> on some machines after resume and does not need this unnecessary file.
> 
> Mind you: not all machines throw a button/power event when resuming,
> so on those machines that don't, you'll not be able to shut down after
> suspend using the power button. (Yes, you can of course but have to
> push the button twice, but it is ugly).

Aha, okay, I did not know it only happens on some machines.

> > (Well... It is not completely trivial. We want not to create the file
> > in case something fails, else power button will ignore one event).
> > 
> > > > > * After resuming, rerun the laptop_mode script, since the hard disk
> > > > >   standby timeout is reset over suspend.
> > > > 
> > > > This one is harder...
> > > 
> > > But probably just as widespread.
> > 
> > You could argue that this is kernel problem (should preserve hdd
> > state), and see what IDE people say...
> 
> No it isn't. The standby timeout cannot be queried from the disk, so
> the kernel cannot save it before suspend. The kernel could of course
> "filter" the ide commands and if he sees a "set spindown timeout" command
> remember this for the next suspend/resume cycle. I am pretty sure you
> do not want that.

...or we could control spin up/down in kernel like we do on
scsi... Doing it in wrapper scripts is probably easier for now.

> Again, this is something a wrapper script or a daemon should take care of
> and it is very easy to do (just use powersaved ;-). I think we should 
> concentrate on the "hard" stuff in s2ram, the stuff that really is not
> easy and is needed on all machines. The rest can be easily glued around.
> 
> (Man, where have i put that hot-glue gun..... :-)

Of course, we can do it in wrapper scripts.
								Pavel

-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...

[getting offtopic]

On Fri, Feb 24, 2006 at 06:35:18PM +0100, Pavel Machek wrote:
=20
> ...or we could control spin up/down in kernel like we do on
> scsi... Doing it in wrapper scripts is probably easier for now.

just use the APM mode of the disk, then it takes care by itself. Fixed
values for spindown timeout are about the worst you can do. The disk
knows better. And we should be able to query and save the APM mode before
suspend and restore is after resume, no idea if this is being done.
--=20
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=FCrnberg \                    -- Leonard Cohe=
n