Bug 4485

Okay,  
I did an real ugly hack and changed  
line 494 in processor_core.c : 
 else if (object.processor.pblk_length != 6) 
 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid PBLK length [%d]\n", 
 object.processor.pblk_length)); 
  
to 
 
 else if (object.processor.pblk_length != 7) 
 ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid PBLK length [%d]\n", 
 object.processor.pblk_length)); 
 
this solves the problem, but shure isn't the right way to do it. 
 
dmesg: 
ACPI: Video Device [VGA] (multi-head: yes  rom: no  post: no) 
ACPI: CPU0 (power states: C1[C1] C2[C2]) 
ACPI: Processor [CPU1] (supports 8 throttling states) 
ACPI: Thermal Zone [THRM] (72 C) 
 
bash-2.05b# cat /proc/acpi/processor/CPU1/power 
active state:            C2 
max_cstate:              C8 
bus master activity:     00000000 
states: 
    C1:                  type[C1] promotion[C2] demotion[--] latency[001] 
usage[00104030] 
   *C2:                  type[C2] promotion[--] demotion[C1] latency[099] 
usage[01279959] 
 
  
 

Please update latest BIOS. I just checked the latest one is 0214A.

Nope, 208A is the latest BIOS !  
As I mentioned above, this is an M6Ne 
not an M6N ! So 208A is the latest BIOS. 
  

One line patch in bug #4847 should help here. can you please check it.

still an issue in 2.6.13?

jep, still there. 
Thre Problem is as follows:  
The first (and last) C state given by me _CST is 
the C2 C-State and no more.  
This is ACPI 3.0 conform.  
I use a patch that resolves this issue by faking an C1 state,  
I subbmitted it two times to the ACPI-dev list, but it is still not 
in the kernel :-( 
Another way to work arround it would be to remove the (for my opinon sensless)  
C-States given by _CST > 2 
 

Created attachment 5697 [details]
Patch for faking C1

If C2 is the only state in your _CST then patch in comment #7 still will not 
help. Right? Won't you hit the greater than 2 package check before the loop 
and bail out? Or am I missing something?

Created attachment 5810 [details]
Fake C1 when it is not already present in _CST

Mostly same as patch in #7. With additional more than 2 package check removed.
Can you please test this patch and let me know whether this helps?

Thanks.

>Won't you hit the greater than 2 package check before the loop     
>and bail out? Or am I missing something?    
Jup you are missing something ;-)     
Name(_CST, Package()    
{    
    1,         // There are two C-states defined here – C2 and C3    
    Package(){ResourceTemplate(){Register(SystemIO, 8, 0, 0x124)}, 2, 2, 750},    
})    
this is an example from the ACPI-Spec...     
even if it contains only one C-State it still has two "members" thats why I    
don't bail out before entering the loop.    
Meanwhile I testet your patch and (sorry) like I thougt it endet in an endless    
loop...     
There are two things that I don't understand in your patch    
First :   
			pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;   
As far as I can see pr-power.states contains an pointer to an struct  
acpi_processor_cx, so what happens if we set it to a constant instead of an  
valid pointer ?   
Second :   
			i--; /* Revisit the same package in next iteration */ 
			continue; 
Nice idea, but this is the reason for the infinite loop. In my version of the 
patch, there will only be a struct acpi_processor_cx be filled and copied 
nothing of the regular loop is touched, so no need for an continue. 
 
I think it's a bad idea to fake an C1-state AND remove the checks, either do 
one of these things, but not both, following situation : 
The first (and only) given C-State is not vallid for some reason and it is not 
an C1 state. In this case the patch would fake an C1 and be the _CST object 
would be valid. So therefore the _CST is accepted even if contains no vallid 
C-State. And to make it even worse, normaly then there would be an fallback to 
P_BLK which could contain three vallid C-States...  
 
I think the best solution would be to generally add an C1 state and ignore all 
C1 states given by _CST or P_BLK. If you agree, I will write an patch for 
this ;-) 

Oops. That infinite loop was due to stupid code in my patch. After i--, I 
should have made sure that we don't enter that if statement again. By using 
another variable. That answers your second question.

About your first question. We are changing power.states[ACPI_STATES_C1].type 
and not power.states itself. So, we should be OK there.

Yes. I thought about adding C1 state by default in case of _CST. But, we still 
have to do count-- inside the loop, if C1 is not there in _CST. In general I 
think that is a better idea. If you can go ahead and send the patch, that will 
be the best :).

