[PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[Dominik Brodowski]

The acpi cpufreq driver includes a test at startup which detects whether
ACPI P-States are supported on any CPU, and whether transitions work.
However, this test is faulty: it is only run _after_ the acpi driver is
registered, causing race situations. Also, it doesn't save anything _as_ the
driver is already registered. So, it can safely be removed.

 arch/i386/kernel/cpu/cpufreq/acpi.c |   74 ++++--------------------------------
 1 files changed, 10 insertions(+), 64 deletions(-)

diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c linux/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:50:52.000000000 +0100
+++ linux/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:57:10.927599960 +0100
@@ -1,9 +1,9 @@
 /*
- * acpi_processor_perf.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
+ * acpi-cpufreq-io.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
  *
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
- *  Copyright (C) 2002, 2003 Dominik Brodowski <linux@brodo.de>
+ *  Copyright (C) 2002 - 2004 Dominik Brodowski <linux@brodo.de>
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *
@@ -641,7 +641,6 @@
 acpi_cpufreq_init (void)
 {
 	int                     result = 0;
-	int                     current_state = 0;
 	int                     i = 0;
 	struct acpi_processor   *pr = NULL;
 	struct acpi_processor_performance *perf = NULL;
@@ -649,9 +648,6 @@
 	ACPI_FUNCTION_TRACE("acpi_cpufreq_init");
 
 	/* alloc memory */
-	if (performance)
-		return_VALUE(-EBUSY);
-
 	performance = kmalloc(NR_CPUS * sizeof(struct acpi_processor_performance), GFP_KERNEL);
 	if (!performance)
 		return_VALUE(-ENOMEM);
@@ -669,69 +665,19 @@
 			result = acpi_processor_get_performance_info(&performance[i]);
 	}
 
-	/* test it on one CPU */
-	for (i=0; i<NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
-		pr = performance[i].pr;
-		if (pr && pr->flags.performance)
-			goto found_capable_cpu;
-	}
-	result = -ENODEV;
-	goto err0;
-
- found_capable_cpu:
-	
  	result = cpufreq_register_driver(&acpi_cpufreq_driver);
-	if (result) 
-		goto err0;
-	
-	perf = pr->performance;
-	current_state = perf->state;
-
-	if (current_state == pr->limit.state.px) {
-		result = acpi_processor_set_performance(perf, (perf->state_count - 1));
-		if (result) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-			result = -ENODEV;
-			goto err1;
-		}
-	}
-
-	result = acpi_processor_set_performance(perf, pr->limit.state.px);
 	if (result) {
-		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-		result = -ENODEV;
-		goto err1;
-	}
-	
-	if (current_state != 0) {
-		result = acpi_processor_set_performance(perf, current_state);
-		if (result) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-			result = -ENODEV;
-			goto err1;
+		/* unregister struct acpi_processor_performance performance */
+		for (i=0; i<NR_CPUS; i++) {
+			if (performance[i].pr) {
+				performance[i].pr->flags.performance = 0;
+				performance[i].pr->performance = NULL;
+				performance[i].pr = NULL;
+			}
 		}
+		kfree(performance);
 	}
-
-	return_VALUE(0);
-
-	/* error handling */
- err1:
-	cpufreq_unregister_driver(&acpi_cpufreq_driver);
-	
- err0:
-	/* unregister struct acpi_processor_performance performance */
-	for (i=0; i<NR_CPUS; i++) {
-		if (performance[i].pr) {
-			performance[i].pr->flags.performance = 0;
-			performance[i].pr->performance = NULL;
-			performance[i].pr = NULL;
-		}
-	}
-	kfree(performance);
 	
-	printk(KERN_INFO "cpufreq: No CPUs supporting ACPI performance management found.\n");
 	return_VALUE(result);
 }

[PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[Dominik Brodowski]

[re-sent as first try didn't reach the list]

The acpi cpufreq driver includes a test at startup which detects whether
ACPI P-States are supported on any CPU, and whether transitions work.
However, this test is faulty: it is only run _after_ the acpi driver is
registered, causing race situations. Also, it doesn't save anything _as_ the
driver is already registered. So, it can safely be removed.

 arch/i386/kernel/cpu/cpufreq/acpi.c |   74 ++++--------------------------------
 1 files changed, 10 insertions(+), 64 deletions(-)

diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c linux/arch/i386/kernel/cpu/cpufreq/acpi.c
--- linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:50:52.000000000 +0100
+++ linux/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:57:10.927599960 +0100
@@ -1,9 +1,9 @@
 /*
- * acpi_processor_perf.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
+ * acpi-cpufreq-io.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
  *
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
- *  Copyright (C) 2002, 2003 Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>
+ *  Copyright (C) 2002 - 2004 Dominik Brodowski <linux-JhLEnvuH02M@public.gmane.org>
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *
@@ -641,7 +641,6 @@
 acpi_cpufreq_init (void)
 {
 	int                     result = 0;
-	int                     current_state = 0;
 	int                     i = 0;
 	struct acpi_processor   *pr = NULL;
 	struct acpi_processor_performance *perf = NULL;
@@ -649,9 +648,6 @@
 	ACPI_FUNCTION_TRACE("acpi_cpufreq_init");
 
 	/* alloc memory */
-	if (performance)
-		return_VALUE(-EBUSY);
-
 	performance = kmalloc(NR_CPUS * sizeof(struct acpi_processor_performance), GFP_KERNEL);
 	if (!performance)
 		return_VALUE(-ENOMEM);
@@ -669,69 +665,19 @@
 			result = acpi_processor_get_performance_info(&performance[i]);
 	}
 
-	/* test it on one CPU */
-	for (i=0; i<NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
-		pr = performance[i].pr;
-		if (pr && pr->flags.performance)
-			goto found_capable_cpu;
-	}
-	result = -ENODEV;
-	goto err0;
-
- found_capable_cpu:
-	
  	result = cpufreq_register_driver(&acpi_cpufreq_driver);
-	if (result) 
-		goto err0;
-	
-	perf = pr->performance;
-	current_state = perf->state;
-
-	if (current_state == pr->limit.state.px) {
-		result = acpi_processor_set_performance(perf, (perf->state_count - 1));
-		if (result) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-			result = -ENODEV;
-			goto err1;
-		}
-	}
-
-	result = acpi_processor_set_performance(perf, pr->limit.state.px);
 	if (result) {
-		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-		result = -ENODEV;
-		goto err1;
-	}
-	
-	if (current_state != 0) {
-		result = acpi_processor_set_performance(perf, current_state);
-		if (result) {
-			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
-			result = -ENODEV;
-			goto err1;
+		/* unregister struct acpi_processor_performance performance */
+		for (i=0; i<NR_CPUS; i++) {
+			if (performance[i].pr) {
+				performance[i].pr->flags.performance = 0;
+				performance[i].pr->performance = NULL;
+				performance[i].pr = NULL;
+			}
 		}
+		kfree(performance);
 	}
-
-	return_VALUE(0);
-
-	/* error handling */
- err1:
-	cpufreq_unregister_driver(&acpi_cpufreq_driver);
-	
- err0:
-	/* unregister struct acpi_processor_performance performance */
-	for (i=0; i<NR_CPUS; i++) {
-		if (performance[i].pr) {
-			performance[i].pr->flags.performance = 0;
-			performance[i].pr->performance = NULL;
-			performance[i].pr = NULL;
-		}
-	}
-	kfree(performance);
 	
-	printk(KERN_INFO "cpufreq: No CPUs supporting ACPI performance management found.\n");
 	return_VALUE(result);
 }
 



-------------------------------------------------------
This SF.net email is sponsored by: Perforce Software.
Perforce is the Fast Software Configuration Management System offering
advanced branching capabilities and atomic changes on 50+ platforms.
Free Eval! http://www.perforce.com/perforce/loadprog.html

