mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Tejun Heo <tj@kernel.org>, "Rafael J. Wysocki" <rjw@sisk.pl>,
	Andreas Herrmann <andreas.herrmann3@amd.com>,
	stable@vger.kernel.org, Carsten Emde <C.Emde@osadl.org>
Subject: [PATCH] cpufreq: powernow-k8: Remove bogus smp_processor_id() usage
Date: Sat, 27 Oct 2012 19:29:25 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LFD.2.02.1210271927170.2756@ionos> (raw)

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



             reply	other threads:[~2012-10-27 17:29 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-10-27 17:29 Thomas Gleixner [this message]
2012-10-27 19:10 ` Tejun Heo
2012-10-27 19:19   ` Thomas Gleixner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.2.02.1210271927170.2756@ionos \
    --to=tglx@linutronix.de \
    --cc=C.Emde@osadl.org \
    --cc=andreas.herrmann3@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rjw@sisk.pl \
    --cc=stable@vger.kernel.org \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome