* Re: [BUG bisected]: apei_hest_parse explosion [not found] ` <CAE9FiQXV43emyjjCb=D=xFLz-n6q0Eu0uQmVhAA470A=FGRhww@mail.gmail.com> @ 2013-02-22 1:40 ` Rafael J. Wysocki 2013-02-22 2:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2013-02-22 1:40 UTC (permalink / raw) To: Yinghai Lu Cc: Thomas Gleixner, LKML, Rafael J. Wysocki, Toshi Kani, Huang Ying, ACPI Devel Maling List On Thursday, February 21, 2013 05:23:19 PM Yinghai Lu wrote: > On Thu, Feb 21, 2013 at 4:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Friday, February 22, 2013 12:56:01 AM Thomas Gleixner wrote: > >> Just booted linus head on one of my old kvm based testing systems and > >> got the following splat: > >> > >> [ 0.531910] BUG: unable to handle kernel NULL pointer dereference at 0000000000000024 > >> [ 0.532173] IP: [<ffffffff812f1bd2>] apei_hest_parse+0xd2/0xf0 > >> [ 0.532173] PGD 0 > >> [ 0.532173] Oops: 0000 [#1] PREEMPT SMP > >> [ 0.532173] Modules linked in: > >> [ 0.532173] CPU 3 > >> [ 0.532173] Pid: 1, comm: swapper/0 Not tainted 3.8.0+ #195 Bochs Bochs > >> [ 0.532173] RIP: 0010:[<ffffffff812f1bd2>] [<ffffffff812f1bd2>] apei_hest_parse+0xd2/0xf0 > >> [ 0.532173] RSP: 0000:ffff88021685be78 EFLAGS: 00010206 > >> [ 0.532173] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > >> [ 0.532173] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000028 > >> [ 0.532173] RBP: ffff88021685be98 R08: 00000000804efb06 R09: 0000000000000000 > >> [ 0.532173] R10: 0000000000000001 R11: 0000000000000000 R12: ffffffff81af3d40 > >> [ 0.532173] R13: ffffffff812bcb23 R14: 0000000000000000 R15: 0000000000000000 > >> [ 0.532173] FS: 0000000000000000(0000) GS:ffff88021fcc0000(0000) knlGS:0000000000000000 > >> [ 0.532173] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > >> [ 0.532173] CR2: 0000000000000024 CR3: 0000000001a0c000 CR4: 00000000000406a0 > >> [ 0.532173] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > >> [ 0.532173] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > >> [ 0.532173] Process swapper/0 (pid: 1, threadinfo ffff88021685a000, task ffff8802168c0000) > >> [ 0.532173] Stack: > >> [ 0.532173] 00000000fffffffa ffffffff81af3d40 0000000000000000 000000001fb3f55a > >> [ 0.532173] ffff88021685bea8 ffffffff812bcbe4 ffff88021685bec8 ffffffff81af3d5c > >> [ 0.532173] 0000000000000030 0000000000000030 ffff88021685bef8 ffffffff8100023b > >> [ 0.532173] Call Trace: > >> [ 0.532173] [<ffffffff81af3d40>] ? pcie_portdrv_init+0x7a/0x7a > >> [ 0.532173] [<ffffffff812bcbe4>] aer_acpi_firmware_first+0x20/0x30 > >> [ 0.532173] [<ffffffff81af3d5c>] aer_service_init+0x1c/0x35 > >> [ 0.532173] [<ffffffff8100023b>] do_one_initcall+0x57/0x13c > >> [ 0.532173] [<ffffffff81ad2d39>] kernel_init_freeable+0x142/0x1d1 > >> [ 0.532173] [<ffffffff81ad254f>] ? loglevel+0x31/0x31 > >> [ 0.532173] [<ffffffff815a8cef>] ? rest_init+0xc3/0xc3 > >> [ 0.532173] [<ffffffff815a8cfd>] kernel_init+0xe/0xd6 > >> [ 0.532173] [<ffffffff815bc73c>] ret_from_fork+0x7c/0xb0 > >> [ 0.532173] [<ffffffff815a8cef>] ? rest_init+0xc3/0xc3 > >> [ 0.532173] Code: 14 0f b7 77 02 31 c0 48 c7 c7 65 c1 83 81 e8 35 1a 2c 00 eb 23 4c 89 f6 41 ff d5 85 c0 75 1e 4c 89 e7 ff c3 48 8b 05 a6 5b 7c 00 <3b> 58 24 0f 82 5a ff ff ff 31 c0 eb 05 b8 ea ff ff ff 5b 41 5c > >> > >> git bisect identified the following commit: > >> > >> commit 92ef2a25c763338905dce8344a0584606f842920 > >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> Date: Fri Dec 21 00:36:40 2012 +0100 > >> > >> ACPI: Change the ordering of PCI root bridge driver registrarion > >> > >> A revert is impossible w/o major surgery, so I'm going to bed and let > >> the involved folks think about it. > > > > Can you please post a dmesg boot log from v3.8 on the same system? > > > > looks at the code flow, acpi_hest_init() is called early correctly. > > later aer_acpi_firmware_first() should not hit hest_tab is NULL, but > hest_disable is not set. > > could be some data corruption.. near hest_disable or hest_tab. Yes, that's really odd. It looks like the hest_tab memory mapping is unmapped between acpi_hest_init() and aer_acpi_firmware_first(), but I have no idea what may be responsible for that. And the only relevant difference between now and before the commit above seems to be the change of the acpi_hest_init() ordering (which now is called earlier). Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 1:40 ` [BUG bisected]: apei_hest_parse explosion Rafael J. Wysocki @ 2013-02-22 2:19 ` Rafael J. Wysocki 2013-02-22 8:22 ` Thomas Gleixner 0 siblings, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2013-02-22 2:19 UTC (permalink / raw) To: Yinghai Lu, Thomas Gleixner Cc: LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Friday, February 22, 2013 02:40:58 AM Rafael J. Wysocki wrote: > On Thursday, February 21, 2013 05:23:19 PM Yinghai Lu wrote: > > On Thu, Feb 21, 2013 at 4:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Friday, February 22, 2013 12:56:01 AM Thomas Gleixner wrote: > > >> Just booted linus head on one of my old kvm based testing systems and > > >> got the following splat: > > >> > > >> [ 0.531910] BUG: unable to handle kernel NULL pointer dereference at 0000000000000024 > > >> [ 0.532173] IP: [<ffffffff812f1bd2>] apei_hest_parse+0xd2/0xf0 > > >> [ 0.532173] PGD 0 > > >> [ 0.532173] Oops: 0000 [#1] PREEMPT SMP > > >> [ 0.532173] Modules linked in: > > >> [ 0.532173] CPU 3 > > >> [ 0.532173] Pid: 1, comm: swapper/0 Not tainted 3.8.0+ #195 Bochs Bochs > > >> [ 0.532173] RIP: 0010:[<ffffffff812f1bd2>] [<ffffffff812f1bd2>] apei_hest_parse+0xd2/0xf0 > > >> [ 0.532173] RSP: 0000:ffff88021685be78 EFLAGS: 00010206 > > >> [ 0.532173] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 > > >> [ 0.532173] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000028 > > >> [ 0.532173] RBP: ffff88021685be98 R08: 00000000804efb06 R09: 0000000000000000 > > >> [ 0.532173] R10: 0000000000000001 R11: 0000000000000000 R12: ffffffff81af3d40 > > >> [ 0.532173] R13: ffffffff812bcb23 R14: 0000000000000000 R15: 0000000000000000 > > >> [ 0.532173] FS: 0000000000000000(0000) GS:ffff88021fcc0000(0000) knlGS:0000000000000000 > > >> [ 0.532173] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > >> [ 0.532173] CR2: 0000000000000024 CR3: 0000000001a0c000 CR4: 00000000000406a0 > > >> [ 0.532173] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > > >> [ 0.532173] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 > > >> [ 0.532173] Process swapper/0 (pid: 1, threadinfo ffff88021685a000, task ffff8802168c0000) > > >> [ 0.532173] Stack: > > >> [ 0.532173] 00000000fffffffa ffffffff81af3d40 0000000000000000 000000001fb3f55a > > >> [ 0.532173] ffff88021685bea8 ffffffff812bcbe4 ffff88021685bec8 ffffffff81af3d5c > > >> [ 0.532173] 0000000000000030 0000000000000030 ffff88021685bef8 ffffffff8100023b > > >> [ 0.532173] Call Trace: > > >> [ 0.532173] [<ffffffff81af3d40>] ? pcie_portdrv_init+0x7a/0x7a > > >> [ 0.532173] [<ffffffff812bcbe4>] aer_acpi_firmware_first+0x20/0x30 > > >> [ 0.532173] [<ffffffff81af3d5c>] aer_service_init+0x1c/0x35 > > >> [ 0.532173] [<ffffffff8100023b>] do_one_initcall+0x57/0x13c > > >> [ 0.532173] [<ffffffff81ad2d39>] kernel_init_freeable+0x142/0x1d1 > > >> [ 0.532173] [<ffffffff81ad254f>] ? loglevel+0x31/0x31 > > >> [ 0.532173] [<ffffffff815a8cef>] ? rest_init+0xc3/0xc3 > > >> [ 0.532173] [<ffffffff815a8cfd>] kernel_init+0xe/0xd6 > > >> [ 0.532173] [<ffffffff815bc73c>] ret_from_fork+0x7c/0xb0 > > >> [ 0.532173] [<ffffffff815a8cef>] ? rest_init+0xc3/0xc3 > > >> [ 0.532173] Code: 14 0f b7 77 02 31 c0 48 c7 c7 65 c1 83 81 e8 35 1a 2c 00 eb 23 4c 89 f6 41 ff d5 85 c0 75 1e 4c 89 e7 ff c3 48 8b 05 a6 5b 7c 00 <3b> 58 24 0f 82 5a ff ff ff 31 c0 eb 05 b8 ea ff ff ff 5b 41 5c > > >> > > >> git bisect identified the following commit: > > >> > > >> commit 92ef2a25c763338905dce8344a0584606f842920 > > >> Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > >> Date: Fri Dec 21 00:36:40 2012 +0100 > > >> > > >> ACPI: Change the ordering of PCI root bridge driver registrarion > > >> > > >> A revert is impossible w/o major surgery, so I'm going to bed and let > > >> the involved folks think about it. > > > > > > Can you please post a dmesg boot log from v3.8 on the same system? > > > > > > > looks at the code flow, acpi_hest_init() is called early correctly. > > > > later aer_acpi_firmware_first() should not hit hest_tab is NULL, but > > hest_disable is not set. > > > > could be some data corruption.. near hest_disable or hest_tab. > > Yes, that's really odd. > > It looks like the hest_tab memory mapping is unmapped between acpi_hest_init() > and aer_acpi_firmware_first(), but I have no idea what may be responsible for > that. > > And the only relevant difference between now and before the commit above seems > to be the change of the acpi_hest_init() ordering (which now is called earlier). We actually don't really need to do that thing so early, I think. It looks like we only need to make it available early enough for the AER driver to be able to use it, so I wonder if moving the acpi_hest_init() to a separate subsys_initcall() will work around the problem. That is, something like the patch below. But even if this helps, I will be wanting to understand what's up here. Thanks, Rafael --- drivers/acpi/apei/hest.c | 17 +++++++++++------ drivers/acpi/pci_root.c | 2 -- include/acpi/apei.h | 6 ------ 3 files changed, 11 insertions(+), 14 deletions(-) Index: test/drivers/acpi/apei/hest.c =================================================================== --- test.orig/drivers/acpi/apei/hest.c +++ test/drivers/acpi/apei/hest.c @@ -205,7 +205,7 @@ static int __init setup_hest_disable(cha __setup("hest_disable", setup_hest_disable); -void __init acpi_hest_init(void) +static int __init acpi_hest_init(void) { acpi_status status; int rc = -ENODEV; @@ -213,7 +213,7 @@ void __init acpi_hest_init(void) if (hest_disable) { pr_info(HEST_PFX "Table parsing disabled.\n"); - return; + return 0; } if (acpi_disabled) @@ -221,9 +221,10 @@ void __init acpi_hest_init(void) status = acpi_get_table(ACPI_SIG_HEST, 0, (struct acpi_table_header **)&hest_tab); - if (status == AE_NOT_FOUND) + if (status == AE_NOT_FOUND) { + pr_info(HEST_PFX "Table not found.\n"); goto err; - else if (ACPI_FAILURE(status)) { + } else if (ACPI_FAILURE(status)) { const char *msg = acpi_format_exception(status); pr_err(HEST_PFX "Failed to get table, %s\n", msg); rc = -EINVAL; @@ -240,7 +241,11 @@ void __init acpi_hest_init(void) } pr_info(HEST_PFX "Table parsing has been initialized.\n"); - return; -err: + return 0; + + err: hest_disable = 1; + return rc; } + +subsys_initcall(acpi_hest_init); Index: test/drivers/acpi/pci_root.c =================================================================== --- test.orig/drivers/acpi/pci_root.c +++ test/drivers/acpi/pci_root.c @@ -656,8 +656,6 @@ static void acpi_pci_root_remove(struct void __init acpi_pci_root_init(void) { - acpi_hest_init(); - if (!acpi_pci_disabled) { pci_acpi_crs_quirks(); acpi_scan_add_handler(&pci_root_handler); Index: test/include/acpi/apei.h =================================================================== --- test.orig/include/acpi/apei.h +++ test/include/acpi/apei.h @@ -24,12 +24,6 @@ extern bool ghes_disable; #define ghes_disable 1 #endif -#ifdef CONFIG_ACPI_APEI -void __init acpi_hest_init(void); -#else -static inline void acpi_hest_init(void) { return; } -#endif - typedef int (*apei_hest_func_t)(struct acpi_hest_header *hest_hdr, void *data); int apei_hest_parse(apei_hest_func_t func, void *data); -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 2:19 ` Rafael J. Wysocki @ 2013-02-22 8:22 ` Thomas Gleixner 2013-02-22 12:30 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2013-02-22 8:22 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Yinghai Lu, LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Fri, 22 Feb 2013, Rafael J. Wysocki wrote: > On Friday, February 22, 2013 02:40:58 AM Rafael J. Wysocki wrote: > > It looks like the hest_tab memory mapping is unmapped between acpi_hest_init() > > and aer_acpi_firmware_first(), but I have no idea what may be responsible for > > that. > > > > And the only relevant difference between now and before the commit above seems > > to be the change of the acpi_hest_init() ordering (which now is called earlier). > > We actually don't really need to do that thing so early, I think. It looks like > we only need to make it available early enough for the AER driver to be able to > use it, so I wonder if moving the acpi_hest_init() to a separate > subsys_initcall() will work around the problem. That is, something like the > patch below. Yes, that makes the machine boot. > But even if this helps, I will be wanting to understand what's up here. It's very simple. I have "acpi=off" on the command line. With that acpi_hest_init is never called, so hest_disable is not set ..... Brilliant stuff that. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 8:22 ` Thomas Gleixner @ 2013-02-22 12:30 ` Rafael J. Wysocki 2013-02-22 14:39 ` Thomas Gleixner 2013-02-22 23:46 ` Yinghai Lu 0 siblings, 2 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2013-02-22 12:30 UTC (permalink / raw) To: Thomas Gleixner Cc: Yinghai Lu, LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Friday, February 22, 2013 09:22:15 AM Thomas Gleixner wrote: > On Fri, 22 Feb 2013, Rafael J. Wysocki wrote: > > On Friday, February 22, 2013 02:40:58 AM Rafael J. Wysocki wrote: > > > It looks like the hest_tab memory mapping is unmapped between acpi_hest_init() > > > and aer_acpi_firmware_first(), but I have no idea what may be responsible for > > > that. > > > > > > And the only relevant difference between now and before the commit above seems > > > to be the change of the acpi_hest_init() ordering (which now is called earlier). > > > > We actually don't really need to do that thing so early, I think. It looks like > > we only need to make it available early enough for the AER driver to be able to > > use it, so I wonder if moving the acpi_hest_init() to a separate > > subsys_initcall() will work around the problem. That is, something like the > > patch below. > > Yes, that makes the machine boot. Although for a reason I didn't think about. > > But even if this helps, I will be wanting to understand what's up here. > > It's very simple. I have "acpi=off" on the command line. With that > acpi_hest_init is never called, so hest_disable is not set ..... Well, that explains things (and means that acpi=off doesn't really get much test coverage these days). > Brilliant stuff that. The appended patch should fix the breakage too, can you please verify? Rafael --- From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Subject: ACPI / APEI: Fix crash in apei_hest_parse() for acpi=off After commit 92ef2a2 (ACPI: Change the ordering of PCI root bridge driver registrarion), acpi_hest_init() is never called for acpi=off (acpi_disabled), so hest_disable is not set, but hest_tab is NULL, which causes apei_hest_parse() to crash when it is called from aer_acpi_firmware_first(). Fix that by making apei_hest_parse() check if hest_tab is not NULL in addition to checking hest_disable. Also remove the now useless acpi_disabled check from apei_hest_parse(). Reported-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/acpi/apei/hest.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) Index: test/drivers/acpi/apei/hest.c =================================================================== --- test.orig/drivers/acpi/apei/hest.c +++ test/drivers/acpi/apei/hest.c @@ -89,7 +89,7 @@ int apei_hest_parse(apei_hest_func_t fun struct acpi_hest_header *hest_hdr; int i, rc, len; - if (hest_disable) + if (hest_disable || !hest_tab) return -EINVAL; hest_hdr = (struct acpi_hest_header *)(hest_tab + 1); @@ -216,9 +216,6 @@ void __init acpi_hest_init(void) 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) -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 12:30 ` Rafael J. Wysocki @ 2013-02-22 14:39 ` Thomas Gleixner 2013-02-22 21:00 ` Rafael J. Wysocki 2013-02-22 23:46 ` Yinghai Lu 1 sibling, 1 reply; 7+ messages in thread From: Thomas Gleixner @ 2013-02-22 14:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Yinghai Lu, LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Fri, 22 Feb 2013, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: ACPI / APEI: Fix crash in apei_hest_parse() for acpi=off > > After commit 92ef2a2 (ACPI: Change the ordering of PCI root bridge > driver registrarion), acpi_hest_init() is never called for acpi=off > (acpi_disabled), so hest_disable is not set, but hest_tab is NULL, > which causes apei_hest_parse() to crash when it is called from > aer_acpi_firmware_first(). > > Fix that by making apei_hest_parse() check if hest_tab is not NULL > in addition to checking hest_disable. Also remove the now useless > acpi_disabled check from apei_hest_parse(). > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Tested-by: Thomas Gleixner <tglx@linutronix.de> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 14:39 ` Thomas Gleixner @ 2013-02-22 21:00 ` Rafael J. Wysocki 0 siblings, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2013-02-22 21:00 UTC (permalink / raw) To: Thomas Gleixner Cc: Yinghai Lu, LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Friday, February 22, 2013 03:39:44 PM Thomas Gleixner wrote: > On Fri, 22 Feb 2013, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Subject: ACPI / APEI: Fix crash in apei_hest_parse() for acpi=off > > > > After commit 92ef2a2 (ACPI: Change the ordering of PCI root bridge > > driver registrarion), acpi_hest_init() is never called for acpi=off > > (acpi_disabled), so hest_disable is not set, but hest_tab is NULL, > > which causes apei_hest_parse() to crash when it is called from > > aer_acpi_firmware_first(). > > > > Fix that by making apei_hest_parse() check if hest_tab is not NULL > > in addition to checking hest_disable. Also remove the now useless > > acpi_disabled check from apei_hest_parse(). > > > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Tested-by: Thomas Gleixner <tglx@linutronix.de> Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [BUG bisected]: apei_hest_parse explosion 2013-02-22 12:30 ` Rafael J. Wysocki 2013-02-22 14:39 ` Thomas Gleixner @ 2013-02-22 23:46 ` Yinghai Lu 1 sibling, 0 replies; 7+ messages in thread From: Yinghai Lu @ 2013-02-22 23:46 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Thomas Gleixner, LKML, Toshi Kani, Huang Ying, ACPI Devel Maling List On Fri, Feb 22, 2013 at 4:30 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Friday, February 22, 2013 09:22:15 AM Thomas Gleixner wrote: >> On Fri, 22 Feb 2013, Rafael J. Wysocki wrote: >> > On Friday, February 22, 2013 02:40:58 AM Rafael J. Wysocki wrote: >> > > It looks like the hest_tab memory mapping is unmapped between acpi_hest_init() >> > > and aer_acpi_firmware_first(), but I have no idea what may be responsible for >> > > that. >> > > >> > > And the only relevant difference between now and before the commit above seems >> > > to be the change of the acpi_hest_init() ordering (which now is called earlier). >> > >> > We actually don't really need to do that thing so early, I think. It looks like >> > we only need to make it available early enough for the AER driver to be able to >> > use it, so I wonder if moving the acpi_hest_init() to a separate >> > subsys_initcall() will work around the problem. That is, something like the >> > patch below. >> >> Yes, that makes the machine boot. > > Although for a reason I didn't think about. > >> > But even if this helps, I will be wanting to understand what's up here. >> >> It's very simple. I have "acpi=off" on the command line. With that >> acpi_hest_init is never called, so hest_disable is not set ..... > > Well, that explains things (and means that acpi=off doesn't really get much > test coverage these days). > >> Brilliant stuff that. > > The appended patch should fix the breakage too, can you please verify? > > Rafael > > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Subject: ACPI / APEI: Fix crash in apei_hest_parse() for acpi=off > > After commit 92ef2a2 (ACPI: Change the ordering of PCI root bridge > driver registrarion), acpi_hest_init() is never called for acpi=off > (acpi_disabled), so hest_disable is not set, but hest_tab is NULL, > which causes apei_hest_parse() to crash when it is called from > aer_acpi_firmware_first(). > > Fix that by making apei_hest_parse() check if hest_tab is not NULL > in addition to checking hest_disable. Also remove the now useless > acpi_disabled check from apei_hest_parse(). > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/apei/hest.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > Index: test/drivers/acpi/apei/hest.c > =================================================================== > --- test.orig/drivers/acpi/apei/hest.c > +++ test/drivers/acpi/apei/hest.c > @@ -89,7 +89,7 @@ int apei_hest_parse(apei_hest_func_t fun > struct acpi_hest_header *hest_hdr; > int i, rc, len; > > - if (hest_disable) > + if (hest_disable || !hest_tab) > return -EINVAL; > > hest_hdr = (struct acpi_hest_header *)(hest_tab + 1); > @@ -216,9 +216,6 @@ void __init acpi_hest_init(void) > 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) > Acked-by: Yinghai Lu <yinghai@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-02-22 23:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.LFD.2.02.1302220002040.22263@ionos>
[not found] ` <1447813.geP1BIVYu5@vostro.rjw.lan>
[not found] ` <CAE9FiQXV43emyjjCb=D=xFLz-n6q0Eu0uQmVhAA470A=FGRhww@mail.gmail.com>
2013-02-22 1:40 ` [BUG bisected]: apei_hest_parse explosion Rafael J. Wysocki
2013-02-22 2:19 ` Rafael J. Wysocki
2013-02-22 8:22 ` Thomas Gleixner
2013-02-22 12:30 ` Rafael J. Wysocki
2013-02-22 14:39 ` Thomas Gleixner
2013-02-22 21:00 ` Rafael J. Wysocki
2013-02-22 23:46 ` Yinghai Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox