Re: PATCH: Voyager, tty locking

Re: PATCH: Voyager, tty locking

[Zach Brown]

> The signal handling also appears to be incorrect as it does an
> unprotected sigfillset that also appears unneccessary. As I don't have a
> bowtie and am therefore not a qualified voyager maintainer I leave that
> to James.

Bowtie or not, you touched it last!

This seems like a decent candidate for being moved over to the kthread API..

- z

Re: PATCH: Voyager, tty locking

[James Bottomley]

On Tue, 2006-08-08 at 10:57 -0700, Zach Brown wrote:
> > The signal handling also appears to be incorrect as it does an
> > unprotected sigfillset that also appears unneccessary. As I don't have a
> > bowtie and am therefore not a qualified voyager maintainer I leave that
> > to James.
> 
> Bowtie or not, you touched it last!
> 
> This seems like a decent candidate for being moved over to the kthread API..

Alan's patch looks fine so you can add my ack to it (and that of my
bowtie).

I'll add the kthread conversion to my list ... unless the suggestor also
wants to become the implementor?

James

PATCH: Voyager, tty locking

[Alan Cox]

Voyager fiddles with current->signal.tty without locking. It turns out
that the code in question has already cleared current->signal.tty
correctly because daemonize() does the right thing already.

The signal handling also appears to be incorrect as it does an
unprotected sigfillset that also appears unneccessary. As I don't have a
bowtie and am therefore not a qualified voyager maintainer I leave that
to James.

Signed-off-by: Alan Cox <alan@redhat.com>

--- linux.vanilla-2.6.18-rc3-mm2/arch/i386/mach-voyager/voyager_thread.c	2006-08-07 16:15:02.000000000 +0100
+++ linux-2.6.18-rc3-mm2/arch/i386/mach-voyager/voyager_thread.c	2006-08-08 18:19:11.496378872 +0100
@@ -130,7 +130,6 @@
 	init_timer(&wakeup_timer);
 
 	sigfillset(&current->blocked);
-	current->signal->tty = NULL;
 
 	printk(KERN_NOTICE "Voyager starting monitor thread\n");
 

Re: PATCH: Voyager, tty locking

[Zach Brown]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]


> I'll add the kthread conversion to my list ... unless the suggestor also
> wants to become the implementor?

Well, here's a start that builds.  I didn't want to get too close to the
nutty sem and timeout logic.

- z
[ apologies for the attachment ]

[-- Attachment #2: voyager-thread-to-kthread.patch --]
[-- Type: text/x-patch, Size: 2638 bytes --]

Index: 2.6.18-rc4-removing-foot-from-mouth/arch/i386/mach-voyager/voyager_thread.c
===================================================================
--- 2.6.18-rc4-removing-foot-from-mouth.orig/arch/i386/mach-voyager/voyager_thread.c
+++ 2.6.18-rc4-removing-foot-from-mouth/arch/i386/mach-voyager/voyager_thread.c
@@ -24,34 +24,21 @@
 #include <linux/kmod.h>
 #include <linux/completion.h>
 #include <linux/sched.h>
+#include <linux/kthread.h>
 #include <asm/desc.h>
 #include <asm/voyager.h>
 #include <asm/vic.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
 
-#define THREAD_NAME "kvoyagerd"
-
 /* external variables */
 int kvoyagerd_running = 0;
 DECLARE_MUTEX_LOCKED(kvoyagerd_sem);
 
-static int thread(void *);
+static struct task_struct *voyager_task;
 
 static __u8 set_timeout = 0;
 
-/* Start the machine monitor thread.  Return 1 if OK, 0 if fail */
-static int __init
-voyager_thread_start(void)
-{
-	if(kernel_thread(thread, NULL, CLONE_KERNEL) < 0) {
-		/* This is serious, but not fatal */
-		printk(KERN_ERR "Voyager: Failed to create system monitor thread!!!\n");
-		return 1;
-	}
-	return 0;
-}
-
 static int
 execute(const char *string)
 {
@@ -116,25 +103,19 @@ wakeup(unsigned long unused)
 	up(&kvoyagerd_sem);
 }
 
-static int
-thread(void *unused)
+static int voyager_thread(void *unused)
 {
 	struct timer_list wakeup_timer;
 
 	kvoyagerd_running = 1;
 
-	daemonize(THREAD_NAME);
-
 	set_timeout = 0;
 
 	init_timer(&wakeup_timer);
 
-	sigfillset(&current->blocked);
-	current->signal->tty = NULL;
-
 	printk(KERN_NOTICE "Voyager starting monitor thread\n");
 
-	for(;;) {
+	while(!kthread_should_stop()) {
 		down_interruptible(&kvoyagerd_sem);
 		VDEBUG(("Voyager Daemon awoken\n"));
 		if(voyager_status.request_from_kernel == 0) {
@@ -151,13 +132,28 @@ thread(void *unused)
 			add_timer(&wakeup_timer);
 		}
 	}
+
+	return 0;
+}
+
+/* Start the machine monitor thread.  Return 1 if OK, 0 if fail */
+static int __init voyager_thread_init(void)
+{
+	voyager_task = kthread_run(voyager_thread, NULL, "kvoyagerd");
+	if (IS_ERR(voyager_task)) {
+		/* This is serious, but not fatal */
+		printk(KERN_ERR "Voyager: Failed to create system monitor thread! error %ld\n", PTR_ERR(voyager_task));
+		return PTR_ERR(voyager_task);
+	}
+
+	return 0;
 }
+module_init(voyager_thread_init);
 
-static void __exit
-voyager_thread_stop(void)
+static void __exit voyager_thread_exit(void)
 {
-	/* FIXME: do nothing at the moment */
+	if (voyager_task && !IS_ERR(voyager_task))
+		kthread_stop(voyager_task);
 }
+module_exit(voyager_thread_exit);
 
-module_init(voyager_thread_start);
-//module_exit(voyager_thread_stop);