| Message ID | 1276715857-6013-1-git-send-email-kamal@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
On 16/06/10 20:17, Kamal Mostafa wrote: > I propose the following patch for -stable. This fixes a hang on resume for > all Core i3/i5/i7 machines, by always setting SCI_EN instead of just for > models listed in a quirk table. I've tested this in 2.6.32 (Ubuntu Lucid). > > Signed-off-by: Kamal Mostafa <kamal@canonical.com> Hi Kamal, Does this problem only occur when an SD/MMC card is inserted? Kind regards, Lee
Hi Lee- No, the hang on resume occurs all the time, with no SD/MMC card inserted (the problem and the fix have been confirmed by multiple Dell Studio 155x users, and for some other machines as well). -Kamal On Wed, 2010-06-16 at 20:23 +0100, Lee Jones wrote: > On 16/06/10 20:17, Kamal Mostafa wrote: > > I propose the following patch for -stable. This fixes a hang on resume for > > all Core i3/i5/i7 machines, by always setting SCI_EN instead of just for > > models listed in a quirk table. I've tested this in 2.6.32 (Ubuntu Lucid). > > > > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > > Hi Kamal, > > Does this problem only occur when an SD/MMC card is inserted? > > Kind regards, > Lee >
On 16/06/10 20:43, Kamal Mostafa wrote: > Hi Lee- > > No, the hang on resume occurs all the time, with no SD/MMC card inserted > (the problem and the fix have been confirmed by multiple Dell Studio > 155x users, and for some other machines as well). Hi Kamal, No problem. I only ask as I am working on a very similar bug which as been reported more than 20 times already. Thanks for your quick reply. Kind regards, Lee
So about this patch... As discussed in the kernel team meeting I have created a new bug for a Lucid SRU: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/594837 Lucid SRU: Intel Core i3/i5/i7 hang on resume from suspend (SCI_EN) And have also submitted it to stable@kernel.org (as cc'd here) Now I wonder if I understood the plan correctly... Was I supposed to wait to see what happens with stable *before* filing a Lucid SRU bug? Do I actually need to submit this patch (again) to our kernel-team ML as a [Lucid SRU] patch referencing that LP bug? I forgot to add a buglink to the patch I submitted to stable (and anyway since its a cherry-pick I don't know that my comment or s-o-b will even reach the changelog). -Kamal On Wed, 2010-06-16 at 12:17 -0700, Kamal Mostafa wrote: > I propose the following patch for -stable. This fixes a hang on resume for > all Core i3/i5/i7 machines, by always setting SCI_EN instead of just for > models listed in a quirk table. I've tested this in 2.6.32 (Ubuntu Lucid). > > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > > > From b6dacf63e9fb2e7a1369843d6cef332f76fca6a3 Mon Sep 17 00:00:00 2001 > From: Matthew Garrett <mjg@redhat.com> > Date: Tue, 11 May 2010 13:49:25 -0400 > Subject: [PATCH] ACPI: Unconditionally set SCI_EN on resume > > The ACPI spec tells us that the firmware will reenable SCI_EN on resume. > Reality disagrees in some cases. The ACPI spec tells us that the only way > to set SCI_EN is via an SMM call. > https://bugzilla.kernel.org/show_bug.cgi?id=13745 shows us that doing so > may break machines. Tracing the ACPI calls made by Windows shows that it > unconditionally sets SCI_EN on resume with a direct register write, and > therefore the overwhelming probability is that everything is fine with > this behaviour. > > Signed-off-by: Matthew Garrett <mjg@redhat.com> > Tested-by: Rafael J. Wysocki <rjw@sisk.pl> > Signed-off-by: Len Brown <len.brown@intel.com> > --- > arch/x86/kernel/acpi/sleep.c | 2 - > drivers/acpi/sleep.c | 157 +----------------------------------------- > include/linux/acpi.h | 1 - > 3 files changed, 2 insertions(+), 158 deletions(-) > > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > index f996103..82e5086 100644 > --- a/arch/x86/kernel/acpi/sleep.c > +++ b/arch/x86/kernel/acpi/sleep.c > @@ -162,8 +162,6 @@ static int __init acpi_sleep_setup(char *str) > #endif > if (strncmp(str, "old_ordering", 12) == 0) > acpi_old_suspend_ordering(); > - if (strncmp(str, "sci_force_enable", 16) == 0) > - acpi_set_sci_en_on_resume(); > str = strchr(str, ','); > if (str != NULL) > str += strspn(str, ", \t"); > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index baa76bb..4ab2275 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -80,22 +80,6 @@ static int acpi_sleep_prepare(u32 acpi_state) > > #ifdef CONFIG_ACPI_SLEEP > static u32 acpi_target_sleep_state = ACPI_STATE_S0; > -/* > - * According to the ACPI specification the BIOS should make sure that ACPI is > - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, > - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI > - * on such systems during resume. Unfortunately that doesn't help in > - * particularly pathological cases in which SCI_EN has to be set directly on > - * resume, although the specification states very clearly that this flag is > - * owned by the hardware. The set_sci_en_on_resume variable will be set in such > - * cases. > - */ > -static bool set_sci_en_on_resume; > - > -void __init acpi_set_sci_en_on_resume(void) > -{ > - set_sci_en_on_resume = true; > -} > > /* > * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the > @@ -253,11 +237,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state) > break; > } > > - /* If ACPI is not enabled by the BIOS, we need to enable it here. */ > - if (set_sci_en_on_resume) > - acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); > - else > - acpi_enable(); > + /* This violates the spec but is required for bug compatibility. */ > + acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); > > /* Reprogram control registers and execute _BFS */ > acpi_leave_sleep_state_prep(acpi_state); > @@ -346,12 +327,6 @@ static int __init init_old_suspend_ordering(const struct dmi_system_id *d) > return 0; > } > > -static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d) > -{ > - set_sci_en_on_resume = true; > - return 0; > -} > - > static struct dmi_system_id __initdata acpisleep_dmi_table[] = { > { > .callback = init_old_suspend_ordering, > @@ -370,22 +345,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { > }, > }, > { > - .callback = init_set_sci_en_on_resume, > - .ident = "Apple MacBook 1,1", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), > - DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Apple MacMini 1,1", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), > - DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"), > - }, > - }, > - { > .callback = init_old_suspend_ordering, > .ident = "Asus Pundit P1-AH2 (M2N8L motherboard)", > .matches = { > @@ -394,94 +353,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { > }, > }, > { > - .callback = init_set_sci_en_on_resume, > - .ident = "Toshiba Satellite L300", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"), > - DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard HP G7000 Notebook PC", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "HP G7000 Notebook PC"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard HP Pavilion dv3 Notebook PC", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv3 Notebook PC"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard Pavilion dv4", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv4"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard Pavilion dv7", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv7"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard Compaq Presario C700 Notebook PC", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario C700 Notebook PC"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Hewlett-Packard Compaq Presario CQ40 Notebook PC", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), > - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario CQ40 Notebook PC"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Lenovo ThinkPad T410", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T410"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Lenovo ThinkPad T510", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T510"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Lenovo ThinkPad W510", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W510"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Lenovo ThinkPad X201[s]", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201"), > - }, > - }, > - { > .callback = init_old_suspend_ordering, > .ident = "Panasonic CF51-2L", > .matches = { > @@ -490,30 +361,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { > DMI_MATCH(DMI_BOARD_NAME, "CF51-2L"), > }, > }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Dell Studio 1558", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1558"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Dell Studio 1557", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1557"), > - }, > - }, > - { > - .callback = init_set_sci_en_on_resume, > - .ident = "Dell Studio 1555", > - .matches = { > - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1555"), > - }, > - }, > {}, > }; > #endif /* CONFIG_SUSPEND */ > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index b926afe..87ca491 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -251,7 +251,6 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, > void __init acpi_no_s4_hw_signature(void); > void __init acpi_old_suspend_ordering(void); > void __init acpi_s4_no_nvs(void); > -void __init acpi_set_sci_en_on_resume(void); > #endif /* CONFIG_PM_SLEEP */ > > struct acpi_osc_context { > -- > 1.7.0.4 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team >
If it looks like it'll come via stable updates, then let things take their natural course. Otherwise, I'd be willing to support this as a Lucid SRU, but its gonna take some widespread testing. rtg On 06/16/2010 02:26 PM, Kamal Mostafa wrote: > So about this patch... As discussed in the kernel team meeting > > I have created a new bug for a Lucid SRU: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/594837 > Lucid SRU: Intel Core i3/i5/i7 hang on resume from suspend (SCI_EN) > > And have also submitted it to stable@kernel.org (as cc'd here) > > Now I wonder if I understood the plan correctly... Was I supposed to > wait to see what happens with stable *before* filing a Lucid SRU bug? > Do I actually need to submit this patch (again) to our kernel-team ML as > a [Lucid SRU] patch referencing that LP bug? > > I forgot to add a buglink to the patch I submitted to stable (and anyway > since its a cherry-pick I don't know that my comment or s-o-b will even > reach the changelog). > > -Kamal > > On Wed, 2010-06-16 at 12:17 -0700, Kamal Mostafa wrote: >> I propose the following patch for -stable. This fixes a hang on resume for >> all Core i3/i5/i7 machines, by always setting SCI_EN instead of just for >> models listed in a quirk table. I've tested this in 2.6.32 (Ubuntu Lucid). >> >> Signed-off-by: Kamal Mostafa<kamal@canonical.com> >> >> >> From b6dacf63e9fb2e7a1369843d6cef332f76fca6a3 Mon Sep 17 00:00:00 2001 >> From: Matthew Garrett<mjg@redhat.com> >> Date: Tue, 11 May 2010 13:49:25 -0400 >> Subject: [PATCH] ACPI: Unconditionally set SCI_EN on resume >> >> The ACPI spec tells us that the firmware will reenable SCI_EN on resume. >> Reality disagrees in some cases. The ACPI spec tells us that the only way >> to set SCI_EN is via an SMM call. >> https://bugzilla.kernel.org/show_bug.cgi?id=13745 shows us that doing so >> may break machines. Tracing the ACPI calls made by Windows shows that it >> unconditionally sets SCI_EN on resume with a direct register write, and >> therefore the overwhelming probability is that everything is fine with >> this behaviour. >> >> Signed-off-by: Matthew Garrett<mjg@redhat.com> >> Tested-by: Rafael J. Wysocki<rjw@sisk.pl> >> Signed-off-by: Len Brown<len.brown@intel.com> >> --- >> arch/x86/kernel/acpi/sleep.c | 2 - >> drivers/acpi/sleep.c | 157 +----------------------------------------- >> include/linux/acpi.h | 1 - >> 3 files changed, 2 insertions(+), 158 deletions(-) >> >> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c >> index f996103..82e5086 100644 >> --- a/arch/x86/kernel/acpi/sleep.c >> +++ b/arch/x86/kernel/acpi/sleep.c >> @@ -162,8 +162,6 @@ static int __init acpi_sleep_setup(char *str) >> #endif >> if (strncmp(str, "old_ordering", 12) == 0) >> acpi_old_suspend_ordering(); >> - if (strncmp(str, "sci_force_enable", 16) == 0) >> - acpi_set_sci_en_on_resume(); >> str = strchr(str, ','); >> if (str != NULL) >> str += strspn(str, ", \t"); >> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c >> index baa76bb..4ab2275 100644 >> --- a/drivers/acpi/sleep.c >> +++ b/drivers/acpi/sleep.c >> @@ -80,22 +80,6 @@ static int acpi_sleep_prepare(u32 acpi_state) >> >> #ifdef CONFIG_ACPI_SLEEP >> static u32 acpi_target_sleep_state = ACPI_STATE_S0; >> -/* >> - * According to the ACPI specification the BIOS should make sure that ACPI is >> - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, >> - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI >> - * on such systems during resume. Unfortunately that doesn't help in >> - * particularly pathological cases in which SCI_EN has to be set directly on >> - * resume, although the specification states very clearly that this flag is >> - * owned by the hardware. The set_sci_en_on_resume variable will be set in such >> - * cases. >> - */ >> -static bool set_sci_en_on_resume; >> - >> -void __init acpi_set_sci_en_on_resume(void) >> -{ >> - set_sci_en_on_resume = true; >> -} >> >> /* >> * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the >> @@ -253,11 +237,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state) >> break; >> } >> >> - /* If ACPI is not enabled by the BIOS, we need to enable it here. */ >> - if (set_sci_en_on_resume) >> - acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); >> - else >> - acpi_enable(); >> + /* This violates the spec but is required for bug compatibility. */ >> + acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); >> >> /* Reprogram control registers and execute _BFS */ >> acpi_leave_sleep_state_prep(acpi_state); >> @@ -346,12 +327,6 @@ static int __init init_old_suspend_ordering(const struct dmi_system_id *d) >> return 0; >> } >> >> -static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d) >> -{ >> - set_sci_en_on_resume = true; >> - return 0; >> -} >> - >> static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >> { >> .callback = init_old_suspend_ordering, >> @@ -370,22 +345,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >> }, >> }, >> { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Apple MacBook 1,1", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), >> - DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Apple MacMini 1,1", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"), >> - }, >> - }, >> - { >> .callback = init_old_suspend_ordering, >> .ident = "Asus Pundit P1-AH2 (M2N8L motherboard)", >> .matches = { >> @@ -394,94 +353,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >> }, >> }, >> { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Toshiba Satellite L300", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard HP G7000 Notebook PC", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "HP G7000 Notebook PC"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard HP Pavilion dv3 Notebook PC", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv3 Notebook PC"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard Pavilion dv4", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv4"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard Pavilion dv7", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv7"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard Compaq Presario C700 Notebook PC", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario C700 Notebook PC"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Hewlett-Packard Compaq Presario CQ40 Notebook PC", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario CQ40 Notebook PC"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Lenovo ThinkPad T410", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T410"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Lenovo ThinkPad T510", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T510"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Lenovo ThinkPad W510", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W510"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Lenovo ThinkPad X201[s]", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201"), >> - }, >> - }, >> - { >> .callback = init_old_suspend_ordering, >> .ident = "Panasonic CF51-2L", >> .matches = { >> @@ -490,30 +361,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >> DMI_MATCH(DMI_BOARD_NAME, "CF51-2L"), >> }, >> }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Dell Studio 1558", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1558"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Dell Studio 1557", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1557"), >> - }, >> - }, >> - { >> - .callback = init_set_sci_en_on_resume, >> - .ident = "Dell Studio 1555", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1555"), >> - }, >> - }, >> {}, >> }; >> #endif /* CONFIG_SUSPEND */ >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >> index b926afe..87ca491 100644 >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -251,7 +251,6 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, >> void __init acpi_no_s4_hw_signature(void); >> void __init acpi_old_suspend_ordering(void); >> void __init acpi_s4_no_nvs(void); >> -void __init acpi_set_sci_en_on_resume(void); >> #endif /* CONFIG_PM_SLEEP */ >> >> struct acpi_osc_context { >> -- >> 1.7.0.4 >> >> >> -- >> kernel-team mailing list >> kernel-team@lists.ubuntu.com >> https://lists.ubuntu.com/mailman/listinfo/kernel-team >> >
On 06/17/2010 06:49 AM, Tim Gardner wrote: > If it looks like it'll come via stable updates, then let things take > their natural course. > > Otherwise, I'd be willing to support this as a Lucid SRU, but its gonna > take some widespread testing. > > rtg > > On 06/16/2010 02:26 PM, Kamal Mostafa wrote: >> So about this patch... As discussed in the kernel team meeting >> >> I have created a new bug for a Lucid SRU: >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/594837 >> Lucid SRU: Intel Core i3/i5/i7 hang on resume from suspend (SCI_EN) >> >> And have also submitted it to stable@kernel.org (as cc'd here) >> >> Now I wonder if I understood the plan correctly... Was I supposed to >> wait to see what happens with stable *before* filing a Lucid SRU bug? >> Do I actually need to submit this patch (again) to our kernel-team ML as >> a [Lucid SRU] patch referencing that LP bug? >> >> I forgot to add a buglink to the patch I submitted to stable (and anyway >> since its a cherry-pick I don't know that my comment or s-o-b will even >> reach the changelog). >> >> -Kamal >> >> On Wed, 2010-06-16 at 12:17 -0700, Kamal Mostafa wrote: >>> I propose the following patch for -stable. This fixes a hang on resume for >>> all Core i3/i5/i7 machines, by always setting SCI_EN instead of just for >>> models listed in a quirk table. I've tested this in 2.6.32 (Ubuntu Lucid). >>> >>> Signed-off-by: Kamal Mostafa<kamal@canonical.com> >>> >>> >>> From b6dacf63e9fb2e7a1369843d6cef332f76fca6a3 Mon Sep 17 00:00:00 2001 >>> From: Matthew Garrett<mjg@redhat.com> >>> Date: Tue, 11 May 2010 13:49:25 -0400 >>> Subject: [PATCH] ACPI: Unconditionally set SCI_EN on resume >>> >>> The ACPI spec tells us that the firmware will reenable SCI_EN on resume. >>> Reality disagrees in some cases. The ACPI spec tells us that the only way >>> to set SCI_EN is via an SMM call. >>> https://bugzilla.kernel.org/show_bug.cgi?id=13745 shows us that doing so >>> may break machines. Tracing the ACPI calls made by Windows shows that it >>> unconditionally sets SCI_EN on resume with a direct register write, and >>> therefore the overwhelming probability is that everything is fine with >>> this behaviour. >>> >>> Signed-off-by: Matthew Garrett<mjg@redhat.com> >>> Tested-by: Rafael J. Wysocki<rjw@sisk.pl> >>> Signed-off-by: Len Brown<len.brown@intel.com> >>> --- >>> arch/x86/kernel/acpi/sleep.c | 2 - >>> drivers/acpi/sleep.c | 157 +----------------------------------------- >>> include/linux/acpi.h | 1 - >>> 3 files changed, 2 insertions(+), 158 deletions(-) >>> >>> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c >>> index f996103..82e5086 100644 >>> --- a/arch/x86/kernel/acpi/sleep.c >>> +++ b/arch/x86/kernel/acpi/sleep.c >>> @@ -162,8 +162,6 @@ static int __init acpi_sleep_setup(char *str) >>> #endif >>> if (strncmp(str, "old_ordering", 12) == 0) >>> acpi_old_suspend_ordering(); >>> - if (strncmp(str, "sci_force_enable", 16) == 0) >>> - acpi_set_sci_en_on_resume(); >>> str = strchr(str, ','); >>> if (str != NULL) >>> str += strspn(str, ", \t"); >>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c >>> index baa76bb..4ab2275 100644 >>> --- a/drivers/acpi/sleep.c >>> +++ b/drivers/acpi/sleep.c >>> @@ -80,22 +80,6 @@ static int acpi_sleep_prepare(u32 acpi_state) >>> >>> #ifdef CONFIG_ACPI_SLEEP >>> static u32 acpi_target_sleep_state = ACPI_STATE_S0; >>> -/* >>> - * According to the ACPI specification the BIOS should make sure that ACPI is >>> - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, >>> - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI >>> - * on such systems during resume. Unfortunately that doesn't help in >>> - * particularly pathological cases in which SCI_EN has to be set directly on >>> - * resume, although the specification states very clearly that this flag is >>> - * owned by the hardware. The set_sci_en_on_resume variable will be set in such >>> - * cases. >>> - */ >>> -static bool set_sci_en_on_resume; >>> - >>> -void __init acpi_set_sci_en_on_resume(void) >>> -{ >>> - set_sci_en_on_resume = true; >>> -} >>> >>> /* >>> * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the >>> @@ -253,11 +237,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state) >>> break; >>> } >>> >>> - /* If ACPI is not enabled by the BIOS, we need to enable it here. */ >>> - if (set_sci_en_on_resume) >>> - acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); >>> - else >>> - acpi_enable(); >>> + /* This violates the spec but is required for bug compatibility. */ >>> + acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); >>> >>> /* Reprogram control registers and execute _BFS */ >>> acpi_leave_sleep_state_prep(acpi_state); >>> @@ -346,12 +327,6 @@ static int __init init_old_suspend_ordering(const struct dmi_system_id *d) >>> return 0; >>> } >>> >>> -static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d) >>> -{ >>> - set_sci_en_on_resume = true; >>> - return 0; >>> -} >>> - >>> static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >>> { >>> .callback = init_old_suspend_ordering, >>> @@ -370,22 +345,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >>> }, >>> }, >>> { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Apple MacBook 1,1", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Apple MacMini 1,1", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"), >>> - }, >>> - }, >>> - { >>> .callback = init_old_suspend_ordering, >>> .ident = "Asus Pundit P1-AH2 (M2N8L motherboard)", >>> .matches = { >>> @@ -394,94 +353,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >>> }, >>> }, >>> { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Toshiba Satellite L300", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard HP G7000 Notebook PC", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "HP G7000 Notebook PC"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard HP Pavilion dv3 Notebook PC", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv3 Notebook PC"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard Pavilion dv4", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv4"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard Pavilion dv7", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv7"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard Compaq Presario C700 Notebook PC", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario C700 Notebook PC"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Hewlett-Packard Compaq Presario CQ40 Notebook PC", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario CQ40 Notebook PC"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Lenovo ThinkPad T410", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T410"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Lenovo ThinkPad T510", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T510"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Lenovo ThinkPad W510", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W510"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Lenovo ThinkPad X201[s]", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >>> - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201"), >>> - }, >>> - }, >>> - { >>> .callback = init_old_suspend_ordering, >>> .ident = "Panasonic CF51-2L", >>> .matches = { >>> @@ -490,30 +361,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { >>> DMI_MATCH(DMI_BOARD_NAME, "CF51-2L"), >>> }, >>> }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Dell Studio 1558", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1558"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Dell Studio 1557", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1557"), >>> - }, >>> - }, >>> - { >>> - .callback = init_set_sci_en_on_resume, >>> - .ident = "Dell Studio 1555", >>> - .matches = { >>> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >>> - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1555"), >>> - }, >>> - }, >>> {}, >>> }; >>> #endif /* CONFIG_SUSPEND */ >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h >>> index b926afe..87ca491 100644 >>> --- a/include/linux/acpi.h >>> +++ b/include/linux/acpi.h >>> @@ -251,7 +251,6 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, >>> void __init acpi_no_s4_hw_signature(void); >>> void __init acpi_old_suspend_ordering(void); >>> void __init acpi_s4_no_nvs(void); >>> -void __init acpi_set_sci_en_on_resume(void); >>> #endif /* CONFIG_PM_SLEEP */ >>> >>> struct acpi_osc_context { >>> -- >>> 1.7.0.4 >>> >>> >>> -- >>> kernel-team mailing list >>> kernel-team@lists.ubuntu.com >>> https://lists.ubuntu.com/mailman/listinfo/kernel-team >>> >> > > This was picked up as a stable update. Brad
diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index f996103..82e5086 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -162,8 +162,6 @@ static int __init acpi_sleep_setup(char *str) #endif if (strncmp(str, "old_ordering", 12) == 0) acpi_old_suspend_ordering(); - if (strncmp(str, "sci_force_enable", 16) == 0) - acpi_set_sci_en_on_resume(); str = strchr(str, ','); if (str != NULL) str += strspn(str, ", \t"); diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index baa76bb..4ab2275 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -80,22 +80,6 @@ static int acpi_sleep_prepare(u32 acpi_state) #ifdef CONFIG_ACPI_SLEEP static u32 acpi_target_sleep_state = ACPI_STATE_S0; -/* - * According to the ACPI specification the BIOS should make sure that ACPI is - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI - * on such systems during resume. Unfortunately that doesn't help in - * particularly pathological cases in which SCI_EN has to be set directly on - * resume, although the specification states very clearly that this flag is - * owned by the hardware. The set_sci_en_on_resume variable will be set in such - * cases. - */ -static bool set_sci_en_on_resume; - -void __init acpi_set_sci_en_on_resume(void) -{ - set_sci_en_on_resume = true; -} /* * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the @@ -253,11 +237,8 @@ static int acpi_suspend_enter(suspend_state_t pm_state) break; } - /* If ACPI is not enabled by the BIOS, we need to enable it here. */ - if (set_sci_en_on_resume) - acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); - else - acpi_enable(); + /* This violates the spec but is required for bug compatibility. */ + acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1); /* Reprogram control registers and execute _BFS */ acpi_leave_sleep_state_prep(acpi_state); @@ -346,12 +327,6 @@ static int __init init_old_suspend_ordering(const struct dmi_system_id *d) return 0; } -static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d) -{ - set_sci_en_on_resume = true; - return 0; -} - static struct dmi_system_id __initdata acpisleep_dmi_table[] = { { .callback = init_old_suspend_ordering, @@ -370,22 +345,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { }, }, { - .callback = init_set_sci_en_on_resume, - .ident = "Apple MacBook 1,1", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), - DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Apple MacMini 1,1", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."), - DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"), - }, - }, - { .callback = init_old_suspend_ordering, .ident = "Asus Pundit P1-AH2 (M2N8L motherboard)", .matches = { @@ -394,94 +353,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { }, }, { - .callback = init_set_sci_en_on_resume, - .ident = "Toshiba Satellite L300", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"), - DMI_MATCH(DMI_PRODUCT_NAME, "Satellite L300"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard HP G7000 Notebook PC", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "HP G7000 Notebook PC"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard HP Pavilion dv3 Notebook PC", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv3 Notebook PC"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard Pavilion dv4", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv4"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard Pavilion dv7", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "HP Pavilion dv7"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard Compaq Presario C700 Notebook PC", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario C700 Notebook PC"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Hewlett-Packard Compaq Presario CQ40 Notebook PC", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"), - DMI_MATCH(DMI_PRODUCT_NAME, "Compaq Presario CQ40 Notebook PC"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Lenovo ThinkPad T410", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T410"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Lenovo ThinkPad T510", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad T510"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Lenovo ThinkPad W510", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad W510"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Lenovo ThinkPad X201[s]", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), - DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad X201"), - }, - }, - { .callback = init_old_suspend_ordering, .ident = "Panasonic CF51-2L", .matches = { @@ -490,30 +361,6 @@ static struct dmi_system_id __initdata acpisleep_dmi_table[] = { DMI_MATCH(DMI_BOARD_NAME, "CF51-2L"), }, }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Dell Studio 1558", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1558"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Dell Studio 1557", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1557"), - }, - }, - { - .callback = init_set_sci_en_on_resume, - .ident = "Dell Studio 1555", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), - DMI_MATCH(DMI_PRODUCT_NAME, "Studio 1555"), - }, - }, {}, }; #endif /* CONFIG_SUSPEND */ diff --git a/include/linux/acpi.h b/include/linux/acpi.h index b926afe..87ca491 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -251,7 +251,6 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, void __init acpi_no_s4_hw_signature(void); void __init acpi_old_suspend_ordering(void); void __init acpi_s4_no_nvs(void); -void __init acpi_set_sci_en_on_resume(void); #endif /* CONFIG_PM_SLEEP */ struct acpi_osc_context {