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@

1 files changed, 137 insertions, 78 deletions

diff --git a/sys/dev/vscsi.c b/sys/dev/vscsi.c
index 3e9c95acc08..ce48f495df2 100644
--- 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);
+}