Re: [patch] hibernation: utilize ACPI hardware signature

From: Rafael J. Wysocki <hidden>
Date: 2008-01-04 20:44:19

On Friday, 4 of January 2008, Shaohua Li wrote:

On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote:

quoted

On Thursday, 3 of January 2008, Shaohua Li wrote:

quoted

On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote:

quoted

On Wednesday, 2 of January 2008, Shaohua Li wrote:

quoted

ACPI defines a hardware signature. BIOS calculates the signature
according to hardware configure, if hardware changes, the

signature

quoted

will

quoted

change, in this case, S4 resume should fail.

The idea is fine, but I'd prefer to do that in a more

straightforward

quoted

way.
Namely, we can just:
* write the signature into a variable in, for example,
  acpi_hibernation_prepare() (then, the "old" signature value will

be

quoted

  automatically saved in the image)
* compare it with a the "new" value read from the BIOS in
  acpi_hibernation_leave() and panic if there's a mismatch
* add a configuration option to disable this behavior (just in

case)

quoted

This way we can avoid modifying the entire generic interface to

add

quoted

the feature
specific to ACPI.

it would be better we do the check in boot kernel.

Franky, I think we should also check in the image kernel, in case the
boot
one doesn't support ACPI as I said.

Ok, makes sense. I changed to check the signature in .higberation_leave

Thanks, comments below.

quoted hunk

Signed-off-by: Shaohua Li <redacted>
---
 drivers/acpi/sleep/main.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux/drivers/acpi/sleep/main.c
===================================================================

--- linux.orig/drivers/acpi/sleep/main.c	2008-01-03 13:37:08.000000000 +0800
+++ linux/drivers/acpi/sleep/main.c	2008-01-04 13:36:10.000000000 +0800

@@ -256,6 +256,17 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
+static unsigned long s4_hardware_signature;
+static struct acpi_table_facs *facs;
+static int nosigcheck;

Use bool perhaps?

quoted hunk

+
+static int __init acpi_s4_nosigcheck(char *str)
+{
+	nosigcheck = 1;

And "true" here?

quoted hunk

+	return 1;
+}
+__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck);
+

Please put this function at the end of the file.  Also, I'd call it
"acpi_s4_nosig", but whatever.

quoted hunk

 static void acpi_hibernation_leave(void)
 {
 	/*

@@ -263,6 +274,11 @@ static void acpi_hibernation_leave(void)
 	 * enable it here.
 	 */
 	acpi_enable();
+	if (facs && s4_hardware_signature != facs->hardware_signature) {

I think we don't need to evaluate the condition if nosigcheck is true, but ...

quoted hunk

+		printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't resume\n");
+		if (!nosigcheck)
+			panic("S4 resume error");
+	}
 }
 
 static void acpi_hibernation_finish(void)

@@ -449,6 +465,10 @@ int __init acpi_sleep_init(void)
 		sleep_states[ACPI_STATE_S4] = 1;
 		printk(" S4");
 	}

... I'd put "if (!nosigcheck)" around the following 4 lines and then we
wouldn't need to check nosigcheck in acpi_hibernation_leave(), because facs
would be NULL.

Also, please put that into the block that contains the 'printk(" S4")' above.

quoted hunk

+	acpi_get_table_by_index(ACPI_TABLE_INDEX_FACS,
+		(struct acpi_table_header **)&facs);
+	if (facs)
+		s4_hardware_signature = facs->hardware_signature;
 #endif
 	status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b);
 	if (ACPI_SUCCESS(status)) {

Greetings,
Rafael

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help