RE: [PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[Ow Mun Heng]

> -----Original Message-----
> From: Dominik Brodowski [mailto:linux-X3ehHDuj6sIIGcDfoQAp7BvVK+yQ3ZXh@public.gmane.org]
> Sent: Tuesday, January 13, 2004 5:20 PM
> To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Subject: [ACPI] [PATCH 2.6] remove unnecessary check in acpi-cpufreq
> driver
> 
> 
> [re-sent as first try didn't reach the list]
> 
> The acpi cpufreq driver includes a test at startup which 
> detects whether
> ACPI P-States are supported on any CPU, and whether transitions work.
> However, this test is faulty: it is only run _after_ the acpi 
> driver is
> registered, causing race situations. Also, it doesn't save 
> anything _as_ the
> driver is already registered. So, it can safely be removed.

Which means that, for current users, there's no pending need to really 
change anything Right??


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Re: [PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[Dominik Brodowski]

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

On Mon, Jan 19, 2004 at 06:03:01PM +0800, Ow Mun Heng wrote:
> > -----Original Message-----
> > From: Dominik Brodowski [mailto:linux-X3ehHDuj6sIIGcDfoQAp7BvVK+yQ3ZXh@public.gmane.org]
> > Sent: Tuesday, January 13, 2004 5:20 PM
> > To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> > Subject: [ACPI] [PATCH 2.6] remove unnecessary check in acpi-cpufreq
> > driver
> > 
> > 
> > [re-sent as first try didn't reach the list]
> > 
> > The acpi cpufreq driver includes a test at startup which 
> > detects whether
> > ACPI P-States are supported on any CPU, and whether transitions work.
> > However, this test is faulty: it is only run _after_ the acpi 
> > driver is
> > registered, causing race situations. Also, it doesn't save 
> > anything _as_ the
> > driver is already registered. So, it can safely be removed.
> 
> Which means that, for current users, there's no pending need to really 
> change anything Right??

The test complicates matters in a way that doesn't justify its existence.

	Dominik

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2.6] remove unnecessary check in acpi-cpufreq driver

[Len Brown]

Accepted into ACPI test tree
http://linux-acpi.bkbits.net/linux-acpi-test-2.6.0
http://linux-acpi.bkbits.net/linux-acpi-test-2.6.1
http://linux-acpi.bkbits.net/linux-acpi-test-2.6.2

This means it will be pulled into AKPM's mm tree on the next update.

thanks Dominik,
-Len

On Sun, 2004-01-11 at 16:00, Dominik Brodowski wrote:
> The acpi cpufreq driver includes a test at startup which detects whether
> ACPI P-States are supported on any CPU, and whether transitions work.
> However, this test is faulty: it is only run _after_ the acpi driver is
> registered, causing race situations. Also, it doesn't save anything _as_ the
> driver is already registered. So, it can safely be removed.
> 
>  arch/i386/kernel/cpu/cpufreq/acpi.c |   74 ++++--------------------------------
>  1 files changed, 10 insertions(+), 64 deletions(-)
> 
> diff -ruN linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c linux/arch/i386/kernel/cpu/cpufreq/acpi.c
> --- linux-original/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:50:52.000000000 +0100
> +++ linux/arch/i386/kernel/cpu/cpufreq/acpi.c	2004-01-11 20:57:10.927599960 +0100
> @@ -1,9 +1,9 @@
>  /*
> - * acpi_processor_perf.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
> + * acpi-cpufreq-io.c - ACPI Processor P-States Driver ($Revision: 1.3 $)
>   *
>   *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
>   *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
> - *  Copyright (C) 2002, 2003 Dominik Brodowski <linux@brodo.de>
> + *  Copyright (C) 2002 - 2004 Dominik Brodowski <linux@brodo.de>
>   *
>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   *
> @@ -641,7 +641,6 @@
>  acpi_cpufreq_init (void)
>  {
>  	int                     result = 0;
> -	int                     current_state = 0;
>  	int                     i = 0;
>  	struct acpi_processor   *pr = NULL;
>  	struct acpi_processor_performance *perf = NULL;
> @@ -649,9 +648,6 @@
>  	ACPI_FUNCTION_TRACE("acpi_cpufreq_init");
>  
>  	/* alloc memory */
> -	if (performance)
> -		return_VALUE(-EBUSY);
> -
>  	performance = kmalloc(NR_CPUS * sizeof(struct acpi_processor_performance), GFP_KERNEL);
>  	if (!performance)
>  		return_VALUE(-ENOMEM);
> @@ -669,69 +665,19 @@
>  			result = acpi_processor_get_performance_info(&performance[i]);
>  	}
>  
> -	/* test it on one CPU */
> -	for (i=0; i<NR_CPUS; i++) {
> -		if (!cpu_online(i))
> -			continue;
> -		pr = performance[i].pr;
> -		if (pr && pr->flags.performance)
> -			goto found_capable_cpu;
> -	}
> -	result = -ENODEV;
> -	goto err0;
> -
> - found_capable_cpu:
> -	
>   	result = cpufreq_register_driver(&acpi_cpufreq_driver);
> -	if (result) 
> -		goto err0;
> -	
> -	perf = pr->performance;
> -	current_state = perf->state;
> -
> -	if (current_state == pr->limit.state.px) {
> -		result = acpi_processor_set_performance(perf, (perf->state_count - 1));
> -		if (result) {
> -			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
> -			result = -ENODEV;
> -			goto err1;
> -		}
> -	}
> -
> -	result = acpi_processor_set_performance(perf, pr->limit.state.px);
>  	if (result) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
> -		result = -ENODEV;
> -		goto err1;
> -	}
> -	
> -	if (current_state != 0) {
> -		result = acpi_processor_set_performance(perf, current_state);
> -		if (result) {
> -			ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Disabled P-States due to failure while switching.\n"));
> -			result = -ENODEV;
> -			goto err1;
> +		/* unregister struct acpi_processor_performance performance */
> +		for (i=0; i<NR_CPUS; i++) {
> +			if (performance[i].pr) {
> +				performance[i].pr->flags.performance = 0;
> +				performance[i].pr->performance = NULL;
> +				performance[i].pr = NULL;
> +			}
>  		}
> +		kfree(performance);
>  	}
> -
> -	return_VALUE(0);
> -
> -	/* error handling */
> - err1:
> -	cpufreq_unregister_driver(&acpi_cpufreq_driver);
> -	
> - err0:
> -	/* unregister struct acpi_processor_performance performance */
> -	for (i=0; i<NR_CPUS; i++) {
> -		if (performance[i].pr) {
> -			performance[i].pr->flags.performance = 0;
> -			performance[i].pr->performance = NULL;
> -			performance[i].pr = NULL;
> -		}
> -	}
> -	kfree(performance);
>  	
> -	printk(KERN_INFO "cpufreq: No CPUs supporting ACPI performance management found.\n");
>  	return_VALUE(result);
>  }
>