handle locking when creating or destroying vchans better

2 files changed, 104 insertions, 38 deletions

diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c
index c184a69..a7914fe 100644
--- a/sys/dev/sound/pcm/sound.c
+++ b/sys/dev/sound/pcm/sound.c
@@ -33,6 +33,7 @@
 
 SND_DECLARE_FILE("$FreeBSD$");
 
+#ifndef	PCM_DEBUG_MTX
 struct snddev_channel {
 	SLIST_ENTRY(snddev_channel) link;
 	struct pcm_channel *channel;
@@ -52,6 +53,7 @@ struct snddev_info {
 	struct sysctl_oid *sysctl_tree_top;
 	struct mtx *lock;
 };
+#endif
 
 devclass_t pcm_devclass;
 
@@ -157,6 +159,7 @@ snd_setup_intr(device_t dev, struct resource *res, int flags, driver_intr_t hand
 	return bus_setup_intr(dev, res, flags, hand, param, cookiep);
 }
 
+#ifndef	PCM_DEBUG_MTX
 void
 pcm_lock(struct snddev_info *d)
 {
@@ -168,6 +171,7 @@ pcm_unlock(struct snddev_info *d)
 {
 	snd_mtxunlock(d->lock);
 }
+#endif
 
 struct pcm_channel *
 pcm_getfakechan(struct snddev_info *d)
@@ -244,8 +248,17 @@ pcm_chnref(struct pcm_channel *c, int ref)
 int
 pcm_inprog(struct snddev_info *d, int delta)
 {
+	int r;
+
+	if (delta == 0)
+		return d->inprog;
+
+	/* backtrace(); */
+	pcm_lock(d);
 	d->inprog += delta;
-	return d->inprog;
+	r = d->inprog;
+	pcm_unlock(d);
+	return r;
 }
 
 static void
@@ -277,7 +290,9 @@ pcm_setmaxautovchans(struct snddev_info *d, int num)
 				c = sce->channel;
 				if ((c->flags & CHN_F_VIRTUAL) && !(c->flags & CHN_F_BUSY)) {
 					done = 0;
+					snd_mtxlock(d->lock);
 					err = vchan_destroy(c);
+					snd_mtxunlock(d->lock);
 					if (err)
 						device_printf(d->dev, "vchan_destroy(%s) == %d\n", c->name, err);
 					break;		/* restart */
@@ -460,13 +475,20 @@ pcm_chn_remove(struct snddev_info *d, struct pcm_channel *ch, int rmdev)
 {
     	struct snddev_channel *sce;
     	int unit = device_get_unit(d->dev);
+	int ourlock;
+
+	ourlock = 0;
+	if (!mtx_owned(d->lock)) {
+		snd_mtxlock(d->lock);
+		ourlock = 1;
+	}
 
-	snd_mtxlock(d->lock);
 	SLIST_FOREACH(sce, &d->channels, link) {
 		if (sce->channel == ch)
 			goto gotit;
 	}
-	snd_mtxunlock(d->lock);
+	if (ourlock)
+		snd_mtxunlock(d->lock);
 	return EINVAL;
 gotit:
 	SLIST_REMOVE(&d->channels, sce, snddev_channel, link);
@@ -483,7 +505,8 @@ gotit:
 	else
 		d->playcount--;
 
-	snd_mtxunlock(d->lock);
+	if (ourlock)
+		snd_mtxunlock(d->lock);
 	free(sce, M_DEVBUF);
 
 	return 0;
@@ -745,12 +768,17 @@ sndstat_prepare_pcm(struct sbuf *s, device_t dev, int verbose)
 				""
 #endif
 				);
-		if (verbose <= 1)
-			goto skipverbose;
+
+		if (verbose <= 1) {
+			snd_mtxunlock(d->lock);
+			return 0;
+		}
+
 		SLIST_FOREACH(sce, &d->channels, link) {
 			c = sce->channel;
 			sbuf_printf(s, "\n\t");
 
+			/* it would be bettet to indent child channels */
 			sbuf_printf(s, "%s[%s]: ", c->parentchannel? c->parentchannel->name : "", c->name);
 			sbuf_printf(s, "spd %d", c->speed);
 			if (c->speed != sndbuf_getspd(c->bufhard))
@@ -758,7 +786,7 @@ sndstat_prepare_pcm(struct sbuf *s, device_t dev, int verbose)
 			sbuf_printf(s, ", fmt 0x%08x", c->format);
 			if (c->format != sndbuf_getfmt(c->bufhard))
 				sbuf_printf(s, "/0x%08x", sndbuf_getfmt(c->bufhard));
-			sbuf_printf(s, ", flags %08x", c->flags);
+			sbuf_printf(s, ", flags 0x%08x, 0x%08x", c->flags, c->feederflags);
 			if (c->pid != -1)
 				sbuf_printf(s, ", pid %d", c->pid);
 			sbuf_printf(s, "\n\t");
@@ -794,7 +822,6 @@ sndstat_prepare_pcm(struct sbuf *s, device_t dev, int verbose)
 		}
 	} else
 		sbuf_printf(s, " (mixer only)");
-skipverbose:
 	snd_mtxunlock(d->lock);
 
 	return 0;
@@ -809,36 +836,38 @@ sysctl_hw_snd_vchans(SYSCTL_HANDLER_ARGS)
 	struct snddev_info *d;
     	struct snddev_channel *sce;
 	struct pcm_channel *c;
-	int err, newcnt, cnt;
+	int err, newcnt, cnt, busy;
+	int x;
 
 	d = oidp->oid_arg1;
 
-	pcm_lock(d);
+	x = pcm_inprog(d, 1);
+	if (x != 1) {
+		printf("x: %d\n", x);
+		pcm_inprog(d, -1);
+		return EINPROGRESS;
+	}
+
+	busy = 0;
 	cnt = 0;
 	SLIST_FOREACH(sce, &d->channels, link) {
 		c = sce->channel;
-		if ((c->direction == PCMDIR_PLAY) && (c->flags & CHN_F_VIRTUAL))
+		if ((c->direction == PCMDIR_PLAY) && (c->flags & CHN_F_VIRTUAL)) {
 			cnt++;
+			if (c->flags & CHN_F_BUSY)
+				busy++;
+		}
 	}
+
 	newcnt = cnt;
 
-	pcm_unlock(d);
 	err = sysctl_handle_int(oidp, &newcnt, sizeof(newcnt), req);
-	pcm_lock(d);
-	/*
-	 * Since we dropped the pcm_lock, reload cnt now as it may
-	 * have changed.
-	 */
-	cnt = 0;
-	SLIST_FOREACH(sce, &d->channels, link) {
-		c = sce->channel;
-		if ((c->direction == PCMDIR_PLAY) && (c->flags & CHN_F_VIRTUAL))
-			cnt++;
-	}
+
 	if (err == 0 && req->newptr != NULL) {
+
 		if (newcnt < 0 || newcnt > SND_MAXVCHANS) {
-			pcm_unlock(d);
-			return EINVAL;
+			pcm_inprog(d, -1);
+			return E2BIG;
 		}
 
 		if (newcnt > cnt) {
@@ -863,7 +892,7 @@ sysctl_hw_snd_vchans(SYSCTL_HANDLER_ARGS)
 				 */
 				goto addok;
 			}
-			pcm_unlock(d);
+			pcm_inprog(d, -1);
 			return EBUSY;
 addok:
 			c->flags |= CHN_F_BUSY;
@@ -875,23 +904,31 @@ addok:
 			if (SLIST_EMPTY(&c->children))
 				c->flags &= ~CHN_F_BUSY;
 		} else if (newcnt < cnt) {
+			if (busy > newcnt) {
+				printf("cnt %d, newcnt %d, busy %d\n", cnt, newcnt, busy);
+				pcm_inprog(d, -1);
+				return EBUSY;
+			}
+
+			snd_mtxlock(d->lock);
 			while (err == 0 && newcnt < cnt) {
 				SLIST_FOREACH(sce, &d->channels, link) {
 					c = sce->channel;
 					if ((c->flags & (CHN_F_BUSY | CHN_F_VIRTUAL)) == CHN_F_VIRTUAL)
 						goto remok;
 				}
-				pcm_unlock(d);
+				snd_mtxunlock(d->lock);
+				pcm_inprog(d, -1);
 				return EINVAL;
 remok:
 				err = vchan_destroy(c);
 				if (err == 0)
 					cnt--;
 			}
+			snd_mtxunlock(d->lock);
 		}
 	}
-
-	pcm_unlock(d);
+	pcm_inprog(d, -1);
 	return err;
 }
 #endif
