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

[Roman Kagan]

  Hi Duncan,

On Fri, Jan 28, 2005 at 01:38:55PM +0100, Duncan Sands wrote:
> > Even more so: you're relying on the .heavy_init to be prepared to be
> > SIGTERMed at any time.  This IMO is a much stronger assumption than
> > its being quick to exit.
> 
> I don't see what the problem is.  You won't be terminated - you're a
> kernel thread after all.  But if you are doing an interruptible sleep
> then you will be woken up - and that's all I'm looking for.  I could
> be wrong - I've no in-kernel signal experience.

Neither do I, but you're probably right here.

Still, to complete the experiment with the asynchronous heavy_init
kthread shutdown (and to keep you busy reviewing :) I've made up the
patch below against current CVS.

It adds .cleanup method to struct usbatm_driver which is supposed to
release the allocated resources when the last reference to the subdriver
instance is dropped.  To preserve the lifetime rules which you've
implemented, I've introduced another kref to struct usbatm_data to
control the lifetime of the subdriver instance.

I see the following arguments advocating this approach:

 1) it isn't any more complex than the synchronous one

 2) the requirements to the .cleanup method in the async case are
    obvious from the interface, unlike the obscure requirements to the
    .heavy_init to be interruptible and quick to exit in the sync case

 3) if we ever come to publish some info from the subdrivers in sysfs,
    the .cleanup will be a must (as the only way to handle the
    degenerate case of open files in sysfs while removing the device)

How do you feel about it?

Thanks and cheers,
  Roman.


Index: usbatm.h
===================================================================
RCS file: /home/cvs/usbatm/usbatm.h,v
retrieving revision 1.8
diff -u -r1.8 usbatm.h
--- usbatm.h	1 Feb 2005 23:20:49 -0000	1.8
+++ usbatm.h	2 Feb 2005 20:22:27 -0000
@@ -74,9 +74,12 @@
 	/* additional device initialization that is too slow to be done in probe() */
         int (*heavy_init) (struct usbatm_data *, struct usb_interface *);
 
-	/* cleanup device ... can sleep, but can't fail */
+	/* disconnect device ... can sleep, but can't fail */
         void (*unbind) (struct usbatm_data *, struct usb_interface *);
 
+	/* release the resources allocated to the device */
+	void (*cleanup) (struct usbatm_data *);
+
 	/* init ATM device ... can sleep, or cause ATM initialization failure */
 	int (*atm_start) (struct usbatm_data *, struct atm_dev *);
 
