PATCH default C1

PATCH Janosch Machowinski 2 participants 5 messages Sep 13, 2005 – Sep 17, 2005 4 messages not in the archive ACPI

Share & tools

Janosch Machowinski

author

2005-09-13 09:11 UTC #0 lore raw

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

drivers/acpi/processor_idle.c +38 ~~-43~~

@@ -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);
 }

-------------------------------------------------------
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

Janosch Machowinski

author

2005-09-16 19:03 UTC #4 lore raw

In reply to a message not in the archive

Same patch with corrected codestyle,
please test and apply
Janosch

Quoted 21 lines · Dominik Brodowski

Review

Quoted 6 lines

(quoted diff) +6 -0

+		/* 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

Review

Quoted 6 lines

(quoted diff) +6 -0

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

same here.

Quoted 1 line

Dominik

drivers/acpi/processor_idle.c +38 ~~-43~~

@@ -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);
 }

Dominik Brodowski

2005-09-17 09:13 UTC #6 lore raw

In reply to a message not in the archive

Hi,

drivers/acpi/processor_idle.c line 678

Quoted 11 lines · Janosch Machowinski

drivers/acpi/processor_idle.c +7 -0

@@ -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

Janosch Machowinski

author

2005-09-17 11:29 UTC #8 lore raw

In reply to a message not in the archive

Quoted 7 lines · Dominik Brodowski

Hi,
Quoted 2 lines · Janosch Machowinski
(quoted diff) +0 -0
@@ -678,6 +669,13 @@
 

Quoted 16 lines

drivers/acpi/processor_idle.c line 678
Quoted 9 lines
drivers/acpi/processor_idle.c +7 -0
 		(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