diff mbox

[bisected] PS/2 keyboard and mouse dead on resume on Intel D845BG

Message ID 201210072100.09813.linux@rainbow-software.org (mailing list archive)
State RFC, archived
Headers show

Commit Message

Ondrej Zary Oct. 7, 2012, 7 p.m. UTC
On Sunday 07 October 2012 15:13:27 Ondrej Zary wrote:
> Hello,
> Intel D845BG board comes out of S3 with PS/2 keyboard and mouse completely
> dead. The machine works otherwise (with USB keyboard or over network). When
> rebooted in this state, the BIOS hangs with blank screen. I have the latest
> BIOS installed (P08).
>
> Old kernels worked. Bisection pointed to commit:
> b6dacf63e9fb2e7a1369843d6cef332f76fca6a3
> (ACPI: Unconditionally set SCI_EN on resume)
>
> Commenting out this line in drivers/acpi/sleep.c:
> 	acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> fixes the problem.
>
> Any ideas why this breaks on this system?

Added acpi_read_bit_register there and it seems that SCI_EN is already set!

This patch fixes the problem here. I wonder how this affects systems that
require SCI_EN to be set.

Comments

Rafael Wysocki Oct. 7, 2012, 7:59 p.m. UTC | #1
On Sunday 07 of October 2012 21:00:09 Ondrej Zary wrote:
> On Sunday 07 October 2012 15:13:27 Ondrej Zary wrote:
> > Hello,
> > Intel D845BG board comes out of S3 with PS/2 keyboard and mouse completely
> > dead. The machine works otherwise (with USB keyboard or over network). When
> > rebooted in this state, the BIOS hangs with blank screen. I have the latest
> > BIOS installed (P08).
> >
> > Old kernels worked. Bisection pointed to commit:
> > b6dacf63e9fb2e7a1369843d6cef332f76fca6a3
> > (ACPI: Unconditionally set SCI_EN on resume)
> >
> > Commenting out this line in drivers/acpi/sleep.c:
> > 	acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> > fixes the problem.
> >
> > Any ideas why this breaks on this system?
> 
> Added acpi_read_bit_register there and it seems that SCI_EN is already set!
> 
> This patch fixes the problem here. I wonder how this affects systems that
> require SCI_EN to be set.

I'm not sure if reading SCI_EN is actually safe on all systems.

Matthew, what do you think?

Rafael


> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -271,6 +271,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>  	acpi_status status = AE_OK;
>  	u32 acpi_state = acpi_target_sleep_state;
>  	int error;
> +	u32 sci_enabled;
>  
>  	ACPI_FLUSH_CPU_CACHE();
>  
> @@ -289,7 +290,9 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>  	}
>  
>  	/* This violates the spec but is required for bug compatibility. */
> -	acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> +	acpi_read_bit_register(ACPI_BITREG_SCI_ENABLE, &sci_enabled);
> +	if (!sci_enabled)
> +		acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
>  
>  	/* Reprogram control registers */
>  	acpi_leave_sleep_state_prep(acpi_state);
> 
>
Matthew Garrett Oct. 8, 2012, 2:42 p.m. UTC | #2
On Sun, Oct 07, 2012 at 09:00:09PM +0200, Ondrej Zary wrote:

> Added acpi_read_bit_register there and it seems that SCI_EN is already set!
> 
> This patch fixes the problem here. I wonder how this affects systems that
> require SCI_EN to be set.

I /think/ that this will be safe, but it doesn't match my recollection 
of how Windows behaves so it may break something. Any chance you can 
find someone with one of the machines mentioned in 
https://bugzilla.kernel.org/show_bug.cgi?id=13745 and make sure that 
they still work with your patch?
Len Brown Oct. 9, 2012, 5:11 a.m. UTC | #3
On 10/08/2012 10:42 AM, Matthew Garrett wrote:
> On Sun, Oct 07, 2012 at 09:00:09PM +0200, Ondrej Zary wrote:
> 
>> Added acpi_read_bit_register there and it seems that SCI_EN is already set!
>>
>> This patch fixes the problem here. I wonder how this affects systems that
>> require SCI_EN to be set.
> 
> I /think/ that this will be safe, but it doesn't match my recollection 
> of how Windows behaves so it may break something. Any chance you can 
> find someone with one of the machines mentioned in 
> https://bugzilla.kernel.org/show_bug.cgi?id=13745 and make sure that 
> they still work with your patch?

yikes.
we started with a white-list,
then we made setting SCI_EN the default,
and here is at least one box that wants to be on a black list
so that we don't set SCI_EN:-(



--
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
Ondrej Zary Oct. 9, 2012, 6:17 a.m. UTC | #4
On Tuesday 09 October 2012, Len Brown wrote:
> On 10/08/2012 10:42 AM, Matthew Garrett wrote:
> > On Sun, Oct 07, 2012 at 09:00:09PM +0200, Ondrej Zary wrote:
> >> Added acpi_read_bit_register there and it seems that SCI_EN is already
> >> set!
> >>
> >> This patch fixes the problem here. I wonder how this affects systems
> >> that require SCI_EN to be set.
> >
> > I /think/ that this will be safe, but it doesn't match my recollection
> > of how Windows behaves so it may break something. Any chance you can
> > find someone with one of the machines mentioned in
> > https://bugzilla.kernel.org/show_bug.cgi?id=13745 and make sure that
> > they still work with your patch?
>
> yikes.
> we started with a white-list,
> then we made setting SCI_EN the default,
> and here is at least one box that wants to be on a black list
> so that we don't set SCI_EN:-(

I wonder what Windows does here. Haven't tested it on this machine but I 
assume that it works.
diff mbox

Patch

--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -271,6 +271,7 @@  static int acpi_suspend_enter(suspend_state_t pm_state)
 	acpi_status status = AE_OK;
 	u32 acpi_state = acpi_target_sleep_state;
 	int error;
+	u32 sci_enabled;
 
 	ACPI_FLUSH_CPU_CACHE();
 
@@ -289,7 +290,9 @@  static int acpi_suspend_enter(suspend_state_t pm_state)
 	}
 
 	/* This violates the spec but is required for bug compatibility. */
-	acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
+	acpi_read_bit_register(ACPI_BITREG_SCI_ENABLE, &sci_enabled);
+	if (!sci_enabled)
+		acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
 
 	/* Reprogram control registers */
 	acpi_leave_sleep_state_prep(acpi_state);