PATCH default C1

PATCH default C1

[Janosch Machowinski]

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

Hey,
please test and include this patch.
It sets up an C1 state per default
and ignores all C1-states given by
_CST and P_BLK. Also it does some
cleanup.
    Janosch

[-- Attachment #2: mypatch.patch --]
[-- Type: text/plain, Size: 5045 bytes --]

--- linux-2.6.13/drivers/acpi/processor_idle.c	2005-09-02 22:36:45.000000000 +0200
+++ linux-2.6.13-patch/drivers/acpi/processor_idle.c	2005-09-02 22:36:09.000000000 +0200
@@ -513,20 +513,16 @@
 
 	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));
+	
+	/* We got C0 an C1 by default starting at 2 */
+	for (i = 2; i < ACPI_PROCESSOR_MAX_POWER; i++)
+		memset(&(pr->power.states[i]), 0, 
+		       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;
 	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
 	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
 
-	/* the C0 state only exists as a filler in our array,
-	 * and all processors need to support C1 */
-	pr->power.states[ACPI_STATE_C0].valid = 1;
-	pr->power.states[ACPI_STATE_C1].valid = 1;
-
 	/* determine C2 and C3 address from pblk */
 	pr->power.states[ACPI_STATE_C2].address = pr->pblk + 4;
 	pr->power.states[ACPI_STATE_C3].address = pr->pblk + 5;
@@ -554,10 +550,8 @@
 		memset(&(pr->power.states[i]), 0, 
 		       sizeof(struct acpi_processor_cx));
 
-	/* if info is obtained from pblk/fadt, type equals state */
+	/* set the first C-State to C1 */
 	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
-	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
-	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
 
 	/* the C0 state only exists as a filler in our array,
 	 * and all processors need to support C1 */
@@ -581,8 +575,11 @@
 	if (nocst)
 		return_VALUE(-ENODEV);
 
-	pr->power.count = 0;
-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
+	/*We allready have C1 */
+	pr->power.count = 1;
+	
+	/* We don't want to delete C0 and C1 so we start at 2 */
+	for (i = 2; i < ACPI_PROCESSOR_MAX_POWER; i++)
 		memset(&(pr->power.states[i]), 0, 
 		       sizeof(struct acpi_processor_cx));
 
@@ -610,13 +607,6 @@
 		goto end;
 	}
 
-	/* We support up to ACPI_PROCESSOR_MAX_POWER. */
-	if (count > ACPI_PROCESSOR_MAX_POWER) {
-		printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
-		printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
-		count = ACPI_PROCESSOR_MAX_POWER;
-	}
-
 	/* Tell driver that at least _CST is supported. */
 	pr->flags.has_cst = 1;
 
@@ -660,7 +650,8 @@
 		    (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO))
 			continue;
 
-		if ((cx.type < ACPI_STATE_C1) ||
+		/* We got C1 by default, so we only accept C2 and C3 states */
+		if ((cx.type < ACPI_STATE_C2) ||
 		    (cx.type > ACPI_STATE_C3))
 			continue;
 
@@ -678,6 +669,13 @@
 
 		(pr->power.count)++;
 		memcpy(&(pr->power.states[pr->power.count]), &cx, sizeof(cx));
+
+		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
+		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
+		printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
+		printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
+		break;
+		}
 	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n", pr->power.count));
@@ -796,6 +794,11 @@
 	 */
 	cx->valid = 1;
 	cx->latency_ticks = US_TO_PM_TIMER_TICKS(cx->latency);
+	
+	/*
+	 *  mark this CPU as being "idle manageable"
+	 */
+	pr->flags.power = 1;
 
 	return_VOID;
 }
@@ -806,16 +809,19 @@
 	unsigned int i;
 	unsigned int working = 0;
 
