Hi Stefan,
On Wed, 15 Aug 2007, Stefan Richter wrote:
Yes, I think it is. You're clearly expecting the read to actually happen
when you call get_hpsb_generation(). It's clearly not a busy-loop, so
cpu_relax() sounds pointless / wrong solution for this case, so I'm now
somewhat beginning to appreciate the motivation behind this series :-)
But as I said, there are ways to achieve the same goals of this series
without using "volatile".
I think I'll submit a RFC/patch or two on this myself (will also fix
the code pieces listed here).
Totally unrelated, but this looks weird. IMHO you actually wanted:
msleep_interruptible(63);
if (kthread_should_stop())
goto exit;
here, didn't you? Otherwise the thread will exit even when
kthread_should_stop() != TRUE (just because it received a signal),
and it is not good for a kthread to exit on its own if it uses
kthread_should_stop() or if some other piece of kernel code could
eventually call kthread_stop(tsk) on it.
Ok, probably the thread will never receive a signal in the first
place because it's spawned off kthreadd which ignores all signals
beforehand, but still ...
[PATCH] ieee1394: Fix kthread stopping in nodemgr_host_thread
The nodemgr host thread can exit on its own even when kthread_should_stop
is not true, on receiving a signal (might never happen in practice, as
it ignores signals). But considering kthread_stop() must not be mixed with
kthreads that can exit on their own, I think changing the code like this
is clearer. This change means the thread can cut its sleep short when
receive a signal but looking at the code around, that sounds okay (and
again, it might never actually recieve a signal in practice).
Signed-off-by: Satyam Sharma <satyam@infradead.org>
---
drivers/ieee1394/nodemgr.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/ieee1394/nodemgr.c b/drivers/ieee1394/nodemgr.c
index 2ffd534..981a7da 100644
--- a/drivers/ieee1394/nodemgr.c
+++ b/drivers/ieee1394/nodemgr.c
@@ -1721,7 +1721,8 @@ static int nodemgr_host_thread(void *__hi)
* to make sure things settle down. */
g = get_hpsb_generation(host);
for (i = 0; i < 4 ; i++) {
- if (msleep_interruptible(63) || kthread_should_stop())
+ msleep_interruptible(63);
+ if (kthread_should_stop())
goto exit;
/* Now get the generation in which the node ID's we collect
-