I stills don't understand the 2 package issue. In ACPI 3.0 spec, I don't see 
this example. The '1' and a Single "Package" object indeed says there is only 
one package in _CST. I see examples with 2 and 4 objects in the spec.
Name(_CST, Package()    
{    
    1,         // There are two C-states defined here – C2 and C3    
    Package(){ResourceTemplate(){Register(SystemIO, 8, 0, 0x124)}, 2, 2, 
750},    
})

Can you attach the acpidump or disassembly from your system?
Thanks,
Venki

does loading the processor module with "nocst" help?
It may be that other operating systems reject a broken _CST
and fall back to the FADT.

I should have written "nocst=1"
eg. modprobe processor nocst=1

My _CST is vallid, and my P_BLK has a length of 7 and is so disabled... 
And if I do a little DSDT patching it even gives me back C2 C3 C3 ;-) 
The generall problem is that a valid _CST containing just one C2 state is 
rejected. 

Back to the example :
Name(_CST, Package()    
{    
    1,         // There is one C-states defined here C2    
    Package(){ResourceTemplate(){Register(SystemIO, 8, 0, 0x124)}, 2, 2, 
750},    
})    

This is an acpi_object it contains two members
{
Integer //number of C-States in the package
Package //The C-State itself
}

Venkatesh you're right, this is an example from the ACPIspec, that I modified...

Okay, I will start working on that patch...
Greets 
   Janosch 

Created attachment 5871 [details]
Patch that adds C1 by default

This patch adds a C1 by default and optimizes the logic for discovering the
C-States.
This patch works for me, please test it on a system with _CST and C1 and P_BLK
if no errors occur please apply.

Okay,  
so far it is working, but there is still a little issue ;-) 
It is also possible to get an C3 state, but therefore an ACPI-Method has to be 
called (I think).  
Here some code from the DSDT : 
 
Processor (CPU1, 0x01, 0x00000410, 0x07) 
        { 
            Method (_CST, 0, NotSerialized) 
            { 
                Name (ACST, Package (0x03) 
                { 
                    0x02,  
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x00000000000005FF) 
                        },  
 
                        0x02,  
                        0x01,  
                        0x03E8 
                    },  
 
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x0000000000000415) 
                        },  
 
                        0x03,  
                        0x01,  
                        0x01F4 
                    } 
                }) 
                Add (\PMBS, 0x14, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (ACST, 
0x01)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (ACST, 0x01)), 0x00)), 0x08)) 
                Add (\PMBS, 0x15, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (ACST, 
0x02)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (ACST, 0x02)), 0x00)), 0x08)) 
                Name (BCST, Package (0x04) 
                { 
                    0x03,  
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x00000000000005FF) 
                        },  
 
                        0x02,  
                        0x01,  
                        0x03E8 
                    },  
 
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x0000000000000415) 
                        },  
 
                        0x03,  
                        0x01,  
                        0x01F4 
                    },  
 
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x0000000000000416) 
                        },  
 
                        0x04,  
                        0x02,  
                        0xFA 
                    } 
                }) 
                Add (\PMBS, 0x14, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (BCST, 
0x01)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (BCST, 0x01)), 0x00)), 0x08)) 
                Add (\PMBS, 0x15, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (BCST, 
0x02)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (BCST, 0x02)), 0x00)), 0x08)) 
                Add (\PMBS, 0x16, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (BCST, 
0x03)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (BCST, 0x01)), 0x00)), 0x08)) 
                Name (NOC3, Package (0x02) 
                { 
                    0x01,  
                    Package (0x04) 
                    { 
                        ResourceTemplate () 
                        { 
                            Register (SystemIO, 0x08, 0x00, 0x00000000000005FF) 
                        },  
 
                        0x02,  
                        0x01,  
                        0x03E8 
                    } 
                }) 
                Add (\PMBS, 0x14, Local0) 
                Store (Local0, Index (DerefOf (Index (DerefOf (Index (NOC3, 
0x01)), 0x00)), 0x07)) 
                Store (ShiftRight (Local0, 0x08), Index (DerefOf (Index 
(DerefOf (Index (NOC3, 0x01)), 0x00)), 0x08)) 
                If (\_SB.INV7) 
                { 
                    Return (NOC3) 
                } 
 
                If (\_SB.BATO) 
                { 
                    Return (BCST) 
                } 
                Else 
                { 
                    Return (ACST) 
                } 
            } 
 
