diff mbox

ACPI:remove panic in case hardware has changed after S4

Message ID 1373544870-15135-1-git-send-email-oliver@neukum.org (mailing list archive)
State Deferred, archived
Headers show

Commit Message

Oliver Neukum July 11, 2013, 12:14 p.m. UTC 
From: Oliver Neukum <oneukum@suse.de>

Some BIOSes change hardware based on the state of
a laptop's lid. If the lid is closed, the touchpad is
disabled and the checksum changes.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/acpi/sleep.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

joeyli July 11, 2013, 8:50 p.m. UTC | #1 
? ??2013-07-11 ? 14:14 +0200?oliver@neukum.org ???
> From: Oliver Neukum <oneukum@suse.de>
> 
> Some BIOSes change hardware based on the state of
> a laptop's lid. If the lid is closed, the touchpad is
> disabled and the checksum changes.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/acpi/sleep.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 9c1a435..14744e5 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -664,11 +664,9 @@ static void acpi_hibernation_leave(void)
>  	/* Reprogram control registers */
>  	acpi_leave_sleep_state_prep(ACPI_STATE_S4);
>  	/* Check the hardware signature */
> -	if (facs && s4_hardware_signature != facs->hardware_signature) {
> -		printk(KERN_EMERG "ACPI: Hardware changed while hibernated, "
> -			"cannot resume!\n");
> -		panic("ACPI S4 hardware signature mismatch");
> -	}
> +	if (facs && s4_hardware_signature != facs->hardware_signature)
> +		printk(KERN_CRIT "ACPI: Hardware changed while hibernated, "
> +			"success doubtful!\n");

This panic is also reflect to the memory size changed by user when
machine in S4 state, BIOS should update hardware_signature to raise
memory changed.

Does it a good idea don't panic here if memory size changed?

Should we need try to detect the change of memory or touchpad for know
should we panic or not?

>  	/* Restore the NVS memory area */
>  	suspend_nvs_restore();
>  	/* Allow EC transactions to happen. */

On the other hand, I saw another BIOS changed hardware_signature because
the VGA detect output resolution changed when S4 resume. It's a problem
for sometimes BIOS widely used hardware_signature on non-critical
hardware change.


Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Renninger July 12, 2013, 7:31 a.m. UTC | #2 
On Friday, July 12, 2013 04:50:22 AM joeyli wrote:
> ? ??2013-07-11 ? 14:14 +0200?oliver@neukum.org ???
...
 
> On the other hand, I saw another BIOS changed hardware_signature because
> the VGA detect output resolution changed when S4 resume. It's a problem
> for sometimes BIOS widely used hardware_signature on non-critical
> hardware change.

I wonder how Windows is handling that.
The spec is rather clear about aborting the resuming:

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Renninger July 12, 2013, 7:37 a.m. UTC | #3 
On Friday, July 12, 2013 04:50:22 AM joeyli wrote:
> ? ??2013-07-11 ? 14:14 +0200?oliver@neukum.org ???
...
> On the other hand, I saw another BIOS changed hardware_signature because
> the VGA detect output resolution changed when S4 resume. It's a problem
> for sometimes BIOS widely used hardware_signature on non-critical
> hardware change.

I wonder how Windows is handling that.
The spec is rather clear and OS should abort the resuming:
------
5.2.10 Firmware ACPI Control Structure (FACS)
...
If the values are not the same, OSPM assumes
that the saved non-volatile image is from a different hardware
configuration and cannot be restored.
------
I would say either:
   - someone checks Windows behavior and if they do it wrong
     and still resume, resuming unconditionally is an option.
     But they might fix this at some point of time and force vendors
     to do things right, then we would still try to resume even we
     must not.

  - a boot parameter for the poor souls with such BIOS defects
    so that they can still resume


My 2 cents,

           Thomas
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joeyli July 12, 2013, 8:45 a.m. UTC | #4 
? ??2013-07-12 ? 09:37 +0200?Thomas Renninger ???
> On Friday, July 12, 2013 04:50:22 AM joeyli wrote:
> > ? ??2013-07-11 ? 14:14 +0200?oliver@neukum.org ???
> ...
> > On the other hand, I saw another BIOS changed hardware_signature because
> > the VGA detect output resolution changed when S4 resume. It's a problem
> > for sometimes BIOS widely used hardware_signature on non-critical
> > hardware change.
> 
> I wonder how Windows is handling that.
> The spec is rather clear and OS should abort the resuming:
> ------
> 5.2.10 Firmware ACPI Control Structure (FACS)
> ...
> If the values are not the same, OSPM assumes
> that the saved non-volatile image is from a different hardware
> configuration and cannot be restored.

