[PATCH] cpufreq: fix UP build

[PATCH] cpufreq: fix UP build

[Alexander Beregalov]

From: Alexander Beregalov <a.beregalov@gmail.com> 

Fix this build error when CONFIG_SMP is not set:
drivers/cpufreq/cpufreq.c:941: 'managed_policy' undeclared

Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
---

Tested on AthlonXP/NForce2


 drivers/cpufreq/cpufreq.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c668ac8..287872f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -773,11 +773,11 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 	struct cpufreq_policy new_policy;
 	struct cpufreq_policy *policy;
 	struct freq_attr **drv_attr;
-	struct sys_device *cpu_sys_dev;
 	unsigned long flags;
 	unsigned int j;
 #ifdef CONFIG_SMP
 	struct cpufreq_policy *managed_policy;
+	struct sys_device *cpu_sys_dev;
 #endif
 
 	if (cpu_is_offline(cpu))
@@ -930,6 +930,7 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 	}
 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+#ifdef CONFIG_SMP
 	/* symlink affected CPUs */
 	for_each_cpu(j, policy->cpus) {
 		if (j == cpu)
@@ -947,6 +948,7 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 			goto err_out_unregister;
 		}
 	}
+#endif
 
 	policy->governor = NULL; /* to assure that the starting sequence is
 				  * run in cpufreq_set_policy */
-- 
1.6.3.3

Re: [PATCH] cpufreq: fix UP build

[Linus Torvalds]

On Wed, 8 Jul 2009, Alexander Beregalov wrote:
>
> From: Alexander Beregalov <a.beregalov@gmail.com> 
> 
> Fix this build error when CONFIG_SMP is not set:
> drivers/cpufreq/cpufreq.c:941: 'managed_policy' undeclared

Grr. DaveJ?

That said, I'd much prefer the fix that does _not_ have this crap in it 
(not new to your diff - it's pre-existing crap):

	>  #ifdef CONFIG_SMP
	>       struct cpufreq_policy *managed_policy;
	> +     struct sys_device *cpu_sys_dev;
	>  #endif

and instead those variables should be declared inside the blocks where 
they are used, not at the top.

The rule should always be: make the scope of a variable as small as 
possible. Don't declare it at the top and try to "save" a declaration when 
it can be used inside multiple blocks as multiple different variables.

Also, that whole function could damn well be split into smaller pieces, 
which would make it much more readable than that horrible 250+ line piece 
of crap monster-function with #ifdef's inside the code.

Please, somebody?

		Linus

Re: [PATCH] cpufreq: fix UP build

[Dave Jones]

On Wed, Jul 08, 2009 at 09:57:47PM +0400, Alexander Beregalov wrote:
 > From: Alexander Beregalov <a.beregalov@gmail.com> 
 > 
 > Fix this build error when CONFIG_SMP is not set:
 > drivers/cpufreq/cpufreq.c:941: 'managed_policy' undeclared
 > 
 > Signed-off-by: Alexander Beregalov <a.beregalov@gmail.com>
 
Ugh, should have caught this at merge time. Thanks.

I'll add a test to my scripts to do non-SMP build testing before merging.

	Dave
 

Re: [PATCH] cpufreq: fix UP build

[Dave Jones]

On Wed, Jul 08, 2009 at 11:08:12AM -0700, Linus Torvalds wrote:

 > (not new to your diff - it's pre-existing crap):
 > 
 > 	>  #ifdef CONFIG_SMP
 > 	>       struct cpufreq_policy *managed_policy;
 > 	> +     struct sys_device *cpu_sys_dev;
 > 	>  #endif
 > 
 > and instead those variables should be declared inside the blocks where 
 > they are used, not at the top.
 > 
 > The rule should always be: make the scope of a variable as small as 
 > possible. Don't declare it at the top and try to "save" a declaration when 
 > it can be used inside multiple blocks as multiple different variables.
 > 
 > Also, that whole function could damn well be split into smaller pieces, 
 > which would make it much more readable than that horrible 250+ line piece 
 > of crap monster-function with #ifdef's inside the code.

Yes, cpufreq_add_dev is a monster. Its complexity keeps biting us in new ways.

 > Please, somebody?
 
It's something to look at for .32, agreed.

	Dave

Re: [PATCH] cpufreq: fix UP build

[Dave Jones]

On Wed, Jul 08, 2009 at 11:08:12AM -0700, Linus Torvalds wrote:
 > 
 > 
 > On Wed, 8 Jul 2009, Alexander Beregalov wrote:
 > >
 > > From: Alexander Beregalov <a.beregalov@gmail.com> 
 > > 
 > > Fix this build error when CONFIG_SMP is not set:
 > > drivers/cpufreq/cpufreq.c:941: 'managed_policy' undeclared
 > 
 > Grr. DaveJ?
 > 
 > That said, I'd much prefer the fix that does _not_ have this crap in it 
 > (not new to your diff - it's pre-existing crap):
 > 
 > 	>  #ifdef CONFIG_SMP
 > 	>       struct cpufreq_policy *managed_policy;
 > 	> +     struct sys_device *cpu_sys_dev;
 > 	>  #endif
 > 
 > and instead those variables should be declared inside the blocks where 
 > they are used, not at the top.
 > 
 > The rule should always be: make the scope of a variable as small as 
 > possible. Don't declare it at the top and try to "save" a declaration when 
 > it can be used inside multiple blocks as multiple different variables.
 > 
 > Also, that whole function could damn well be split into smaller pieces, 
 > which would make it much more readable than that horrible 250+ line piece 
 > of crap monster-function with #ifdef's inside the code.
 > 
 > Please, somebody?

This seems to be a minimal step in the direction of declaring it locally..
Not boot-tested, but should do the right thing.

I'll take a stab at chopping that function up further separately.

	Dave


commit 2381f2a2d3b329313b375bc0bf5df34802b9b4ba
Author: Dave Jones <davej@redhat.com>
Date:   Wed Jul 8 16:14:23 2009 -0400

    [CPUFREQ] Fix compile failure in cpufreq.c
    
    managed_policy is out of scope for the non-smp case.
    Declare it locally where used (twice)
    
    Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c668ac8..b90eda8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -776,9 +776,6 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 	struct sys_device *cpu_sys_dev;
 	unsigned long flags;
 	unsigned int j;
-#ifdef CONFIG_SMP
-	struct cpufreq_policy *managed_policy;
-#endif
 
 	if (cpu_is_offline(cpu))
 		return 0;
@@ -854,6 +851,8 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 #endif
 
 	for_each_cpu(j, policy->cpus) {
+		struct cpufreq_policy *managed_policy;
+
 		if (cpu == j)
 			continue;
 
@@ -932,6 +931,8 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 
 	/* symlink affected CPUs */
 	for_each_cpu(j, policy->cpus) {
+		struct cpufreq_policy *managed_policy;
+
 		if (j == cpu)
 			continue;
 		if (!cpu_online(j))