From owner-freebsd-current@FreeBSD.ORG Thu Jul 15 18:08:55 2004 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from green.homeunix.org (freefall.freebsd.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id 7ED0416A4CE for ; Thu, 15 Jul 2004 18:08:55 +0000 (GMT) Received: from green.homeunix.org (green@localhost [127.0.0.1]) by green.homeunix.org (8.12.11/8.12.11) with ESMTP id i6FI8tYA075809 for ; Thu, 15 Jul 2004 14:08:55 -0400 (EDT) (envelope-from green@green.homeunix.org) Received: (from green@localhost) by green.homeunix.org (8.12.11/8.12.11/Submit) id i6FI8srE075808 for current@FreeBSD.org; Thu, 15 Jul 2004 14:08:54 -0400 (EDT) (envelope-from green) Date: Thu, 15 Jul 2004 14:08:54 -0400 From: Brian Fundakowski Feldman To: current@FreeBSD.org Message-ID: <20040715180854.GZ1626@green.homeunix.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.6i Subject: pccbb crashes when detaching (unsafe interrupt handler) X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Jul 2004 18:08:55 -0000 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 #include #include +#include #include #include #include @@ -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. \,,,,,,,,,,,,,,,,,,,,,,\