From owner-cvs-all@FreeBSD.ORG Tue Jun 3 10:34:05 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C1BF337B405 for ; Tue, 3 Jun 2003 10:34:05 -0700 (PDT) Received: from rootlabs.com (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id 71A3F43F93 for ; Tue, 3 Jun 2003 10:34:04 -0700 (PDT) (envelope-from nate@rootlabs.com) Received: (qmail 23231 invoked by uid 1000); 3 Jun 2003 17:34:04 -0000 Date: Tue, 3 Jun 2003 10:34:04 -0700 (PDT) From: Nate Lawson To: Sam Leffler In-Reply-To: <088401c329de$e82f7a50$52557f42@errno.com> Message-ID: <20030603101652.V23167@root.org> References: <20030602233211.2492D37B4A2@hub.freebsd.org> <20030602234329.T22029@root.org> <088401c329de$e82f7a50$52557f42@errno.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/ubsec ubsec.c ubsecvar.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Jun 2003 17:34:06 -0000 On Tue, 3 Jun 2003, Sam Leffler wrote: > > On Mon, 2 Jun 2003, Sam Leffler wrote: > > > o don't use locking on detach; disabling interrupts is sufficient (I > think) > > > > Unfortunately it's not if you're on a shared interrupt. You need to > > call bus_teardown_intr() before you are detached from the ithread. See > > the thread near this message: > > Message-ID: <1742240000.1051807110@aslan.btc.adaptec.com> > > If you're interested look at the change. I understood the device could be > on a shared irq. Reviewing the change is what prompted the comment: #### @@ -500,11 +502,11 @@ ubsec_detach(device_t dev) { struct ubsec_softc *sc = device_get_softc(dev); -KASSERT(sc != NULL, ("ubsec_detach: null software carrier")); - /* XXX wait/abort active ops */ -UBSEC_LOCK(sc); +/* disable interrupts */ +WRITE_REG(sc, BS_CTRL, READ_REG(sc, BS_CTRL) &~ +(BS_CTRL_MCR2INT | BS_CTRL_MCR1INT | BS_CTRL_DMAERR)); callout_stop(&sc->sc_rngto); #### That disables the device's interrupts but does not unhook the ih. That happens further down. So if interrupts occur, you are depending on ubsec_intr()'s check whether it is enabled or not (BS_STAT) to see if the interrupt is for ubsec. That seems fine to me (after further review). But if an instance of ubsec_intr() is already active, might there be problems? I'm thinking of it locking mcr1lock and then detach() calling mtx_destroy() while it is active or other similar issues. This is probably why you have "/* XXX wait/abort active ops */" at the top. My question was whether this should be a function of the underlying ithread subsystem and not the responsibility of each driver. -Nate