Add locking to mcd(4) and mark MPSAFE.

- Actually use existing per-softc mutex.
- Use mutex in cdev routines and remove D_NEEDGIANT.
- Use callout(9) instead of timeout(9).
- Don't check for impossible conditions (e.g. MCDINIT being clear).
- Remove critical_enter/exit when sending a PIO command.
- Use bus_*() instead of bus_space_*().

Tested by:	no one

sys/dev/mcd/mcd.c

@@ -128,7 +128,7 @@ static void	hsg2msf(int hsg, bcd_t *msf);
 static int	msf2hsg(bcd_t *msf, int relative);
 static int	mcd_volinfo(struct mcd_softc *);
 static int	mcd_waitrdy(struct mcd_softc *,int dly);
-static timeout_t mcd_timeout;
+static void	mcd_timeout(void *arg);
 static void	mcd_doread(struct mcd_softc *, int state, struct mcd_mbx *mbxin);
 static void	mcd_soft_reset(struct mcd_softc *);
 static int	mcd_hard_reset(struct mcd_softc *);
@@ -168,7 +168,7 @@ static struct cdevsw mcd_cdevsw = {
 	.d_ioctl =	mcdioctl,
 	.d_strategy =	mcdstrategy,
 	.d_name =	"mcd",
-	.d_flags =	D_DISK | D_NEEDGIANT,
+	.d_flags =	D_DISK,
 };
 
 #define MCD_RETRYS	5
@@ -193,6 +193,7 @@ mcd_attach(struct mcd_softc *sc)
 
 	unit = device_get_unit(sc->dev);
 
+	MCD_LOCK(sc);
 	sc->data.flags |= MCDINIT;
 	mcd_soft_reset(sc);
 	bioq_init(&sc->data.head);
@@ -201,11 +202,13 @@ mcd_attach(struct mcd_softc *sc)
 	/* wire controller for interrupts and dma */
 	mcd_configure(sc);
 #endif
+	MCD_UNLOCK(sc);
 	/* name filled in probe */
 	sc->mcd_dev_t = make_dev(&mcd_cdevsw, 8 * unit,
 				 UID_ROOT, GID_OPERATOR, 0640, "mcd%d", unit);
 
 	sc->mcd_dev_t->si_drv1 = (void *)sc;
+	callout_init_mtx(&sc->timer, &sc->mtx, 0);
 
 	return (0);
 }
@@ -218,41 +221,49 @@ mcdopen(struct cdev *dev, int flags, int fmt, struct thread *td)
 
 	sc = (struct mcd_softc *)dev->si_drv1;
 
-	/* not initialized*/
-	if (!(sc->data.flags & MCDINIT))
-		return (ENXIO);
-
 	/* invalidated in the meantime? mark all open part's invalid */
-	if (!(sc->data.flags & MCDVALID) && sc->data.openflags)
+	MCD_LOCK(sc);
+	if (!(sc->data.flags & MCDVALID) && sc->data.openflags) {
+		MCD_UNLOCK(sc);
 		return (ENXIO);
+	}
 
-	if (mcd_getstat(sc, 1) == -1)
+	if (mcd_getstat(sc, 1) == -1) {
+		MCD_UNLOCK(sc);
 		return (EIO);
+	}
 
 	if (    (sc->data.status & (MCDDSKCHNG|MCDDOOROPEN))
 	    || !(sc->data.status & MCDDSKIN))
 		for (retry = 0; retry < DISK_SENSE_SECS * WAIT_FRAC; retry++) {
-			(void) tsleep((caddr_t)sc, PSOCK | PCATCH, "mcdsn1", hz/WAIT_FRAC);
+			(void) mtx_sleep(sc, &sc->mtx, PSOCK | PCATCH,
-			if ((r = mcd_getstat(sc, 1)) == -1)
+			    "mcdsn1", hz/WAIT_FRAC);
+			if ((r = mcd_getstat(sc, 1)) == -1) {
+				MCD_UNLOCK(sc);
 				return (EIO);
+			}
 			if (r != -2)
 				break;
 		}
 
 	if (sc->data.status & MCDDOOROPEN) {
+		MCD_UNLOCK(sc);
 		device_printf(sc->dev, "door is open\n");
 		return (ENXIO);
 	}
 	if (!(sc->data.status & MCDDSKIN)) {
+		MCD_UNLOCK(sc);
 		device_printf(sc->dev, "no CD inside\n");
 		return (ENXIO);
 	}
 	if (sc->data.status & MCDDSKCHNG) {
+		MCD_UNLOCK(sc);
 		device_printf(sc->dev, "CD not sensed\n");
 		return (ENXIO);
 	}
 
 	if (mcd_size(dev) < 0) {
+		MCD_UNLOCK(sc);
 		device_printf(sc->dev, "failed to get disk size\n");
 		return (ENXIO);
 	}
