_CST Patches

_CST Patches

[Janosch Machowinski]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

Hey
it's the guy with the stupid questions again ;-) 
And now I got a few answers I think... 
ACPI specification says, that every processor has a C1
state, and on page 262 is an example of an _CST package,
where the first given C state is C2. So I would suggest following patch
to the processor_idle.c Line: 611

cx.type = obj->integer.value;

+//Test if the first entry is a C1 state, if not we fake one
+if ((cx.type != ACPI_STATE_C1) && i == 1) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Faking C1 State"));
+		struct acpi_processor_cx c1_fake;
+		memset(&c1_fake, 0, sizeof(c1_fake));
+
+		c1_fake.address = 0;
+		c1_fake.type = ACPI_STATE_C1;
+		c1_fake.latency = 1; //How long needs a HLT instruction to execute ?
+		c1_fake.power = 0; //We don't know anything about the power
consumption so we set it to 0
+		//We copy our fake C1 state over to the power states
+		(pr->power.count)++;
+		memcpy(&(pr->power.states[pr->power.count]), &c1_fake,
sizeof(c1_fake));
+		//Finally we avoid to have morepower 				+		//states than
ACPI_PROCESSOR_MAX_POWER
+		if(count == ACPI_PROCESSOR_MAX_POWER)
+		count--;
+		
+		//c1_fake should be freed automaticaly
+
+		}
+

		if ((cx.type != ACPI_STATE_C1) &&

This patch fakes an C1 state if the given states by the _CST object
start with anything else than C1 (like on my M6NE here it start's with
C2)

By looking throug the code I also discovered some errors :
on line 492 and 539 
for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
memset(pr->power.states, 0, sizeof(struct acpi_processor_cx))

my patch also replaces these lines by 
for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
memset(&(pr->power.states[i]), 0, sizeof(struct acpi_processor_cx))

Greets
	Janosch Machowinski

P.S.: Note the attached file is only a diff to the original
processor_idle.c

[-- Attachment #2: _CST_fakeC1.patch --]
[-- Type: text/x-patch, Size: 1179 bytes --]

493c493
< 		memset(&(pr->power.states[i]), 0, sizeof(struct acpi_processor_cx));
---
> 		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
540c540
< 		memset(&(pr->power.states[i]), 0, sizeof(struct acpi_processor_cx));
---
> 		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
612,632d611
< 		//Test if the first entry is a C1 state, if not we fake one
< 		if ((cx.type != ACPI_STATE_C1) && i == 1) {
< 				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Faking C1 State"));
< 				struct acpi_processor_cx c1_fake;
< 				memset(&c1_fake, 0, sizeof(c1_fake));
< 
< 				c1_fake.address = 0;
< 				c1_fake.type = ACPI_STATE_C1;
< 				c1_fake.latency = 1; //How long needs a HLT instruction to execute ?
< 				c1_fake.power = 0; //We don't know anything about the power consumption so we set it to 0
< 				//We copy our fake C1 state over to the power states
< 				(pr->power.count)++;
< 				memcpy(&(pr->power.states[pr->power.count]), &c1_fake, sizeof(c1_fake));
< 				//Finally we avoid to have more power states than ACPI_PROCESSOR_MAX_POWER
< 				if(count == ACPI_PROCESSOR_MAX_POWER)
< 				  count--;
< 				
< 				//c1_fake should be freed automaticaly
< 		}
< 
< 

Re: _CST Patches

[Bruno Ducrot]

On Sat, Apr 16, 2005 at 05:25:41PM +0200, Janosch Machowinski wrote:
> Hey
> it's the guy with the stupid questions again ;-) 
> And now I got a few answers I think... 
> ACPI specification says, that every processor has a C1
> state, and on page 262 is an example of an _CST package,
> where the first given C state is C2. So I would suggest following patch
> to the processor_idle.c Line: 611
> 
> cx.type = obj->integer.value;
> 
> +//Test if the first entry is a C1 state, if not we fake one
> +if ((cx.type != ACPI_STATE_C1) && i == 1) {
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Faking C1 State"));
> +		struct acpi_processor_cx c1_fake;
> +		memset(&c1_fake, 0, sizeof(c1_fake));
> +
> +		c1_fake.address = 0;
> +		c1_fake.type = ACPI_STATE_C1;
> +		c1_fake.latency = 1; //How long needs a HLT instruction to execute ?
> +		c1_fake.power = 0; //We don't know anything about the power
> consumption so we set it to 0
> +		//We copy our fake C1 state over to the power states
> +		(pr->power.count)++;
> +		memcpy(&(pr->power.states[pr->power.count]), &c1_fake,
> sizeof(c1_fake));
> +		//Finally we avoid to have morepower 				+		//states than
> ACPI_PROCESSOR_MAX_POWER
> +		if(count == ACPI_PROCESSOR_MAX_POWER)
> +		count--;
> +		
> +		//c1_fake should be freed automaticaly
> +
> +		}
> +
> 
> 		if ((cx.type != ACPI_STATE_C1) &&
> 
> This patch fakes an C1 state if the given states by the _CST object
> start with anything else than C1 (like on my M6NE here it start's with
> C2)

It should not be needed.  I've taken care that it work by testing
in almost all cases by overriding the DSDT when I wrote this part to
handle properly almost all cases (I forgot the case when C1 and C3, but
no C2 though, that why I wrote 'almost').

Normally, we register a C1 state no matter what before we check
the content of _CST, then we just skip if a C1 type is found.

If your patch is needed, then the patch I send some times ago have not been
fully integrated in the original form, or maybe the wrong one was considered
(the first was buggy in that regard, but the second was OK).

> By looking throug the code I also discovered some errors :
> on line 492 and 539 
> for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
> memset(pr->power.states, 0, sizeof(struct acpi_processor_cx))
> 
> my patch also replaces these lines by 
> for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
> memset(&(pr->power.states[i]), 0, sizeof(struct acpi_processor_cx))

I would prefer
memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
(without the loop)

> 
> Greets
> 	Janosch Machowinski
> 
> P.S.: Note the attached file is only a diff to the original
> processor_idle.c

Please post unified patch (diff with option '-u').


-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: _CST Patches

[Janosch Machowinski]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]


> > This patch fakes an C1 state if the given states by the _CST object
> > start with anything else than C1 (like on my M6NE here it start's with
> > C2)
> 
> It should not be needed.  I've taken care that it work by testing
> in almost all cases by overriding the DSDT when I wrote this part to
> handle properly almost all cases (I forgot the case when C1 and C3, but
> no C2 though, that why I wrote 'almost').
> 
> Normally, we register a C1 state no matter what before we check
> the content of _CST, then we just skip if a C1 type is found.
> 
> If your patch is needed, then the patch I send some times ago have not been
> fully integrated in the original form, or maybe the wrong one was considered
> (the first was buggy in that regard, but the second was OK).
> 
Your first patch has been modified, and now my patch is needed. 
In your first implementation you did an (pr->power)++ before the loop
that started the polling of the _CST object. At the moment all C-States
are deleted (see the memset) and generated from scratch. Your idea
sounds faster to me !


> I would prefer
> memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
> (without the loop)

I agree, looks faster to me. 

A new patch is attached

  Janosch

[-- Attachment #2: _CST_fakeC1.patch --]
[-- Type: text/x-patch, Size: 2081 bytes --]

--- processor_idle.original	2005-04-16 17:13:44.000000000 +0200
+++ processor_idle.c	2005-04-18 14:13:52.000000000 +0200
@@ -479,8 +479,6 @@
 
 static int acpi_processor_get_power_info_fadt (struct acpi_processor *pr)
 {
-	int i;
-
 	ACPI_FUNCTION_TRACE("acpi_processor_get_power_info_fadt");
 
 	if (!pr)
@@ -489,8 +487,7 @@
 	if (!pr->pblk)
 		return_VALUE(-ENODEV);
 
-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
-		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
+	memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
 
 	/* if info is obtained from pblk/fadt, type equals state */
 	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
@@ -536,8 +533,8 @@
 		return_VALUE(-ENODEV);
 
 	pr->power.count = 0;
-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
-		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
+
+	memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
 
 	status = acpi_evaluate_object(pr->handle, "_CST", NULL, &buffer);
 	if (ACPI_FAILURE(status)) {
@@ -609,6 +606,27 @@
 
 		cx.type = obj->integer.value;
 
+		//Test if the first entry is a C1 state, if not we fake one
+		if ((cx.type != ACPI_STATE_C1) && i == 1) {
+				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Faking C1 State"));
+				struct acpi_processor_cx c1_fake;
+				memset(&c1_fake, 0, sizeof(c1_fake));
+
+				c1_fake.address = 0;
+				c1_fake.type = ACPI_STATE_C1;
+				c1_fake.latency = 0; //How long needs a HLT instruction to execute ?
+				c1_fake.power = 0; //We don't know anything about the power consumption so we set it to 0
+				//We copy our fake C1 state over to the power states
+				(pr->power.count)++;
+				memcpy(&(pr->power.states[pr->power.count]), &c1_fake, sizeof(c1_fake));
+				//Finally we avoid to have more power states than ACPI_PROCESSOR_MAX_POWER
+				if(count == ACPI_PROCESSOR_MAX_POWER)
+				  count--;
+				
+				//c1_fake should be freed automaticaly
+		}
+
+
 		if ((cx.type != ACPI_STATE_C1) &&
 		    (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO))
 			continue;

Re: _CST Patches

[Bruno Ducrot]

On Mon, Apr 18, 2005 at 02:16:25PM +0200, Janosch Machowinski wrote:
> 
> > > This patch fakes an C1 state if the given states by the _CST object
> > > start with anything else than C1 (like on my M6NE here it start's with
> > > C2)
> > 
> > It should not be needed.  I've taken care that it work by testing
> > in almost all cases by overriding the DSDT when I wrote this part to
> > handle properly almost all cases (I forgot the case when C1 and C3, but
> > no C2 though, that why I wrote 'almost').
> > 
> > Normally, we register a C1 state no matter what before we check
> > the content of _CST, then we just skip if a C1 type is found.
> > 
> > If your patch is needed, then the patch I send some times ago have not been
> > fully integrated in the original form, or maybe the wrong one was considered
> > (the first was buggy in that regard, but the second was OK).
> > 
> Your first patch has been modified, and now my patch is needed. 
> In your first implementation you did an (pr->power)++ before the loop
> that started the polling of the _CST object.

Since C1 is not handled by the loop itself, it is needed to count it
before the loop.

> At the moment all C-States
> are deleted (see the memset) and generated from scratch.

I see...  Sound like it has been modified.  I don't know why.

> Your idea
> sounds faster to me !
> 
> 
> > I would prefer
> > memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
> > (without the loop)
> 
> I agree, looks faster to me. 
> 
> A new patch is attached
> 
>   Janosch

> --- processor_idle.original	2005-04-16 17:13:44.000000000 +0200
> +++ processor_idle.c	2005-04-18 14:13:52.000000000 +0200
> @@ -479,8 +479,6 @@
>  
>  static int acpi_processor_get_power_info_fadt (struct acpi_processor *pr)
>  {
> -	int i;
> -
>  	ACPI_FUNCTION_TRACE("acpi_processor_get_power_info_fadt");
>  
>  	if (!pr)
> @@ -489,8 +487,7 @@
>  	if (!pr->pblk)
>  		return_VALUE(-ENODEV);
>  
> -	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
> -		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
> +	memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
>  
>  	/* if info is obtained from pblk/fadt, type equals state */
>  	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
> @@ -536,8 +533,8 @@
>  		return_VALUE(-ENODEV);
>  
>  	pr->power.count = 0;
> -	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
> -		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
> +
> +	memset(pr->power.states, 0, ACPI_PROCESSOR_MAX_POWER * sizeof(struct acpi_processor_cx));
>  
>  	status = acpi_evaluate_object(pr->handle, "_CST", NULL, &buffer);
>  	if (ACPI_FAILURE(status)) {
> @@ -609,6 +606,27 @@
>  
>  		cx.type = obj->integer.value;
>  
> +		//Test if the first entry is a C1 state, if not we fake one
> +		if ((cx.type != ACPI_STATE_C1) && i == 1) {
> +				ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Faking C1 State"));
> +				struct acpi_processor_cx c1_fake;
> +				memset(&c1_fake, 0, sizeof(c1_fake));
> +
> +				c1_fake.address = 0;
> +				c1_fake.type = ACPI_STATE_C1;
> +				c1_fake.latency = 0; //How long needs a HLT instruction to execute ?
> +				c1_fake.power = 0; //We don't know anything about the power consumption so we set it to 0
> +				//We copy our fake C1 state over to the power states
> +				(pr->power.count)++;
> +				memcpy(&(pr->power.states[pr->power.count]), &c1_fake, sizeof(c1_fake));
> +				//Finally we avoid to have more power states than ACPI_PROCESSOR_MAX_POWER
> +				if(count == ACPI_PROCESSOR_MAX_POWER)
> +				  count--;
> +				
> +				//c1_fake should be freed automaticaly
> +		}
> +
> +
>  		if ((cx.type != ACPI_STATE_C1) &&
>  		    (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO))
>  			continue;


-- 
Bruno Ducrot

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click