Bug 4485

Summary: Processor C states not working with 2.6.11.6 - ASUS M6Ne
Product: ACPI Reporter: Janosch Machowinski (jmachowinski)
Component: Power-ProcessorAssignee: Venkatesh Pallipadi (venki)
Status: CLOSED CODE_FIX    
Severity: normal CC: acpi-bugzilla, luming.yu
Priority: P2    
Hardware: i386   
OS: Linux   
Kernel Version: 2.6.11.6 Subsystem:
Regression: --- Bisected commit-id:
Attachments: Patch for faking C1
Fake C1 when it is not already present in _CST
Patch that adds C1 by default
DSDT for further investigation
Patch in comment 15 with minor changes..
Do not check for C1 in _CST
ACPI dump
re-diffed patch from venki

Description Janosch Machowinski 2005-04-13 12:54:08 UTC
Distribution:  
Gentoo Linux 
 
Hardware Environment:  
ASUS M6Ne with BIOS 2.08A 
(note this is an Ne, and 2.08A is the latest bios version) 
 
Software Environment:  
Vanilla Sources 2.6.11.6 with "implicit return fix" 
 
Problem Description: 
The C Processor states are not working:  
 
bash-2.05b# cat /proc/acpi/processor/CPU1/ 
info        limit       power       throttling 
bash-2.05b# cat /proc/acpi/processor/CPU1/power 
active state:            C0 
max_cstate:              C8 
bus master activity:     00000000 
states: 
    C1:                  <not supported> 
bash-2.05b# 
 
dmesg showes : 
 
ACPI: Sleep Button (CM) [SLPB] 
ACPI: Video Device [VGA] (multi-head: yes  rom: no  post: no) 
acpi_processor-0496 [67] acpi_processor_get_inf: Invalid PBLK length [7] 
ACPI: Thermal Zone [THRM] (47 C) 
 
The C1 and C2 states are working, if you use an  
2.6.9 kernel with the "_cst" patches, which should  
do the same as the "implicit return fix" afaik. 
Everything else works great on 2.6.11 (nice work ;-) ) 
 
I use following kernel parameters :   
nolapic acpi_fake_ecdt resume=/dev/hda6 
 
Greetings 
    Janosch
Comment 1 Janosch Machowinski 2005-04-13 14:26:27 UTC
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] 
 
  
 
Comment 2 Luming Yu 2005-04-20 00:17:43 UTC
Please update latest BIOS. I just checked the latest one is 0214A.
Comment 3 Janosch Machowinski 2005-04-20 03:46:07 UTC
Nope, 208A is the latest BIOS !  
As I mentioned above, this is an M6Ne 
not an M6N ! So 208A is the latest BIOS. 
  
Comment 4 Venkatesh Pallipadi 2005-07-27 19:03:04 UTC
One line patch in bug #4847 should help here. can you please check it.
Comment 5 Len Brown 2005-08-17 15:36:59 UTC
still an issue in 2.6.13?
Comment 6 Janosch Machowinski 2005-08-20 08:01:12 UTC
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 
 
Comment 7 Janosch Machowinski 2005-08-20 08:02:21 UTC
Created attachment 5697 [details]
Patch for faking C1
Comment 8 Venkatesh Pallipadi 2005-08-29 17:17:19 UTC
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?

Comment 9 Venkatesh Pallipadi 2005-08-29 17:18:56 UTC
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.
Comment 10 Janosch Machowinski 2005-08-31 16:24:36 UTC
>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 &#8211; 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 ;-) 
Comment 11 Venkatesh Pallipadi 2005-08-31 17:41:49 UTC
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 &#8211; 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






Comment 12 Len Brown 2005-08-31 19:23:30 UTC
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.
Comment 13 Len Brown 2005-08-31 19:56:20 UTC
I should have written "nocst=1"
eg. modprobe processor nocst=1
Comment 14 Janosch Machowinski 2005-09-01 04:24:22 UTC
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 
Comment 15 Janosch Machowinski 2005-09-02 13:51:05 UTC
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.
Comment 16 Janosch Machowinski 2005-09-02 14:10:17 UTC
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) 
Comment 17 Janosch Machowinski 2005-09-02 14:11:24 UTC
Created attachment 5872 [details]
DSDT for further investigation
Comment 18 Julien HENRY 2005-09-21 07:11:53 UTC
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.
Comment 19 Venkatesh Pallipadi 2005-09-26 17:34:10 UTC
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.
Comment 20 Venkatesh Pallipadi 2005-09-26 17:36:57 UTC
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.
Comment 21 Luming Yu 2005-09-26 20:22:20 UTC
 
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. 
 
 
Comment 22 Janosch Machowinski 2005-09-27 11:31:42 UTC
> 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. 
Comment 23 Venkatesh Pallipadi 2005-09-27 16:11:37 UTC
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.
Comment 24 Venkatesh Pallipadi 2005-09-27 16:13:00 UTC
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?
Comment 25 Venkatesh Pallipadi 2005-09-27 16:22:07 UTC
Created attachment 6170 [details]
Do not check for C1 in _CST

I attached wrong patch earlier. Here is the final patch.
Comment 26 Luming Yu 2005-09-27 20:32:16 UTC
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 
Comment 27 Luming Yu 2005-09-27 20:33:25 UTC
Notes, please use linux-2.6.14-rc1 or later. 
Comment 28 Janosch Machowinski 2005-10-04 00:39:54 UTC
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 ?  
Comment 29 Venkatesh Pallipadi 2005-10-23 17:01:11 UTC
Can you post the complete acpidump using the latest pmtools here.
http://www.kernel.org/pub/linux/kernel/people/lenb/acpi/utils/
Comment 30 Janosch Machowinski 2005-10-24 14:10:25 UTC
Created attachment 6380 [details]
ACPI dump
Comment 31 Janosch Machowinski 2005-10-24 14:18:51 UTC
Can I do anything else to help solving this issue ?  
And did the Patch went into 2.6.14 ? 
 
  
Comment 32 Venkatesh Pallipadi 2005-10-24 14:22:59 UTC
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?
Comment 33 Janosch Machowinski 2005-10-24 14:27:51 UTC
> 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.  
Comment 34 Len Brown 2005-12-01 17:34:00 UTC
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
Comment 35 Len Brown 2006-02-02 14:30:55 UTC
Shipped in 2.6.16-rc1-git6 -- closing.