@@ -262,10 +273,14 @@ mcdopen(struct cdev *dev, int flags, int fmt, struct thread *td)
 	sc->data.flags |= MCDVALID;
 
 	(void) mcd_lock_door(sc, MCD_LK_LOCK);
-	if (!(sc->data.flags & MCDVALID))
+	if (!(sc->data.flags & MCDVALID)) {
+		MCD_UNLOCK(sc);
 		return (ENXIO);
+	}
 
-	return mcd_read_toc(sc);
+	r = mcd_read_toc(sc);
+	MCD_UNLOCK(sc);
+	return (r);
 }
 
 static int
@@ -275,12 +290,13 @@ mcdclose(struct cdev *dev, int flags, int fmt, struct thread *td)
 
 	sc = (struct mcd_softc *)dev->si_drv1;
 
-	if (!(sc->data.flags & MCDINIT) || !sc->data.openflags)
+	MCD_LOCK(sc);
-		return (ENXIO);
+	KASSERT(sc->data.openflags, ("device not open"));
 
 	(void) mcd_lock_door(sc, MCD_LK_UNLOCK);
 	sc->data.openflags = 0;
 	sc->data.partflags &= ~MCDREADRAW;
+	MCD_UNLOCK(sc);
 
 	return (0);
 }
@@ -293,6 +309,7 @@ mcdstrategy(struct bio *bp)
 	sc = (struct mcd_softc *)bp->bio_dev->si_drv1;
 
 	/* if device invalidated (e.g. media change, door open), error */
