Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 25 Jan 2004 12:32:44 -0500
From:      Mathew Kanner <mat@cnd.mcgill.ca>
To:        Don Lewis <truckman@freebsd.org>
Cc:        current@freebsd.org
Subject:   Re: dev/sound/pcm/* patch testers wanted
Message-ID:  <20040125173244.GA95238@cnd.mcgill.ca>
In-Reply-To: <200401250204.i0P2427E073698@gw.catspoiler.org>
References:  <200401250204.i0P2427E073698@gw.catspoiler.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Jan 24, Don Lewis wrote:
> The is a buffer overflow in the interrupt handler in the vchan code that
> occasionally stomps on something on the kernel heap and causes panics in
> other parts of the kernel.  I tracked down the cause of the buffer
> overflow with the help of Stefan Ehmann and made a number of changes to
> the sound/pcm/* code to try to avoid triggering the overflow.

> [...]

	Don,
	I think it should be noted that vchans aren't always enabled
by default and those testing this patch should explicitly make sure
that vchans are enabled either via

	sysctl hw.snd.pcmX.vchans=5
	
	I've tested this patch with xmms and mplayer and forcing them
to cycle through many songs, and it seems at least functional.  I
would like to see the patch commited very soon.  (Say tommorow?)

	btw, the locking cleanup is very helpful.

	--Mat

> 
> Because of the architecture of the sound code, it may not be totally
> possible to avoid the potential for a buffer overflow, so I added code
> to print a diagnostic message if this situation occurs, as well as code
> to panic() the machine if an interrupt happens at the "wrong" time in
> this situation and a buffer overflow is imminent.
> 
> I cleaned up the channel locking code, but there are still issues with
> the usage of pcm_lock() that have not been addressed.
> 
> I also fixed up a NULL pointer dereference that was noticed by Ryan
> Somers.
> 
> You will need a fairly recent version of -CURRENT, with
> src/sys/dev/sound/pcm/dsp.c rev 1.70.  I highly recommend testing this
> patch with the INVARIANTS kernel option, as well as WITNESS, if
> possible, to flush out any potential problems and track them quickly to
> their source.
> 
> Please let me know if you get the "Danger!" diagnostic message.
> 
> 
> Here is the complete list of changes:
> 
>  Change KASSERT() in feed_vchan16() into an explicit test and call to
>  panic() so that the buffer overflow just beyond this point is always
>  caught, even when the code is not compiled with INVARIANTS.
> 
>  Change chn_setblocksize() buffer reallocation code to attempt to avoid
>  the feed_vchan16() buffer overflow by attempting to always keep the
>  bufsoft buffer at least as large as the bufhard buffer.
> 
>  Print a diagnositic message
>  	Danger! %s bufsoft size increasing from %d to %d after CHANNEL_SETBLOCKSIZE()
>  if our best attempts fail.  If feed_vchan16() were to be called by
>  the interrupt handler while locks are dropped in chn_setblocksize()
>  to increase the size bufsoft to match the size of bufhard, the panic()
>  code in feed_vchan16() will be triggered.  If the diagnostic message
>  is printed, it is a warning that a panic is possible if the system
>  were to see events in an "unlucky" order.
> 
>  Change the locking code to avoid the need for MTX_RECURSIVE mutexes.
> 
>  Add the MTX_DUPOK option to the channel mutexes and change the locking
>  sequence to always lock the parent channel before its children to avoid
>  the possibility of deadlock.
> 
>  Actually implement locking assertions for the channel mutexes and fix
>  the problems found by the resulting assertion violations.
> 
>  Clean up the locking code in dsp_ioctl().
> 
>  Allocate the channel buffers using the malloc() M_WAITOK option instead
>  of M_NOWAIT so that buffer allocation won't fail.  Drop locks across
>  the malloc() calls.
> 
>  Add/modify KASSERTS() in attempt to detect problems early.
> 
>  Don't dereference a NULL pointer when setting hw.snd.maxautovchans
>  if a hardware driver is not loaded.  Noticed by Ryan Sommers
>  <ryans at gamersimpact.com>.
> 
> 
> Index: sys/dev/sound/pcm/buffer.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/buffer.c,v
> retrieving revision 1.21
> diff -u -r1.21 buffer.c
> --- sys/dev/sound/pcm/buffer.c	27 Nov 2003 19:51:44 -0000	1.21
> +++ sys/dev/sound/pcm/buffer.c	19 Jan 2004 00:33:51 -0000
> @@ -30,6 +30,14 @@
>  
>  SND_DECLARE_FILE("$FreeBSD: src/sys/dev/sound/pcm/buffer.c,v 1.21 2003/11/27 19:51:44 matk Exp $");
>  
> +#ifdef USING_MUTEX
> +#define	MTX_LOCK(lock)		mtx_lock(lock);
> +#define	MTX_UNLOCK(lock)	mtx_unlock(lock);
> +#else
> +#define	MTX_LOCK(lock)
> +#define	MTX_UNLOCK(lock)
> +#endif
> +
>  struct snd_dbuf *
>  sndbuf_create(device_t dev, char *drv, char *desc)
>  {
> @@ -128,7 +136,7 @@
>  	b->blksz = blksz;
>  	b->bufsize = blkcnt * blksz;
>  
> -	tmpbuf = malloc(b->bufsize, M_DEVBUF, M_NOWAIT);
> +	tmpbuf = malloc(b->bufsize, M_DEVBUF, M_WAITOK);
>  	if (tmpbuf == NULL)
>  		return ENOMEM;
>  	free(b->tmpbuf, M_DEVBUF);
> @@ -138,25 +146,32 @@
>  }
>  
>  int
> -sndbuf_remalloc(struct snd_dbuf *b, unsigned int blkcnt, unsigned int blksz)
> +sndbuf_remalloc(struct snd_dbuf *b, unsigned int blkcnt, unsigned int blksz,
> +    struct mtx *lock)
>  {
>          u_int8_t *buf, *tmpbuf, *f1, *f2;
>          unsigned int bufsize;
> +	int ret;
>  
>  	if (blkcnt < 2 || blksz < 16)
>  		return EINVAL;
>  
>  	bufsize = blksz * blkcnt;
>  
> -	buf = malloc(bufsize, M_DEVBUF, M_NOWAIT);
> -	if (buf == NULL)
> -		return ENOMEM;
> +	MTX_UNLOCK(lock);
> +	buf = malloc(bufsize, M_DEVBUF, M_WAITOK);
> +	if (buf == NULL) {
> +		ret = ENOMEM;
> +		goto out;
> +	}
>  
> -	tmpbuf = malloc(bufsize, M_DEVBUF, M_NOWAIT);
> +	tmpbuf = malloc(bufsize, M_DEVBUF, M_WAITOK);
>     	if (tmpbuf == NULL) {
>  		free(buf, M_DEVBUF);
> -		return ENOMEM;
> +		ret = ENOMEM;
> +		goto out;
>  	}
> +	MTX_LOCK(lock);
>  
>  	b->blkcnt = blkcnt;
>  	b->blksz = blksz;
> @@ -167,13 +182,18 @@
>  	b->buf = buf;
>  	b->tmpbuf = tmpbuf;
>  
> +	sndbuf_reset(b);
> +
> +	MTX_UNLOCK(lock);
>        	if (f1)
>  		free(f1, M_DEVBUF);
>        	if (f2)
>  		free(f2, M_DEVBUF);
>  
> -	sndbuf_reset(b);
> -	return 0;
> +	ret = 0;
> +out:
> +	MTX_LOCK(lock);
> +	return ret;
>  }
>  
>  void
> Index: sys/dev/sound/pcm/buffer.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/buffer.h,v
> retrieving revision 1.8
> diff -u -r1.8 buffer.h
> --- sys/dev/sound/pcm/buffer.h	27 Nov 2003 19:51:44 -0000	1.8
> +++ sys/dev/sound/pcm/buffer.h	17 Jan 2004 05:40:24 -0000
> @@ -65,7 +65,7 @@
>  int sndbuf_setup(struct snd_dbuf *b, void *buf, unsigned int size);
>  void sndbuf_free(struct snd_dbuf *b);
>  int sndbuf_resize(struct snd_dbuf *b, unsigned int blkcnt, unsigned int blksz);
> -int sndbuf_remalloc(struct snd_dbuf *b, unsigned int blkcnt, unsigned int blksz);
> +int sndbuf_remalloc(struct snd_dbuf *b, unsigned int blkcnt, unsigned int blksz, struct mtx *lock);
>  void sndbuf_reset(struct snd_dbuf *b);
>  void sndbuf_clear(struct snd_dbuf *b, unsigned int length);
>  void sndbuf_fillsilence(struct snd_dbuf *b);
> Index: sys/dev/sound/pcm/channel.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/channel.c,v
> retrieving revision 1.93
> diff -u -r1.93 channel.c
> --- sys/dev/sound/pcm/channel.c	5 Dec 2003 02:08:13 -0000	1.93
> +++ sys/dev/sound/pcm/channel.c	20 Jan 2004 20:19:37 -0000
> @@ -70,9 +70,9 @@
>  chn_lockinit(struct pcm_channel *c, int dir)
>  {
>  	if (dir == PCMDIR_PLAY)
> -		c->lock = snd_mtxcreate(c->name, "pcm play channel");
> +		c->lock = snd_chnmtxcreate(c->name, "pcm play channel");
>  	else
> -		c->lock = snd_mtxcreate(c->name, "pcm record channel");
> +		c->lock = snd_chnmtxcreate(c->name, "pcm record channel");
>  }
>  
>  static void
> @@ -205,16 +205,19 @@
>  	unsigned int ret, amt;
>  
>  	CHN_LOCKASSERT(c);
> -    	DEB(
> +/*    	DEB(
>  	if (c->flags & CHN_F_CLOSING) {
>  		sndbuf_dump(b, "b", 0x02);
>  		sndbuf_dump(bs, "bs", 0x02);
> -	})
> +	}) */
>  
>  	if (c->flags & CHN_F_MAPPED)
>  		sndbuf_acquire(bs, NULL, sndbuf_getfree(bs));
>  
>  	amt = sndbuf_getfree(b);
> +	KASSERT(amt <= sndbuf_getsize(bs),
> +	    ("%s(%s): amt %d > source size %d, flags 0x%x", __func__, c->name,
> +	   amt, sndbuf_getsize(bs), c->flags));
>  	if (sndbuf_getready(bs) < amt)
>  		c->xruns++;
>  
> @@ -746,13 +749,6 @@
>  	c->devinfo = NULL;
>  	c->feeder = NULL;
>  
> -	ret = EINVAL;
> -	fc = feeder_getclass(NULL);
> -	if (fc == NULL)
> -		goto out;
> -	if (chn_addfeeder(c, fc, NULL))
> -		goto out;
> -
>  	ret = ENOMEM;
>  	b = sndbuf_create(c->dev, c->name, "primary");
>  	if (b == NULL)
> @@ -760,6 +756,16 @@
>  	bs = sndbuf_create(c->dev, c->name, "secondary");
>  	if (bs == NULL)
>  		goto out;
> +
> +	CHN_LOCK(c);
> +
> +	ret = EINVAL;
> +	fc = feeder_getclass(NULL);
> +	if (fc == NULL)
> +		goto out;
> +	if (chn_addfeeder(c, fc, NULL))
> +		goto out;
> +
>  	sndbuf_setup(bs, NULL, 0);
>  	c->bufhard = b;
>  	c->bufsoft = bs;
> @@ -767,7 +773,9 @@
>  	c->feederflags = 0;
>  
>  	ret = ENODEV;
> +	CHN_UNLOCK(c); /* XXX - Unlock for CHANNEL_INIT() malloc() call */
>  	c->devinfo = CHANNEL_INIT(c->methods, devinfo, b, c, dir);
> +	CHN_LOCK(c);
>  	if (c->devinfo == NULL)
>  		goto out;
>  
> @@ -789,6 +797,7 @@
>  
>  
>  out:
> +	CHN_UNLOCK(c);
>  	if (ret) {
>  		if (c->devinfo) {
>  			if (CHANNEL_FREE(c->methods, c->devinfo))
> @@ -971,11 +980,17 @@
>  {
>  	struct snd_dbuf *b = c->bufhard;
>  	struct snd_dbuf *bs = c->bufsoft;
> -	int bufsz, irqhz, tmp, ret;
> +	int irqhz, tmp, ret, maxsize, reqblksz, tmpblksz;
>  
>  	CHN_LOCKASSERT(c);
> -	if (!CANCHANGE(c) || (c->flags & CHN_F_MAPPED))
> +	if (!CANCHANGE(c) || (c->flags & CHN_F_MAPPED)) {
> +		KASSERT(sndbuf_getsize(bs) ==  0 ||
> +		    sndbuf_getsize(bs) >= sndbuf_getsize(b),
> +		    ("%s(%s): bufsoft size %d < bufhard size %d", __func__,
> +		    c->name, sndbuf_getsize(bs), sndbuf_getsize(b)));
>  		return EINVAL;
> +	}
> +	c->flags |= CHN_F_SETBLOCKSIZE;
>  
>  	ret = 0;
>  	DEB(printf("%s(%d, %d)\n", __func__, blkcnt, blksz));
> @@ -1007,36 +1022,68 @@
>  		c->flags |= CHN_F_HAS_SIZE;
>  	}
>  
> -	bufsz = blkcnt * blksz;
> -
> -	ret = sndbuf_remalloc(bs, blkcnt, blksz);
> -	if (ret)
> -		goto out;
> +	reqblksz = blksz;
>  
>  	/* adjust for different hw format/speed */
> -	irqhz = (sndbuf_getbps(bs) * sndbuf_getspd(bs)) / sndbuf_getblksz(bs);
> +	irqhz = (sndbuf_getbps(bs) * sndbuf_getspd(bs)) / blksz;
>  	DEB(printf("%s: soft bps %d, spd %d, irqhz == %d\n", __func__, sndbuf_getbps(bs), sndbuf_getspd(bs), irqhz));
>  	RANGE(irqhz, 16, 512);
>  
> -	sndbuf_setblksz(b, (sndbuf_getbps(b) * sndbuf_getspd(b)) / irqhz);
> +	tmpblksz = (sndbuf_getbps(b) * sndbuf_getspd(b)) / irqhz;
>  
>  	/* round down to 2^x */
>  	blksz = 32;
> -	while (blksz <= sndbuf_getblksz(b))
> +	while (blksz <= tmpblksz)
>  		blksz <<= 1;
>  	blksz >>= 1;
>  
>  	/* round down to fit hw buffer size */
> -	RANGE(blksz, 16, sndbuf_getmaxsize(b) / 2);
> +	if (sndbuf_getmaxsize(b) > 0)
> +		RANGE(blksz, 16, sndbuf_getmaxsize(b) / 2);
> +	else
> +		/* virtual channels don't appear to allocate bufhard */
> +		RANGE(blksz, 16, CHN_2NDBUFMAXSIZE / 2);
>  	DEB(printf("%s: hard blksz requested %d (maxsize %d), ", __func__, blksz, sndbuf_getmaxsize(b)));
>  
> +	/* Increase the size of bufsoft if before increasing bufhard. */
> +	maxsize = sndbuf_getsize(b);
> +	if (sndbuf_getsize(bs) > maxsize)
> +		maxsize = sndbuf_getsize(bs);
> +	if (reqblksz * blkcnt > maxsize)
> +		maxsize = reqblksz * blkcnt;
> +	if (sndbuf_getsize(bs) != maxsize || sndbuf_getblksz(bs) != reqblksz) {
> +		ret = sndbuf_remalloc(bs, maxsize/reqblksz, reqblksz, c->lock);
> +		if (ret)
> +			goto out1;
> +	}
> +
>  	sndbuf_setblksz(b, CHANNEL_SETBLOCKSIZE(c->methods, c->devinfo, blksz));
>  
> +	/* Decrease the size of bufsoft after decreasing bufhard. */
> +	maxsize = sndbuf_getsize(b);
> +	if (reqblksz * blkcnt > maxsize)
> +		maxsize = reqblksz * blkcnt;
> +	if (maxsize > sndbuf_getsize(bs))
> +		printf("Danger! %s bufsoft size increasing from %d to %d after CHANNEL_SETBLOCKSIZE()\n",
> +		    c->name, sndbuf_getsize(bs), maxsize);
> +	if (sndbuf_getsize(bs) != maxsize || sndbuf_getblksz(bs) != reqblksz) {
> +		ret = sndbuf_remalloc(bs, maxsize/reqblksz, reqblksz, c->lock);
> +		if (ret)
> +			goto out1;
> +	}
> +
>  	irqhz = (sndbuf_getbps(b) * sndbuf_getspd(b)) / sndbuf_getblksz(b);
>  	DEB(printf("got %d, irqhz == %d\n", sndbuf_getblksz(b), irqhz));
>  
>  	chn_resetbuf(c);
> +out1:
> +	KASSERT(sndbuf_getsize(bs) ==  0 ||
> +	    sndbuf_getsize(bs) >= sndbuf_getsize(b),
> +	    ("%s(%s): bufsoft size %d < bufhard size %d, reqblksz=%d blksz=%d maxsize=%d blkcnt=%d",
> +	    __func__, c->name, sndbuf_getsize(bs), sndbuf_getsize(b), reqblksz,
> +	    blksz, maxsize, blkcnt));
>  out:
> +	c->flags &= ~CHN_F_SETBLOCKSIZE;
>  	return ret;
>  }
>  
> @@ -1216,8 +1263,12 @@
>  	struct pcm_channel *child;
>  	int run;
>  
> -	if (SLIST_EMPTY(&c->children))
> +	CHN_LOCK(c);
> +
> +	if (SLIST_EMPTY(&c->children)) {
> +		CHN_UNLOCK(c);
>  		return ENODEV;
> +	}
>  
>  	run = (c->flags & CHN_F_TRIGGERED)? 1 : 0;
>  	/*
> @@ -1253,8 +1304,10 @@
>  		blksz = sndbuf_getmaxsize(c->bufhard) / 2;
>  		SLIST_FOREACH(pce, &c->children, link) {
>  			child = pce->channel;
> +			CHN_LOCK(child);
>  			if (sndbuf_getblksz(child->bufhard) < blksz)
>  				blksz = sndbuf_getblksz(child->bufhard);
> +			CHN_UNLOCK(child);
>  		}
>  		chn_setblocksize(c, 2, blksz);
>  	}
> @@ -1268,13 +1321,16 @@
>  		nrun = 0;
>  		SLIST_FOREACH(pce, &c->children, link) {
>  			child = pce->channel;
> +			CHN_LOCK(child);
>  			if (child->flags & CHN_F_TRIGGERED)
>  				nrun = 1;
> +			CHN_UNLOCK(child);
>  		}
>  		if (nrun && !run)
>  			chn_start(c, 1);
>  		if (!nrun && run)
>  			chn_abort(c);
>  	}
> +	CHN_UNLOCK(c);
>  	return 0;
>  }
> Index: sys/dev/sound/pcm/channel.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/channel.h,v
> retrieving revision 1.28
> diff -u -r1.28 channel.h
> --- sys/dev/sound/pcm/channel.h	7 Sep 2003 16:28:03 -0000	1.28
> +++ sys/dev/sound/pcm/channel.h	19 Jan 2004 04:32:36 -0000
> @@ -103,7 +103,7 @@
>  #ifdef	USING_MUTEX
>  #define CHN_LOCK(c) mtx_lock((struct mtx *)((c)->lock))
>  #define CHN_UNLOCK(c) mtx_unlock((struct mtx *)((c)->lock))
> -#define CHN_LOCKASSERT(c)
> +#define CHN_LOCKASSERT(c) mtx_assert((struct mtx *)((c)->lock), MA_OWNED)
>  #else
>  #define CHN_LOCK(c)
>  #define CHN_UNLOCK(c)
> @@ -134,6 +134,7 @@
>  #define CHN_F_MAPPED		0x00010000  /* has been mmap()ed */
>  #define CHN_F_DEAD		0x00020000
>  #define CHN_F_BADSETTING	0x00040000
> +#define CHN_F_SETBLOCKSIZE	0x00080000
>  
>  #define	CHN_F_VIRTUAL		0x10000000  /* not backed by hardware */
>  
> Index: sys/dev/sound/pcm/dsp.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/dsp.c,v
> retrieving revision 1.70
> diff -u -r1.70 dsp.c
> --- sys/dev/sound/pcm/dsp.c	20 Jan 2004 05:30:09 -0000	1.70
> +++ sys/dev/sound/pcm/dsp.c	23 Jan 2004 09:47:54 -0000
> @@ -113,8 +113,8 @@
>  
>  	flags = dsp_get_flags(dev);
>  	d = dsp_get_info(dev);
> -	pcm_lock(d);
>  	pcm_inprog(d, 1);
> +	pcm_lock(d);
>  	KASSERT((flags & SD_F_PRIO_SET) != SD_F_PRIO_SET, \
>  		("getchns: read and write both prioritised"));
>  
> @@ -159,9 +159,7 @@
>  		CHN_UNLOCK(wrch);
>  	if (rdch && rdch != pcm_getfakechan(d) && (prio & SD_F_PRIO_RD))
>  		CHN_UNLOCK(rdch);
> -	pcm_lock(d);
>  	pcm_inprog(d, -1);
> -	pcm_unlock(d);
>  }
>  
>  static int
> @@ -476,6 +474,11 @@
>  		wrch = NULL;
>  	if (kill & 2)
>  		rdch = NULL;
> +	
> +	if (rdch != NULL)
> +		CHN_LOCK(rdch);
> +	if (wrch != NULL)
> +		CHN_LOCK(wrch);
>  
>      	switch(cmd) {
>  #ifdef OLDPCM_IOCTL
> @@ -497,16 +500,12 @@
>  			p->play_size = 0;
>  			p->rec_size = 0;
>  	    		if (wrch) {
> -				CHN_LOCK(wrch);
>  				chn_setblocksize(wrch, 2, p->play_size);
>  				p->play_size = sndbuf_getblksz(wrch->bufsoft);
> -				CHN_UNLOCK(wrch);
>  			}
>  	    		if (rdch) {
> -				CHN_LOCK(rdch);
>  				chn_setblocksize(rdch, 2, p->rec_size);
>  				p->rec_size = sndbuf_getblksz(rdch->bufsoft);
> -				CHN_UNLOCK(rdch);
>  			}
>  		}
>  		break;
> @@ -526,16 +525,12 @@
>  	    		snd_chan_param *p = (snd_chan_param *)arg;
>  
>  	    		if (wrch) {
> -				CHN_LOCK(wrch);
>  				chn_setformat(wrch, p->play_format);
>  				chn_setspeed(wrch, p->play_rate);
> -				CHN_UNLOCK(wrch);
>  	    		}
>  	    		if (rdch) {
> -				CHN_LOCK(rdch);
>  				chn_setformat(rdch, p->rec_format);
>  				chn_setspeed(rdch, p->rec_rate);
> -				CHN_UNLOCK(rdch);
>  	    		}
>  		}
>  		/* FALLTHROUGH */
> @@ -557,14 +552,10 @@
>  			struct pcmchan_caps *pcaps = NULL, *rcaps = NULL;
>  			dev_t pdev;
>  
> -			if (rdch) {
> -				CHN_LOCK(rdch);
> +			if (rdch)
>  				rcaps = chn_getcaps(rdch);
> -			}
> -			if (wrch) {
> -				CHN_LOCK(wrch);
> +			if (wrch)
>  				pcaps = chn_getcaps(wrch);
> -			}
>  	    		p->rate_min = max(rcaps? rcaps->minspeed : 0,
>  	                      		  pcaps? pcaps->minspeed : 0);
>  	    		p->rate_max = min(rcaps? rcaps->maxspeed : 1000000,
> @@ -580,10 +571,6 @@
>  	    		p->mixers = 1; /* default: one mixer */
>  	    		p->inputs = pdev->si_drv1? mix_getdevs(pdev->si_drv1) : 0;
>  	    		p->left = p->right = 100;
> -			if (wrch)
> -				CHN_UNLOCK(wrch);
> -			if (rdch)
> -				CHN_UNLOCK(rdch);
>  		}
>  		break;
>  
> @@ -646,59 +633,42 @@
>  
>      	case SNDCTL_DSP_SETBLKSIZE:
>  		RANGE(*arg_i, 16, 65536);
> -		if (wrch) {
> -			CHN_LOCK(wrch);
> +		if (wrch)
>  			chn_setblocksize(wrch, 2, *arg_i);
> -			CHN_UNLOCK(wrch);
> -		}
> -		if (rdch) {
> -			CHN_LOCK(rdch);
> +		if (rdch)
>  			chn_setblocksize(rdch, 2, *arg_i);
> -			CHN_UNLOCK(rdch);
> -		}
>  		break;
>  
>      	case SNDCTL_DSP_RESET:
>  		DEB(printf("dsp reset\n"));
>  		if (wrch) {
> -			CHN_LOCK(wrch);
>  			chn_abort(wrch);
>  			chn_resetbuf(wrch);
> -			CHN_UNLOCK(wrch);
>  		}
>  		if (rdch) {
> -			CHN_LOCK(rdch);
>  			chn_abort(rdch);
>  			chn_resetbuf(rdch);
> -			CHN_UNLOCK(rdch);
>  		}
>  		break;
>  
>      	case SNDCTL_DSP_SYNC:
>  		DEB(printf("dsp sync\n"));
>  		/* chn_sync may sleep */
> -		if (wrch) {
> -			CHN_LOCK(wrch);
> +		if (wrch)
>  			chn_sync(wrch, sndbuf_getsize(wrch->bufsoft) - 4);
> -			CHN_UNLOCK(wrch);
> -		}
>  		break;
>  
>      	case SNDCTL_DSP_SPEED:
>  		/* chn_setspeed may sleep */
>  		tmp = 0;
>  		if (wrch) {
> -			CHN_LOCK(wrch);
>  			ret = chn_setspeed(wrch, *arg_i);
>  			tmp = wrch->speed;
> -			CHN_UNLOCK(wrch);
>  		}
>  		if (rdch && ret == 0) {
> -			CHN_LOCK(rdch);
>  			ret = chn_setspeed(rdch, *arg_i);
>  			if (tmp == 0)
>  				tmp = rdch->speed;
> -			CHN_UNLOCK(rdch);
>  		}
>  		*arg_i = tmp;
>  		break;
> @@ -711,17 +681,13 @@
>  		tmp = -1;
>  		*arg_i = (*arg_i)? AFMT_STEREO : 0;
>  		if (wrch) {
> -			CHN_LOCK(wrch);
>  			ret = chn_setformat(wrch, (wrch->format & ~AFMT_STEREO) | *arg_i);
>  			tmp = (wrch->format & AFMT_STEREO)? 1 : 0;
> -			CHN_UNLOCK(wrch);
>  		}
>  		if (rdch && ret == 0) {
> -			CHN_LOCK(rdch);
>  			ret = chn_setformat(rdch, (rdch->format & ~AFMT_STEREO) | *arg_i);
>  			if (tmp == -1)
>  				tmp = (rdch->format & AFMT_STEREO)? 1 : 0;
> -			CHN_UNLOCK(rdch);
>  		}
>  		*arg_i = tmp;
>  		break;
> @@ -732,22 +698,17 @@
>  			tmp = 0;
>  			*arg_i = (*arg_i != 1)? AFMT_STEREO : 0;
>  	  		if (wrch) {
> -				CHN_LOCK(wrch);
>  				ret = chn_setformat(wrch, (wrch->format & ~AFMT_STEREO) | *arg_i);
>  				tmp = (wrch->format & AFMT_STEREO)? 2 : 1;
> -				CHN_UNLOCK(wrch);
>  			}
>  			if (rdch && ret == 0) {
> -				CHN_LOCK(rdch);
>  				ret = chn_setformat(rdch, (rdch->format & ~AFMT_STEREO) | *arg_i);
>  				if (tmp == 0)
>  					tmp = (rdch->format & AFMT_STEREO)? 2 : 1;
> -				CHN_UNLOCK(rdch);
>  			}
>  			*arg_i = tmp;
> -		} else {
> +		} else
>  			*arg_i = ((wrch? wrch->format : rdch->format) & AFMT_STEREO)? 2 : 1;
> -		}
>  		break;
>  
>      	case SOUND_PCM_READ_CHANNELS:
> @@ -763,17 +724,13 @@
>  		if ((*arg_i != AFMT_QUERY)) {
>  			tmp = 0;
>  			if (wrch) {
> -				CHN_LOCK(wrch);
>  				ret = chn_setformat(wrch, (*arg_i) | (wrch->format & AFMT_STEREO));
>  				tmp = wrch->format & ~AFMT_STEREO;
> -				CHN_UNLOCK(wrch);
>  			}
>  			if (rdch && ret == 0) {
> -				CHN_LOCK(rdch);
>  				ret = chn_setformat(rdch, (*arg_i) | (rdch->format & AFMT_STEREO));
>  				if (tmp == 0)
>  					tmp = rdch->format & ~AFMT_STEREO;
> -				CHN_UNLOCK(rdch);
>  			}
>  			*arg_i = tmp;
>  		} else
> @@ -800,18 +757,14 @@
>  
>  			DEB(printf("SNDCTL_DSP_SETFRAGMENT %d frags, %d sz\n", maxfrags, fragsz));
>  		    	if (rdch) {
> -				CHN_LOCK(rdch);
>  				ret = chn_setblocksize(rdch, maxfrags, fragsz);
>  				maxfrags = sndbuf_getblkcnt(rdch->bufsoft);
>  				fragsz = sndbuf_getblksz(rdch->bufsoft);
> -				CHN_UNLOCK(rdch);
>  			}
>  		    	if (wrch && ret == 0) {
> -				CHN_LOCK(wrch);
>  				ret = chn_setblocksize(wrch, maxfrags, fragsz);
>   				maxfrags = sndbuf_getblkcnt(wrch->bufsoft);
>  				fragsz = sndbuf_getblksz(wrch->bufsoft);
> -				CHN_UNLOCK(wrch);
>  			}
>  
>  			fragln = 0;
> @@ -830,12 +783,10 @@
>  	    		if (rdch) {
>  	        		struct snd_dbuf *bs = rdch->bufsoft;
>  
> -				CHN_LOCK(rdch);
>  				a->bytes = sndbuf_getready(bs);
>  	        		a->fragments = a->bytes / sndbuf_getblksz(bs);
>  	        		a->fragstotal = sndbuf_getblkcnt(bs);
>  	        		a->fragsize = sndbuf_getblksz(bs);
> -				CHN_UNLOCK(rdch);
>  	    		}
>  		}
>  		break;
> @@ -847,13 +798,11 @@
>  	    		if (wrch) {
>  	        		struct snd_dbuf *bs = wrch->bufsoft;
>  
> -				CHN_LOCK(wrch);
>  				chn_wrupdate(wrch);
>  				a->bytes = sndbuf_getfree(bs);
>  	        		a->fragments = a->bytes / sndbuf_getblksz(bs);
>  	        		a->fragstotal = sndbuf_getblkcnt(bs);
>  	        		a->fragsize = sndbuf_getblksz(bs);
> -				CHN_UNLOCK(wrch);
>  	    		}
>  		}
>  		break;
> @@ -864,13 +813,11 @@
>  	    		if (rdch) {
>  	        		struct snd_dbuf *bs = rdch->bufsoft;
>  
> -				CHN_LOCK(rdch);
>  				chn_rdupdate(rdch);
>  	        		a->bytes = sndbuf_gettotal(bs);
>  	        		a->blocks = sndbuf_getblocks(bs) - rdch->blocks;
>  	        		a->ptr = sndbuf_getreadyptr(bs);
>  				rdch->blocks = sndbuf_getblocks(bs);
> -				CHN_UNLOCK(rdch);
>  	    		} else
>  				ret = EINVAL;
>  		}
> @@ -882,13 +829,11 @@
>  	    		if (wrch) {
>  	        		struct snd_dbuf *bs = wrch->bufsoft;
>  
> -				CHN_LOCK(wrch);
>  				chn_wrupdate(wrch);
>  	        		a->bytes = sndbuf_gettotal(bs);
>  	        		a->blocks = sndbuf_getblocks(bs) - wrch->blocks;
>  	        		a->ptr = sndbuf_getreadyptr(bs);
>  				wrch->blocks = sndbuf_getblocks(bs);
> -				CHN_UNLOCK(wrch);
>  	    		} else
>  				ret = EINVAL;
>  		}
> @@ -906,22 +851,18 @@
>  
>      	case SNDCTL_DSP_SETTRIGGER:
>  		if (rdch) {
> -			CHN_LOCK(rdch);
>  			rdch->flags &= ~(CHN_F_TRIGGERED | CHN_F_NOTRIGGER);
>  		    	if (*arg_i & PCM_ENABLE_INPUT)
>  				chn_start(rdch, 1);
>  			else
>  				rdch->flags |= CHN_F_NOTRIGGER;
> -			CHN_UNLOCK(rdch);
>  		}
>  		if (wrch) {
> -			CHN_LOCK(wrch);
>  			wrch->flags &= ~(CHN_F_TRIGGERED | CHN_F_NOTRIGGER);
>  		    	if (*arg_i & PCM_ENABLE_OUTPUT)
>  				chn_start(wrch, 1);
>  			else
>  				wrch->flags |= CHN_F_NOTRIGGER;
> -		 	CHN_UNLOCK(wrch);
>  		}
>  		break;
>  
> @@ -938,20 +879,16 @@
>  			struct snd_dbuf *b = wrch->bufhard;
>  	        	struct snd_dbuf *bs = wrch->bufsoft;
>  
> -			CHN_LOCK(wrch);
>  			chn_wrupdate(wrch);
>  			*arg_i = sndbuf_getready(b) + sndbuf_getready(bs);
> -			CHN_UNLOCK(wrch);
>  		} else
>  			ret = EINVAL;
>  		break;
>  
>      	case SNDCTL_DSP_POST:
>  		if (wrch) {
> -			CHN_LOCK(wrch);
>  			wrch->flags &= ~CHN_F_NOTRIGGER;
>  			chn_start(wrch, 1);
> -			CHN_UNLOCK(wrch);
>  		}
>  		break;
>  
> @@ -969,6 +906,10 @@
>  		ret = EINVAL;
>  		break;
>      	}
> +	if (rdch != NULL)
> +		CHN_UNLOCK(rdch);
> +	if (wrch != NULL)
> +		CHN_UNLOCK(wrch);
>  	relchns(i_dev, rdch, wrch, 0);
>  	splx(s);
>      	return ret;
> Index: sys/dev/sound/pcm/sound.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/sound.c,v
> retrieving revision 1.88
> diff -u -r1.88 sound.c
> --- sys/dev/sound/pcm/sound.c	20 Jan 2004 03:58:57 -0000	1.88
> +++ sys/dev/sound/pcm/sound.c	20 Jan 2004 10:34:46 -0000
> @@ -75,7 +75,23 @@
>  	m = malloc(sizeof(*m), M_DEVBUF, M_WAITOK | M_ZERO);
>  	if (m == NULL)
>  		return NULL;
> -	mtx_init(m, desc, type, MTX_DEF | MTX_RECURSE);
> +	mtx_init(m, desc, type, MTX_DEF);
> +	return m;
> +#else
> +	return (void *)0xcafebabe;
> +#endif
> +}
> +
> +void *
> +snd_chnmtxcreate(const char *desc, const char *type)
> +{
> +#ifdef USING_MUTEX
> +	struct mtx *m;
> +
> +	m = malloc(sizeof(*m), M_DEVBUF, M_WAITOK | M_ZERO);
> +	if (m == NULL)
> +		return NULL;
> +	mtx_init(m, desc, type, MTX_DEF | MTX_DUPOK);
>  	return m;
>  #else
>  	return (void *)0xcafebabe;
> @@ -188,13 +204,16 @@
>  			/* try to create a vchan */
>  			SLIST_FOREACH(sce, &d->channels, link) {
>  				c = sce->channel;
> +				CHN_LOCK(c);
>  				if (!SLIST_EMPTY(&c->children)) {
>  					err = vchan_create(c);
> +					CHN_UNLOCK(c);
>  					if (!err)
>  						return pcm_chnalloc(d, direction, pid, -1);
>  					else
>  						device_printf(d->dev, "vchan_create(%s) == %d\n", c->name, err);
> -				}
> +				} else
> +					CHN_UNLOCK(c);
>  			}
>  		}
>  	}
> @@ -250,15 +269,19 @@
>  	if (num > 0 && d->vchancount == 0) {
>  		SLIST_FOREACH(sce, &d->channels, link) {
>  			c = sce->channel;
> +			CHN_LOCK(c);
>  			if ((c->direction == PCMDIR_PLAY) && !(c->flags & CHN_F_BUSY)) {
>  				c->flags |= CHN_F_BUSY;
>  				err = vchan_create(c);
>  				if (err) {
> -					device_printf(d->dev, "vchan_create(%s) == %d\n", c->name, err);
>  					c->flags &= ~CHN_F_BUSY;
> -				}
> +					CHN_UNLOCK(c);
> +					device_printf(d->dev, "vchan_create(%s) == %d\n", c->name, err);
> +				} else
> +					CHN_UNLOCK(c);
>  				return;
>  			}
> +			CHN_UNLOCK(c);
>  		}
>  	}
>  	if (num == 0 && d->vchancount > 0) {
> @@ -313,7 +336,7 @@
>  	v = snd_maxautovchans;
>  	error = sysctl_handle_int(oidp, &v, sizeof(v), req);
>  	if (error == 0 && req->newptr != NULL) {
> -		if (v < 0 || v >= SND_MAXVCHANS)
> +		if (v < 0 || v >= SND_MAXVCHANS || pcm_devclass == NULL)
>  			return EINVAL;
>  		if (v != snd_maxautovchans) {
>  			for (i = 0; i < devclass_get_maxunit(pcm_devclass); i++) {
> @@ -529,20 +552,23 @@
>  	err = pcm_chn_add(d, ch);
>  	if (err) {
>  		device_printf(d->dev, "pcm_chn_add(%s) failed, err=%d\n", ch->name, err);
> -		snd_mtxunlock(d->lock);
>  		pcm_chn_destroy(ch);
>  		return err;
>  	}
>  
> +	CHN_LOCK(ch);
>  	if (snd_maxautovchans > 0 && (d->flags & SD_F_AUTOVCHAN) &&
>  	    ch->direction == PCMDIR_PLAY && d->vchancount == 0) {
>  		ch->flags |= CHN_F_BUSY;
>  		err = vchan_create(ch);
>  		if (err) {
> -			device_printf(d->dev, "vchan_create(%s) == %d\n", ch->name, err);
>  			ch->flags &= ~CHN_F_BUSY;
> +			CHN_UNLOCK(ch);
> +			device_printf(d->dev, "vchan_create(%s) == %d\n", ch->name, err);
> +			return err;
>  		}
>  	}
> +	CHN_UNLOCK(ch);
>  
>  	return err;
>  }
> @@ -866,11 +892,13 @@
>  	cnt = 0;
>  	SLIST_FOREACH(sce, &d->channels, link) {
>  		c = sce->channel;
> +		CHN_LOCK(c);
>  		if ((c->direction == PCMDIR_PLAY) && (c->flags & CHN_F_VIRTUAL)) {
>  			cnt++;
>  			if (c->flags & CHN_F_BUSY)
>  				busy++;
>  		}
> +		CHN_UNLOCK(c);
>  	}
>  
>  	newcnt = cnt;
> @@ -888,23 +916,28 @@
>  			/* add new vchans - find a parent channel first */
>  			SLIST_FOREACH(sce, &d->channels, link) {
>  				c = sce->channel;
> +				CHN_LOCK(c);
>  				/* not a candidate if not a play channel */
>  				if (c->direction != PCMDIR_PLAY)
> -					continue;
> +					goto next;
>  				/* not a candidate if a virtual channel */
>  				if (c->flags & CHN_F_VIRTUAL)
> -					continue;
> +					goto next;
>  				/* not a candidate if it's in use */
> -				if ((c->flags & CHN_F_BUSY) && (SLIST_EMPTY(&c->children)))
> -					continue;
> -				/*
> -				 * if we get here we're a nonvirtual play channel, and either
> -				 * 1) not busy
> -				 * 2) busy with children, not directly open
> -				 *
> -				 * thus we can add children
> -				 */
> -				goto addok;
> +				if (!(c->flags & CHN_F_BUSY) ||
> +				    !(SLIST_EMPTY(&c->children)))
> +					/*
> +					 * if we get here we're a nonvirtual
> +					 * play channel, and either
> +					 * 1) not busy
> +					 * 2) busy with children, not directly
> +					 *    open
> +					 *
> +					 * thus we can add children
> +					 */
> +					goto addok;
> +next:
> +				CHN_UNLOCK(c);
>  			}
>  			pcm_inprog(d, -1);
>  			return EBUSY;
> @@ -917,6 +950,7 @@
>  			}
>  			if (SLIST_EMPTY(&c->children))
>  				c->flags &= ~CHN_F_BUSY;
> +			CHN_UNLOCK(c);
>  		} else if (newcnt < cnt) {
>  			if (busy > newcnt) {
>  				printf("cnt %d, newcnt %d, busy %d\n", cnt, newcnt, busy);
> @@ -928,13 +962,17 @@
>  			while (err == 0 && newcnt < cnt) {
>  				SLIST_FOREACH(sce, &d->channels, link) {
>  					c = sce->channel;
> +					CHN_LOCK(c);
>  					if ((c->flags & (CHN_F_BUSY | CHN_F_VIRTUAL)) == CHN_F_VIRTUAL)
>  						goto remok;
> +
> +					CHN_UNLOCK(c);
>  				}
>  				snd_mtxunlock(d->lock);
>  				pcm_inprog(d, -1);
>  				return EINVAL;
>  remok:
> +				CHN_UNLOCK(c);
>  				err = vchan_destroy(c);
>  				if (err == 0)
>  					cnt--;
> Index: sys/dev/sound/pcm/sound.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/sound.h,v
> retrieving revision 1.54
> diff -u -r1.54 sound.h
> --- sys/dev/sound/pcm/sound.h	20 Jan 2004 03:58:57 -0000	1.54
> +++ sys/dev/sound/pcm/sound.h	20 Jan 2004 10:34:46 -0000
> @@ -238,6 +238,7 @@
>  		   driver_intr_t hand, void *param, void **cookiep);
>  
>  void *snd_mtxcreate(const char *desc, const char *type);
> +void *snd_chnmtxcreate(const char *desc, const char *type);
>  void snd_mtxfree(void *m);
>  void snd_mtxassert(void *m);
>  #define	snd_mtxlock(m) mtx_lock(m)
> Index: sys/dev/sound/pcm/vchan.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/sound/pcm/vchan.c,v
> retrieving revision 1.15
> diff -u -r1.15 vchan.c
> --- sys/dev/sound/pcm/vchan.c	20 Jan 2004 03:58:57 -0000	1.15
> +++ sys/dev/sound/pcm/vchan.c	25 Jan 2004 01:54:21 -0000
> @@ -77,7 +77,9 @@
>  	int16_t *tmp, *dst;
>  	unsigned int cnt;
>  
> -	KASSERT(sndbuf_getsize(src) >= count, ("bad bufsize"));
> +	if (sndbuf_getsize(src) < count)
> +		panic("feed_vchan_s16(%s): tmp buffer size %d < count %d, flags = 0x%x",
> +		    c->name, sndbuf_getsize(src), count, c->flags);
>  	count &= ~1;
>  	bzero(b, count);
>  
> @@ -92,12 +94,14 @@
>  	bzero(tmp, count);
>  	SLIST_FOREACH(cce, &c->children, link) {
>  		ch = cce->channel;
> +   		CHN_LOCK(ch);
>  		if (ch->flags & CHN_F_TRIGGERED) {
>  			if (ch->flags & CHN_F_MAPPED)
>  				sndbuf_acquire(ch->bufsoft, NULL, sndbuf_getfree(ch->bufsoft));
>  			cnt = FEEDER_FEED(ch->feeder, ch, (u_int8_t *)tmp, count, ch->bufsoft);
>  			vchan_mix_s16(dst, tmp, cnt / 2);
>  		}
> +   		CHN_UNLOCK(ch);
>  	}
>  
>  	return count;
> @@ -145,13 +149,16 @@
>  {
>  	struct vchinfo *ch = data;
>  	struct pcm_channel *parent = ch->parent;
> +	struct pcm_channel *channel = ch->channel;
>  
>  	ch->fmt = format;
>  	ch->bps = 1;
>  	ch->bps <<= (ch->fmt & AFMT_STEREO)? 1 : 0;
>  	ch->bps <<= (ch->fmt & AFMT_16BIT)? 1 : 0;
>  	ch->bps <<= (ch->fmt & AFMT_32BIT)? 2 : 0;
> +   	CHN_UNLOCK(channel);
>  	chn_notify(parent, CHN_N_FORMAT);
> +   	CHN_LOCK(channel);
>  	return 0;
>  }
>  
> @@ -160,9 +167,12 @@
>  {
>  	struct vchinfo *ch = data;
>  	struct pcm_channel *parent = ch->parent;
> +	struct pcm_channel *channel = ch->channel;
>  
>  	ch->spd = speed;
> +   	CHN_UNLOCK(channel);
>  	chn_notify(parent, CHN_N_RATE);
> +   	CHN_LOCK(channel);
>  	return speed;
>  }
>  
> @@ -171,14 +181,19 @@
>  {
>  	struct vchinfo *ch = data;
>  	struct pcm_channel *parent = ch->parent;
> +	struct pcm_channel *channel = ch->channel;
>  	int prate, crate;
>  
>  	ch->blksz = blocksize;
> +   	CHN_UNLOCK(channel);
>  	chn_notify(parent, CHN_N_BLOCKSIZE);
> +   	CHN_LOCK(parent);
> +   	CHN_LOCK(channel);
>  
>  	crate = ch->spd * ch->bps;
>  	prate = sndbuf_getspd(parent->bufhard) * sndbuf_getbps(parent->bufhard);
>  	blocksize = sndbuf_getblksz(parent->bufhard);
> +   	CHN_UNLOCK(parent);
>  	blocksize *= prate;
>  	blocksize /= crate;
>  
> @@ -190,12 +205,15 @@
>  {
>  	struct vchinfo *ch = data;
>  	struct pcm_channel *parent = ch->parent;
> +	struct pcm_channel *channel = ch->channel;
>  
>  	if (go == PCMTRIG_EMLDMAWR || go == PCMTRIG_EMLDMARD)
>  		return 0;
>  
>  	ch->run = (go == PCMTRIG_START)? 1 : 0;
> +   	CHN_UNLOCK(channel);
>  	chn_notify(parent, CHN_N_TRIGGER);
> +   	CHN_LOCK(channel);
>  
>  	return 0;
>  }
> @@ -235,8 +253,11 @@
>  	struct pcm_channel *child;
>  	int err, first;
>  
> +   	CHN_UNLOCK(parent);
> +
>  	pce = malloc(sizeof(*pce), M_DEVBUF, M_WAITOK | M_ZERO);
>  	if (!pce) {
> +   		CHN_LOCK(parent);
>  		return ENOMEM;
>  	}
>  
> @@ -244,14 +265,13 @@
>  	child = pcm_chn_create(d, parent, &vchan_class, PCMDIR_VIRTUAL, parent);
>  	if (!child) {
>  		free(pce, M_DEVBUF);
> +   		CHN_LOCK(parent);
>  		return ENODEV;
>  	}
>  
>     	CHN_LOCK(parent);
> -	if (!(parent->flags & CHN_F_BUSY)) {
> -		CHN_UNLOCK(parent);
> +	if (!(parent->flags & CHN_F_BUSY))
>  		return EBUSY;
> -	}
>  
>  	first = SLIST_EMPTY(&parent->children);
>  	/* add us to our parent channel's children */
> @@ -269,6 +289,7 @@
>  		free(pce, M_DEVBUF);
>  	}
>  
> +   	CHN_LOCK(parent);
>  	/* XXX gross ugly hack, murder death kill */
>  	if (first && !err) {
>  		err = chn_reset(parent, AFMT_STEREO | AFMT_S16_LE);
> 
> _______________________________________________
> freebsd-current@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"

-- 
	If you optimize everything, you will always be unhappy.
			- Don Knuth



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040125173244.GA95238>