matthew@ noted a possible misuse of the rwlock used to protect vscsi

from being opened by multiple processes at the same time. then he
pointed out that pool_get can sleep in the middle of scsi_cmd,
during which time the process processing the vscsi requests can go
away.

this uses a state variable to protect vscsi from multiple opens
which is checked by all the process and scsi midlayer entrypoints.
it avoids a potential sleep in the middle of the scsi_cmd handler
by moving to iopools.

lots of tweaks and feedback by matthew@

diff --git a/sys/dev/vscsi.c b/sys/dev/vscsi.c
index 3e9c95a..ce48f49 100644 (file)
--- a/sys/dev/vscsi.c
+++ b/sys/dev/vscsi.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vscsi.c,v 1.14 2010/06/28 18:31:01 krw Exp $ */
+/*     $OpenBSD: vscsi.c,v 1.15 2010/07/23 09:41:16 dlg Exp $ */
 
 /*
  * Copyright (c) 2008 David Gwynne <dlg@openbsd.org>
@@ -49,23 +49,33 @@ struct vscsi_ccb {
 
 TAILQ_HEAD(vscsi_ccb_list, vscsi_ccb);
 
+enum vscsi_state {
+       VSCSI_S_CLOSED,
+       VSCSI_S_CONFIG,
+       VSCSI_S_RUNNING
+};
+
 struct vscsi_softc {
        struct device           sc_dev;
        struct scsi_link        sc_link;
        struct scsibus_softc    *sc_scsibus;
 
+       struct mutex            sc_state_mtx;
+       enum vscsi_state        sc_state;
+       u_int                   sc_ccb_count;
        struct pool             sc_ccb_pool;
+
+       struct scsi_iopool      sc_iopool;
+
        struct vscsi_ccb_list   sc_ccb_i2t;
        struct vscsi_ccb_list   sc_ccb_t2i;
        int                     sc_ccb_tag;
        struct mutex            sc_ccb_mtx;
-       struct rwlock           sc_ccb_polling;
+       struct mutex            sc_poll_mtx;
+       struct rwlock           sc_ioc_lock;
 
        struct selinfo          sc_sel;
        struct mutex            sc_sel_mtx;
-
-       struct rwlock           sc_open;
-       volatile int            sc_opened;
 };
 
 #define DEVNAME(_s) ((_s)->sc_dev.dv_xname)
@@ -93,15 +103,16 @@ struct scsi_adapter vscsi_switch = {
        NULL
 };
 
-void           vscsi_xs_stuffup(struct scsi_xfer *);
-
+int            vscsi_running(struct vscsi_softc *);
 
 int            vscsi_i2t(struct vscsi_softc *, struct vscsi_ioc_i2t *);
 int            vscsi_data(struct vscsi_softc *, struct vscsi_ioc_data *, int);
 int            vscsi_t2i(struct vscsi_softc *, struct vscsi_ioc_t2i *);
 
-struct vscsi_ccb *     vscsi_ccb_get(struct vscsi_softc *, int);
-#define vscsi_ccb_put(_s, _c) pool_put(&(_s)->sc_ccb_pool, (_c))
+void           vscsi_done(struct vscsi_softc *, struct vscsi_ccb *);
+
+void *         vscsi_ccb_get(void *);
+void           vscsi_ccb_put(void *, void *);
 
 void           filt_vscsidetach(struct knote *);
 int            filt_vscsiread(struct knote *, long);
@@ -128,14 +139,23 @@ vscsi_attach(struct device *parent, struct device *self, void *aux)
 
        printf("\n");
 
-       rw_init(&sc->sc_open, DEVNAME(sc));
-       rw_init(&sc->sc_ccb_polling, DEVNAME(sc));
+       mtx_init(&sc->sc_state_mtx, IPL_BIO);
+       sc->sc_state = VSCSI_S_CLOSED;
+
+       TAILQ_INIT(&sc->sc_ccb_i2t);
+       TAILQ_INIT(&sc->sc_ccb_t2i);
+       mtx_init(&sc->sc_ccb_mtx, IPL_BIO);
+       mtx_init(&sc->sc_poll_mtx, IPL_BIO);
+       mtx_init(&sc->sc_sel_mtx, IPL_BIO);
+       rw_init(&sc->sc_ioc_lock, "vscsiioc");
+       scsi_iopool_init(&sc->sc_iopool, sc, vscsi_ccb_get, vscsi_ccb_put);
 
        sc->sc_link.adapter = &vscsi_switch;
        sc->sc_link.adapter_softc = sc;
        sc->sc_link.adapter_target = 256;
        sc->sc_link.adapter_buswidth = 256;
        sc->sc_link.openings = 1;
+       sc->sc_link.pool = &sc->sc_iopool;
 
        bzero(&saa, sizeof(saa));
        saa.saa_sc_link = &sc->sc_link;
@@ -144,29 +164,37 @@ vscsi_attach(struct device *parent, struct device *self, void *aux)
            &saa, scsiprint);
 }
 
+int
+vscsi_running(struct vscsi_softc *sc)
+{
+       int                             running;
+
+       mtx_enter(&sc->sc_state_mtx);
+       running = (sc->sc_state == VSCSI_S_RUNNING);
+       mtx_leave(&sc->sc_state_mtx);
+
+       return (running);
+}
+
 void
 vscsi_cmd(struct scsi_xfer *xs)
 {
        struct scsi_link                *link = xs->sc_link;
        struct vscsi_softc              *sc = link->adapter_softc;
-       struct vscsi_ccb                *ccb;
+       struct vscsi_ccb                *ccb = xs->io;
        int                             polled = ISSET(xs->flags, SCSI_POLL);
 
-       if (sc->sc_opened == 0) { 
-               vscsi_xs_stuffup(xs);
+       if (!vscsi_running(sc)) {
+               xs->error = XS_DRIVER_STUFFUP;
+               scsi_done(xs);
                return;
        }
 
        if (ISSET(xs->flags, SCSI_POLL) && ISSET(xs->flags, SCSI_NOSLEEP)) {
                printf("%s: POLL && NOSLEEP for 0x%02x\n", DEVNAME(sc),
                    xs->cmd->opcode);
-               vscsi_xs_stuffup(xs);
-               return;
-       }
-
-       ccb = vscsi_ccb_get(sc, ISSET(xs->flags, SCSI_NOSLEEP) ? 0 : 1);
-       if (ccb == NULL) {
-               vscsi_xs_stuffup(xs);
+               xs->error = XS_DRIVER_STUFFUP;
+               scsi_done(xs);
                return;
        }
 
@@ -178,19 +206,26 @@ vscsi_cmd(struct scsi_xfer *xs)
        selwakeup(&sc->sc_sel);
 
        if (polled) {
-               rw_enter_read(&sc->sc_ccb_polling);
+               mtx_enter(&sc->sc_poll_mtx);
                while (ccb->ccb_xs != NULL)
-                       tsleep(ccb, PRIBIO, "vscsipoll", 0);
-               vscsi_ccb_put(sc, ccb);
-               rw_exit_read(&sc->sc_ccb_polling);
+                       msleep(ccb, &sc->sc_poll_mtx, PRIBIO, "vscsipoll", 0);
+               mtx_leave(&sc->sc_poll_mtx);
+               scsi_done(xs);
        }
 }
 
 void
-vscsi_xs_stuffup(struct scsi_xfer *xs)
+vscsi_done(struct vscsi_softc *sc, struct vscsi_ccb *ccb)
 {
-       xs->error = XS_DRIVER_STUFFUP;
-       scsi_done(xs);
+       struct scsi_xfer                *xs = ccb->ccb_xs;
+
+       if (ISSET(xs->flags, SCSI_POLL)) {
+               mtx_enter(&sc->sc_poll_mtx);
+               ccb->ccb_xs = NULL;
+               wakeup(ccb);
+               mtx_leave(&sc->sc_poll_mtx);
+       } else
+               scsi_done(xs);
 }
 
 int
@@ -198,7 +233,7 @@ vscsi_probe(struct scsi_link *link)
 {
        struct vscsi_softc              *sc = link->adapter_softc;
 
-       if (sc->sc_opened == 0)
+       if (!vscsi_running(sc))
                return (ENXIO);
 
        return (0);
@@ -208,26 +243,38 @@ int
 vscsiopen(dev_t dev, int flags, int mode, struct proc *p)
 {
        struct vscsi_softc              *sc = DEV2SC(dev);
-       int                             rv;
+       enum vscsi_state                state = VSCSI_S_RUNNING;
+       int                             rv = 0;
 
        if (sc == NULL)
                return (ENXIO);
 
-       rv = rw_enter(&sc->sc_open, RW_WRITE | RW_NOSLEEP);
+       mtx_enter(&sc->sc_state_mtx);
+       if (sc->sc_state != VSCSI_S_CLOSED)
+               rv = EBUSY;
+       else
+               sc->sc_state = VSCSI_S_CONFIG;
+       mtx_leave(&sc->sc_state_mtx);
+
        if (rv != 0)
                return (rv);
 
        pool_init(&sc->sc_ccb_pool, sizeof(struct vscsi_ccb), 0, 0, 0,
            "vscsiccb", NULL);
-       pool_setipl(&sc->sc_ccb_pool, IPL_BIO);
-       TAILQ_INIT(&sc->sc_ccb_i2t);
-       TAILQ_INIT(&sc->sc_ccb_t2i);
-       mtx_init(&sc->sc_ccb_mtx, IPL_BIO);
-       mtx_init(&sc->sc_sel_mtx, IPL_BIO);
 
-       sc->sc_opened = 1;
+       /* we need to guarantee some ccbs will be available for the iopool */
+       rv = pool_prime(&sc->sc_ccb_pool, 8);
+       if (rv != 0) {
+               pool_destroy(&sc->sc_ccb_pool);
+               state = VSCSI_S_CLOSED;
+       }
 
