Re: Linux ACPI processor driver patch: user-definable power state limit

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

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

Jos,
Can you modify this patch to use a kernel boot parameter
rather than a module load parameter?  Some folks don't
load this driver as a module.  Also, it gives you the
chance to add a couple of lines to Documentation/kernel-parameters.txt

thanks,
-Len

On Sat, 2004-08-07 at 13:59, Jos Delbar wrote:
> On Monday 02 August 2004 02:54, you wrote:
> > I think we need a patch like this for a number of reasons.
> > Some systems experience data corruption with C3,
> > so until it is fixed we need an easy way to disable C3.
> >
> > boot parameter names must begin with acpi though.
> > maybe acpi_cstate_limit=3 or something?
> >
> > Please send me the exact model numbers of the Dells
> > that you know have this problem.  I met a guy from
> > Dell who I may be able to get to help us with
> > those models.
> >
> > thanks,
> > -Len
> 
> I browsed through the Dell community forums and I believe it is safe
> to 
> conclude that at least the following notebook models may have this
> problem:
> 
> - Inspiron 300m, 500m, 600m
> - Inspiron 4000, 4100, 4150
> - Inspiron 8100, 8200, 8500, 8600
> - Latitude C840
> - Latitude D500, D600, D800
> 
> As expected, all of these systems use a mobile processor chipset, with
> either 
> an Intel Celeron-M, Pentium III-M, Pentium 4-M or Pentium M processor.
> All of 
> the people who have posted about this buzzing noise are using an
> ACPI-enabled 
> operating system, the majority Windows 2000/XP. Windows 98 does not
> have this 
> problem, for example. On systems that support it, disabling the power 
> management of the USB Root Hub(s) apparently softens--but does not 
> eliminate--the buzzing.
> 
> I updated the patch with the new module parameter name
> "acpi_cstate_limit" and 
> cleaned up the results of /proc/acpi/processor/.../power a little. The
> output 
> using acpi_cstate_limit=2 will now resemble:
> 
> active state: C2
> default state: C1
> user limit: C2
> bus master activity: ffffffff
> states:
> C1: promotion[C2] demotion[--] latency[000] usage[00300510]
> *C2: promotion[--] demotion[C1] latency[050] usage[04964774]
> C3: <disabled>
> 
> If acpi_cstate_limit is passed as a kernel boot parameter, the kernel
> ring 
> buffer will reflect this. For example, using
> "processor.acpi_cstate_limit=1", 
> the relevant output for my system is:
> 
> ACPI: Processor [CPU0] (supports C0 C1, 8 throttling states)
> 
> I chose to always display C0 so this message will still make sense
> when using 
> acpi_cstate_limit=0.
> 
> One of the changes that I made, is unrelated to the C state limit.
> 
> @@ -2417,7 +2446,7 @@
>         pr = (struct acpi_processor *) acpi_driver_data(device);
> 
>         /* Unregister the idle handler when processor #0 is removed.
> */
> -       if (pr->id == 0)
> +       if ((pr->id == 0) && (pr->flags.power))
>                 pm_idle = pm_idle_save;
> 
>         status = acpi_remove_notify_handler(pr->handle,
> ACPI_DEVICE_NOTIFY,
> 
> Without the check for pr->flags.power, it's possible that the old idle
> handler 
> would be "restored" without ever being stored in the function 
> acpi_processor_add(). In practice this shouldn't cause a problem since
> the 
> pm_idle_save global variable is implicitly initialized to NULL, but it
> feels 
> safer to do the check anyway.
> 
> Regards,
> 
> - Jos Delbar
>   jos.delbar-Cru1EgDzd7c@public.gmane.org
> 

[-- Attachment #2: processor.c.diff --]
[-- Type: text/plain, Size: 4384 bytes --]

--- /usr/src/linux/drivers/acpi/processor.c	2004-08-07 18:25:15.064497551 +0200
+++ processor.c	2004-08-07 18:31:24.013809421 +0200
@@ -30,6 +30,14 @@
  *	4. Need C1 timing -- must modify kernel (IRQ handler) to get this.
  */
 
+/*
+ * 07/08/04; Jos Delbar <jos.delbar-Cru1EgDzd7c@public.gmane.org>:
+ * Recognize power state limit as a boot parameter.
+ *
+ * 31/07/04; Jos Delbar <jos.delbar-Cru1EgDzd7c@public.gmane.org>:
+ * Add user-definable processor power state limit.
+ */
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -80,6 +88,16 @@
 MODULE_DESCRIPTION(ACPI_PROCESSOR_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
+/*
+ * The acpi_cstate_limit module parameter represents the maximum processor power state or
+ * C state to promote to. Values of 1, 2 and 3 are equivalent to the C1, C2 and C3 sleep
+ * states, respectively. A value of 0 disables processor power management altogether.
+ */
+
+static int acpi_cstate_limit = ACPI_C_STATES_MAX;
+
+module_param(acpi_cstate_limit, int, S_IRUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(acpi_cstate_limit, "Limit the processor power state (0 = disable power management)");
 
 static int acpi_processor_add (struct acpi_device *device);
 static int acpi_processor_remove (struct acpi_device *device, int type);
@@ -449,6 +467,7 @@
 		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C3_OVERHEAD;
 		break;
 
+	case ACPI_STATE_C0:
 	default:
 		local_irq_enable();
 		return;
@@ -535,7 +554,7 @@
 	 * C0/C1
 	 * -----
 	 */
-	pr->power.state = ACPI_STATE_C1;
+	pr->power.state = (acpi_cstate_limit != ACPI_STATE_C0 ? ACPI_STATE_C1 : ACPI_STATE_C0);
 	pr->power.default_state = ACPI_STATE_C1;
 
 	/*
@@ -554,7 +573,7 @@
 	 * TBD: Demote to default C-State after long periods of activity.
 	 * TBD: Investigate policy's use of CPU utilization -vs- sleep duration.
 	 */
-	if (pr->power.states[ACPI_STATE_C2].valid) {
+	if (pr->power.states[ACPI_STATE_C2].valid && (acpi_cstate_limit >= ACPI_STATE_C2)) {
 		pr->power.states[ACPI_STATE_C1].promotion.threshold.count = 10;
 		pr->power.states[ACPI_STATE_C1].promotion.threshold.ticks =
 			pr->power.states[ACPI_STATE_C2].latency_ticks;
@@ -575,7 +594,7 @@
 	 * short or whenever bus mastering activity occurs.
 	 */
 	if ((pr->power.states[ACPI_STATE_C2].valid) &&
-		(pr->power.states[ACPI_STATE_C3].valid)) {
+		(pr->power.states[ACPI_STATE_C3].valid) && (acpi_cstate_limit >= ACPI_STATE_C3)) {
 		pr->power.states[ACPI_STATE_C2].promotion.threshold.count = 4;
 		pr->power.states[ACPI_STATE_C2].promotion.threshold.ticks =
 			pr->power.states[ACPI_STATE_C3].latency_ticks;
@@ -1859,10 +1878,15 @@
 		goto end;
 
 	seq_printf(seq, "active state:            C%d\n"
-			"default state:           C%d\n"
-			"bus master activity:     %08x\n",
+			"default state:           C%d\n",
 			pr->power.state,
-			pr->power.default_state,
+			pr->power.default_state);
+
+	if (acpi_cstate_limit < ACPI_C_STATES_MAX) seq_printf(seq,
+			"user limit:              C%d\n",
+			acpi_cstate_limit);
+
+	seq_printf(seq, "bus master activity:     %08x\n",
 			pr->power.bm_activity);
 
 	seq_puts(seq, "states:\n");
@@ -1876,6 +1900,11 @@
 			continue;
 		}
 
+		if (i > acpi_cstate_limit) {
+			seq_puts(seq, "<disabled>\n");
+			continue;
+		}
+
 		if (pr->power.states[i].promotion.state)
 			seq_printf(seq, "promotion[C%d] ",
 				pr->power.states[i].promotion.state);
@@ -2384,7 +2413,7 @@
 	
 	printk(KERN_INFO PREFIX "%s [%s] (supports",
 		acpi_device_name(device), acpi_device_bid(device));
-	for (i=1; i<ACPI_C_STATE_COUNT; i++)
+	for (i=0; i<=acpi_cstate_limit; i++)
 		if (pr->power.states[i].valid)
 			printk(" C%d", i);
 	if (pr->flags.throttling)
@@ -2417,7 +2446,7 @@
 	pr = (struct acpi_processor *) acpi_driver_data(device);
 
 	/* Unregister the idle handler when processor #0 is removed. */
-	if (pr->id == 0)
+	if ((pr->id == 0) && (pr->flags.power))
 		pm_idle = pm_idle_save;
 
 	status = acpi_remove_notify_handler(pr->handle, ACPI_DEVICE_NOTIFY, 
@@ -2447,6 +2476,10 @@
 	memset(&processors, 0, sizeof(processors));
 	memset(&errata, 0, sizeof(errata));
 
+	/* Check for illegal user limits. */
+	if(acpi_cstate_limit < ACPI_STATE_C0 || acpi_cstate_limit > ACPI_C_STATES_MAX)
+		return_VALUE(-EINVAL);
+
 	acpi_processor_dir = proc_mkdir(ACPI_PROCESSOR_CLASS, acpi_root_dir);
 	if (!acpi_processor_dir)
 		return_VALUE(-ENODEV);

Re: Linux ACPI processor driver patch: user-definable power state limit

[Jos Delbar]

As Dominik Brodowski pointed out, the module parameter currently exported in 
this patch should be sufficient. I have the ACPI processor module compiled 
into my kernel, yet I can still use "processor.acpi_cstate_limit" on the boot 
line.

Do you require a new version of the patch without the "acpi_" prefix?

On Saturday 09 October 2004 07:52, you wrote:
> Jos,
> Can you modify this patch to use a kernel boot parameter
> rather than a module load parameter?  Some folks don't
> load this driver as a module.  Also, it gives you the
> chance to add a couple of lines to Documentation/kernel-parameters.txt
>
> thanks,
> -Len

-- 

Jos Delbar
jos.delbar-Cru1EgDzd7c@public.gmane.org


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

RE: Linux ACPI processor driver patch: user-definable power state limit

[Brown, Len]

 
>Do you require a new version of the patch without the "acpi_" prefix?

sure, and if you can re-send a version that applies cleanly
to the current tree, that would be great.

thanks,
-Len

ps. FYI, if patches are generated with "diff -Naur from "/usr/src/linux"
then that is easier for me to deal with.  eg.

a/drivers/acpi/processor.c
vs.
b/drivers/acpi/processor.c

also, attachments are ok.


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: Linux ACPI processor driver patch: user-definable power state limit

[Jos Delbar]

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

Here is a new version of the power state limit patch that applies to ACPI 
20040816. I also fixed a few bugs I bumped into along the way...

in acpi_processor_get_power_info_cst()

> @@ -671,7 +696,7 @@
>  
>         /* The first state is always C0, which is already filled. */
>         pr->power.count = 1;
> -       for (i = 1; i <= count; i++) {
> +       for (i = 1; i < count; i++) {
>                 union acpi_object *element;
>                 union acpi_object *obj;
>                 struct acpi_power_register *reg;

The count value extracted from _CST is the number of the highest C state plus 
1 and is not to be confused with the meaning of the 
acpi_processor_power.count member (pr->power.count) which is simply the 
highest C state. An extra iteration was made creating an invalid C4 state for 
my processor, visible in /proc/acpi/processor/CPU0/power.

I think the current usage of acpi_processor_power.count is misleading and 
propose to either rename the member to something like "max_state", or to give 
the member a new meaning as the highest C state plus 1 and rework the code to 
reflect that change.

in acpi_processor_get_power_info_fadt()

> @@ -901,7 +926,7 @@
>          */
>  
>         for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
> -               memset(pr->power.states, 0, sizeof(struct 
acpi_processor_cx));
> +               memset(&pr->power.states[i], 0, sizeof(struct 
acpi_processor_cx));
>  
>         if (pr->pblk) {
>                 pr->power.states[ACPI_STATE_C2].address = pr->pblk + 4;

The acpi_processor_power.states (pr->power.states) array was not properly 
initialized.

- Jos

On Sunday 10 October 2004 18:56, you wrote:
> >Do you require a new version of the patch without the "acpi_" prefix?
>
> sure, and if you can re-send a version that applies cleanly
> to the current tree, that would be great.
>
> thanks,
> -Len
>
> ps. FYI, if patches are generated with "diff -Naur from "/usr/src/linux"
> then that is easier for me to deal with.  eg.
>
> a/drivers/acpi/processor.c
> vs.
> b/drivers/acpi/processor.c
>
> also, attachments are ok.

-- 

Jos Delbar
jos.delbar-Cru1EgDzd7c@public.gmane.org

[-- Attachment #2: processor.c.diff --]
[-- Type: text/x-diff, Size: 6070 bytes --]

--- a/drivers/acpi/processor.c	2004-10-11 22:09:52.000000000 +0200
+++ b/drivers/acpi/processor.c	2004-10-11 22:56:06.666140905 +0200
@@ -30,6 +30,13 @@
  *	4. Need C1 timing -- must modify kernel (IRQ handler) to get this.
  */
 
+/*
+ * 11/10/2004; Jos Delbar <jos.delbar-Cru1EgDzd7c@public.gmane.org>
+ * Add user-definable processor power state limit (cstate_limit).
+ * Various bug fixes.
+ * Patch applies to acpi-20040816-26-stable.
+ */
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -80,6 +87,17 @@
 MODULE_DESCRIPTION(ACPI_PROCESSOR_DRIVER_NAME);
 MODULE_LICENSE("GPL");
 
+/*
+ * The cstate_limit module parameter represents the maximum processor power state or C
+ * state to promote to. Values of 1, 2 and 3 are equivalent to the C1, C2 and C3 sleep
+ * states, respectively. A value of 0 disables processor power management altogether.
+ */
+
+static int cstate_limit = ACPI_C_STATES_MAX;
+
+module_param(cstate_limit, int, S_IRUSR | S_IRGRP | S_IROTH);
+MODULE_PARM_DESC(cstate_limit, "Limit the processor power state (0 = disable power management)");
+
 
 static int acpi_processor_add (struct acpi_device *device);
 static int acpi_processor_remove (struct acpi_device *device, int type);
@@ -454,6 +472,7 @@
 		sleep_ticks = ticks_elapsed(t1, t2) - cx->latency_ticks - C3_OVERHEAD;
 		break;
 
+	case ACPI_STATE_C0:
 	default:
 		local_irq_enable();
 		return;
@@ -544,7 +563,7 @@
 	 * C0/C1
 	 * -----
 	 */
-	pr->power.state = ACPI_STATE_C1;
+	pr->power.state = (cstate_limit != ACPI_STATE_C0 ? ACPI_STATE_C1 : ACPI_STATE_C0);
 	pr->power.default_state = ACPI_STATE_C1;
 
 	for (i = 0; i <= pr->power.count; i++) {
@@ -571,15 +590,18 @@
 		 * TBD: Investigate policy's use of CPU utilization -vs- sleep duration.
 		 * XXX update comment.
 		 */
-		pr->power.states[i-1].promotion.threshold.count = 10;
-		pr->power.states[i-1].promotion.threshold.ticks =
-			pr->power.states[i].latency_ticks;
-		pr->power.states[i-1].promotion.state = i;
-
-		pr->power.states[i].demotion.threshold.count = 1;
-		pr->power.states[i].demotion.threshold.ticks =
-			pr->power.states[i].latency_ticks;
-		pr->power.states[i].demotion.state = i-1;
+		if (cstate_limit >= ACPI_STATE_C2)
+		{
+			pr->power.states[i-1].promotion.threshold.count = 10;
+			pr->power.states[i-1].promotion.threshold.ticks =
+				pr->power.states[i].latency_ticks;
+			pr->power.states[i-1].promotion.state = i;
+	
+			pr->power.states[i].demotion.threshold.count = 1;
+			pr->power.states[i].demotion.threshold.ticks =
+				pr->power.states[i].latency_ticks;
+			pr->power.states[i].demotion.state = i-1;
+		}
 			break;
   
 		case ACPI_STATE_C3:
@@ -593,17 +615,20 @@
 		 *
 		 * XXX update comment.
 		 */
-		pr->power.states[i-1].promotion.threshold.count = 4;
-		pr->power.states[i-1].promotion.threshold.ticks =
-			pr->power.states[i].latency_ticks;
-		pr->power.states[i-1].promotion.threshold.bm = 0x0F;
-		pr->power.states[i-1].promotion.state = i;
-
-		pr->power.states[i].demotion.threshold.count = 1;
-		pr->power.states[i].demotion.threshold.ticks =
-			pr->power.states[i].latency_ticks;
-		pr->power.states[i].demotion.threshold.bm = 0x0F;
-		pr->power.states[i].demotion.state = i-1;
+		if (cstate_limit >= ACPI_STATE_C3)
+		{
+			pr->power.states[i-1].promotion.threshold.count = 4;
+			pr->power.states[i-1].promotion.threshold.ticks =
+				pr->power.states[i].latency_ticks;
+			pr->power.states[i-1].promotion.threshold.bm = 0x0F;
+			pr->power.states[i-1].promotion.state = i;
+	
+			pr->power.states[i].demotion.threshold.count = 1;
+			pr->power.states[i].demotion.threshold.ticks =
+				pr->power.states[i].latency_ticks;
+			pr->power.states[i].demotion.threshold.bm = 0x0F;
+			pr->power.states[i].demotion.state = i-1;
+		}
 			break;
 		default:
 			return_VALUE(-EINVAL);
@@ -671,7 +696,7 @@
 
 	/* The first state is always C0, which is already filled. */
 	pr->power.count = 1;
-	for (i = 1; i <= count; i++) {
+	for (i = 1; i < count; i++) {
 		union acpi_object *element;
 		union acpi_object *obj;
 		struct acpi_power_register *reg;
@@ -901,7 +926,7 @@
 	 */
 
 	for (i = 0; i < ACPI_PROCESSOR_MAX_POWER; i++)
-		memset(pr->power.states, 0, sizeof(struct acpi_processor_cx));
+		memset(&pr->power.states[i], 0, sizeof(struct acpi_processor_cx));
 
 	if (pr->pblk) {
 		pr->power.states[ACPI_STATE_C2].address = pr->pblk + 4;
@@ -2185,10 +2210,15 @@
 		goto end;
 
 	seq_printf(seq, "active state:            C%d\n"
-			"default state:           C%d\n"
-			"bus master activity:     %08x\n",
+			"default state:           C%d\n",
 			pr->power.state,
-			pr->power.default_state,
+			pr->power.default_state);
+
+	if (cstate_limit < ACPI_C_STATES_MAX) seq_printf(seq,
+			"user limit:              C%d\n",
+			cstate_limit);
+
+	seq_printf(seq, "bus master activity:     %08x\n",
 			pr->power.bm_activity);
 
 	seq_puts(seq, "states:\n");
@@ -2204,6 +2234,11 @@
 
 		seq_printf(seq, "type[%d] ", pr->power.states[i].type);
 
+		if (i > cstate_limit) {
+			seq_puts(seq, "<disabled>\n");
+			continue;
+		}
+
 		if (pr->power.states[i].promotion.state)
 			seq_printf(seq, "promotion[C%d] ",
 				pr->power.states[i].promotion.state);
@@ -2727,7 +2762,7 @@
 
 	printk(KERN_INFO PREFIX "%s [%s] (supports",
 		acpi_device_name(device), acpi_device_bid(device));
-	for (i = 1; i <= pr->power.count; i++)
+	for (i = 0; i <= cstate_limit; i++)
 		if (pr->power.states[i].valid)
 			printk(" C%d", i);
 	if (pr->flags.throttling)
@@ -2770,7 +2805,7 @@
 	}
 
 	/* Unregister the idle handler when processor #0 is removed. */
-	if (pr->id == 0)
+	if ((pr->id == 0) && (pr->flags.power))
 		pm_idle = pm_idle_save;
 
 	acpi_processor_remove_fs(device);
@@ -2793,6 +2828,10 @@
 	memset(&processors, 0, sizeof(processors));
 	memset(&errata, 0, sizeof(errata));
 
+	/* Check for illegal user limits. */
+	if(cstate_limit < ACPI_STATE_C0 || cstate_limit > ACPI_C_STATES_MAX)
+		return_VALUE(-EINVAL);
+
 	acpi_processor_dir = proc_mkdir(ACPI_PROCESSOR_CLASS, acpi_root_dir);
 	if (!acpi_processor_dir)
 		return_VALUE(-ENODEV);

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

Jos,
I applied a simpler patch from Andi Kleen to disable c2/c3
http://bugme.osdl.org/show_bug.cgi?id=3549

Let me know if it works for you, or if you have any updates
on top of it.

thanks,
-Len




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

Jos,
I agree with you that a single parameter is simpler.

Another thing we need to address -- with either scheme --
is that the parameter must be set in the kernel, not
in the processor modules.

This is because it is necessary for modules, such as ipw2100
to be able to disable c3 automatically when they detect
that it is interfering with their operation.

so we'd export a function from the base kernel for
modules to set the limit, and we'd simply export
the value of acpi_cstate_limit for processor.c
to observe at run-time.

int
acpi_set_cstate_limit(int limit)

int acpi_cstate_limit;

I think it can return the old limit so that
the caller can potentially un-do its call,
or perhaps setting the limit to 0 should
simply mean clear any limit.

cheers,
-Len




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Dominik Brodowski]

On Fri, Nov 05, 2004 at 02:45:07PM -0500, Len Brown wrote:
> Jos,
> I agree with you that a single parameter is simpler.
> 
> Another thing we need to address -- with either scheme --
> is that the parameter must be set in the kernel, not
> in the processor modules.
> 
> This is because it is necessary for modules, such as ipw2100
> to be able to disable c3 automatically when they detect
> that it is interfering with their operation.
> 
> so we'd export a function from the base kernel for
> modules to set the limit, and we'd simply export
> the value of acpi_cstate_limit for processor.c
> to observe at run-time.
> 
> int
> acpi_set_cstate_limit(int limit)
> 
> int acpi_cstate_limit;
> 
> I think it can return the old limit so that
> the caller can potentially un-do its call,
> or perhaps setting the limit to 0 should
> simply mean clear any limit.

That sounds to be quite racy. Multiple callers will get confused.
You probably need a 

struct acpi_cstate_limit {
	list_head	next;
	unsigned int	limit;
}

and you export

struct *acpi_cstate_limimt acpi_set_cstate_limit(unsigned int limit);

int acpi_modify_cstate_limit(struct acpi_cstate_limit * cstate_limit);

void acpi_remove_cstate_limit(struct acpi_cstate_limit * cstate_limit);

Whenever such an operation occurs, you need to walk all these limit structs
for the highest "limit", and use that.

Other than that, the current "limit" interface to processor.c seems to be
pretty much useless as P-States aren't really handled.

I'll cook up a patch for the thing above in a few minutes... stay tuned.

Thanks,
	Dominik


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

Please try out this acpi_cstate_limit patch.

This applies to 2.6.8.1, but in the likely event
that may mailer wraps this patch or you need 2.6.9, go here: 
http://ftp.kernel.org/pub/linux/kernel/people/lenb/acpi/patches/test/cstate_limit/

thanks,
-Len

-----------
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/11/05 17:39:51-05:00 len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org 
#   [ACPI] Allow limiting idle C-States.
#   
#   Useful to workaround C3 ipw2100 packet loss
#   and resonating noises heard on some laptops.
#   
#   For static processor driver, boot cmdline:
#   processor.acpi_cstate_limit=2
#   
#   For processor module, /etc/modprobe.conf:
#   options processor acpi_cstate_limit=2
#   
#   For manual processor module load:
#   # modprobe processor acpi_cstate_limit=2
#   
#   From kernel or kernel module:
#   #include <linux/acpi.h>
#   acpi_set_cstate_limit(2);
#   
#   Inspired by patches from Jos Delbar and Andi Kleen
#   Signed-off-by: Len Brown <len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
# 
# include/linux/acpi.h
#   2004/11/05 17:39:45-05:00 len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org +26 -0
#   define API to set and get acpi_cstate_limit
# 
# drivers/acpi/processor.c
#   2004/11/05 17:39:45-05:00 len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org +10 -1
#   set and obey acpi_cstate_limit
# 
# drivers/acpi/osl.c
#   2004/11/05 17:39:45-05:00 len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org +9 -0
#   define acpi_cstate_limit
# 
diff -Nru a/drivers/acpi/osl.c b/drivers/acpi/osl.c
--- a/drivers/acpi/osl.c	2004-11-05 17:40:07 -05:00
+++ b/drivers/acpi/osl.c	2004-11-05 17:40:07 -05:00
@@ -1078,3 +1078,12 @@
 
 __setup("acpi_leave_gpes_disabled", acpi_leave_gpes_disabled_setup);
 
+/*
+ * acpi_cstate_limit is defined in the base kernel so modules can
+ * change it w/o depending on the state of the processor module.
+ */
+unsigned int acpi_cstate_limit = ACPI_C_STATES_MAX;
+
+
+EXPORT_SYMBOL(acpi_cstate_limit);
+
diff -Nru a/drivers/acpi/processor.c b/drivers/acpi/processor.c
--- a/drivers/acpi/processor.c	2004-11-05 17:40:07 -05:00
+++ b/drivers/acpi/processor.c	2004-11-05 17:40:07 -05:00
@@ -459,8 +459,9 @@
 	 * Track the number of longs (time asleep is greater than threshold)
 	 * and promote when the count threshold is reached.  Note that bus
 	 * mastering activity may prevent promotions.
+	 * Do not promote above acpi_cstate_limit.
 	 */
-	if (cx->promotion.state) {
+	if (cx->promotion.state && (cx->promotion.state <= acpi_cstate_limit))
{
 		if (sleep_ticks > cx->promotion.threshold.ticks) {
 			cx->promotion.count++;
  			cx->demotion.count = 0;
@@ -498,6 +499,13 @@
 
 end:
 	/*
+	 * Demote if current state exceeds acpi_cstate_limit
+	 */
+	if (pr->power.state > acpi_cstate_limit) {
+		next_state = acpi_cstate_limit;
+	}
+
+	/*
 	 * New Cx State?
 	 * -------------
 	 * If we're going to start using a new Cx state we must clean up
@@ -2441,5 +2449,6 @@
 
 module_init(acpi_processor_init);
 module_exit(acpi_processor_exit);
+module_param_named(acpi_cstate_limit, acpi_cstate_limit, uint, 0);
 
 EXPORT_SYMBOL(acpi_processor_set_thermal_limit);
diff -Nru a/include/linux/acpi.h b/include/linux/acpi.h
--- a/include/linux/acpi.h	2004-11-05 17:40:07 -05:00
+++ b/include/linux/acpi.h	2004-11-05 17:40:07 -05:00
@@ -471,4 +471,30 @@
 
 #endif /*!CONFIG_ACPI_INTERPRETER*/
 
+#define	ACPI_CSTATE_LIMIT_DEFINED	/* for driver builds */
+#ifdef	CONFIG_ACPI
+
+/*
+ * Set highest legal C-state
+ * 0: C0 okay, but not C1
+ * 1: C1 okay, but not C2
+ * 2: C2 okay, but not C3 etc.
+ */
+
+extern unsigned int acpi_cstate_limit;
+
+static inline unsigned int acpi_get_cstate_limit(void)
+{
+	return acpi_cstate_limit;
+}
+static inline void acpi_set_cstate_limit(unsigned int new_limit)
+{
+	acpi_cstate_limit = new_limit;
+	return;
+}
+#else
+static inline unsigned int acpi_get_cstate_limit(void) { return 0; }
+static inline void acpi_set_cstate_limit(unsigned int new_limit) {
return; }
+#endif
+
 #endif /*_LINUX_ACPI_H*/





-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

On Fri, 2004-11-05 at 17:54, Dominik Brodowski wrote:

> That sounds to be quite racy. Multiple callers will get confused.
> You probably need a
> 

comitted design defect;-)

Yeah, James mentioned this too and suggested he send in a pci_dev
from the ipw2100 driver.  But I didn't think this workaround
justified the complexity.

After all, 99% of the time it will be not used.
the other 99% of 1% the time it is used, it will be used
only at module load/init time.

but if you're excited about enhancing it,
please go ahead.  I do want to lock down the driver API
ASAP, however, since james needs to release his driver.

> Other than that, the current "limit" interface to processor.c seems to
> be
> pretty much useless as P-States aren't really handled.

right, this doesn't have anything to do with p-states.
if we need that too, fine, but we need an api we
can use for c-states locked now.

thanks,
-Len




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Linux ACPI processor driver patch: user-definable power state limit

[Jos Delbar]

Hey,

To allow modifying the C state limit on the fly, I think we might need to use 
a combination of both schemes.

In Andi Kleen's patch the C state is "permanently" disabled in 
acpi_processor_get_power_info() and this is probably the best way to go for 
the blacklist. However, when another module needs to limit the processor to a 
certain C state, I think it would be best to handle this in 
acpi_processor_set_power_policy(). This seems to make the most sense 
semantically: you can change the power management policy at runtime, not the 
actual capabilities of the processor.

Also, if you have both a blacklist and an option to change the limit at 
runtime, there has to be some logic to prevent a module from lifting a limit 
imposed by the blacklist.

Your patch just came in while I was writing this, I'll be sure to try it later 
today. I'm not sure if the idle handler should be concerned with handling the 
limit though, but it does make the code simpler.

Regards,

- Jos

On Friday 05 November 2004 20:45, Len Brown wrote:
> Jos,
> I agree with you that a single parameter is simpler.
>
> Another thing we need to address -- with either scheme --
> is that the parameter must be set in the kernel, not
> in the processor modules.
>
> This is because it is necessary for modules, such as ipw2100
> to be able to disable c3 automatically when they detect
> that it is interfering with their operation.
>
> so we'd export a function from the base kernel for
> modules to set the limit, and we'd simply export
> the value of acpi_cstate_limit for processor.c
> to observe at run-time.
>
> int
> acpi_set_cstate_limit(int limit)
>
> int acpi_cstate_limit;
>
> I think it can return the old limit so that
> the caller can potentially un-do its call,
> or perhaps setting the limit to 0 should
> simply mean clear any limit.
>
> cheers,
> -Len

-- 

Jos Delbar
jos.delbar-Cru1EgDzd7c@public.gmane.org


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Jos Delbar]

Is it safe to return a pointer to an internal module structure? I think a bad 
driver could potentially ruin the linked list by changing the next pointer.

On Friday 05 November 2004 23:54, Dominik Brodowski wrote:
> That sounds to be quite racy. Multiple callers will get confused.
> You probably need a
>
> struct acpi_cstate_limit {
>  list_head next;
>  unsigned int limit;
> }
>
> and you export
>
> struct *acpi_cstate_limimt acpi_set_cstate_limit(unsigned int limit);
>
> int acpi_modify_cstate_limit(struct acpi_cstate_limit * cstate_limit);
>
> void acpi_remove_cstate_limit(struct acpi_cstate_limit * cstate_limit);
>
> Whenever such an operation occurs, you need to walk all these limit structs
> for the highest "limit", and use that.

-- 

Jos Delbar
jos.delbar-Cru1EgDzd7c@public.gmane.org


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Dominik Brodowski]

On Fri, Nov 05, 2004 at 06:11:33PM -0500, Len Brown wrote:
> On Fri, 2004-11-05 at 17:54, Dominik Brodowski wrote:
> 
> > That sounds to be quite racy. Multiple callers will get confused.
> > You probably need a
> > 
> 
> comitted design defect;-)
> 
> Yeah, James mentioned this too and suggested he send in a pci_dev
> from the ipw2100 driver.  But I didn't think this workaround
> justified the complexity.
> 
> After all, 99% of the time it will be not used.
> the other 99% of 1% the time it is used, it will be used
> only at module load/init time.

not if I understand all the corruption messages I see on my ipw2100 adaptor.
AFAICS, it C3 type sleep needs to be disabled for short periods of time
quite often then...

> can use for c-states locked now.

My suggestion is just a few minutes away...

	Dominik


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Dominik Brodowski]

[ACPI-PROCESSOR] Dynamic limitation of available C-State types

Some drivers and/or some operations need to disable certain types of ACPI
CPU power states (a.k.a. C-States). Drivers which have requirements >= 99%
of the time should do:

1.) during module_init, get a struct acpi_cstate_limit *acpi_set_cstate_limit
    and pass the appropriate limit to acpi_set_cstate_limit().
2.) pass the aquired struct acpi_cstate_limit * to acpi_remove_cstate_limit() in
    module_exit.

A driver with dynamic requirements can do the following

1.) get a struct acpi_cstate_limit *acpi_set_cstate_limit(0);
    [zero means no limit!]
2.) a critical section should be marked like:
	acpi_modify_cstate_limit(cstate_limit, NEW_LIMIT);
	/* no C3 may be going on here... */
	...
	/* the CPU can sleep safely again */
	acpi_modify_cstate_limit(cstate_limit, 0);
3.) pass the aquired struct acpi_cstate_limit * to acpi_remove_cstate_limit() in
    module_exit.

Signed-off-by: Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>

diff -ruN linux-original/drivers/acpi/processor.c linux/drivers/acpi/processor.c
--- linux-original/drivers/acpi/processor.c	2004-11-05 23:59:49.586947488 +0100
+++ linux/drivers/acpi/processor.c	2004-11-06 00:46:45.852810000 +0100
@@ -44,6 +44,7 @@
 #include <linux/seq_file.h>
 #include <linux/dmi.h>
 #include <linux/moduleparam.h>
+#include <linux/list.h>
 
 #include <asm/io.h>
 #include <asm/system.h>
@@ -304,6 +305,80 @@
                                 Power Management
    -------------------------------------------------------------------------- */
 
+/* others shall not modify anything in here directly, so the layout isn't
+ * exported anywhere else 
+ */
+struct acpi_cstate_limit {
+	struct list_head	list;
+	unsigned int		limit;
+};
+
+static unsigned int cstate_limit;
+static LIST_HEAD(cstate_limit_list);
+static spinlock_t cstate_limit_lock = SPIN_LOCK_UNLOCKED;
+
+static inline void acpi_update_cstate_limit(void)
+{
+	struct acpi_cstate_limit *cur;
+	unsigned int new_limit = 0;
+
+	list_for_each_entry(cur, &cstate_limit_list, list) {
+		if (cur->limit > new_limit)
+			new_limit = cur->limit;
+	}
+	cstate_limit = new_limit;
+}
+
+struct acpi_cstate_limit *acpi_set_cstate_limit(unsigned int limit)
+{
+	struct acpi_cstate_limit *data;
+	unsigned long flags;
+
+	might_sleep();
+
+	data = kmalloc(sizeof(struct acpi_cstate_limit), GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	data->limit = limit;
+	
+	spin_lock_irqsave(&cstate_limit_lock, flags);
+	list_add(&data->list, &cstate_limit_list);
+	acpi_update_cstate_limit();
+	spin_unlock_irqrestore(&cstate_limit_lock, flags);
+
+	return data;
+}
+
+int acpi_modify_cstate_limit(struct acpi_cstate_limit *data, unsigned int limit)
+{
+	unsigned long flags;
+	if (!data)
+		return -ENODEV;
+
+	spin_lock_irqsave(&cstate_limit_lock, flags);
+	data->limit = limit;
+	acpi_update_cstate_limit();
+	spin_unlock_irqrestore(&cstate_limit_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(acpi_modify_cstate_limit);
+
+void acpi_remove_cstate_limit(struct acpi_cstate_limit *data)
+{
+	unsigned long flags;
+	if (!data)
+		return;
+
+	spin_lock_irqsave(&cstate_limit_lock, flags);
+	list_del(&data->list);
+	acpi_update_cstate_limit();
+	spin_unlock_irqrestore(&cstate_limit_lock, flags);
+}
+EXPORT_SYMBOL(acpi_remove_cstate_limit);
+
+
 static inline u32
 ticks_elapsed (
 	u32			t1,
@@ -359,6 +434,8 @@
 	int			next_state = 0;
 	int			sleep_ticks = 0;
 	u32			t1, t2 = 0;
+	unsigned long flags;
+
 
 	pr = processors[smp_processor_id()];
 	if (!pr)
@@ -419,6 +496,21 @@
 		}
 	}
 
+	/* Dynamic limitation? */
+	spin_lock_irqsave(&cstate_limit_lock, flags);
+	if ((cstate_limit) && (cstate_limit <= pr->power.state)) {
+		next_state = cstate_limit;
+		while (next_state > 1) {
+			if (!pr->power.states[next_state].valid)
+				next_state--;
+		}
+		spin_unlock_irqrestore(&cstate_limit_lock, flags);
+		local_irq_enable();
+		goto end;
+	}
+	spin_unlock_irqrestore(&cstate_limit_lock, flags);
+
+
 	cx->usage++;
 
 	/*
@@ -487,6 +579,7 @@
 
 	next_state = pr->power.state;
 
+
 	/*
 	 * Promotion?
 	 * ----------
@@ -494,7 +587,7 @@
 	 * and promote when the count threshold is reached.  Note that bus
 	 * mastering activity may prevent promotions.
 	 */
-	if (cx->promotion.state) {
+	if (cx->promotion.state && !t2) {
 		if (sleep_ticks > cx->promotion.threshold.ticks) {
 			cx->promotion.count++;
  			cx->demotion.count = 0;
@@ -537,8 +630,16 @@
 	 * If we're going to start using a new Cx state we must clean up
 	 * from the previous and prepare to use the new.
 	 */
-	if (next_state != pr->power.state)
-		acpi_processor_power_activate(pr, next_state);
+	if (next_state != pr->power.state) {
+		/* Dynamic limitation? */
+		spin_lock_irqsave(&cstate_limit_lock, flags);
+		if (cstate_limit && (cstate_limit >= next_state)) {
+			spin_unlock_irqrestore(&cstate_limit_lock, flags);
+		} else {
+			spin_unlock_irqrestore(&cstate_limit_lock, flags);
+			acpi_processor_power_activate(pr, next_state);
+		}
+	}
 
 	return;
 }
diff -ruN linux-original/include/acpi/processor.h linux/include/acpi/processor.h
--- linux-original/include/acpi/processor.h	2004-11-05 23:59:51.923592264 +0100
+++ linux/include/acpi/processor.h	2004-11-06 00:46:39.284808488 +0100
@@ -44,6 +44,13 @@
 	struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
 };
 
+struct acpi_cstate_limit;
+
+struct acpi_cstate_limit* acpi_set_cstate_limit(unsigned int limit);
+int acpi_modify_cstate_limit(struct acpi_cstate_limit *data, unsigned int limit);
+void acpi_remove_cstate_limit(struct acpi_cstate_limit *data);
+
+
 /* Performance Management */
 
 struct acpi_pct_register {


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Dominik Brodowski]

On Sat, Nov 06, 2004 at 12:39:27AM +0100, Jos Delbar wrote:
> Is it safe to return a pointer to an internal module structure? I think
> a bad driver could potentially ruin the linked list by changing the next
> pointer.

A bad driver can potentially ruin your whole hard drive, contain security
holes and so on. But you're right, it's better to be safe than sorry. My
patch doesn't export the internals of "struct acpi_cstate_limit", just a
pointer ... and that is common practice, AFAICS.

Thanks,
	Dominik


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

On Fri, 2004-11-05 at 18:41, Dominik Brodowski wrote:
> On Fri, Nov 05, 2004 at 06:11:33PM -0500, Len Brown wrote:

> > After all, 99% of the time it will be not used.
> > the other 99% of 1% the time it is used, it will be used
> > only at module load/init time.
> 
> not if I understand all the corruption messages I see on my ipw2100
> adaptor.
> AFAICS, it C3 type sleep needs to be disabled for short periods of
> time
> quite often then...

my understanding is that when the ipw2100 detects the error,
that it will disable C3 and leave it disabled as long as ipw2100 is
loaded.  James can clarify if he's got other ideas.

-Len





-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Dominik Brodowski]

On Fri, Nov 05, 2004 at 07:25:14PM -0500, Len Brown wrote:
> On Fri, 2004-11-05 at 18:41, Dominik Brodowski wrote:
> > On Fri, Nov 05, 2004 at 06:11:33PM -0500, Len Brown wrote:
> 
> > > After all, 99% of the time it will be not used.
> > > the other 99% of 1% the time it is used, it will be used
> > > only at module load/init time.
> > 
> > not if I understand all the corruption messages I see on my ipw2100
> > adaptor.
> > AFAICS, it C3 type sleep needs to be disabled for short periods of
> > time
> > quite often then...
> 
> my understanding is that when the ipw2100 detects the error,
> that it will disable C3 and leave it disabled as long as ipw2100 is
> loaded.  James can clarify if he's got other ideas.

If that is the case, my patch is "overkill". If there's a chance to do more
fine-tuned disabling of C3, my patch seems to be better.

	Dominik


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

I think it is okay for the ipw2100 to depend on CONFIG_ACPI, but I don't
think it is okay for the ipw2100 driver to depend on the processor
driver being loaded (and staying loaded) for this to work.

thanks,
-Len




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

On Fri, 2004-11-05 at 19:29, Dominik Brodowski wrote:
> On Fri, Nov 05, 2004 at 07:25:14PM -0500, Len Brown wrote:

> > my understanding is that when the ipw2100 detects the error,
> > that it will disable C3 and leave it disabled as long as ipw2100 is
> > loaded.  James can clarify if he's got other ideas.
> 
> If that is the case, my patch is "overkill". If there's a chance to do
> more fine-tuned disabling of C3, my patch seems to be better.

I agree, and you produced it so quickly!

---- begin editorial on workarounds-----
My ongoing concern is that we can spend a huge amount of time and
complexity maintaining code for workarounds when we'd be better off
focusing on more important functionality.  So I hesitate to add the
world's most feature-rich workaround code unless we reeeeeeeeealy need
it.  Indeed, the ultimate goal should be to find out how to delete the
workaround code entirely.
---- end editorial ----

So I'd rather go with the simpler patch until it is proven to be too
simple;-)

thanks,
-Len





-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

On Fri, 2004-11-05 at 18:21, Jos Delbar wrote:

> 
> Also, if you have both a blacklist and an option to change the limit
> at runtime, there has to be some logic to prevent a module from
> lifting a limit imposed by the blacklist.

the logic could be in the user of the API to simply check
the limit first and be sure that it only lowers and doesn't raise it.
yeah, no guarantees here, but for the users at hand, maybe sufficient.

i'll think about this when i merge on top of andy's patch.  maybe keep
the blacklist limit as a hard limit.  But frankly I'm not sure it is
worth a single byte more of code b/c it is the beginning of the
over-designed workaround slippery slope;-)

> Your patch just came in while I was writing this, I'll be sure to try
> it later today. I'm not sure if the idle handler should be concerned
> with handling the limit though, but it does make the code simpler.

since we had to handle a limit change on the fly i did it that way, and
then i realized that maybe on-the-fly checking is sufficient.

-Len




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Linux ACPI processor driver patch: user-definable power state limit

[Len Brown]

Andi's c2/c3 blacklist patch alters the promote/demote
structures and the acpi_cstate_limit patch
lives within those bounds.

So we keep both for now and acpi_state_limit
will never break the blacklist.

Hopefully we'll figure out the problem with the
two c2/c3 blacklisted systems and delete them
and that workaround over time.

thanks,
-Len




-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click

Re: Re: Linux ACPI processor driver patch: user-definable power state limit

[Jos Delbar]

On Saturday 06 November 2004 01:29, Dominik Brodowski wrote:
> If that is the case, my patch is "overkill". If there's a chance to do more
> fine-tuned disabling of C3, my patch seems to be better.

Maybe you could reduce the overkill by using a counter instead of a linked 
list to keep track of the limits imposed by other modules. I don't think you 
need any extra code in the idle handler, Len's additions should be enough to 
handle a limit change. The only possible danger that I can see is one or more 
extra idle cycles with an outdated limit.

As it is, is there a way for a module such as ipw2100 to detect when its limit 
request has been commited? Does the processor module export the current C 
state, or should the module rely on the /proc file?

Anyway, here is some code to illustrate the counter. And now I'm going to stop 
spending time on workarounds! ;-)

static unsigned int cstate_limit_counter[ACPI_C_STATES_MAX];
static spinlock_t cstate_limit_lock = SPIN_LOCK_UNLOCKED;

inline void acpi_update_cstate_limit(unsigned int limit, int gain)
{
 unsigned long flags;
 int c;

 if(new_limit >= ACPI_C_STATES_MAX)
  return;

 spin_lock_irqsave(&cstate_limit_lock, flags);
 if (gain > 0)
  cstate_limit_counter[limit]++;
 else if (cstate_limit_counter[limit] > 0)
  cstate_limit_counter[limit]--;

 for (c = 0; c < ACPI_C_STATES_MAX; c++) {
  if (cstate_limit_counter[c]) {
   acpi_cstate_limit = c;
   spin_unlock_irqrestore(&cstate_limit_lock, flags);
   return;
  }
 }
 acpi_cstate_limit = ACPI_C_STATES_MAX;
 spin_unlock_irqrestore(&cstate_limit_lock, flags);
}

static void acpi_remove_cstate_limit(unsigned int limit)
{
 acpi_update_cstate_limit(limit, -1);
}

static void acpi_set_cstate_limit(unsigned int limit)
{
 acpi_update_cstate_limit(limit, 1);
}

-- 

Jos Delbar
jos.delbar-Cru1EgDzd7c@public.gmane.org


-------------------------------------------------------
This SF.Net email is sponsored by:
Sybase ASE Linux Express Edition - download now for FREE
LinuxWorld Reader's Choice Award Winner for best database on Linux.
http://ads.osdn.com/?ad_id=5588&alloc_id=12065&op=click