Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Len Brown]

> I've hit the following oops on boot on an XO-1 with current linus git:
> 
> [    0.662755] BUG: unable to handle kernel NULL pointer dereference at 00000024
> [    0.666349] IP: [<c11a0514>] apei_hest_parse+0xbc/0xcc
> [    0.666349] *pde = 00000000
> [    0.666349] Oops: 0000 [#1] SMP
> [    0.666349] last sysfs file:
> [    0.666349] Modules linked in:
> [    0.666349]
> [    0.666349] Pid: 1, comm: swapper Not tainted 2.6.37+ #74 /
> [    0.666349] EIP: 0060:[<c11a0514>] EFLAGS: 00010206 CPU: 0
> [    0.666349] EIP is at apei_hest_parse+0xbc/0xcc
> [    0.666349] EAX: 00000028 EBX: 00000000 ECX: 00000000 EDX: 00000000
> [    0.666349] ESI: 00000001 EDI: 00000000 EBP: c1161171 ESP: cb441fa8
> [    0.666349]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> [    0.666349] Process swapper (pid: 1, ti=cb440000 task=cb43c000 task.ti=cb440000)
> [    0.666349] Stack:
> [    0.666349]  c1450960 00000001 00000020 c141fa10 c11611aa c141fa1e c1001151 00000000
> [    0.666349]  c1450960 00000001 00000020 00000000 c1401346 00000000 c1401221 00000000
> [    0.666349]  c10034be 00000000 00000000 00000000 00000000 00000000
> [    0.666349] Call Trace:
> [    0.666349]  [<c141fa10>] ? aer_service_init+0x0/0x22
> [    0.666349]  [<c11611aa>] ? aer_acpi_firmware_first+0x15/0x22
> [    0.666349]  [<c141fa1e>] ? aer_service_init+0xe/0x22
> [    0.666349]  [<c1001151>] ? do_one_initcall+0x68/0x10f
> [    0.666349]  [<c1401346>] ? kernel_init+0x125/0x19d
> [    0.666349]  [<c1401221>] ? kernel_init+0x0/0x19d
> [    0.666349]  [<c10034be>] ? kernel_thread_helper+0x6/0x10
> [    0.666349] Code: 76 18 0f b7 40 02 50 68 c2 ae 35 c1 e8 3e e8 0f 00 b8 ea ff ff ff 59 5b eb 1c 89 fa ff d5 85 c0 75 14 89 f0 43 8b 0d 50 ed 3f c1 <3b> 59
>  24 0f 82 64 ff ff ff 31 c0 5b 5e 5f 5d c3 8b 50 0c 8d 4a
> 
> The apei_hest_parse function is using hest_tab, which hasn't been initialized.
> This is because acpi_hest_init has never been called (as acpi_pci_disabled is 1,
> so it never gets called from acpi_pci_root_init), and hest_disabled is never
> set to 1.  When the aer_driver calls apei_hest_parse, the hest_disabled check
> passes, and it oopses processing hest_tab.  Note that the aer code that
> calls apei_hest_parse was added in commit b22c3d82.
> 
> This patch causes apei_hest_parse to check both hest_disabled and
> acpi_pci_disabled before continuing.  With it, the XO-1 boots properly.

The X0-1 has no ACPI support, and thus you are running
with acpi_disabled=1, yes?

It would be better for the fix to depend on acpi_disabled.
acpi_disbled=1 is a supported configuration, while
acpi_pci_disabled is a just a debug flag, maybe someday
it will go away.

Looks like acpi_hest_init() bails for acpi_disabled,
and thus hest_ghes_dev_register()doens't run and invoke
apei_hest_parse(), so we are set there.

Seems like drivers/pci/pcie/aer/aerdrv_acpi.c,
should not invoke apei_hest_parse() when acpi_disabled=1.
Either that or we have to have apei_hest_parse(),
which is exported to modules check acpi_disabled --
and its callers need to check for failure.

thanks,
-Len Brown, Intel Open Source Technolgy Center

> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/acpi/apei/hest.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
> index 4ee58e7..2b8c355 100644
> --- a/drivers/acpi/apei/hest.c
> +++ b/drivers/acpi/apei/hest.c
> @@ -89,7 +89,7 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
>  	struct acpi_hest_header *hest_hdr;
>  	int i, rc, len;
>  
> -	if (hest_disable)
> +	if (hest_disable || acpi_pci_disabled)
>  		return -EINVAL;
>  
>  	hest_hdr = (struct acpi_hest_header *)(hest_tab + 1);
> -- 
> 1.7.2.3
> 

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Andres Salomon]

On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
Len Brown <lenb@kernel.org> wrote:

[...]
> > This patch causes apei_hest_parse to check both hest_disabled and
> > acpi_pci_disabled before continuing.  With it, the XO-1 boots
> > properly.
> 
> The X0-1 has no ACPI support, and thus you are running
> with acpi_disabled=1, yes?

The XO-1 has no ACPI support, but ACPI support is enabled in the kernel
(as the XO-1.5 does have ACPI support, and the same kernel is used
between both) and I'm not passing any arguments to the kernel regarding
it. The kernel's ACPI code detects that it's not there and disables it
(the kernel message that's seen is "ACPI: Interpreter disabled.")

This is what sets apci_pci_disabled to 1; I'm not doing it manually.

> 
> It would be better for the fix to depend on acpi_disabled.
> acpi_disbled=1 is a supported configuration, while
> acpi_pci_disabled is a just a debug flag, maybe someday
> it will go away.

Ah, ok.  The reason I went with acpi_pci_disabled is because that's
acpi_pci_root_init() checks, and that's what specifies whether or not
acpi_hest_init gets called.. but it's all the same to me. :)

> 
> Looks like acpi_hest_init() bails for acpi_disabled,
> and thus hest_ghes_dev_register()doens't run and invoke
> apei_hest_parse(), so we are set there.
> 
> Seems like drivers/pci/pcie/aer/aerdrv_acpi.c,
> should not invoke apei_hest_parse() when acpi_disabled=1.
> Either that or we have to have apei_hest_parse(),
> which is exported to modules check acpi_disabled --
> and its callers need to check for failure.

>From a safety standpoint, I'd prefer to have apei_hest_parse
check for acpi_disabled, rather than relying on clients to know to
check for it.  Here's an updated patch.





From: Andres Salomon <dilinger@queued.net>

I've hit the following oops on boot on an XO-1:

[    0.662755] BUG: unable to handle kernel NULL pointer dereference at 00000024
[    0.666349] IP: [<c11a0514>] apei_hest_parse+0xbc/0xcc
[    0.666349] *pde = 00000000
[    0.666349] Oops: 0000 [#1] SMP
[    0.666349] last sysfs file:
[    0.666349] Modules linked in:
[    0.666349]
[    0.666349] Pid: 1, comm: swapper Not tainted 2.6.37+ #74 /
[    0.666349] EIP: 0060:[<c11a0514>] EFLAGS: 00010206 CPU: 0
[    0.666349] EIP is at apei_hest_parse+0xbc/0xcc
[    0.666349] EAX: 00000028 EBX: 00000000 ECX: 00000000 EDX: 00000000
[    0.666349] ESI: 00000001 EDI: 00000000 EBP: c1161171 ESP: cb441fa8
[    0.666349]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[    0.666349] Process swapper (pid: 1, ti=cb440000 task=cb43c000 task.ti=cb440000)
[    0.666349] Stack:
[    0.666349]  c1450960 00000001 00000020 c141fa10 c11611aa c141fa1e c1001151 00000000
[    0.666349]  c1450960 00000001 00000020 00000000 c1401346 00000000 c1401221 00000000
[    0.666349]  c10034be 00000000 00000000 00000000 00000000 00000000
[    0.666349] Call Trace:
[    0.666349]  [<c141fa10>] ? aer_service_init+0x0/0x22
[    0.666349]  [<c11611aa>] ? aer_acpi_firmware_first+0x15/0x22
[    0.666349]  [<c141fa1e>] ? aer_service_init+0xe/0x22
[    0.666349]  [<c1001151>] ? do_one_initcall+0x68/0x10f
[    0.666349]  [<c1401346>] ? kernel_init+0x125/0x19d
[    0.666349]  [<c1401221>] ? kernel_init+0x0/0x19d
[    0.666349]  [<c10034be>] ? kernel_thread_helper+0x6/0x10
[    0.666349] Code: 76 18 0f b7 40 02 50 68 c2 ae 35 c1 e8 3e e8 0f 00 b8 ea ff ff ff 59 5b eb 1c 89 fa ff d5 85 c0 75 14 89 f0 43 8b 0d 50 ed 3f c1 <3b> 59
 24 0f 82 64 ff ff ff 31 c0 5b 5e 5f 5d c3 8b 50 0c 8d 4a

The apei_hest_parse function is using hest_tab, which hasn't been initialized.
This is because acpi_hest_init has never been called (as acpi_pci_disabled is 1,
so it never gets called from acpi_pci_root_init), and hest_disabled is never
set to 1.  When the aer_driver calls apei_hest_parse, the hest_disabled check
passes, and it oopses processing hest_tab.

This patch causes apei_hest_parse to check both hest_disabled and
acpi_disabled before continuing.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/acpi/apei/hest.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 4ee58e7..7262eaf 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -89,7 +89,7 @@ int apei_hest_parse(apei_hest_func_t func, void *data)
 	struct acpi_hest_header *hest_hdr;
 	int i, rc, len;
 
-	if (hest_disable)
+	if (hest_disable || acpi_disabled)
 		return -EINVAL;
 
 	hest_hdr = (struct acpi_hest_header *)(hest_tab + 1);
-- 
1.7.2.3

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Rafael J. Wysocki]

On Saturday, January 15, 2011, Andres Salomon wrote:
> On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> Len Brown <lenb@kernel.org> wrote:
> 
> [...]
> > > This patch causes apei_hest_parse to check both hest_disabled and
> > > acpi_pci_disabled before continuing.  With it, the XO-1 boots
> > > properly.
> > 
> > The X0-1 has no ACPI support, and thus you are running
> > with acpi_disabled=1, yes?
> 
> The XO-1 has no ACPI support, but ACPI support is enabled in the kernel
> (as the XO-1.5 does have ACPI support, and the same kernel is used
> between both) and I'm not passing any arguments to the kernel regarding
> it. The kernel's ACPI code detects that it's not there and disables it
> (the kernel message that's seen is "ACPI: Interpreter disabled.")
> 
> This is what sets apci_pci_disabled to 1; I'm not doing it manually.

Hmm.  Does the appended patch help instead?

Rafael

---
 drivers/acpi/pci_root.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
 
 static int __init acpi_pci_root_init(void)
 {
+	acpi_hest_init();
+
 	if (acpi_pci_disabled)
 		return 0;
 
-	acpi_hest_init();
-
 	pci_acpi_crs_quirks();
 	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
 		return -ENODEV;

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Andres Salomon]

On Sat, 15 Jan 2011 14:01:18 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Saturday, January 15, 2011, Andres Salomon wrote:
> > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > Len Brown <lenb@kernel.org> wrote:
> > 
> > [...]
> > > > This patch causes apei_hest_parse to check both hest_disabled
> > > > and acpi_pci_disabled before continuing.  With it, the XO-1
> > > > boots properly.
> > > 
> > > The X0-1 has no ACPI support, and thus you are running
> > > with acpi_disabled=1, yes?
> > 
> > The XO-1 has no ACPI support, but ACPI support is enabled in the
> > kernel (as the XO-1.5 does have ACPI support, and the same kernel
> > is used between both) and I'm not passing any arguments to the
> > kernel regarding it. The kernel's ACPI code detects that it's not
> > there and disables it (the kernel message that's seen is "ACPI:
> > Interpreter disabled.")
> > 
> > This is what sets apci_pci_disabled to 1; I'm not doing it manually.
> 
> Hmm.  Does the appended patch help instead?

Nope.  acpi_hest_init returns immediately if acpi_disabled, and thus
never sets hest_disable.  You either need to set hest_disable, or
ensure apei_hest_parse checks more than just hest_disable.




> 
> Rafael
> 
> ---
>  drivers/acpi/pci_root.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6/drivers/acpi/pci_root.c
> @@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
>  
>  static int __init acpi_pci_root_init(void)
>  {
> +	acpi_hest_init();
> +
>  	if (acpi_pci_disabled)
>  		return 0;
>  
> -	acpi_hest_init();
> -
>  	pci_acpi_crs_quirks();
>  	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
>  		return -ENODEV;

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Rafael J. Wysocki]

On Saturday, January 15, 2011, Andres Salomon wrote:
> On Sat, 15 Jan 2011 14:01:18 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > > Len Brown <lenb@kernel.org> wrote:
> > > 
> > > [...]
> > > > > This patch causes apei_hest_parse to check both hest_disabled
> > > > > and acpi_pci_disabled before continuing.  With it, the XO-1
> > > > > boots properly.
> > > > 
> > > > The X0-1 has no ACPI support, and thus you are running
> > > > with acpi_disabled=1, yes?
> > > 
> > > The XO-1 has no ACPI support, but ACPI support is enabled in the
> > > kernel (as the XO-1.5 does have ACPI support, and the same kernel
> > > is used between both) and I'm not passing any arguments to the
> > > kernel regarding it. The kernel's ACPI code detects that it's not
> > > there and disables it (the kernel message that's seen is "ACPI:
> > > Interpreter disabled.")
> > > 
> > > This is what sets apci_pci_disabled to 1; I'm not doing it manually.
> > 
> > Hmm.  Does the appended patch help instead?
> 
> Nope.  acpi_hest_init returns immediately if acpi_disabled, and thus
> never sets hest_disable.  You either need to set hest_disable, or
> ensure apei_hest_parse checks more than just hest_disable.

OK, so the new one below should help, right?

Thanks,
Rafael

---
 drivers/acpi/apei/hest.c |    4 +++-
 drivers/acpi/pci_root.c  |    4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
 
 static int __init acpi_pci_root_init(void)
 {
+	acpi_hest_init();
+
 	if (acpi_pci_disabled)
 		return 0;
 
-	acpi_hest_init();
-
 	pci_acpi_crs_quirks();
 	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
 		return -ENODEV;
Index: linux-2.6/drivers/acpi/apei/hest.c
===================================================================
--- linux-2.6.orig/drivers/acpi/apei/hest.c
+++ linux-2.6/drivers/acpi/apei/hest.c
@@ -201,8 +201,10 @@ void __init acpi_hest_init(void)
 	int rc = -ENODEV;
 	unsigned int ghes_count = 0;
 
-	if (acpi_disabled)
+	if (acpi_disabled) {
+		hest_disable = 1;
 		return;
+	}
 
 	if (hest_disable) {
 		pr_info(HEST_PFX "Table parsing disabled.\n");

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Andres Salomon]

On Sat, 15 Jan 2011 21:38:10 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Saturday, January 15, 2011, Andres Salomon wrote:
> > On Sat, 15 Jan 2011 14:01:18 +0100
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > > > Len Brown <lenb@kernel.org> wrote:
> > > > 
> > > > [...]
> > > > > > This patch causes apei_hest_parse to check both
> > > > > > hest_disabled and acpi_pci_disabled before continuing.
> > > > > > With it, the XO-1 boots properly.
> > > > > 
> > > > > The X0-1 has no ACPI support, and thus you are running
> > > > > with acpi_disabled=1, yes?
> > > > 
> > > > The XO-1 has no ACPI support, but ACPI support is enabled in the
> > > > kernel (as the XO-1.5 does have ACPI support, and the same
> > > > kernel is used between both) and I'm not passing any arguments
> > > > to the kernel regarding it. The kernel's ACPI code detects that
> > > > it's not there and disables it (the kernel message that's seen
> > > > is "ACPI: Interpreter disabled.")
> > > > 
> > > > This is what sets apci_pci_disabled to 1; I'm not doing it
> > > > manually.
> > > 
> > > Hmm.  Does the appended patch help instead?
> > 
> > Nope.  acpi_hest_init returns immediately if acpi_disabled, and thus
> > never sets hest_disable.  You either need to set hest_disable, or
> > ensure apei_hest_parse checks more than just hest_disable.
> 
> OK, so the new one below should help, right?

Yes, that looks better (though you probably just want to use 'goto
err;' instead). I can test it later today.


> 
> Thanks,
> Rafael
> 
> ---
>  drivers/acpi/apei/hest.c |    4 +++-
>  drivers/acpi/pci_root.c  |    4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6/drivers/acpi/pci_root.c
> @@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
>  
>  static int __init acpi_pci_root_init(void)
>  {
> +	acpi_hest_init();
> +
>  	if (acpi_pci_disabled)
>  		return 0;
>  
> -	acpi_hest_init();
> -
>  	pci_acpi_crs_quirks();
>  	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
>  		return -ENODEV;
> Index: linux-2.6/drivers/acpi/apei/hest.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/apei/hest.c
> +++ linux-2.6/drivers/acpi/apei/hest.c
> @@ -201,8 +201,10 @@ void __init acpi_hest_init(void)
>  	int rc = -ENODEV;
>  	unsigned int ghes_count = 0;
>  
> -	if (acpi_disabled)
> +	if (acpi_disabled) {
> +		hest_disable = 1;
>  		return;
> +	}
>  
>  	if (hest_disable) {
>  		pr_info(HEST_PFX "Table parsing disabled.\n");
> --
> 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
> 

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Rafael J. Wysocki]

On Saturday, January 15, 2011, Andres Salomon wrote:
> On Sat, 15 Jan 2011 21:38:10 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > On Sat, 15 Jan 2011 14:01:18 +0100
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > 
> > > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > > > > Len Brown <lenb@kernel.org> wrote:
> > > > > 
> > > > > [...]
> > > > > > > This patch causes apei_hest_parse to check both
> > > > > > > hest_disabled and acpi_pci_disabled before continuing.
> > > > > > > With it, the XO-1 boots properly.
> > > > > > 
> > > > > > The X0-1 has no ACPI support, and thus you are running
> > > > > > with acpi_disabled=1, yes?
> > > > > 
> > > > > The XO-1 has no ACPI support, but ACPI support is enabled in the
> > > > > kernel (as the XO-1.5 does have ACPI support, and the same
> > > > > kernel is used between both) and I'm not passing any arguments
> > > > > to the kernel regarding it. The kernel's ACPI code detects that
> > > > > it's not there and disables it (the kernel message that's seen
> > > > > is "ACPI: Interpreter disabled.")
> > > > > 
> > > > > This is what sets apci_pci_disabled to 1; I'm not doing it
> > > > > manually.
> > > > 
> > > > Hmm.  Does the appended patch help instead?
> > > 
> > > Nope.  acpi_hest_init returns immediately if acpi_disabled, and thus
> > > never sets hest_disable.  You either need to set hest_disable, or
> > > ensure apei_hest_parse checks more than just hest_disable.
> > 
> > OK, so the new one below should help, right?
> 
> Yes, that looks better

OK

> (though you probably just want to use 'goto err;' instead).

Indeed.

> I can test it later today.

Cool, thanks.  Please try the version below with the 'goto'.

Rafael

---
 drivers/acpi/apei/hest.c |    6 +++---
 drivers/acpi/pci_root.c  |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root.c
+++ linux-2.6/drivers/acpi/pci_root.c
@@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
 
 static int __init acpi_pci_root_init(void)
 {
+	acpi_hest_init();
+
 	if (acpi_pci_disabled)
 		return 0;
 
-	acpi_hest_init();
-
 	pci_acpi_crs_quirks();
 	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
 		return -ENODEV;
Index: linux-2.6/drivers/acpi/apei/hest.c
===================================================================
--- linux-2.6.orig/drivers/acpi/apei/hest.c
+++ linux-2.6/drivers/acpi/apei/hest.c
@@ -201,14 +201,14 @@ void __init acpi_hest_init(void)
 	int rc = -ENODEV;
 	unsigned int ghes_count = 0;
 
-	if (acpi_disabled)
-		return;
-
 	if (hest_disable) {
 		pr_info(HEST_PFX "Table parsing disabled.\n");
 		return;
 	}
 
+	if (acpi_disabled)
+		goto err;
+
 	status = acpi_get_table(ACPI_SIG_HEST, 0,
 				(struct acpi_table_header **)&hest_tab);
 	if (status == AE_NOT_FOUND) {

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Andres Salomon]

On Sat, 15 Jan 2011 21:48:43 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Saturday, January 15, 2011, Andres Salomon wrote:
> > On Sat, 15 Jan 2011 21:38:10 +0100
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > On Sat, 15 Jan 2011 14:01:18 +0100
> > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > > 
> > > > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > > > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > > > > > Len Brown <lenb@kernel.org> wrote:
> > > > > > 
> > > > > > [...]
> > > > > > > > This patch causes apei_hest_parse to check both
> > > > > > > > hest_disabled and acpi_pci_disabled before continuing.
> > > > > > > > With it, the XO-1 boots properly.
> > > > > > > 
> > > > > > > The X0-1 has no ACPI support, and thus you are running
> > > > > > > with acpi_disabled=1, yes?
> > > > > > 
> > > > > > The XO-1 has no ACPI support, but ACPI support is enabled
> > > > > > in the kernel (as the XO-1.5 does have ACPI support, and
> > > > > > the same kernel is used between both) and I'm not passing
> > > > > > any arguments to the kernel regarding it. The kernel's ACPI
> > > > > > code detects that it's not there and disables it (the
> > > > > > kernel message that's seen is "ACPI: Interpreter disabled.")
> > > > > > 
> > > > > > This is what sets apci_pci_disabled to 1; I'm not doing it
> > > > > > manually.
> > > > > 
> > > > > Hmm.  Does the appended patch help instead?
> > > > 
> > > > Nope.  acpi_hest_init returns immediately if acpi_disabled, and
> > > > thus never sets hest_disable.  You either need to set
> > > > hest_disable, or ensure apei_hest_parse checks more than just
> > > > hest_disable.
> > > 
> > > OK, so the new one below should help, right?
> > 
> > Yes, that looks better
> 
> OK
> 
> > (though you probably just want to use 'goto err;' instead).
> 
> Indeed.
> 
> > I can test it later today.
> 
> Cool, thanks.  Please try the version below with the 'goto'.
> 
> Rafael


Yep, just tested it out; it works fine.


> 
> ---
>  drivers/acpi/apei/hest.c |    6 +++---
>  drivers/acpi/pci_root.c  |    4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/pci_root.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/pci_root.c
> +++ linux-2.6/drivers/acpi/pci_root.c
> @@ -631,11 +631,11 @@ static int acpi_pci_root_remove(struct a
>  
>  static int __init acpi_pci_root_init(void)
>  {
> +	acpi_hest_init();
> +
>  	if (acpi_pci_disabled)
>  		return 0;
>  
> -	acpi_hest_init();
> -
>  	pci_acpi_crs_quirks();
>  	if (acpi_bus_register_driver(&acpi_pci_root_driver) < 0)
>  		return -ENODEV;
> Index: linux-2.6/drivers/acpi/apei/hest.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/apei/hest.c
> +++ linux-2.6/drivers/acpi/apei/hest.c
> @@ -201,14 +201,14 @@ void __init acpi_hest_init(void)
>  	int rc = -ENODEV;
>  	unsigned int ghes_count = 0;
>  
> -	if (acpi_disabled)
> -		return;
> -
>  	if (hest_disable) {
>  		pr_info(HEST_PFX "Table parsing disabled.\n");
>  		return;
>  	}
>  
> +	if (acpi_disabled)
> +		goto err;
> +
>  	status = acpi_get_table(ACPI_SIG_HEST, 0,
>  				(struct acpi_table_header
> **)&hest_tab); if (status == AE_NOT_FOUND) {

Re: [PATCH] ACPI: don't attempt to use hest_tab unless !acpi_pci_disabled

[Rafael J. Wysocki]

On Sunday, January 16, 2011, Andres Salomon wrote:
> On Sat, 15 Jan 2011 21:48:43 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > On Sat, 15 Jan 2011 21:38:10 +0100
> > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > 
> > > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > > On Sat, 15 Jan 2011 14:01:18 +0100
> > > > > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > > > > 
> > > > > > On Saturday, January 15, 2011, Andres Salomon wrote:
> > > > > > > On Fri, 14 Jan 2011 23:24:27 -0500 (EST)
> > > > > > > Len Brown <lenb@kernel.org> wrote:
> > > > > > > 
> > > > > > > [...]
> > > > > > > > > This patch causes apei_hest_parse to check both
> > > > > > > > > hest_disabled and acpi_pci_disabled before continuing.
> > > > > > > > > With it, the XO-1 boots properly.
> > > > > > > > 
> > > > > > > > The X0-1 has no ACPI support, and thus you are running
> > > > > > > > with acpi_disabled=1, yes?
> > > > > > > 
> > > > > > > The XO-1 has no ACPI support, but ACPI support is enabled
> > > > > > > in the kernel (as the XO-1.5 does have ACPI support, and
> > > > > > > the same kernel is used between both) and I'm not passing
> > > > > > > any arguments to the kernel regarding it. The kernel's ACPI
> > > > > > > code detects that it's not there and disables it (the
> > > > > > > kernel message that's seen is "ACPI: Interpreter disabled.")
> > > > > > > 
> > > > > > > This is what sets apci_pci_disabled to 1; I'm not doing it
> > > > > > > manually.
> > > > > > 
> > > > > > Hmm.  Does the appended patch help instead?
> > > > > 
> > > > > Nope.  acpi_hest_init returns immediately if acpi_disabled, and
> > > > > thus never sets hest_disable.  You either need to set
> > > > > hest_disable, or ensure apei_hest_parse checks more than just
> > > > > hest_disable.
> > > > 
> > > > OK, so the new one below should help, right?
> > > 
> > > Yes, that looks better
> > 
> > OK
> > 
> > > (though you probably just want to use 'goto err;' instead).
> > 
> > Indeed.
> > 
> > > I can test it later today.
> > 
> > Cool, thanks.  Please try the version below with the 'goto'.
> > 
> > Rafael
> 
> 
> Yep, just tested it out; it works fine.

Great, thanks!

Rafael