[PATCH] cpufreq: powernow-k8: Remove bogus smp_processor_id() usage

[PATCH] cpufreq: powernow-k8: Remove bogus smp_processor_id() usage

[Thomas Gleixner]

commit 6889125b (cpufreq/powernow-k8: workqueue user shouldn't migrate
the kworker to another CPU) has a broken optimization of calling
powernowk8_target_fn() directly from powernowk8_target() which
results in the following splat:

[   11.789468] BUG: using smp_processor_id() in preemptible [00000000] code:
    	       modprobe/505
[   11.809594] caller is powernowk8_target+0x20/0x48 [powernow_k8]
[   12.001748] Pid: 505, comm: modprobe Not tainted 3.6.3 #3
[   12.016836] Call Trace:
[   12.025971]  [<ffffffff81241554>] debug_smp_processor_id+0xcc/0xe8
[   12.042518]  [<ffffffffa05bb07f>] powernowk8_target+0x20/0x48 [powernow_k8]
[   12.060733]  [<ffffffff813b3c23>] __cpufreq_driver_target+0x82/0x8a
[   12.077550]  [<ffffffff813b64a9>] cpufreq_governor_userspace+0x265/0x2c0
[   12.120378]  [<ffffffff81063c5c>] ? __blocking_notifier_call_chain+0x56/0x60
[   12.138862]  [<ffffffff813b3d8b>] __cpufreq_governor+0x8c/0xc9
[   12.155193]  [<ffffffff813b4031>] __cpufreq_set_policy+0x212/0x21e
[   12.172148]  [<ffffffff813b501e>] cpufreq_add_dev_interface+0x2a2/0x2bc
[   12.189855]  [<ffffffff813b602b>] ? cpufreq_update_policy+0x124/0x124
[   12.207096]  [<ffffffff813b54dc>] cpufreq_add_dev+0x4a4/0x4b4
[   12.223161]  [<ffffffff812f8136>] subsys_interface_register+0x95/0xc5
[   12.240386]  [<ffffffff8149aaf9>] ? _raw_spin_lock_irqsave+0x24/0x46
[   12.257477]  [<ffffffff813b5928>] cpufreq_register_driver+0xd2/0x1bf
[   12.274545]  [<ffffffffa05bc087>] powernowk8_init+0x193/0x1dc [powernow_k8]
[   12.292794]  [<ffffffffa05bbef4>] ? powernowk8_cpu_init+0xc53/0xc53 [powernow_k8]
[   12.312004]  [<ffffffff81002195>] do_one_initcall+0x7f/0x136
[   12.327594]  [<ffffffff8108f48f>] sys_init_module+0x17b0/0x197e
[   12.343718]  [<ffffffff81249494>] ? ddebug_proc_write+0xde/0xde
[   12.359767]  [<ffffffff8149f639>] system_call_fastpath+0x16/0x1b

This is fully preemptible non cpu bound context though the comment in the
code says:

	 * Must run on @pol->cpu.  cpufreq core is responsible for ensuring
	 * that we're bound to the current CPU and pol->cpu stays online.

The core only guarantees that pol->cpu stays online, but it has no way
to bind the thread and this needs to be fully preemptible context as
powernowk8_target_fn() calls functions which might sleep.

So the correct solution is to always go through work_on_cpu().

Reported-and-tested-by: Carsten Emde <C.Emde@osadl.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Andreas Herrmann <andreas.herrmann3@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/cpufreq/powernow-k8.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/cpufreq/powernow-k8.c
===================================================================
--- linux-2.6.orig/drivers/cpufreq/powernow-k8.c
+++ linux-2.6/drivers/cpufreq/powernow-k8.c
@@ -1053,13 +1053,12 @@ static int powernowk8_target(struct cpuf
 					     .relation = relation };
 
 	/*
-	 * Must run on @pol->cpu.  cpufreq core is responsible for ensuring
-	 * that we're bound to the current CPU and pol->cpu stays online.
+	 * Must run on @pol->cpu. We queue it on the target cpu even
+	 * if we are currently on the target cpu. This is preemptible
+	 * non cpu bound context, so we can't call the target function
+	 * directly.
 	 */
-	if (smp_processor_id() == pol->cpu)
-		return powernowk8_target_fn(&pta);
-	else
-		return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
+	return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
 }
 
 /* Driver entry point to verify the policy and range of frequencies */

Re: [PATCH] cpufreq: powernow-k8: Remove bogus smp_processor_id() usage

[Tejun Heo]

Hello, Thomas.

On Sat, Oct 27, 2012 at 07:29:25PM +0200, Thomas Gleixner wrote:
> This is fully preemptible non cpu bound context though the comment in the
> code says:
> 
> 	 * Must run on @pol->cpu.  cpufreq core is responsible for ensuring
> 	 * that we're bound to the current CPU and pol->cpu stays online.
> 
> The core only guarantees that pol->cpu stays online, but it has no way
> to bind the thread and this needs to be fully preemptible context as
> powernowk8_target_fn() calls functions which might sleep.
> 
> So the correct solution is to always go through work_on_cpu().

Yeah, that was stupid.  Rafael already has a patch queued.

  http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=commit;h=e4df1cbcc1f329e53a1fff7450b2229e0addff20

Thanks.

-- 
tejun

Re: [PATCH] cpufreq: powernow-k8: Remove bogus smp_processor_id() usage

[Thomas Gleixner]

On Sat, 27 Oct 2012, Tejun Heo wrote:

> Hello, Thomas.
> 
> On Sat, Oct 27, 2012 at 07:29:25PM +0200, Thomas Gleixner wrote:
> > This is fully preemptible non cpu bound context though the comment in the
> > code says:
> > 
> > 	 * Must run on @pol->cpu.  cpufreq core is responsible for ensuring
> > 	 * that we're bound to the current CPU and pol->cpu stays online.
> > 
> > The core only guarantees that pol->cpu stays online, but it has no way
> > to bind the thread and this needs to be fully preemptible context as
> > powernowk8_target_fn() calls functions which might sleep.
> > 
> > So the correct solution is to always go through work_on_cpu().
> 
> Yeah, that was stupid.  Rafael already has a patch queued.
> 
>   http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=commit;h=e4df1cbcc1f329e53a1fff7450b2229e0addff20
> 

Good. So more people noticed :)

      tglx