On Monday, 7 of January 2008, Shaohua Li wrote: > > On Sat, 2008-01-05 at 04:44 +0800, Rafael J. Wysocki wrote: > > On Friday, 4 of January 2008, Shaohua Li wrote: > > > > > > On Fri, 2008-01-04 at 01:04 +0800, Rafael J. Wysocki wrote: > > > > On Thursday, 3 of January 2008, Shaohua Li wrote: > > > > > > > > > > On Wed, 2008-01-02 at 22:05 +0800, Rafael J. Wysocki wrote: > > > > > > On Wednesday, 2 of January 2008, Shaohua Li wrote: > > > > > > > ACPI defines a hardware signature. BIOS calculates the > > signature > > > > > > > according to hardware configure, if hardware changes, the > > > > signature > > > > > > will > > > > > > > change, in this case, S4 resume should fail. > > > > > > > > > > > > The idea is fine, but I'd prefer to do that in a more > > > > straightforward > > > > > > way. > > > > > > Namely, we can just: > > > > > > * write the signature into a variable in, for example, > > > > > > acpi_hibernation_prepare() (then, the "old" signature value > > will > > > > be > > > > > > 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) > > > > > > This way we can avoid modifying the entire generic interface > > to > > > > add > > > > > > 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. > > > > > Signed-off-by: Shaohua Li <[EMAIL PROTECTED]> > > > --- > > > 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? > > > > > + > > > +static int __init acpi_s4_nosigcheck(char *str) > > > +{ > > > + nosigcheck = 1; > > > > And "true" here? > > > > > + 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. > Fixed all except this one, the routine is defined with HIBERATION > configed.
I've just realized that we're not doing the right thing here ... > Signed-off-by: Shaohua Li <[EMAIL PROTECTED]> > --- > drivers/acpi/sleep/main.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > Index: linux/drivers/acpi/sleep/main.c > =================================================================== > --- linux.orig/drivers/acpi/sleep/main.c 2008-01-04 13:44:40.000000000 > +0800 > +++ linux/drivers/acpi/sleep/main.c 2008-01-07 09:31:42.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 bool nosigcheck; > + > +static int __init acpi_s4_nosigcheck(char *str) > +{ > + nosigcheck = true; > + return 1; > +} > +__setup("acpi_s4_nosigcheck", acpi_s4_nosigcheck); > + > static void acpi_hibernation_leave(void) > { > /* > @@ -263,6 +274,10 @@ static void acpi_hibernation_leave(void) > * enable it here. > */ > acpi_enable(); > + if (facs && s4_hardware_signature != facs->hardware_signature) { ... because we should read the signature from the hardware here, while we're comparing two values read from memory. They will always be equal. :-) > + printk(KERN_EMERG"PM: Hardware changed in the S4 circle, can't > resume\n"); > + panic("S4 resume error"); > + } > } > > static void acpi_hibernation_finish(void) > @@ -448,6 +463,13 @@ int __init acpi_sleep_init(void) > hibernation_set_ops(&acpi_hibernation_ops); > sleep_states[ACPI_STATE_S4] = 1; > printk(" S4"); > + if (!nosigcheck) { > + 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); > > > - Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html