Date: Thu, 15 Jul 2004 14:08:54 -0400 From: Brian Fundakowski Feldman <green@FreeBSD.org> To: current@FreeBSD.org Subject: pccbb crashes when detaching (unsafe interrupt handler) Message-ID: <20040715180854.GZ1626@green.homeunix.org>
next in thread | raw e-mail | index | archive | help
Devices that attach to a pccbb end up getting screwed if they share
interrupt handlers with anything else and one comes in at such time
that the interrupt handler has been deallocated but not torn down.
There is no interrupt protection for the pccbb interrupt handler,
and the cleanest method I can think of is add a "don't run" count to
the real ithread interrupt handler (parent); however, this is a fair
bit of infrastructure if it's not something very useful, and in SMP,
concurrent accesses of the local interrupt handler list in pccbb do
need to be protected, not just preventing interrupt handlers.
This is what I have so far, but I'm not happy with it because it
seems like it would be so much nicer to increment a count on the
ithread's intrhand and drain any current interrupt out of the
ithread, but possibly making each interrupt a lot more expensive.
Index: pccbb.c
===================================================================
RCS file: /usr/ncvs/src/sys/dev/pccbb/pccbb.c,v
retrieving revision 1.114
diff -u -r1.114 pccbb.c
--- pccbb.c 23 Jun 2004 13:49:46 -0000 1.114
+++ pccbb.c 14 Jul 2004 21:59:49 -0000
@@ -85,6 +85,7 @@
#include <sys/lock.h>
#include <sys/malloc.h>
#include <sys/mutex.h>
+#include <sys/sx.h>
#include <sys/sysctl.h>
#include <sys/kthread.h>
#include <sys/bus.h>
@@ -696,6 +697,7 @@
parent = device_get_parent(brdev);
mtx_init(&sc->mtx, device_get_nameunit(brdev), "cbb", MTX_DEF);
cv_init(&sc->cv, "cbb cv");
+ sx_init(&sc->intrsx, "cbb intr sx");
sc->chipset = cbb_chipset(pci_get_devid(brdev), NULL);
sc->dev = brdev;
sc->cbdev = NULL;
@@ -715,6 +717,7 @@
device_printf(brdev, "Could not map register memory\n");
mtx_destroy(&sc->mtx);
cv_destroy(&sc->cv);
+ sx_destroy(&sc->intrsx);
return (ENOMEM);
} else {
DEVPRINTF((brdev, "Found memory at %08lx\n",
@@ -813,6 +816,7 @@
}
mtx_destroy(&sc->mtx);
cv_destroy(&sc->cv);
+ sx_destroy(&sc->intrsx);
return (ENOMEM);
}
@@ -852,6 +856,7 @@
sc->base_res);
mtx_destroy(&sc->mtx);
cv_destroy(&sc->cv);
+ sx_destroy(&sc->intrsx);
return (0);
}
@@ -903,7 +908,9 @@
ih->intr = intr;
ih->arg = arg;
ih->flags = flags & INTR_MPSAFE;
+ sx_xlock(&sc->intrsx);
STAILQ_INSERT_TAIL(&sc->intr_handlers, ih, entries);
+ sx_xunlock(&sc->intrsx);
cbb_enable_func_intr(sc);
/*
* XXX need to turn on ISA interrupts, if we ever support them, but
@@ -921,7 +928,9 @@
/* XXX Need to do different things for ISA interrupts. */
ih = (struct cbb_intrhand *) cookie;
+ sx_xlock(&sc->intrsx);
STAILQ_REMOVE(&sc->intr_handlers, ih, cbb_intrhand, entries);
+ sx_xunlock(&sc->intrsx);
free(ih, M_DEVBUF);
return (0);
}
@@ -1147,13 +1156,28 @@
* If the card is OK, call all the interrupt handlers.
*/
if (sc->flags & CBB_CARD_OK) {
+ int needgiant = 0;
+
+ sx_slock(&sc->intrsx);
STAILQ_FOREACH(ih, &sc->intr_handlers, entries) {
- if ((ih->flags & INTR_MPSAFE) == 0)
- mtx_lock(&Giant);
- (*ih->intr)(ih->arg);
- if ((ih->flags & INTR_MPSAFE) == 0)
- mtx_unlock(&Giant);
+ if ((ih->flags & INTR_MPSAFE) == 0) {
+ needgiant = 1;
+ break;
+ }
+ }
+ if (!needgiant) {
+ STAILQ_FOREACH(ih, &sc->intr_handlers, entries)
+ (*ih->intr)(ih->arg);
+ sx_sunlock(&sc->intrsx);
+ return;
}
+ sx_sunlock(&sc->intrsx);
+ mtx_lock(&Giant);
+ sx_slock(&sc->intrsx);
+ STAILQ_FOREACH(ih, &sc->intr_handlers, entries)
+ (*ih->intr)(ih->arg);
+ sx_sunlock(&sc->intrsx);
+ mtx_unlock(&Giant);
}
}
Index: pccbbvar.h
===================================================================
RCS file: /usr/ncvs/src/sys/dev/pccbb/pccbbvar.h,v
retrieving revision 1.20
diff -u -r1.20 pccbbvar.h
--- pccbbvar.h 21 May 2004 06:10:13 -0000 1.20
+++ pccbbvar.h 14 Jul 2004 21:58:22 -0000
@@ -65,6 +65,7 @@
u_int8_t subbus;
struct mtx mtx;
struct cv cv;
+ struct sx intrsx;
u_int32_t flags;
#define CBB_CARD_OK 0x08000000
#define CBB_16BIT_CARD 0x20000000
--
Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\
<> green@FreeBSD.org \ The Power to Serve! \
Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040715180854.GZ1626>