cvs commit: src/sys/dev/ubsec ubsec.c ubsecvar.h

[Nate Lawson]

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 at 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