-       return (0);
+       /* commit changes */
+       mtx_enter(&sc->sc_state_mtx);
+       sc->sc_state = state;
+       mtx_leave(&sc->sc_state_mtx);
+
+       return (rv);
 }
 
 int
@@ -238,6 +285,8 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
        int                             read = 0;
        int                             err = 0;
 
+       rw_enter_write(&sc->sc_ioc_lock);
+
        switch (cmd) {
        case VSCSI_I2T:
                err = vscsi_i2t(sc, (struct vscsi_ioc_i2t *)addr);
@@ -267,6 +316,8 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
                break;
        }
 
+       rw_exit_write(&sc->sc_ioc_lock);
+
        return (err);
 }
 
@@ -369,7 +420,6 @@ vscsi_t2i(struct vscsi_softc *sc, struct vscsi_ioc_t2i *t2i)
        struct scsi_xfer                *xs;
        struct scsi_link                *link;
        int                             rv = 0;
-       int                             polled;
 
        TAILQ_FOREACH(ccb, &sc->sc_ccb_t2i, ccb_entry) {
                if (ccb->ccb_tag == t2i->tag)
@@ -400,15 +450,7 @@ vscsi_t2i(struct vscsi_softc *sc, struct vscsi_ioc_t2i *t2i)
                break;
        }
 
