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>