-	for (i=1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
+	/* C1 is allways valid, starting at 2 */
+	for (i=2; i < ACPI_PROCESSOR_MAX_POWER; i++) {
 		struct acpi_processor_cx *cx = &pr->power.states[i];
 
 		switch (cx->type) {
-		case ACPI_STATE_C1:
-			cx->valid = 1;
-			break;
-
 		case ACPI_STATE_C2:
 			acpi_processor_power_verify_c2(cx);
+			if (cx->valid) {
+			  /*
+			   *  mark this CPU as being "idle manageable"
+			   */
+			  pr->flags.power = 1;
+			}
 			break;
 
 		case ACPI_STATE_C3:
@@ -833,19 +839,19 @@
 static int acpi_processor_get_power_info (
 	struct acpi_processor	*pr)
 {
-	unsigned int i;
 	int result;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_get_power_info");
 
 	/* NOTE: the idle thread may not be running while calling
 	 * this function */
-
+	
+	/* Adding C1 state */
+	acpi_processor_get_power_info_default_c1(pr);
 	result = acpi_processor_get_power_info_cst(pr);
-	if ((result) || (acpi_processor_power_verify(pr) < 2)) {
+	if ((result) || (!acpi_processor_power_verify(pr))) {
 		result = acpi_processor_get_power_info_fadt(pr);
-		if ((result) || (acpi_processor_power_verify(pr) < 2))
-			result = acpi_processor_get_power_info_default_c1(pr);
+		acpi_processor_power_verify(pr);
 	}
 
 	/*
@@ -860,17 +866,6 @@
 	if (result)
 		return_VALUE(result);
 
-	/*
-	 * if one state of type C2 or C3 is available, mark this
-	 * CPU as being "idle manageable"
-	 */
-	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
-		if (pr->power.states[i].valid) {
-			pr->power.count = i;
-			pr->flags.power = 1;
-		}
-	}
-
 	return_VALUE(0);
 }
 

Re: PATCH default C1

[Dominik Brodowski]

> +		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
> +		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
> +		printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
> +		printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
> +		break;
> +		}

broken indent

> +			if (cx->valid) {
> +			  /*
> +			   *  mark this CPU as being "idle manageable"
> +			   */
> +			  pr->flags.power = 1;
> +			}

same here.

	Dominik


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf

Re: PATCH default C1

[Janosch Machowinski]

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

Same patch with corrected codestyle,
please test and apply
     Janosch

Dominik Brodowski wrote:
>>+		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
>>+		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
>>+		printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
>>+		printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
>>+		break;
>>+		}
> 
> 
> broken indent
> 
> 
>>+			if (cx->valid) {
>>+			  /*
>>+			   *  mark this CPU as being "idle manageable"
>>+			   */
>>+			  pr->flags.power = 1;
>>+			}
> 
> 
> same here.
> 
> 	Dominik

[-- Attachment #2: mypatch.patch --]
[-- Type: text/plain, Size: 5065 bytes --]

--- linux-2.6.13/drivers/acpi/processor_idle.c	2005-09-02 21:49:32.000000000 +0200
+++ linux-2.6.13-patch/drivers/acpi/processor_idle.c	2005-09-14 17:16:13.000000000 +0200
@@ -513,20 +513,16 @@
 
 	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));
+	
+	/* We got C0 an C1 by default starting at 2 */
+	for (i = 2; i < ACPI_PROCESSOR_MAX_POWER; i++)
+		memset(&(pr->power.states[i]), 0, 
+		       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;
 	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
 	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
 
-	/* the C0 state only exists as a filler in our array,
-	 * and all processors need to support C1 */
-	pr->power.states[ACPI_STATE_C0].valid = 1;
-	pr->power.states[ACPI_STATE_C1].valid = 1;
-
 	/* determine C2 and C3 address from pblk */
 	pr->power.states[ACPI_STATE_C2].address = pr->pblk + 4;
 	pr->power.states[ACPI_STATE_C3].address = pr->pblk + 5;
@@ -554,10 +550,8 @@
 		memset(&(pr->power.states[i]), 0, 
 		       sizeof(struct acpi_processor_cx));
 
-	/* if info is obtained from pblk/fadt, type equals state */
+	/* set the first C-State to C1 */
 	pr->power.states[ACPI_STATE_C1].type = ACPI_STATE_C1;
-	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
-	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
 
 	/* the C0 state only exists as a filler in our array,
 	 * and all processors need to support C1 */
@@ -581,8 +575,11 @@
 	if (nocst)
 		return_VALUE(-ENODEV);
 
-	pr->power.count = 0;
-	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
+	/*We allready have C1 */
+	pr->power.count = 1;
+	
+	/* We don't want to delete C0 and C1 so we start at 2 */
+	for (i = 2; i < ACPI_PROCESSOR_MAX_POWER; i++)
 		memset(&(pr->power.states[i]), 0, 
 		       sizeof(struct acpi_processor_cx));
 
@@ -610,13 +607,6 @@
 		goto end;
 	}
 
-	/* We support up to ACPI_PROCESSOR_MAX_POWER. */
-	if (count > ACPI_PROCESSOR_MAX_POWER) {
-		printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
-		printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
-		count = ACPI_PROCESSOR_MAX_POWER;
-	}
-
 	/* Tell driver that at least _CST is supported. */
 	pr->flags.has_cst = 1;
 
@@ -660,7 +650,8 @@
 		    (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO))
 			continue;
 
-		if ((cx.type < ACPI_STATE_C1) ||
+		/* We got C1 by default, so we only accept C2 and C3 states */
+		if ((cx.type < ACPI_STATE_C2) ||
 		    (cx.type > ACPI_STATE_C3))
 			continue;
 
@@ -678,6 +669,13 @@
 
 		(pr->power.count)++;
 		memcpy(&(pr->power.states[pr->power.count]), &cx, sizeof(cx));
+
+		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
+		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
+		        printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
+			printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
+			break;
+		}
 	}
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n", pr->power.count));
@@ -796,6 +794,11 @@
 	 */
 	cx->valid = 1;
 	cx->latency_ticks = US_TO_PM_TIMER_TICKS(cx->latency);