-       polled = ISSET(xs->flags, SCSI_POLL);
-
-       scsi_done(xs);
-
-       if (polled) {
-               ccb->ccb_xs = NULL;
-               wakeup(ccb);
-       } else
-               vscsi_ccb_put(sc, ccb);
+       vscsi_done(sc, ccb);
 
        return (rv);
 }
@@ -487,60 +529,77 @@ vscsiclose(dev_t dev, int flags, int mode, struct proc *p)
 {
        struct vscsi_softc              *sc = DEV2SC(dev);
        struct vscsi_ccb                *ccb;
-       int                             polled;
        int                             i;
 
-       sc->sc_opened = 0;
+       mtx_enter(&sc->sc_state_mtx);
+       KASSERT(sc->sc_state == VSCSI_S_RUNNING);
+       sc->sc_state = VSCSI_S_CONFIG;
+       mtx_leave(&sc->sc_state_mtx);
 
        while ((ccb = TAILQ_FIRST(&sc->sc_ccb_t2i)) != NULL) {
                TAILQ_REMOVE(&sc->sc_ccb_t2i, ccb, ccb_entry);
-               polled = ISSET(ccb->ccb_xs->flags, SCSI_POLL);
-
-               vscsi_xs_stuffup(ccb->ccb_xs);
-
-               if (polled) {
-                       ccb->ccb_xs = NULL;
-                       wakeup(ccb);
-               } else
-                       vscsi_ccb_put(sc, ccb);
+               ccb->ccb_xs->error = XS_DRIVER_STUFFUP;
+               vscsi_done(sc, ccb);
        }
 
        while ((ccb = TAILQ_FIRST(&sc->sc_ccb_i2t)) != NULL) {
                TAILQ_REMOVE(&sc->sc_ccb_i2t, ccb, ccb_entry);
-               polled = ISSET(ccb->ccb_xs->flags, SCSI_POLL);
-
-               vscsi_xs_stuffup(ccb->ccb_xs);
+               ccb->ccb_xs->error = XS_DRIVER_STUFFUP;
+               vscsi_done(sc, ccb);
+       }
 
-               if (polled) {
-                       ccb->ccb_xs = NULL;
-                       wakeup(ccb);
-               } else
-                       vscsi_ccb_put(sc, ccb);
+       mtx_enter(&sc->sc_state_mtx);
+       while (sc->sc_ccb_count > 0) {
+               msleep(&sc->sc_ccb_count, &sc->sc_state_mtx,
+                   PRIBIO, "vscsiccb", 0);
        }
+       mtx_leave(&sc->sc_state_mtx);
 
-       rw_enter_write(&sc->sc_ccb_polling);
        pool_destroy(&sc->sc_ccb_pool);
-       rw_exit_write(&sc->sc_ccb_polling);
 
        for (i = 0; i < sc->sc_link.adapter_buswidth; i++)
                scsi_detach_target(sc->sc_scsibus, i, DETACH_FORCE);
 
-       rw_exit(&sc->sc_open);
+       mtx_enter(&sc->sc_state_mtx);
+       sc->sc_state = VSCSI_S_CLOSED;
+       mtx_leave(&sc->sc_state_mtx);
 
        return (0);
 }
 
