cvs commit: src/sys/dev/fdc fdc.c fdcvar.h

[M. Warner Losh]

In message: <200409231923.40285.jhb at FreeBSD.org>
            John Baldwin <jhb at freebsd.org> writes:
: Note that relying on a wakeup from your own code is not safe if you
: expect fdc to be a module since you could wakeup the thread doing the
: kldunload (and thus detach) and then be interrupted for an interrupt and
: it could unmap the memory backing that function before you get a chance
: to run again resulting in a panic.  There is a special wakeup on the
: proc pointer in exit1() for kthreads to handle this case.

Wouldn't simply unregistering your interrupt before killing the worker
thread handle this case?  You are sleeping in the detach path then
until the worker thread goes away.  Once it is gone, it is safe to
proceed to detach.  cbb does this and I've not had any problems
unloading it.  In fact, it was how you told me to write cbb a long
time ago.  Once the bus_teardown_intr() returns, you are guaranteed
the interrupt won't get called.

The floppy driver, however, doesn't appear to do this.  It kills the
thread and then releases the interrupt.  It should do something more
like:

Index: fdc.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/fdc/fdc.c,v
retrieving revision 1.291
diff -u -r1.291 fdc.c
--- fdc.c	23 Sep 2004 21:12:21 -0000	1.291
+++ fdc.c	24 Sep 2004 01:16:09 -0000
@@ -1636,9 +1636,14 @@
 	if ((error = bus_generic_detach(dev)))
 		return (error);
 
+	/* Tear down the interrupt before killing the thread */
+	if (fdc->fdc_intr)
+		bus_teardown_intr(dev, fdc->res_irq, fdc->fdc_intr);
+	fdc->fdc_intr = NULL;
+
 	/* kill worker thread */
-	fdc->flags |= FDC_KTHREAD_EXIT;
 	mtx_lock(&fdc->fdc_mtx);
+	fdc->flags |= FDC_KTHREAD_EXIT;
 	while ((fdc->flags & FDC_KTHREAD_ALIVE) != 0)
 		msleep(&fdc->fdc_thread, &fdc->fdc_mtx, PRIBIO, "fdcdet", 0);
 	mtx_unlock(&fdc->fdc_mtx);


To resolve the problem (and fix the race on flags I noted earlier).
Without the interrtupt firing, then you can't have what you described.

: I should
: likely move that into kthread_exit() however (which wouldn't be a
: functional change as far as you are concerned).

I'm not sure I understand this part of your statement.

Warner