+	MCD_LOCK(sc);
 	if (!(sc->data.flags & MCDVALID)) {
 		device_printf(sc->dev, "media changed\n");
 		bp->bio_error = EIO;
@@ -321,11 +338,13 @@ mcdstrategy(struct bio *bp)
 
 	/* now check whether we can perform processing */
 	mcd_start(sc);
+	MCD_UNLOCK(sc);
 	return;
 
 bad:
 	bp->bio_flags |= BIO_ERROR;
 done:
+	MCD_UNLOCK(sc);
 	bp->bio_resid = bp->bio_bcount;
 	biodone(bp);
 	return;
@@ -336,6 +355,7 @@ mcd_start(struct mcd_softc *sc)
 {
 	struct bio *bp;
 
+	MCD_ASSERT_LOCKED(sc);
 	if (sc->data.flags & MCDMBXBSY) {
 		return;
 	}
@@ -365,8 +385,11 @@ mcdioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *t
 
 	sc = (struct mcd_softc *)dev->si_drv1;
 
-	if (mcd_getstat(sc, 1) == -1) /* detect disk change too */
+	MCD_LOCK(sc);
+	if (mcd_getstat(sc, 1) == -1) { /* detect disk change too */
+		MCD_UNLOCK(sc);
 		return (EIO);
+	}
 MCD_TRACE("ioctl called 0x%lx\n", cmd);
 
 	switch (cmd) {
@@ -378,83 +401,114 @@ MCD_TRACE("ioctl called 0x%lx\n", cmd);
 	case CDIOCSETMUTE:
 	case CDIOCSETLEFT:
 	case CDIOCSETRIGHT:
+		MCD_UNLOCK(sc);
 		return (EINVAL);
 	case CDIOCEJECT:
-		return mcd_eject(sc);
+		r = mcd_eject(sc);
+		MCD_UNLOCK(sc);
+		return (r);
 	case CDIOCSETDEBUG:
 		sc->data.debug = 1;
+		MCD_UNLOCK(sc);
 		return (0);
 	case CDIOCCLRDEBUG:
 		sc->data.debug = 0;
+		MCD_UNLOCK(sc);
 		return (0);
 	case CDIOCRESET:
-		return mcd_hard_reset(sc);
+		r = mcd_hard_reset(sc);
+		MCD_UNLOCK(sc);
+		return (r);
 	case CDIOCALLOW:
-		return mcd_lock_door(sc, MCD_LK_UNLOCK);
+		r = mcd_lock_door(sc, MCD_LK_UNLOCK);
+		MCD_UNLOCK(sc);
+		return (r);
 	case CDIOCPREVENT:
-		return mcd_lock_door(sc, MCD_LK_LOCK);
+		r = mcd_lock_door(sc, MCD_LK_LOCK);
+		MCD_UNLOCK(sc);
+		return (r);
 	case CDIOCCLOSE:
-		return mcd_inject(sc);
+		r = mcd_inject(sc);
+		MCD_UNLOCK(sc);
+		return (r);
 	}
 
 	if (!(sc->data.flags & MCDVALID)) {
 		if (    (sc->data.status & (MCDDSKCHNG|MCDDOOROPEN))
 		    || !(sc->data.status & MCDDSKIN))
 			for (retry = 0; retry < DISK_SENSE_SECS * WAIT_FRAC; retry++) {
-				(void) tsleep((caddr_t)sc, PSOCK | PCATCH, "mcdsn2", hz/WAIT_FRAC);
+				(void) mtx_sleep(sc, &sc->mtx, PSOCK | PCATCH,
-				if ((r = mcd_getstat(sc, 1)) == -1)
+				    "mcdsn2", hz/WAIT_FRAC);
+				if ((r = mcd_getstat(sc, 1)) == -1) {
+					MCD_UNLOCK(sc);
 					return (EIO);
+				}
 				if (r != -2)
 					break;
 			}
 		if (   (sc->data.status & (MCDDOOROPEN|MCDDSKCHNG))
 		    || !(sc->data.status & MCDDSKIN)
 		    || mcd_size(dev) < 0
-		   )
+		   ) {
+			MCD_UNLOCK(sc);
 			return (ENXIO);
+		}
 		sc->data.flags |= MCDVALID;
 		sc->data.partflags |= MCDREADRAW;
 		(void) mcd_lock_door(sc, MCD_LK_LOCK);
-		if (!(sc->data.flags & MCDVALID))
+		if (!(sc->data.flags & MCDVALID)) {
+			MCD_UNLOCK(sc);
 			return (ENXIO);
+		}
 	}
 
 	switch (cmd) {
 	case DIOCGMEDIASIZE:
 		*(off_t *)addr = (off_t)sc->data.disksize * sc->data.blksize;
-		return (0);
+		r = 0;
+		break;
 	case DIOCGSECTORSIZE:
 		*(u_int *)addr = sc->data.blksize;
-		return (0);
+		r = 0;
-
+		break;
 	case CDIOCPLAYTRACKS:
-		return mcd_playtracks(sc, (struct ioc_play_track *) addr);
+		r = mcd_playtracks(sc, (struct ioc_play_track *) addr);
+		break;
 	case CDIOCPLAYBLOCKS:
-		return mcd_playblocks(sc, (struct ioc_play_blocks *) addr);
+		r = mcd_playblocks(sc, (struct ioc_play_blocks *) addr);
+		break;
 	case CDIOCPLAYMSF:
-		return mcd_playmsf(sc, (struct ioc_play_msf *) addr);
+		r = mcd_playmsf(sc, (struct ioc_play_msf *) addr);
+		break;
 	case CDIOCREADSUBCHANNEL_SYSSPACE:
 		return mcd_subchan(sc, (struct ioc_read_subchannel *) addr, 1);
 	case CDIOCREADSUBCHANNEL:
 		return mcd_subchan(sc, (struct ioc_read_subchannel *) addr, 0);
 	case CDIOREADTOCHEADER:
-		return mcd_toc_header(sc, (struct ioc_toc_header *) addr);
+		r = mcd_toc_header(sc, (struct ioc_toc_header *) addr);
+		break;
 	case CDIOREADTOCENTRYS:
 		return mcd_toc_entrys(sc, (struct ioc_read_toc_entry *) addr);
 	case CDIOCRESUME:
-		return mcd_resume(sc);
+		r = mcd_resume(sc);
+		break;
 	case CDIOCPAUSE:
-		return mcd_pause(sc);
+		r = mcd_pause(sc);
+		break;
 	case CDIOCSTART:
 		if (mcd_setmode(sc, MCD_MD_COOKED) != 0)
-			return (EIO);
+			r = EIO;
-		return (0);
+		else
+			r = 0;
+		break;
 	case CDIOCSTOP:
-		return mcd_stop(sc);
+		r = mcd_stop(sc);
+		break;
 	default:
-		return (ENOTTY);
+		r = ENOTTY;
 	}
-	/*NOTREACHED*/
+	MCD_UNLOCK(sc);
+	return (r);
 }
 
 static int