+	
+	/*
+	 *  mark this CPU as being "idle manageable"
+	 */
+	pr->flags.power = 1;
 
 	return_VOID;
 }
@@ -806,16 +809,19 @@
 	unsigned int i;
 	unsigned int working = 0;
 
-	for (i=1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
+	/* C1 is allways valid, starting at 2 */
+	for (i=2; i < ACPI_PROCESSOR_MAX_POWER; i++) {
 		struct acpi_processor_cx *cx = &pr->power.states[i];
 
 		switch (cx->type) {
-		case ACPI_STATE_C1:
-			cx->valid = 1;
-			break;
-
 		case ACPI_STATE_C2:
 			acpi_processor_power_verify_c2(cx);
+			if (cx->valid) {
+			        /*
+				 *  mark this CPU as being "idle manageable"
+				 */
+			        pr->flags.power = 1;
+			}
 			break;
 
 		case ACPI_STATE_C3:
@@ -833,19 +839,19 @@
 static int acpi_processor_get_power_info (
 	struct acpi_processor	*pr)
 {
-	unsigned int i;
 	int result;
 
 	ACPI_FUNCTION_TRACE("acpi_processor_get_power_info");
 
 	/* NOTE: the idle thread may not be running while calling
 	 * this function */
-
+	
+	/* Adding C1 state */
+	acpi_processor_get_power_info_default_c1(pr);
 	result = acpi_processor_get_power_info_cst(pr);
-	if ((result) || (acpi_processor_power_verify(pr) < 2)) {
+	if ((result) || (!acpi_processor_power_verify(pr))) {
 		result = acpi_processor_get_power_info_fadt(pr);
-		if ((result) || (acpi_processor_power_verify(pr) < 2))
-			result = acpi_processor_get_power_info_default_c1(pr);
+		acpi_processor_power_verify(pr);
 	}
 
 	/*
@@ -860,17 +866,6 @@
 	if (result)
 		return_VALUE(result);
 
-	/*
-	 * if one state of type C2 or C3 is available, mark this
-	 * CPU as being "idle manageable"
-	 */
-	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
-		if (pr->power.states[i].valid) {
-			pr->power.count = i;
-			pr->flags.power = 1;
-		}
-	}
-
 	return_VALUE(0);
 }
 

Re: PATCH default C1

[Dominik Brodowski]

Hi,


On Fri, Sep 16, 2005 at 09:03:36PM +0200, Janosch Machowinski wrote:
> @@ -678,6 +669,13 @@
>  
>  		(pr->power.count)++;
>  		memcpy(&(pr->power.states[pr->power.count]), &cx, sizeof(cx));
> +
> +		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
> +		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
> +		        printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
> +			printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
> +			break;
> +		}


Memory corruption: If pr->power.count is ACPI_PROCESSOR_MAX_POWER (or
larger), pr->power.states[pr->power.count] is pointing to an invalid memory
address. Therefore, the check needs to stay at the place it was before.


	Dominik


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. 
Download it for free - -and be entered to win a 42" plasma tv or your very
own Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php

Re: PATCH default C1

[Janosch Machowinski]

Dominik Brodowski schrieb:
> Hi,
> 
> 
> On Fri, Sep 16, 2005 at 09:03:36PM +0200, Janosch Machowinski wrote:
> 
>>@@ -678,6 +669,13 @@
>> 
>> 		(pr->power.count)++;
>> 		memcpy(&(pr->power.states[pr->power.count]), &cx, sizeof(cx));
>>+
>>+		/* We support up to ACPI_PROCESSOR_MAX_POWER. */
>>+		if (pr->power.count >= ACPI_PROCESSOR_MAX_POWER) {
>>+		        printk(KERN_WARNING "Limiting number of power states to max (%d)\n", ACPI_PROCESSOR_MAX_POWER);
>>+			printk(KERN_WARNING "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
>>+			break;
>>+		}
> 
> 
> 
> Memory corruption: If pr->power.count is ACPI_PROCESSOR_MAX_POWER (or
> larger), pr->power.states[pr->power.count] is pointing to an invalid memory
> address. Therefore, the check needs to stay at the place it was before.
> 
Hu ?
I think teh check is just right where I placed it...
Following case :
A CST contains 9 C-states,
MAX_POWER = 8.
One C-State is invallid (not the last one)
In this case we would only detect 7 C-states (with the check in old place)
pr->power.count tells us how many C-States where actually discovered, 
that's why I moved the check.

But you're right, it runs out of the array...
what about ?
Is there a nicer way to do this ?
+		if (pr->power.count >= (ACPI_PROCESSOR_MAX_POWER -1)) {

    Janosch


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. 
Download it for free - -and be entered to win a 42" plasma tv or your very
own Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php