-struct vscsi_ccb *
-vscsi_ccb_get(struct vscsi_softc *sc, int waitok)
+void *
+vscsi_ccb_get(void *cookie)
 {
-       struct vscsi_ccb                *ccb;
+       struct vscsi_softc              *sc = cookie;
+       struct vscsi_ccb                *ccb = NULL;
 
-       ccb = pool_get(&sc->sc_ccb_pool, waitok ? PR_WAITOK : PR_NOWAIT);
-       if (ccb == NULL)
-               return (NULL);
+       mtx_enter(&sc->sc_state_mtx);
+       if (sc->sc_state == VSCSI_S_RUNNING &&
+           (ccb = pool_get(&sc->sc_ccb_pool, PR_NOWAIT)) != NULL) {
+               ccb->ccb_tag = sc->sc_ccb_tag++;
+               ccb->ccb_datalen = 0;
 
-       ccb->ccb_tag = sc->sc_ccb_tag++;
-       ccb->ccb_datalen = 0;
+               sc->sc_ccb_count++;
+       }
+       mtx_leave(&sc->sc_state_mtx);
 
        return (ccb);
 }
+
+void
+vscsi_ccb_put(void *cookie, void *io)
+{
+       struct vscsi_softc              *sc = cookie;
+       struct vscsi_ccb                *ccb = io;
+
+       mtx_enter(&sc->sc_state_mtx);
+
+       pool_put(&sc->sc_ccb_pool, ccb);
+       sc->sc_ccb_count--;
+
+       if (sc->sc_state != VSCSI_S_RUNNING &&
+           sc->sc_ccb_count == 0)
+               wakeup(&sc->sc_ccb_count);
+
+       mtx_leave(&sc->sc_state_mtx);
+}