@@ -762,6 +816,7 @@ mcd_timeout(void *arg)
 
 	sc = (struct mcd_softc *)arg;
 
+	MCD_ASSERT_LOCKED(sc);
 	mcd_doread(sc, sc->ch_state, sc->ch_mbxsave);
 }
 
@@ -775,6 +830,7 @@ mcd_doread(struct mcd_softc *sc, int state, struct mcd_mbx *mbxin)
 	int blknum;
 	caddr_t	addr;
 
+	MCD_ASSERT_LOCKED(sc);
 	mbx = (state!=MCD_S_BEGIN) ? sc->ch_mbxsave : mbxin;
 	bp = mbx->bp;
 
@@ -789,15 +845,16 @@ retry_status:
 		MCD_WRITE(sc, MCD_REG_COMMAND, MCD_CMDGETSTAT);
 		mbx->count = RDELAY_WAITSTAT;
 		sc->ch_state = MCD_S_WAITSTAT;
-		sc->ch = timeout(mcd_timeout, (caddr_t)sc, hz/100); /* XXX */
+		callout_reset(&sc->timer, hz/100, mcd_timeout, sc); /* XXX */
 		return;
 	case MCD_S_WAITSTAT:
 		sc->ch_state = MCD_S_WAITSTAT;
-		untimeout(mcd_timeout,(caddr_t)sc, sc->ch);
+		callout_stop(&sc->timer);
 		if (mbx->count-- >= 0) {
 			if (MCD_READ(sc, MCD_FLAGS) & MFL_STATUS_NOT_AVAIL) {
 				sc->ch_state = MCD_S_WAITSTAT;
-				timeout(mcd_timeout, (caddr_t)sc, hz/100); /* XXX */
+				callout_reset(&sc->timer, hz/100,
+				    mcd_timeout, sc); /* XXX */
 				return;
 			}
 			sc->data.status = MCD_READ(sc, MCD_REG_STATUS) & 0xFF;
@@ -834,7 +891,7 @@ retry_mode:
 			MCD_WRITE(sc, MCD_REG_COMMAND, rm);
 
 			sc->ch_state = MCD_S_WAITMODE;
-			sc->ch = timeout(mcd_timeout, (caddr_t)sc, hz/100); /* XXX */
+			callout_reset(&sc->timer, hz / 100, mcd_timeout, sc); /* XXX */
 			return;
 		} else {
 			device_printf(sc->dev, "timeout getstatus\n");
@@ -843,14 +900,14 @@ retry_mode:
 
 	case MCD_S_WAITMODE:
 		sc->ch_state = MCD_S_WAITMODE;
-		untimeout(mcd_timeout, (caddr_t)sc, sc->ch);
+		callout_stop(&sc->timer);
 		if (mbx->count-- < 0) {
 			device_printf(sc->dev, "timeout set mode\n");
 			goto readerr;
 		}
 		if (MCD_READ(sc, MCD_FLAGS) & MFL_STATUS_NOT_AVAIL) {
 			sc->ch_state = MCD_S_WAITMODE;
-			sc->ch = timeout(mcd_timeout, (caddr_t)sc, hz/100);
+			callout_reset(&sc->timer, hz / 100, mcd_timeout, sc);
 			return;
 		}
 		sc->data.status = MCD_READ(sc, MCD_REG_STATUS) & 0xFF;
@@ -878,7 +935,6 @@ nextblock:
 		hsg2msf(blknum,rbuf.start_msf);
 retry_read:
 		/* send the read command */
-		critical_enter();
 		MCD_WRITE(sc, MCD_REG_COMMAND, sc->data.read_command);
 		MCD_WRITE(sc, MCD_REG_COMMAND, rbuf.start_msf[0]);
 		MCD_WRITE(sc, MCD_REG_COMMAND, rbuf.start_msf[1]);
@@ -886,7 +942,6 @@ retry_read:
 		MCD_WRITE(sc, MCD_REG_COMMAND, 0);
 		MCD_WRITE(sc, MCD_REG_COMMAND, 0);
 		MCD_WRITE(sc, MCD_REG_COMMAND, 1);
-		critical_exit();
 
 		/* Spin briefly (<= 2ms) to avoid missing next block */
 		for (i = 0; i < 20; i++) {
@@ -898,11 +953,11 @@ retry_read:
 
 		mbx->count = RDELAY_WAITREAD;
 		sc->ch_state = MCD_S_WAITREAD;
-		sc->ch = timeout(mcd_timeout, (caddr_t)sc, hz/100); /* XXX */
+		callout_reset(&sc->timer, hz / 100, mcd_timeout, sc); /* XXX */
 		return;
 	case MCD_S_WAITREAD:
 		sc->ch_state = MCD_S_WAITREAD;
-		untimeout(mcd_timeout, (caddr_t)sc, sc->ch);
+		callout_stop(&sc->timer);
 		if (mbx->count-- > 0) {
 			k = MCD_READ(sc, MCD_FLAGS);
 			if (!(k & MFL_DATA_NOT_AVAIL)) { /* XXX */
@@ -947,7 +1002,7 @@ retry_read:
 					goto changed;
 			}
 			sc->ch_state = MCD_S_WAITREAD;
-			sc->ch = timeout(mcd_timeout, (caddr_t)sc, hz/100); /* XXX */
+			callout_reset(&sc->timer, hz / 100, mcd_timeout, sc); /* XXX */
 			return;
 		} else {
 			device_printf(sc->dev, "timeout read data\n");
@@ -1010,7 +1065,8 @@ mcd_close_tray(struct mcd_softc *sc)
 		MCD_WRITE(sc, MCD_REG_COMMAND, MCD_CMDCLOSETRAY);
 		for (retry = 0; retry < CLOSE_TRAY_SECS * WAIT_FRAC; retry++) {
 			if (MCD_READ(sc, MCD_FLAGS) & MFL_STATUS_NOT_AVAIL)
-				(void) tsleep((caddr_t)sc, PSOCK | PCATCH, "mcdcls", hz/WAIT_FRAC);
+				(void) mtx_sleep(sc, &sc->mtx, PSOCK | PCATCH,
+				    "mcdcls", hz/WAIT_FRAC);
 			else {
 				if ((r = mcd_getstat(sc, 0)) == -1)
 					return (EIO);
@@ -1303,6 +1359,7 @@ mcd_toc_entrys(struct mcd_softc *sc, struct ioc_read_toc_entry *te)
 	}
 
 	/* copy the data back */
+	MCD_UNLOCK(sc);
 	return copyout(entries, te->data, n * sizeof(struct cd_toc_entry));
 }
 
@@ -1418,6 +1475,7 @@ mcd_subchan(struct mcd_softc *sc, struct ioc_read_subchannel *sch, int nocopyout
 		break;
 	}
 
+	MCD_UNLOCK(sc);
 	if (nocopyout == 0)
 		return copyout(&data, sch->data, min(sizeof(struct cd_sub_channel_info), sch->data_len));
 	bcopy(&data, sch->data, min(sizeof(struct cd_sub_channel_info), sch->data_len));

sys/dev/mcd/mcd_isa.c

@@ -126,6 +126,7 @@ mcd_alloc_resources (device_t dev)
 
 	sc = device_get_softc(dev);
 	error = 0;
+	mtx_init(&sc->mtx, "mcd", NULL, MTX_DEF);
 
 	if (sc->port_type) {
 		sc->port = bus_alloc_resource_any(dev, sc->port_type,
@@ -135,8 +136,6 @@ mcd_alloc_resources (device_t dev)
 			error = ENOMEM;
 			goto bad;
 		}
-		sc->port_bst = rman_get_bustag(sc->port);
-		sc->port_bsh = rman_get_bushandle(sc->port);
 	}
 
 	if (sc->irq_type) {
@@ -159,9 +158,6 @@ mcd_alloc_resources (device_t dev)
 		}
 	}
 
-	mtx_init(&sc->mtx, device_get_nameunit(dev),
-		"Interrupt lock", MTX_DEF | MTX_RECURSE);
-
 bad:
 	return (error);
 }
@@ -175,18 +171,14 @@ mcd_release_resources (device_t dev)
 
 	if (sc->irq_ih)
 		bus_teardown_intr(dev, sc->irq, sc->irq_ih);
-	if (sc->port) {
+	if (sc->port)
 		bus_release_resource(dev, sc->port_type, sc->port_rid, sc->port);
-		sc->port_bst = 0;
-		sc->port_bsh = 0;
-	}
 	if (sc->irq)
 		bus_release_resource(dev, sc->irq_type, sc->irq_rid, sc->irq);
 	if (sc->drq)
 		bus_release_resource(dev, sc->drq_type, sc->drq_rid, sc->drq);
 
-	if (mtx_initialized(&sc->mtx) != 0)
+	mtx_destroy(&sc->mtx);
-		mtx_destroy(&sc->mtx);
 
 	return;
 }