diff --git a/sys/dev/sound/pcm/sound.h b/sys/dev/sound/pcm/sound.h
index 1daed2a..6b401af 100644
--- a/sys/dev/sound/pcm/sound.h
+++ b/sys/dev/sound/pcm/sound.h
@@ -214,8 +214,6 @@ SYSCTL_DECL(_hw_snd);
 struct sysctl_ctx_list *snd_sysctl_tree(device_t dev);
 struct sysctl_oid *snd_sysctl_tree_top(device_t dev);
 
-void pcm_lock(struct snddev_info *d);
-void pcm_unlock(struct snddev_info *d);
 struct pcm_channel *pcm_getfakechan(struct snddev_info *d);
 struct pcm_channel *pcm_chnalloc(struct snddev_info *d, int direction, pid_t pid, int chnum);
 int pcm_chnrelease(struct pcm_channel *c);
@@ -242,10 +240,9 @@ int snd_setup_intr(device_t dev, struct resource *res, int flags,
 void *snd_mtxcreate(const char *desc, const char *type);
 void snd_mtxfree(void *m);
 void snd_mtxassert(void *m);
-/*
-void snd_mtxlock(void *m);
-void snd_mtxunlock(void *m);
-*/
+#define	snd_mtxlock(m) mtx_lock(m)
+#define	snd_mtxunlock(m) mtx_unlock(m)
+
 int sysctl_hw_snd_vchans(SYSCTL_HANDLER_ARGS);
 
 typedef int (*sndstat_handler)(struct sbuf *s, device_t dev, int verbose);
@@ -270,12 +267,44 @@ int sndstat_busy(void);
 #define DV_F_DRQ_MASK	0x00000007	/* mask for secondary drq */
 #define	DV_F_DUAL_DMA	0x00000010	/* set to use secondary dma channel */
 
-/* ought to be made obsolete */
+/* ought to be made obsolete but still used by mss */
 #define	DV_F_DEV_MASK	0x0000ff00	/* force device type/class */
 #define	DV_F_DEV_SHIFT	8		/* force device type/class */
 
-#define	snd_mtxlock(m) mtx_lock(m)
-#define	snd_mtxunlock(m) mtx_unlock(m)
+#define	PCM_DEBUG_MTX
+
+/*
+ * this is rather kludgey- we need to duplicate these struct def'ns from sound.c
+ * so that the macro versions of pcm_{,un}lock can dereference them.
+ */
+
+#ifdef	PCM_DEBUG_MTX
+struct snddev_channel {
+	SLIST_ENTRY(snddev_channel) link;
+	struct pcm_channel *channel;
+};
+
+struct snddev_info {
+	SLIST_HEAD(, snddev_channel) channels;
+	struct pcm_channel *fakechan;
+	unsigned devcount, playcount, reccount, vchancount;
+	unsigned flags;
+	int inprog;
+	unsigned int bufsz;
+	void *devinfo;
+	device_t dev;
+	char status[SND_STATUSLEN];
+	struct sysctl_ctx_list sysctl_tree;
+	struct sysctl_oid *sysctl_tree_top;
+	struct mtx *lock;
+};
+
+#define	pcm_lock(d) mtx_lock(((struct snddev_info *)(d))->lock)
+#define	pcm_unlock(d) mtx_unlock(((struct snddev_info *)(d))->lock)
+#else
+void pcm_lock(struct snddev_info *d);
+void pcm_unlock(struct snddev_info *d);
+#endif
 
 #endif /* _KERNEL */