@@ -148,17 +151,13 @@
 
 struct usbatm_data {
 	struct kref refcount;
+	struct kref sub_refcount;
 	struct semaphore serialize;
 
 	/* mini driver part */
 	struct usbatm_driver *driver;
 	void *driver_data;
 
-	/* 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;
Index: usbatm.c
===================================================================
RCS file: /home/cvs/usbatm/usbatm.c,v
retrieving revision 1.4
diff -u -r1.4 usbatm.c
--- usbatm.c	1 Feb 2005 23:20:49 -0000	1.4
+++ usbatm.c	2 Feb 2005 20:22:27 -0000
@@ -763,6 +763,23 @@
 	kref_put(&instance->refcount, usbatm_destroy_instance);
 }
 
+static void usbatm_destroy_sub_instance(struct kref *kref)
+{
+	struct usbatm_data *instance = container_of(kref, struct usbatm_data, sub_refcount);
+
+	if (instance->driver->cleanup)
+		instance->driver->cleanup(instance);
+}
+
+void usbatm_get_sub_instance(struct usbatm_data *instance)
+{
+	kref_get(&instance->sub_refcount);
+}
+
+void usbatm_put_sub_instance(struct usbatm_data *instance)
+{
+	kref_put(&instance->sub_refcount, usbatm_destroy_sub_instance);
+}
 
 /**********
 **  ATM  **
@@ -994,38 +1011,39 @@
 	int ret;
 
 	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);
 
-	down(&instance->serialize);
-	instance->thread_pid = -1;
-	up(&instance->serialize);
+	usbatm_put_sub_instance(instance);
+	module_put(instance->driver->owner);
+	usbatm_put_instance(instance);
 
-	complete_and_exit(&instance->thread_exited, ret);
+	return ret;
 }
 
 static int usbatm_heavy_init(struct usbatm_data *instance)
 {
-	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);
+	int ret;
 
-	wait_for_completion(&instance->thread_started);
+	usbatm_get_instance(instance);
+	try_module_get(instance->driver->owner);
+	usbatm_get_sub_instance(instance);
+	
+	ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_KERNEL);
+
+	if (ret >= 0)
+		return 0;	/* OK */
+
+	dbg("usbatm_heavy_init: failed to create kernel_thread (%d)!", ret);
+
+	usbatm_put_sub_instance(instance);
+	module_put(instance->driver->owner);
+	usbatm_put_instance(instance);
 
-	return 0;
+	return ret;
 }
 
 int usbatm_usb_probe (struct usb_interface *intf, const struct usb_device_id *id,
@@ -1035,11 +1053,12 @@
 	struct usbatm_data *instance;
 	char *buf;
 	int error = -ENOMEM;
-	int ifnum = intf->altsetting->desc.bInterfaceNumber;
 	int i, length;
 	int need_heavy;
 
-	dev_dbg(&intf->dev, "trying device with vendor=0x%x, product=0x%x, ifnum %d\n", dev->descriptor.idVendor, dev->descriptor.idProduct, ifnum);
+	dev_dbg(&intf->dev, "trying device with vendor=0x%x, product=0x%x, ifnum %d\n",
+			dev->descriptor.idVendor, dev->descriptor.idProduct,
+			intf->altsetting->desc.bInterfaceNumber);
 
 	/* instance init */
 	if (!(instance = kmalloc(sizeof(*instance), GFP_KERNEL))) {
@@ -1055,10 +1074,6 @@
 
 	instance->driver = driver;
 
-	instance->thread_pid = -1;
-	init_completion(&instance->thread_started);
-	init_completion(&instance->thread_exited);
-
 	instance->usb_dev = dev;
 	instance->usb_intf = intf;
 	instance->tx_endpoint = usb_sndbulkpipe(dev, driver->out);
@@ -1164,14 +1179,14 @@
  bind:
 	need_heavy = 1;
 	if (driver->bind && (error = driver->bind(instance, intf, &need_heavy)) < 0)
-			goto fail;
+		goto fail;
+
+	kref_init(&instance->sub_refcount);	/* dropped in usbatm_usb_disconnect after ->unbind */
 
 	if (need_heavy && driver->heavy_init)
 		error = usbatm_heavy_init(instance);
-	else {
-		complete(&instance->thread_exited);	/* pretend that heavy_init was run */
+	else
 		error = usbatm_atm_init(instance);
-	}
 
 	if (error)
 		goto fail;
@@ -1215,19 +1230,14 @@
 
 	usb_set_intfdata(intf, NULL);
 
-	down(&instance->serialize);
-	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->driver->unbind)
 		instance->driver->unbind(instance, intf);
 
+	usbatm_put_sub_instance(instance);
+
 	/* receive finalize */
 	tasklet_disable(&instance->receive_tasklet);
 
Index: cxacru.c
===================================================================
RCS file: /home/cvs/usbatm/cxacru.c,v
retrieving revision 1.8
diff -u -r1.8 cxacru.c
--- cxacru.c	1 Feb 2005 23:20:49 -0000	1.8
+++ cxacru.c	2 Feb 2005 20:22:27 -0000
@@ -763,6 +763,18 @@
 	del_timer_sync(&instance->poll_timer);
 	wmb();
 	flush_scheduled_work();
+}
+
+static void cxacru_cleanup(struct usbatm_data *usbatm_instance)
+{
+	struct cxacru_data *instance = usbatm_instance->driver_data;
+
+	dbg("cxacru_cleanup entered");
+
+	if (!instance) {
+		dbg("cxacru_cleanup: NULL instance!");
+		return;
+	}
 
 	usb_kill_urb(instance->snd_urb);
 	usb_kill_urb(instance->rcv_urb);
@@ -828,6 +840,7 @@
 	.bind		= cxacru_bind,
 	.heavy_init	= cxacru_heavy_init,
 	.unbind		= cxacru_unbind,
+	.cleanup	= cxacru_cleanup,
 	.atm_start	= cxacru_atm_start,
 	.in		= CXACRU_EP_DATA,
 	.out		= CXACRU_EP_DATA,
@@ -853,13 +866,13 @@
 	return usb_register(&cxacru_usb_driver);
 }
 
-static void __exit cxacru_cleanup(void)
+static void __exit cxacru_exit(void)
 {
 	usb_deregister(&cxacru_usb_driver);
 }
 
 module_init(cxacru_init);
-module_exit(cxacru_cleanup);
+module_exit(cxacru_exit);
 
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);