Re: [Bug #11989] Suspend failure on NForce4-based boards due to chanes in stop_machine

[Rusty Russell <rusty@rustcorp.com.au>]

On Tuesday 11 November 2008 21:22:14 Ingo Molnar wrote:> * Rafael J. Wysocki <rjw@sisk.pl> wrote:> > So, it evidently fails while re-enabling the non-boot CPU and not> > during disabling it as I thought before.
(Resend, due to HTML version previously)
But what is calling stop_machine in that path?
There *is* a race, but I don't think it could cause this (we should make acopy of active.fnret inside the lock before returning it).
Two patches: one fixes that race, the next adds debugging spew.
stop_machine: fix race with return value
We should not access active.fnret outside the lock; in theory the nextstop_machine could overwrite it.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>--- kernel/stop_machine.c |    5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff -r d7c9a15da615 kernel/stop_machine.c--- a/kernel/stop_machine.c	Mon Nov 10 09:47:45 2008 +1100+++ b/kernel/stop_machine.c	Tue Nov 11 23:19:47 2008 +1030@@ -112,7 +112,7 @@ int __stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus) { 	struct work_struct *sm_work;-	int i;+	int i, ret;  	/* Set up initial state. */ 	mutex_lock(&lock);@@ -137,8 +137,9 @@ 	/* This will release the thread on our CPU. */ 	put_cpu(); 	flush_workqueue(stop_machine_wq);+	ret = active.fnret; 	mutex_unlock(&lock);-	return active.fnret;+	return ret; }  int stop_machine(int (*fn)(void *), void *data, const cpumask_t *cpus)===diff -r fe7dd39b1cff kernel/stop_machine.c--- a/kernel/stop_machine.c	Wed Nov 12 14:07:18 2008 +1030+++ b/kernel/stop_machine.c	Wed Nov 12 14:09:08 2008 +1030@@ -89,6 +89,8 @@ 			case STOPMACHINE_RUN: 				/* On multiple CPUs only a single error code 				 * is needed to tell that something failed. */+				printk("stop_machine: %i running %p\n",+				       smp_processor_id(), smdata->fn); 				err = smdata->fn(smdata->data); 				if (err) 					smdata->fnret = err;@@ -106,6 +108,7 @@ /* Callback for CPUs which aren't supposed to do anything. */ static int chill(void *unused) {+	printk("stop_machine: %i chilling\n", smp_processor_id()); 	return 0; } @@ -126,17 +129,23 @@  	set_state(STOPMACHINE_PREPARE); +	printk("stop_machine: running on %i cpus:\n", num_threads);+	dump_stack();+ 	/* Schedule the stop_cpu work on all cpus: hold this CPU so one 	 * doesn't hit this CPU until we're ready. */ 	get_cpu(); 	for_each_online_cpu(i) {+		printk("stop_machine: setting up cpu %i\n", i); 		sm_work = percpu_ptr(stop_machine_work, i); 		INIT_WORK(sm_work, stop_cpu); 		queue_work_on(i, stop_machine_wq, sm_work); 	} 	/* This will release the thread on our CPU. */+	printk("stop_machine: releasing CPU %i\n", smp_processor_id()); 	put_cpu(); 	flush_workqueue(stop_machine_wq);+	printk("stop_machine: done\n"); 	ret = active.fnret; 	mutex_unlock(&lock); 	return ret;\0ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