Thanks for your kick.

Per information from OEM, Windows 8 didn't really block S4 resume when
hardware_signature not match, I think as Oliver's patch.

> ------
> I would say either:
>    - someone checks Windows behavior and if they do it wrong
>      and still resume, resuming unconditionally is an option.
>      But they might fix this at some point of time and force vendors
>      to do things right, then we would still try to resume even we
>      must not.
> 
>   - a boot parameter for the poor souls with such BIOS defects
>     so that they can still resume
> 
> 
> My 2 cents,
> 
>            Thomas
> --

I agreed your point, we should find a machine make sure the
hardware_signature changed by BIOS then install Windows 8 to verify the
behavior.


Thanks a lot!
Joey Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Oliver Neukum July 15, 2013, 11:29 a.m. UTC | #5 
On Friday 12 July 2013 16:45:15 joeyli wrote:

> Per information from OEM, Windows 8 didn't really block S4 resume when
> hardware_signature not match, I think as Oliver's patch.

Is that confidential information?
 
If it is indeed true, introducing a blacklist makes no sense. And I
see no alternative to my patch. Should I resubmit with an improved
comment?

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki July 15, 2013, 11:40 a.m. UTC | #6 
On Monday, July 15, 2013 01:29:47 PM Oliver Neukum wrote:
> On Friday 12 July 2013 16:45:15 joeyli wrote:
> 
> > Per information from OEM, Windows 8 didn't really block S4 resume when
> > hardware_signature not match, I think as Oliver's patch.
> 
> Is that confidential information?
>  
> If it is indeed true, introducing a blacklist makes no sense. And I
> see no alternative to my patch. Should I resubmit with an improved
> comment?

Yes, it would be good to update the changelog with this info.

Thanks,
Rafael
Oliver Neukum July 15, 2013, 11:44 a.m. UTC | #7 
On Monday 15 July 2013 13:40:09 Rafael J. Wysocki wrote:
> On Monday, July 15, 2013 01:29:47 PM Oliver Neukum wrote:
> > On Friday 12 July 2013 16:45:15 joeyli wrote:
> > 
> > > Per information from OEM, Windows 8 didn't really block S4 resume when
> > > hardware_signature not match, I think as Oliver's patch.
> > 
> > Is that confidential information?
> >  
> > If it is indeed true, introducing a blacklist makes no sense. And I
> > see no alternative to my patch. Should I resubmit with an improved
> > comment?
> 
> Yes, it would be good to update the changelog with this info.

Done.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
joeyli July 15, 2013, 1:59 p.m. UTC | #8 
? ??2013-07-15 ? 13:29 +0200?Oliver Neukum ???
> On Friday 12 July 2013 16:45:15 joeyli wrote:
> 
> > Per information from OEM, Windows 8 didn't really block S4 resume when
> > hardware_signature not match, I think as Oliver's patch.
> 
> Is that confidential information?
>  
> If it is indeed true, introducing a blacklist makes no sense. And I
> see no alternative to my patch. Should I resubmit with an improved
> comment?
> 
> 	Regards
> 		Oliver
> 

It's not confidential but non-official from OEM.

I looking for a additional DRAM module to test this behavior on my Acer
machine. 


Thank
Joey Lee


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9c1a435..14744e5 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -664,11 +664,9 @@  static void acpi_hibernation_leave(void)
 	/* Reprogram control registers */
 	acpi_leave_sleep_state_prep(ACPI_STATE_S4);
 	/* Check the hardware signature */
-	if (facs && s4_hardware_signature != facs->hardware_signature) {
-		printk(KERN_EMERG "ACPI: Hardware changed while hibernated, "
-			"cannot resume!\n");
-		panic("ACPI S4 hardware signature mismatch");
-	}
+	if (facs && s4_hardware_signature != facs->hardware_signature)
+		printk(KERN_CRIT "ACPI: Hardware changed while hibernated, "
+			"success doubtful!\n");
 	/* Restore the NVS memory area */
 	suspend_nvs_restore();
 	/* Allow EC transactions to happen. */