Bug 10454

Created attachment 15751 [details]
acpidump --addr 0x7F6E2A9F --length 0x3B7 -o vist

The (probably broken) Table in DSDT:
OperationRegion (VIST, SystemMemory, 0x7F6E2A9F, 0x000003B7)
acpidump --addr 0x7F6E2A9F --length 0x3B7 -o vist

a patch against 2.6.25-rc9:

--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -448,6 +448,14 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
                     DMI_MATCH(DMI_PRODUCT_NAME, "ESPRIMO Mobile V5505"),
                },
        },
+       {
+       .callback = dmi_disable_osi_vista,
+       .ident = "Fujitsu Siemens",
+       .matches = {
+                    DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU SIEMENS"),
+                    DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK S6410"),
+                },
+       },
        /*
         * Disable OSI(Linux) warnings on all "Hewlett-Packard"
         *

Unfortunately the above patch also removes some features from the bios (the DSDT ahs quite some IF (OSI("Windows 2006")) THEN do something) so a more appropriate solution might be to fix the signature on the fly? like  this: (perhaps this should only be done based on some DMI whitelisting?)

--- a/drivers/acpi/tables/tbinstal.c
+++ b/drivers/acpi/tables/tbinstal.c
@@ -125,6 +125,12 @@ acpi_tb_add_table(struct acpi_table_desc *table_desc,
 
        /* The table must be either an SSDT or a PSDT or an OEMx */
 
+       if (table_desc->pointer->signature[0] == '\0') {
+               ACPI_DEBUG_PRINT((AE_DB_ERROR,
+                           "Table has empty signature, probably VIST table, cor
+               strncpy(table_desc->pointer->signature, "OEM0", 4);
+       }
+
        if (!ACPI_COMPARE_NAME(table_desc->pointer->signature, ACPI_SIG_PSDT)&&
            !ACPI_COMPARE_NAME(table_desc->pointer->signature, ACPI_SIG_SSDT)&&
            strncmp(table_desc->pointer->signature, "OEM", 3)) {
 

oops the patch got corrupted by cut'n paste:

--- a/drivers/acpi/tables/tbinstal.c
+++ b/drivers/acpi/tables/tbinstal.c
@@ -125,6 +125,12 @@ acpi_tb_add_table(struct acpi_table_desc *table_desc,
 
        /* The table must be either an SSDT or a PSDT or an OEMx */
 
+       if (table_desc->pointer->signature[0] == '\0') {
+               ACPI_DEBUG_PRINT((AE_DB_ERROR,
+                           "Table has empty signature, probably VIST table, correcting"));
+               strncpy(table_desc->pointer->signature, "OEM0", 4);
+       }
+
        if (!ACPI_COMPARE_NAME(table_desc->pointer->signature, ACPI_SIG_PSDT)&&
            !ACPI_COMPARE_NAME(table_desc->pointer->signature, ACPI_SIG_SSDT)&&
            strncmp(table_desc->pointer->signature, "OEM", 3)) {

(In reply to comment #3)
> Unfortunately the above patch also removes some features from the bios (the
> DSDT ahs quite some IF (OSI("Windows 2006")) THEN do something)

I disagree.
what feature is lost? If OSI("Windows 2006") brings some errors, it means that Linux doesn't supported it. We can not fix it right now as how the BIOS works in Linux is not not tested before release. :(
acpi_osi= is just a workaround for such problems.

> so a more
> appropriate solution might be to fix the signature on the fly? like  this:
> (perhaps this should only be done based on some DMI whitelisting?)

I think nothing is wrong when loading a NULL signature table. The error message is clear and reasonable.
Linux should NEVER load such a table, maybe that piece of memory is not a table at all.

(In reply to comment #5)
> (In reply to comment #3)
> I disagree.
> what feature is lost? If OSI("Windows 2006") brings some errors, it means
> that
> Linux doesn't supported it. We can not fix it right now as how the BIOS works
> in Linux is not not tested before release. :(
> acpi_osi= is just a workaround for such problems.

* all graphic related notifies are lost since they notify
_SB.PCI0 instead of _SB.PCI0.GFX0 (ok, can also be worked around in udev/acpid)
* the video output switching does not work via the buttons
* some battery information is not available (eg. design_capacity_* is set to 0 and no battery low/warning notifies are generated, can also be worked around in userspace)

> > so a more
> > appropriate solution might be to fix the signature on the fly? like  this:
> > (perhaps this should only be done based on some DMI whitelisting?)
> 
> I think nothing is wrong when loading a NULL signature table. The error
> message
> is clear and reasonable.
> Linux should NEVER load such a table, maybe that piece of memory is not a
> table
> at all.
since this seems to be a common problem on fujitsu machines we could only enable
this for them or handle the
Load (VIST, VSTH)
in the DSDT differently (allowing empty signatures). This is also a good indication that this memory is indeed a table.

(In reply to comment #5)
> what feature is lost? If OSI("Windows 2006") brings some errors, it means
> that

Oh and I forgot brightness button handling is so much saner when OS("Windows 2006"), ie. only notify the os which button was pressed instead of changing the brighness level on its own and notifying the OS that something was done and let the OS figure out what it was.

> Linux should NEVER load such a table, maybe that piece of memory is not a
> table at all.
We should try to find out whether Windows does load it.
Maybe the zeros have been added intentionally, so that M$ also does not load it.
But if Windows does load it, I fear there will pop up more (at least Fujitsu machines) doing such nasty things and we might have to think about a compatibilty hack.

(In reply to comment #8)
> > Linux should NEVER load such a table, maybe that piece of memory is not a
> > table at all.
> We should try to find out whether Windows does load it.
> Maybe the zeros have been added intentionally, so that M$ also does not load
> it.
> But if Windows does load it, I fear there will pop up more (at least Fujitsu
> machines) doing such nasty things and we might have to think about a
> compatibilty hack.

I am pretty sure that windows load it since the brightness buttons work under Vista
and the code in teh DSDT for that is something like:

function handleButtons()
  if (OSI("Windows 2006"))
    use function from the table with null signature
  else
    change brightness directly
    notify userspace
  end
end

Peter, could you please attach the dmesg with the patch in #4 applied?
we should get the base address and length if we can load this "table".
Then we can use acpidump to dump this table to see what's done in it. :)

(In reply to comment #10)
> Peter, could you please attach the dmesg with the patch in #4 applied?
> we should get the base address and length if we can load this "table".
> Then we can use acpidump to dump this table to see what's done in it. :)

the VIST (without signature) table is already attached in comment #1. If you replace the first 4 bytes with a valid signature (SSDT, OEM0 ...) you can disassemble it with iasl.

with patch in #4 the table actually gets listed in /sys/firmware/acpi/tables/

dmesg:
...
Apr 21 10:59:19 kernel: ACPI: OEM0 7F6E2A9F, 03B7 (r1 FUJ    FJNB1D3   1210000 FUJ       100)
Apr 21 10:59:19 kernel: Parsing all Control Methods:
Apr 21 10:59:19 kernel: Table [OEM0](id 0018) - 35 Objects with 0 Devices 15 Methods 6 Regions
...

Peter,
please attach the full acpidump output

Created attachment 15884 [details]
vist table

Created attachment 15903 [details]
acpidump -o acpi.dump

complete acpi dump

(In reply to comment #5)
> Linux should NEVER load such a table, maybe that piece of memory is not a
> table
> at all.

I just did look up the "Load" command in the ACPI spec and it says:
...The Definition Block should contain an ACPI DESCRIPTION_HEADER of type SSDT...
unfortunately the ACPI spec does not contain a definition for the words should and must but it seems to be weaker than the 
... Checksum ... Entire table must sum to zero...
but linux ignores wrong checksums. So perhaps something like the following which 
allows empty signatures in the "Load" command might be a solution:

diff --git a/drivers/acpi/executer/exconfig.c b/drivers/acpi/executer/exconfig.c
--- a/drivers/acpi/executer/exconfig.c
+++ b/drivers/acpi/executer/exconfig.c
@@ -370,7 +370,7 @@ acpi_ex_load_op(union acpi_operand_object *obj_desc,
        /*
         * Install the new table into the local data structures
         */
-       status = acpi_tb_add_table(&table_desc, &table_index);
+       status = acpi_tb_add_table_no_signature(&table_desc, &table_index);
        if (ACPI_FAILURE(status)) {
                goto cleanup;
        }
diff --git a/drivers/acpi/tables/tbinstal.c b/drivers/acpi/tables/tbinstal.c
--- a/drivers/acpi/tables/tbinstal.c
+++ b/drivers/acpi/tables/tbinstal.c
@@ -107,8 +107,8 @@ acpi_status acpi_tb_verify_table(struct acpi_table_desc *table_desc)
  ******************************************************************************/

 acpi_status
-acpi_tb_add_table(struct acpi_table_desc *table_desc,
-                 acpi_native_uint * table_index)
+acpi_tb_add_table_real(struct acpi_table_desc *table_desc,
+                 acpi_native_uint * table_index, int strict_signature_check)
 {
        acpi_native_uint i;
        acpi_native_uint length;
@@ -125,9 +125,9 @@ acpi_tb_add_table(struct acpi_table_desc *table_desc,

        /* The table must be either an SSDT or a PSDT or an OEMx */

-       if (table_desc->pointer->signature[0] == '\0') {
-               ACPI_ERROR((AE_INFO,
-                           "Table has empty signature, probably VIST table, correcting"));
+       if (!strict_signature_check && table_desc->pointer->signature[0] == '\0') {
+               ACPI_DEBUG_PRINT((ACPI_DB_TABLES,
+                           "Table has empty signature, no strict signature check requested, correcting"));
                strncpy(table_desc->pointer->signature, "SSDT", 4);
        }

@@ -196,6 +196,21 @@ acpi_tb_add_table(struct acpi_table_desc *table_desc,
        return_ACPI_STATUS(status);
 }

+acpi_status
+acpi_tb_add_table(struct acpi_table_desc *table_desc,
+                 acpi_native_uint * table_index)
+{
+        return acpi_tb_add_table_real(table_desc, table_index, 1);
+}
+
+acpi_status
+acpi_tb_add_table_no_signature(struct acpi_table_desc *table_desc,
+                 acpi_native_uint * table_index)
+{
+        return acpi_tb_add_table_real(table_desc, table_index, 0);
+}
+
+
 /*******************************************************************************
  *
  * FUNCTION:    acpi_tb_resize_root_table_list
diff --git a/include/acpi/actables.h b/include/acpi/actables.h
--- a/include/acpi/actables.h
+++ b/include/acpi/actables.h
@@ -73,6 +73,10 @@ acpi_tb_add_table(struct acpi_table_desc *table_desc,
                  acpi_native_uint * table_index);

 acpi_status
+acpi_tb_add_table_no_signature(struct acpi_table_desc *table_desc,
+                 acpi_native_uint * table_index);
+
+acpi_status
 acpi_tb_store_table(acpi_physical_address address,
                    struct acpi_table_header *table,
                    u32 length, u8 flags, acpi_native_uint * table_index);


 

Maybe we shouldn't bother looking at the signature at all, just stuff in "SSDT" and be done with it.

(In reply to comment #16)
> Maybe we shouldn't bother looking at the signature at all, just stuff in
> "SSDT"
> and be done with it.

I agree but I think we should distinguish between the table loading at ACPI start, here the spec says for the loading from RSDP: 

... Notice that if OSPM finds a signature in a table that is not listed ... OSPM ignores the table even though the values in the Length and Checksum fields are correct ...

but for the loading with "Load"

...The Definition Block should contain an ACPI DESCRIPTION_HEADER of type
SSDT...

in the linux implementation both is handled by the acpi_tb_add_table function.

Bob, what do you think of the patch in comment #15?
Or should we just override the signature of dynamic tables?

assigning to Lin Ming to look into an upstream ACPICA fix.
at this point, it looks like the only solution is to ignore
both the tag and the checksum for the target of a Load command:-(

We should also propose an update to the ACPI Specification
that it be updated to reflect "common industry practice"....

patch is in latest acpica git tree.

(In reply to comment #20)
> patch is in latest acpica git tree.
> 

small (hopefully) cosmetic problem with that patch:

:/sys/firmware/acpi/tables# ls -l
ls: cannot access : No such file or directory
total 0
-????????? ? ?    ?    ?            ? 
-r--r--r-- 1 root root 0 Jun  4 22:37 APIC
-r--r--r-- 1 root root 0 Jun  4 22:37 BOOT
-r--r--r-- 1 root root 0 Jun  4 22:37 DSDT
-r--r--r-- 1 root root 0 Jun  4 22:37 FACP
-r--r--r-- 1 root root 0 Jun  4 22:37 FACS
-r--r--r-- 1 root root 0 Jun  4 22:37 HPET
-r--r--r-- 1 root root 0 Jun  4 22:37 MCFG
-r--r--r-- 1 root root 0 Jun  4 22:37 SLIC
-r--r--r-- 1 root root 0 Jun  4 22:37 SSDT1
-r--r--r-- 1 root root 0 Jun  4 22:37 SSDT2
-r--r--r-- 1 root root 0 Jun  4 22:37 SSDT3
-r--r--r-- 1 root root 0 Jun  4 22:37 SSDT4
-r--r--r-- 1 root root 0 Jun  4 22:37 SSDT5

Hah, I see the problem.
VIST table is loaded in the \_SB._INI method in acpi_init, before the execution of acpi_system_init.
so ACPI tries to build a sys attribute for the vist table under /sys/firmware/acpi/tables, while it can't handle the NULL table signature.

two solution for this:
1. override the null table signature to "SSDT" or something like this. 
2. check the signature in driver/acpi/system.c. for a null name table, either override the signature or don't export it to user space at all.

And I prefer the first one.
what do you think, bob?

Hmm.
the first solution doesn't make sense as ACPI still needs to map that piece of memory and export it to user space.
I'll try the second one.
Peter, could you please open another bug report please? as the table loading problem is already fixed, and it exposes an ACPI sys I/F bug.

(In reply to comment #23)
> Hmm.
> the first solution doesn't make sense as ACPI still needs to map that piece
> of
> memory and export it to user space.

the first solution was what i used before your patch (comment #15)
                strncpy(table_desc->pointer->signature, "SSDT", 4);
obviously this results in an incorrect checksum ...

> I'll try the second one.
> Peter, could you please open another bug report please? as the table loading
> problem is already fixed, and it exposes an ACPI sys I/F bug.

I will as soon as the acpica patch is in some linux git (acpi/...) otherwise it hard to follow by other people ...
 

fixed in Linus' tree by commit bc45b1d39a925b56796bebf8a397a0491489d85c

*** This bug has been marked as a duplicate of bug 9919 ***