sys/dev/mcd/mcdvar.h

@@ -41,8 +41,6 @@ struct mcd_softc {
 	struct resource *	port;
 	int			port_rid;
 	int			port_type;
-	bus_space_tag_t		port_bst;
-	bus_space_handle_t	port_bsh;
 
 	struct resource *	irq;
 	int			irq_rid;
@@ -55,20 +53,19 @@ struct mcd_softc {
 
 	struct mtx		mtx;
 
-	struct callout_handle	ch;
+	struct callout		timer;
 	int			ch_state;
 	struct mcd_mbx *	ch_mbxsave;
 
 	struct mcd_data		data;
 };
 
-#define	MCD_LOCK(_sc)		splx(&(_sc)->mtx
+#define	MCD_LOCK(_sc)		mtx_lock(&_sc->mtx)
-#define	MCD_UNLOCK(_sc)		splx(&(_sc)->mtx
+#define	MCD_UNLOCK(_sc)		mtx_unlock(&_sc->mtx)
+#define	MCD_ASSERT_LOCKED(_sc)	mtx_assert(&_sc->mtx, MA_OWNED)
 
-#define	MCD_READ(_sc, _reg) \
+#define	MCD_READ(_sc, _reg)		bus_read_1(_sc->port, _reg)
-	bus_space_read_1(_sc->port_bst, _sc->port_bsh, _reg)
+#define	MCD_WRITE(_sc, _reg, _val)	bus_write_1(_sc->port, _reg, _val)
-#define	MCD_WRITE(_sc, _reg, _val) \
-	bus_space_write_1(_sc->port_bst, _sc->port_bsh, _reg, _val)
 
 int	mcd_probe	(struct mcd_softc *);
 int	mcd_attach	(struct mcd_softc *);