speedtch usbatm.h,1.4,1.5 usbatm2.c,1.13,1.14

[Duncan Sands]

Update of /home/cvs/speedtch
In directory phoenix.infradead.org:/tmp/cvs-serv16893

Modified Files:
	usbatm.h usbatm2.c 
Log Message:
(1) Fix the race in which we might send a signal to the kernel thread before it
called allow_signal by waiting for it to start (struct completion thread_started).
(2) Fix the problem with maybe sending a signal after the kernel thread has exited
and shooting down the wrong process by having the kernel thread clear thread_pid
just before it exits.
(3) Shoot down the kernel thread before trying to shut down the ATM device.  That
way we can get rid of instance->bound.

There is maybe a module unloading race: between the kernel thread dropping the semaphore
and actually exiting the module could be unloaded, causing the code the kernel thread is
executing (i.e. the return instruction) to be freed.  I don't think this can happen because
the thread won't sleep before returning, and the module unloader waits for all threads
to schedule before unloading.  To be confirmed.


Index: usbatm.h
===================================================================
RCS file: /home/cvs/speedtch/usbatm.h,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- usbatm.h	28 Jan 2005 11:03:43 -0000	1.4
+++ usbatm.h	28 Jan 2005 12:11:04 -0000	1.5
@@ -158,13 +158,13 @@
 
 	/* heavy init part */
 	int thread_pid;
+	struct completion thread_started;
 	struct completion thread_exited;
 
 	/* USB device part */
 	struct usb_device *usb_dev;
 	struct usb_interface *usb_intf;
 	char description[64];
-	int bound;
 	int tx_endpoint;
 	int rx_endpoint;
 	int tx_padding;

Index: usbatm2.c
===================================================================
RCS file: /home/cvs/speedtch/usbatm2.c,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- usbatm2.c	28 Jan 2005 11:03:43 -0000	1.13
+++ usbatm2.c	28 Jan 2005 12:11:04 -0000	1.14
@@ -825,11 +825,6 @@
 			break;
 		}
 
-		if (instance->bound)
-			strcat(page, ", ready\n");
-		else
-			strcat(page, ", disconnected\n");
-
 		return strlen(page);
 	}
 
@@ -961,20 +956,11 @@
 	struct atm_dev *atm_dev;
 	int ret;
 
-	down(&instance->serialize);
-
-	if (!instance->bound) {
-		dbg("usbatm_atm_init: device disconnected!");
-		ret = -ENODEV;
-		goto fail;
-	}
-
 	/* ATM init */
 	atm_dev = atm_dev_register(instance->driver->driver_name, &udsl_atm_devops, -1, NULL);
 	if (!atm_dev) {
 		dbg("usbatm_atm_init: failed to register ATM device!");
-		ret = -1;
-		goto fail;
+		return -1;
 	}
 
 	instance->atm_dev = atm_dev;
@@ -987,24 +973,18 @@
 	atm_dev->link_rate = 128 * 1000 / 424;
 
 	if (instance->driver->atm_start && ((ret = instance->driver->atm_start(instance, atm_dev)) < 0))
-		goto fail_close;
+		goto fail;
 
 	/* ready for ATM callbacks */
 	udsl_get_instance(instance);	/* dropped in udsl_atm_dev_close */
 	mb();
 	atm_dev->dev_data = instance;
 
-	up(&instance->serialize);
-
 	return 0;
 
- fail_close:
+ fail:
 	instance->atm_dev = NULL;
 	shutdown_atm_dev(atm_dev); /* udsl_atm_dev_close will eventually be called */
-
- fail:
-	up(&instance->serialize);
-
 	return ret;
 }
 
@@ -1021,24 +1001,35 @@
 	daemonize("%s/%s", usbatm_driver_name, instance->driver->driver_name);
 	allow_signal(SIGTERM);
 
+	complete(&instance->thread_started);
+
 	ret = instance->driver->heavy_init(instance, instance->usb_intf);
 
 	if (!ret)
 		ret = usbatm_atm_init(instance);
 
-	complete_and_exit(&instance->thread_exited, ret);
+	down(&instance->serialize);
+	instance->thread_pid = -1;
+	complete(&instance->thread_exited);
+	up(&instance->serialize);
+
+	return ret;
 }
 
 static int usbatm_heavy_init(struct usbatm_data *instance)
 {
-	int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_FS | CLONE_FILES);
+	int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_KERNEL);
 
 	if (ret < 0) {
 		dbg("usbatm_heavy_init: failed to create kernel_thread (%d)!", ret);
 		return ret;
 	}
 
+	down(&instance->serialize);
 	instance->thread_pid = ret;
+	up(&instance->serialize);
+
+	wait_for_completion(&instance->thread_started);
 
 	return 0;
 }
@@ -1076,6 +1067,7 @@
 	instance->driver = driver;
 
 	instance->thread_pid = -1;
+	init_completion(&instance->thread_started);
 	init_completion(&instance->thread_exited);
 
 	instance->usb_dev = dev;
@@ -1186,12 +1178,12 @@
 	if (driver->bind && (error = driver->bind(instance, intf, &need_heavy)) < 0)
 			goto fail;
 
-	instance->bound = 1;
-
 	if (need_heavy && driver->heavy_init)
 		error = usbatm_heavy_init(instance);
-	else
+	else {
+		complete(&instance->thread_exited);	/* pretend that heavy_init was run */
 		error = usbatm_atm_init(instance);
+	}
 
 	if (error)
 		goto fail;
@@ -1235,17 +1227,15 @@
 	usb_set_intfdata(intf, NULL);
 
 	down(&instance->serialize);
-	instance->bound = 0;
+	if (instance->thread_pid >= 0)
+		kill_proc(instance->thread_pid, SIGTERM, 1);
 	up(&instance->serialize);
 
+	wait_for_completion(&instance->thread_exited);
+
 	if (instance->atm_dev && instance->driver->atm_stop)
 		instance->driver->atm_stop(instance, instance->atm_dev);
 
-	if (instance->thread_pid >= 0) {
-		kill_proc(instance->thread_pid, SIGTERM, 1);
-		wait_for_completion(&instance->thread_exited);
-	}
-
 	if (instance->driver->unbind)
 		instance->driver->unbind(instance, intf);