[PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[Peter Martuccelli]

Hello Len,

Stumbled into this problem working on the ipmi_si driver.  When the
ipmi_si driver initialization fails the acpi_tb_get_table 
call, after rsdt_info has been allocated, acpi_get_firmware_table()
will oops trying to reference off rsdt_info->pointer in the cleanup
code.  The following patch resolves the problem.  I noticed this
problem on six different systems, all with slightly different stack
traces, but they all fail in the same manner. 

Patch applies cleanly to 2.6.12.4.  Tested at various sites and on
various systems, no additional problems detected.

Signed-off-by: peterm@redhat.com


Regards,

Peter

--- linux-2.6.9/drivers/acpi/tables/tbxfroot.c.orig	2005-08-11 23:44:01.000000000 -0400
+++ linux-2.6.9/drivers/acpi/tables/tbxfroot.c	2005-08-11 23:52:57.000000000 -0400
@@ -293,9 +293,12 @@ acpi_get_firmware_table (
 
 
 cleanup:
-	acpi_os_unmap_memory (rsdt_info->pointer, (acpi_size) rsdt_info->pointer->length);
-	ACPI_MEM_FREE (rsdt_info);
-
+	if (rsdt_info) {
+	        if (rsdt_info->pointer) {
+         		acpi_os_unmap_memory (rsdt_info->pointer, (acpi_size) rsdt_info->pointer->length);
+		}
+		ACPI_MEM_FREE (rsdt_info);
+	}
 	if (header) {
 		ACPI_MEM_FREE (header);
 	}

Re: [PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[Bjorn Helgaas]

On Friday 12 August 2005 1:44 pm, Peter Martuccelli wrote:
> Stumbled into this problem working on the ipmi_si driver.  When the
> ipmi_si driver initialization fails the acpi_tb_get_table 
> call, after rsdt_info has been allocated, acpi_get_firmware_table()
> will oops trying to reference off rsdt_info->pointer in the cleanup
> code.

I don't know whether the ACPI patch is correct or desirable, but
I think the ipmi_si ACPI discovery is bogus (it was probably
written before the current ACPI and PNPACPI driver registration
interfaces were stable).

Currently, ipmi_si uses the static SPMI table to locate the
device.  But the static table should only be used if we need
the device very early, before the ACPI namespace is available.

I don't think we use the device early, so we should use
pnp_register_driver() to claim the appropriate PNP IDs.
Or we might have to use acpi_bus_register_driver() since
it looks like it uses ACPI-specific features like GPEs.

Re: [PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[Peter Martuccelli]

On Mon, 2005-08-15 at 18:13, Bjorn Helgaas wrote:
> On Friday 12 August 2005 1:44 pm, Peter Martuccelli wrote:
> > Stumbled into this problem working on the ipmi_si driver.  When the
> > ipmi_si driver initialization fails the acpi_tb_get_table 
> > call, after rsdt_info has been allocated, acpi_get_firmware_table()
> > will oops trying to reference off rsdt_info->pointer in the cleanup
> > code.
> 
> I don't know whether the ACPI patch is correct or desirable, but
> I think the ipmi_si ACPI discovery is bogus (it was probably
> written before the current ACPI and PNPACPI driver registration
> interfaces were stable).
> 
> Currently, ipmi_si uses the static SPMI table to locate the
> device.  But the static table should only be used if we need
> the device very early, before the ACPI namespace is available.
> 
> I don't think we use the device early, so we should use
> pnp_register_driver() to claim the appropriate PNP IDs.
> Or we might have to use acpi_bus_register_driver() since
> it looks like it uses ACPI-specific features like GPEs.
Adding in Corey to the discussion regarding ipmi_si initialization,
waiting on Len to decide on the ACPI fix.

Re: [PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[Corey Minyard]

Peter Martuccelli wrote:

>On Mon, 2005-08-15 at 18:13, Bjorn Helgaas wrote:
>  
>
>>On Friday 12 August 2005 1:44 pm, Peter Martuccelli wrote:
>>    
>>
>>>Stumbled into this problem working on the ipmi_si driver.  When the
>>>ipmi_si driver initialization fails the acpi_tb_get_table 
>>>call, after rsdt_info has been allocated, acpi_get_firmware_table()
>>>will oops trying to reference off rsdt_info->pointer in the cleanup
>>>code.
>>>      
>>>
>>I don't know whether the ACPI patch is correct or desirable, but
>>I think the ipmi_si ACPI discovery is bogus (it was probably
>>written before the current ACPI and PNPACPI driver registration
>>interfaces were stable).
>>
>>Currently, ipmi_si uses the static SPMI table to locate the
>>device.  But the static table should only be used if we need
>>the device very early, before the ACPI namespace is available.
>>
>>I don't think we use the device early, so we should use
>>pnp_register_driver() to claim the appropriate PNP IDs.
>>Or we might have to use acpi_bus_register_driver() since
>>it looks like it uses ACPI-specific features like GPEs.
>>    
>>
>Adding in Corey to the discussion regarding ipmi_si initialization,
>waiting on Len to decide on the ACPI fix.
>  
>
I couldn't find any documentation on how the ACPI interfaces work, so 
I'm kind of in the dark.

Basically, the IPMI system interface needs information from a specific 
IPMI table to know how to configure itself.  Those tables can reference 
GPEs, so the driver can use those (though AFAIK it has never been tested).

 From spending 30 minutes searching and looking at things, I have no 
idea how to tie this in.  Can you point me to some docs?  Or do I have 
to spend hours digging?

-Corey

Re: [PATCH 2.6.12.4] ACPI oops during ipmi_si driver init

[Bjorn Helgaas]

On Wednesday 17 August 2005 2:30 pm, Corey Minyard wrote:
> Basically, the IPMI system interface needs information from a specific 
> IPMI table to know how to configure itself.  Those tables can reference 
> GPEs, so the driver can use those (though AFAIK it has never been tested).

The information in the SPMI table *should* also be in the ACPI
namespace.  In general, drivers should claim devices based on the
namespace, not based on tables like SPMI.  The tables are mainly
there for the times when you need a device before the ACPI namespace
is available.

drivers/serial/8250_pnp.c is a basic example of claiming PNP
devices.  In particular, see serial_pnp_probe(), which gets
called for every device with a PNP ID found in pnp_dev_table[].

drivers/serial/8250_acpi.c is an example of claiming a device
directly from the ACPI namespace.  It claims everything with
PNP ID "PNP0501".

If you need to handle GPEs, you probably would need the
8250_acpi.c style, since I don't think PNP can deal with
those.  You would use acpi_register_driver(), and pass it
an acpi_driver struct containing '.ids = "IPI0001"'.  The
add() function you supply will get called for every active
IPI0001 device in the namespace.  Use acpi_walk_resources()
on its _CRS to extract the I/O port or MMIO address of the
controller and its interrupt information.  If the _CRS
contains no interrupt information, look for a _GPE method.