As you can see there is a check against \_SB.INV7. 
The only match of this register (?) I could find in the DSDT was here : 
 
    Scope (\_SB) 
    { 
        OperationRegion (GPIO, SystemIO, IO2B, 0x40) 
        Field (GPIO, ByteAcc, NoLock, Preserve) 
        { 
            Offset (0x0C),  
                ,   7,  
            G07S,   1,  
                ,   9,  
            G17S,   1,  
                ,   9,  
            G27S,   1,  
            G28S,   1,  
            Offset (0x2C),  
                ,   3,  
            BT1S,   1,  
                ,   3,  
            INV7,   1,  
                ,   3,  
            LIDL,   1,  
            Offset (0x38),  
            BKLT,   1,  
            BAYI,   2 
        } 
    } 
 
    Scope (\_GPE) 
    { 
        Method (_L17, 0, NotSerialized) 
        { 
            If (\_SB.INV7) 
            { 
                Store (0x00, \_SB.INV7) 
            } 
            Else 
            { 
                Store (0x01, \_SB.INV7) 
            } 
 
            Notify (\_PR.CPU1, 0x81) 
        } 
So what I think is that if you call the \_GPE._L17 method that you get more 
C-States. Is this correct ? 
If it is correct, would it be a good idea to use the asus_acpi driver and 
create an node in /proc/acpi/asus for calling this methode ?  
And by the way how do I call an ACPI method ? ;-) (C-Code example would be 
nice) 

Created attachment 5872 [details]
DSDT for further investigation

I found on a forum a beta bios from Asus support. The version is 209A, and no
patch is needed to have C states working.

The link for new bios :
http://m6n.ath.cx/forum/viewtopic.php?t=462

I hope it could help.

Looks like laptop is enabling C3 on certain GPE. Probably when you switch to
battery mode from AC mode. Or something similar. Can you post the complete DSDT
disassemebly?

Thanks.

Ohh. OK. You have already attached the DSDT. I missed it.

Luming,
Can you take a look at this DSDT and figure out when this particular method will
be called?

    Scope (\_GPE) 
    { 
        Method (_L17, 0, NotSerialized) 
        { 

Thanks.

 
When GPE 0x17 fire, _L17 will be called.  
 
But don't know which Device is associated with GPE 0x17, and what kind of 
condition could cause GPE 0x17 fire. It should be hidden by BIOS.  
 
There are two points need to be confirmed: 
1. GPE 0x17 is enabled or not. 
2. The accesses to INV7 are correctly interpreterd. 
 
 

> There are two points need to be confirmed:    
> 1. GPE 0x17 is enabled or not.    
> 2. The accesses to INV7 are correctly interpreterd.    
I think a simple way to test this would be to call the _L17 method, but   
unfortunatly I have no idea how to do this :-(. 
 
As far as I can see the method is attached to no device on the laptop. 
Plugin/unplugin the laptop does not change anything. 

Created attachment 6169 [details]
Patch in comment 15 with minor changes..


Attached patch is very much similar to patch in comment #15 with some minor
changes and cleanups. Janosch, Can you please test this patch and make sure C2
works OK. If everything is fine I will push it Len-wards so that it will make
its way to base kernel.

Regarding directly calling _L17 method in _GPE scope..
I am not sure whether there is any clean way to do it. Len, Luming any ideas?

Created attachment 6170 [details]
Do not check for C1 in _CST

I attached wrong patch earlier. Here is the final patch.

To invoke _L17, please do: 
 
build kernel with generic hotkey driver enabled. 
 
#echo "0:\\_GPE:_L17:\\_GPE:_L17:10001:1" > /proc/acpi/hotkey/poll_config 
#echo "10001:0:1:0" > /proc/acpi/hotkey/action 

Notes, please use linux-2.6.14-rc1 or later. 

Okay, I tested the patch,  
it does not apply cleanly to 2.6.14-rc2 but seems to work ;-) 
 
What didn't work was the calling of _L17. As far as I can see the commands of 
Luming worked an calles the _L17 method correctly, but I allways get NOC3 
back :-(. Luming, would you like to habe a dmesg output, if yes, with which 
debug options ?  

Can you post the complete acpidump using the latest pmtools here.
http://www.kernel.org/pub/linux/kernel/people/lenb/acpi/utils/

Created attachment 6380 [details]
ACPI dump

Can I do anything else to help solving this issue ?  
And did the Patch went into 2.6.14 ? 
 
  

I will set the state as resolved with code fix for now, so that Len picks the
patch up. 

We can reopen it and then look at this _CST C3 issue. My guess is BIOS doesn't
want OS to use C3 for some reason. But, we will try to figure out more later.
BTW, does C3 work with Windows?

> BTW, does C3 work with Windows? 
Hm good question, I think yes, with the DSDT-hack I get around 6 to 8 hours of 
battery time. Windows is about the same, so I think the two C3 states are 
enabled.  

Created attachment 6743 [details]
re-diffed patch from venki

applied this version to acpi-test tree
note that it is built on top of the patch in bug 5165

Shipped in 2.6.16-rc1-git6 -- closing.