[PATCH] ACPI: Do not modify SCI_EN directly

[PATCH] ACPI: Do not modify SCI_EN directly

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
According to the ACPI specification the SCI_EN flag is controlled bythe hardware, which sets this flag to inform the kernel that ACPI isenabled.  For this reason, we shouldn't try to modify SCI_ENdirectly.  Also, we don't need to do it in irqrouter_resume(), sincelower-level resume code takes care of enabling ACPI in case it hasn'tbeen enabled by the BIOS before passing control to the kernel (whichby the way is against the ACPI specification).
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>--- drivers/acpi/pci_link.c |    4 ---- 1 file changed, 4 deletions(-)
Index: linux-2.6/drivers/acpi/pci_link.c===================================================================--- linux-2.6.orig/drivers/acpi/pci_link.c+++ linux-2.6/drivers/acpi/pci_link.c@@ -793,10 +793,6 @@ static int irqrouter_resume(struct sys_d 	struct list_head *node = NULL; 	struct acpi_pci_link *link = NULL; --	/* Make sure SCI is enabled again (Apple firmware bug?) */-	acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1);- 	list_for_each(node, &acpi_link.entries) { 		link = list_entry(node, struct acpi_pci_link, node); 		if (!link) {\0˙ôčş{.nÇ+‰ˇŸŽ‰­†+%ŠË˙ąéÝś\x17ĽŠw˙ş{.nÇ+‰ˇĽŠ{ąţGŤé˙Š{ayş\x1dʇڙë,j\a­˘fŁ˘ˇhšďę˙‘ęçz_čŽ\x03(­éšŽŠÝ˘j"ú\x1aś^[m§˙˙ž\aŤţGŤé˙˘¸?™¨č­Ú&Łř§~áśiO•ćŹzˇšvŘ^\x14\x04\x1aś^[m§˙˙Ă\f˙śě˙˘¸?–IĽ

Re: [PATCH] ACPI: Do not modify SCI_EN directly

[Maxim Levitsky]

On Mon, 2008-12-29 at 19:19 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> According to the ACPI specification the SCI_EN flag is controlled by
> the hardware, which sets this flag to inform the kernel that ACPI is
> enabled.  For this reason, we shouldn't try to modify SCI_EN
> directly.  Also, we don't need to do it in irqrouter_resume(), since
> lower-level resume code takes care of enabling ACPI in case it hasn't
> been enabled by the BIOS before passing control to the kernel (which
> by the way is against the ACPI specification).


Hi, 

I am an unfortunate owner of an acer notebook ( aspire 5720 if this
matters) and it doesn't come back after second suspend, subsequently  I
figured out that bios doesn't pass control to linux at all, after second
resume, and moreover, a suspend to disk , 'fixes' this and enables me to
do another suspend/resume cycle. This is a poor man workaround I use
now.

I already told about this here, and many peoples from here (including
you) tried to help me, but unfortunately this is very weird issue, and
nether me nor you can do much about, it is all about guesswork,
something, at least something is 'armed' on resume, so it confuses bios
then (it could be that yet, the critical part that confuses bios happens
in second suspend time)

Needless to say that nothing useful appears in the logs, and everything
seems to work fine following each successful resume (both disk and ram).

so I have a question, do you have a git branch to pull that includes all
patches like this to test, so I build it weekly, and who knows, maybe
something like above will fix it.

Anyway I would gladly test any patch that you suspect might be involved
here.


Also, I tried to do a I/O port and PCI config dump before and after a
resume, aka in 'armed' and 'unarmed' state.

While there were many I/O port registers changes, and I yet to process
it bit after bit (and unfortunately there are plenty of undocumented
stuff), I noticed a meaningful change between bad and good state:


>From ICH8 manual:

"PIRQ[n]_ROUT—PIRQ[A,B,C,D] Routing Control Register

and

PIRQ[n]_ROUT—PIRQ[E,F,G,H]

Interrupt Routing Enable (IRQEN) — R/W.
0 = The corresponding PIRQ is routed to one of the ISA-compatible
interrupts
      specified in bits[3:0].
1 = The PIRQ is not routed to the 8259.

NOTE: BIOS must program this bit to 0 during POST for any of the PIRQs
that are being used. The value of this bit may subsequently be changed
by the OS when setting up for I/O APIC interrupt delivery mode."


bit #7 of all those regs is '1' in good state, and '0' in bad state.

I tried to change it back, but this didn't help, so I post this just in
case, I don't think it is useful.


Best regards,
	Maxim Levitsky

Re: [PATCH] ACPI: Do not modify SCI_EN directly

[Rafael J. Wysocki]

On Monday 29 December 2008, Maxim Levitsky wrote:
> On Mon, 2008-12-29 at 19:19 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > According to the ACPI specification the SCI_EN flag is controlled by
> > the hardware, which sets this flag to inform the kernel that ACPI is
> > enabled.  For this reason, we shouldn't try to modify SCI_EN
> > directly.  Also, we don't need to do it in irqrouter_resume(), since
> > lower-level resume code takes care of enabling ACPI in case it hasn't
> > been enabled by the BIOS before passing control to the kernel (which
> > by the way is against the ACPI specification).
> 
> 
> Hi, 

Hi,

> I am an unfortunate owner of an acer notebook ( aspire 5720 if this
> matters) and it doesn't come back after second suspend, subsequently  I
> figured out that bios doesn't pass control to linux at all, after second
> resume, and moreover, a suspend to disk , 'fixes' this and enables me to
> do another suspend/resume cycle. This is a poor man workaround I use
> now.

I suspect there may be a problem with interrupts handling on your system.

There is http://bugzilla.kernel.org/show_bug.cgi?id=11698 describing a very
similar (if not the same) issue.  Please attach a boot log and the contents
of /proc/interrupts to that bug.

> I already told about this here, and many peoples from here (including
> you) tried to help me, but unfortunately this is very weird issue, and
> nether me nor you can do much about, it is all about guesswork,
> something, at least something is 'armed' on resume, so it confuses bios
> then (it could be that yet, the critical part that confuses bios happens
> in second suspend time)
> 
> Needless to say that nothing useful appears in the logs, and everything
> seems to work fine following each successful resume (both disk and ram).
> 
> so I have a question, do you have a git branch to pull that includes all
> patches like this to test, so I build it weekly, and who knows, maybe
> something like above will fix it.

Unfortunately, I don't.  If I have any patches that can possibly fix the
problem, I'll let you know.
 
> Anyway I would gladly test any patch that you suspect might be involved
> here.
> 
> Also, I tried to do a I/O port and PCI config dump before and after a
> resume, aka in 'armed' and 'unarmed' state.
> 
> While there were many I/O port registers changes, and I yet to process
> it bit after bit (and unfortunately there are plenty of undocumented
> stuff), I noticed a meaningful change between bad and good state:

Please check if the appended patch from Len changes anything.

Thanks,
Rafael

---
Author: Len Brown <len.brown@intel.com>
Date:   Sat Dec 20 12:50:23 2008 +0100

    ACPI: PCI Interrupt Links -- disable when unused
    
    We start the system off with disabled links.
    Here we enable the reference counting
    on the links so that when drivers free IRQs
    the links can return to the disabled state.
    
    Drivers free their IRQ and reference on a link
    when they unload.  They may also do so upon
    suspend.  However, if they don't, irqrouter_resume()
    will still resume any links that were
    not disabled before suspend.
    
    Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 595b131..0f7722d 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -693,18 +693,7 @@ int acpi_pci_link_free_irq(acpi_handle handle)
 		printk(KERN_ERR PREFIX "Link isn't initialized\n");
 		return -1;
 	}
-#ifdef	FUTURE_USE
-	/*
-	 * The Link reference count allows us to _DISable an unused link
-	 * and suspend time, and set it again  on resume.
-	 * However, 2.6.12 still has irq_router.resume
-	 * which blindly restores the link state.
-	 * So we disable the reference count method
-	 * to prevent duplicate acpi_pci_link_set()
-	 * which would harm some systems
-	 */
 	link->refcnt--;
-#endif
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "Link %s is dereferenced %d\n",
 			  acpi_device_bid(link->device), link->refcnt));

Re: [PATCH] ACPI: Do not modify SCI_EN directly

[Maxim Levitsky]

On Tue, 2008-12-30 at 22:59 +0100, Rafael J. Wysocki wrote:
> On Monday 29 December 2008, Maxim Levitsky wrote:
> > On Mon, 2008-12-29 at 19:19 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > According to the ACPI specification the SCI_EN flag is controlled by
> > > the hardware, which sets this flag to inform the kernel that ACPI is
> > > enabled.  For this reason, we shouldn't try to modify SCI_EN
> > > directly.  Also, we don't need to do it in irqrouter_resume(), since
> > > lower-level resume code takes care of enabling ACPI in case it hasn't
> > > been enabled by the BIOS before passing control to the kernel (which
> > > by the way is against the ACPI specification).
> > 
> > 
> > Hi, 
> 
> Hi,
> 
> > I am an unfortunate owner of an acer notebook ( aspire 5720 if this
> > matters) and it doesn't come back after second suspend, subsequently  I
> > figured out that bios doesn't pass control to linux at all, after second
> > resume, and moreover, a suspend to disk , 'fixes' this and enables me to
> > do another suspend/resume cycle. This is a poor man workaround I use
> > now.
> 
> I suspect there may be a problem with interrupts handling on your system.
> 
> There is http://bugzilla.kernel.org/show_bug.cgi?id=11698 describing a very
> similar (if not the same) issue.  Please attach a boot log and the contents
> of /proc/interrupts to that bug.
> 
> > I already told about this here, and many peoples from here (including
> > you) tried to help me, but unfortunately this is very weird issue, and
> > nether me nor you can do much about, it is all about guesswork,
> > something, at least something is 'armed' on resume, so it confuses bios
> > then (it could be that yet, the critical part that confuses bios happens
> > in second suspend time)
> > 
> > Needless to say that nothing useful appears in the logs, and everything
> > seems to work fine following each successful resume (both disk and ram).
> > 
> > so I have a question, do you have a git branch to pull that includes all
> > patches like this to test, so I build it weekly, and who knows, maybe
> > something like above will fix it.
> 
> Unfortunately, I don't.  If I have any patches that can possibly fix the
> problem, I'll let you know.
>  
> > Anyway I would gladly test any patch that you suspect might be involved
> > here.
> > 
> > Also, I tried to do a I/O port and PCI config dump before and after a
> > resume, aka in 'armed' and 'unarmed' state.
> > 
> > While there were many I/O port registers changes, and I yet to process
> > it bit after bit (and unfortunately there are plenty of undocumented
> > stuff), I noticed a meaningful change between bad and good state:
> 
> Please check if the appended patch from Len changes anything.
> 
> Thanks,
> Rafael
> 
> ---
> Author: Len Brown <len.brown@intel.com>
> Date:   Sat Dec 20 12:50:23 2008 +0100
> 
>     ACPI: PCI Interrupt Links -- disable when unused
>     
>     We start the system off with disabled links.
>     Here we enable the reference counting
>     on the links so that when drivers free IRQs
>     the links can return to the disabled state.
>     
>     Drivers free their IRQ and reference on a link
>     when they unload.  They may also do so upon
>     suspend.  However, if they don't, irqrouter_resume()
>     will still resume any links that were
>     not disabled before suspend.
>     
>     Signed-off-by: Len Brown <len.brown@intel.com>
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 595b131..0f7722d 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -693,18 +693,7 @@ int acpi_pci_link_free_irq(acpi_handle handle)
>  		printk(KERN_ERR PREFIX "Link isn't initialized\n");
>  		return -1;
>  	}
> -#ifdef	FUTURE_USE
> -	/*
> -	 * The Link reference count allows us to _DISable an unused link
> -	 * and suspend time, and set it again  on resume.
> -	 * However, 2.6.12 still has irq_router.resume
> -	 * which blindly restores the link state.
> -	 * So we disable the reference count method
> -	 * to prevent duplicate acpi_pci_link_set()
> -	 * which would harm some systems
> -	 */
>  	link->refcnt--;
> -#endif
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			  "Link %s is dereferenced %d\n",
>  			  acpi_device_bid(link->device), link->refcnt));
> 

As usual, same hang after resume.
Just to note, this isn't a regression, it even appears in 2.6.19.


Best regards,
	Maxim Levitsky

Re: [PATCH] ACPI: Do not modify SCI_EN directly

[Len Brown]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1376 bytes --]

applied

-- Len Brown, Intel Open Source Technology Center

On Mon, 29 Dec 2008, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> According to the ACPI specification the SCI_EN flag is controlled by
> the hardware, which sets this flag to inform the kernel that ACPI is
> enabled.  For this reason, we shouldn't try to modify SCI_EN
> directly.  Also, we don't need to do it in irqrouter_resume(), since
> lower-level resume code takes care of enabling ACPI in case it hasn't
> been enabled by the BIOS before passing control to the kernel (which
> by the way is against the ACPI specification).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/pci_link.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/pci_link.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_link.c
> +++ linux-2.6/drivers/acpi/pci_link.c
> @@ -793,10 +793,6 @@ static int irqrouter_resume(struct sys_d
>  	struct list_head *node = NULL;
>  	struct acpi_pci_link *link = NULL;
>  
> -
> -	/* Make sure SCI is enabled again (Apple firmware bug?) */
> -	acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1);
> -
>  	list_for_each(node, &acpi_link.entries) {
>  		link = list_entry(node, struct acpi_pci_link, node);
>  		if (!link) {
> --8323328-415918257-1230681806=:17126--