From owner-freebsd-hackers@FreeBSD.ORG Mon Feb 23 21:44:09 2004 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id EEA0916A4CE for ; Mon, 23 Feb 2004 21:44:09 -0800 (PST) Received: from gw.catspoiler.org (217-ip-163.nccn.net [209.79.217.163]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1CEA443D1D for ; Mon, 23 Feb 2004 21:44:09 -0800 (PST) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.12.9p2/8.12.9) with ESMTP id i1O5ho7E072966; Mon, 23 Feb 2004 21:43:54 -0800 (PST) (envelope-from truckman@FreeBSD.org) Message-Id: <200402240543.i1O5ho7E072966@gw.catspoiler.org> Date: Mon, 23 Feb 2004 21:43:50 -0800 (PST) From: Don Lewis To: b_oshea@yahoo.com In-Reply-To: <20040223182101.29562.qmail@web10506.mail.yahoo.com> MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii cc: freebsd-hackers@FreeBSD.org cc: mat@cnd.mcgill.ca Subject: Re: 5.2.1-RC hangs occasionally when sound files are played using the pcm driver X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Feb 2004 05:44:10 -0000 On 23 Feb, Brian O'Shea wrote: > --- Don Lewis wrote: >> >> The cause of deadlocks is more likely to be caught by WITNESS. In this > > With WITNESS the hang still occurrs, and still no panic. It's hard to > tell since the problem tends to happen at random times, but it seems like > it happens more quickly with the kernel that has WITNESS and INVARIANTS > enabled. Can you see any kernel diagnostic messages, or are you running X? If you are running X, can you set up a serial console, or can you reproduce the hang while switched to a text console? Depending on the cause of the problem, you might need to build a kernel with DDB support and break into DDB from the console while the machine is hung to figure out the cause. >> case it might be the result of a malloc() call while a mutex is held. >> Even the version of the sound code in the most recent -CURRENT has some >> problems in this area. I've got a patch out for testing that will this >> problem to some extent, but it might not be enough. >> > > I'd be willing to test your patch, if that would help. Where can I > find it? The patch below should be applied to a recent version of -CURRENT. Index: sys/dev/sound/pcm/dsp.c =================================================================== RCS file: /home/ncvs/src/sys/dev/sound/pcm/dsp.c,v retrieving revision 1.73 diff -u -r1.73 dsp.c --- sys/dev/sound/pcm/dsp.c 21 Feb 2004 21:10:47 -0000 1.73 +++ sys/dev/sound/pcm/dsp.c 22 Feb 2004 23:11:43 -0000 @@ -444,7 +444,7 @@ static int dsp_ioctl(dev_t i_dev, u_long cmd, caddr_t arg, int mode, struct thread *td) { - struct pcm_channel *wrch, *rdch; + struct pcm_channel *chn, *rdch, *wrch; struct snddev_info *d; intrmask_t s; int kill; @@ -477,22 +477,19 @@ if (kill & 2) rdch = NULL; - if (rdch != NULL) - CHN_LOCK(rdch); - if (wrch != NULL) - CHN_LOCK(wrch); - switch(cmd) { #ifdef OLDPCM_IOCTL /* * we start with the new ioctl interface. */ case AIONWRITE: /* how many bytes can write ? */ + CHN_LOCK(wrch); /* if (wrch && wrch->bufhard.dl) while (chn_wrfeed(wrch) == 0); */ *arg_i = wrch? sndbuf_getfree(wrch->bufsoft) : 0; + CHN_UNLOCK(wrch); break; case AIOSSIZE: /* set the current blocksize */ @@ -502,12 +499,16 @@ 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; @@ -515,37 +516,51 @@ { struct snd_size *p = (struct snd_size *)arg; - if (wrch) + if (wrch) { + CHN_LOCK(wrch); p->play_size = sndbuf_getblksz(wrch->bufsoft); - if (rdch) + CHN_UNLOCK(wrch); + } + if (rdch) { + CHN_LOCK(rdch); p->rec_size = sndbuf_getblksz(rdch->bufsoft); + CHN_UNLOCK(rdch); + } } break; case AIOSFMT: + case AIOGFMT: { snd_chan_param *p = (snd_chan_param *)arg; if (wrch) { - chn_setformat(wrch, p->play_format); - chn_setspeed(wrch, p->play_rate); + CHN_LOCK(wrch); + if (cmd == AIOSFMT) { + chn_setformat(wrch, p->play_format); + chn_setspeed(wrch, p->play_rate); + } + p->play_rate = wrch->speed; + p->play_format = wrch->format; + CHN_UNLOCK(wrch); + } else { + p->play_rate = 0; + p->play_format = 0; } if (rdch) { - chn_setformat(rdch, p->rec_format); - chn_setspeed(rdch, p->rec_rate); + CHN_LOCK(rdch); + if (cmd == AIOSFMT) { + chn_setformat(rdch, p->rec_format); + chn_setspeed(rdch, p->rec_rate); + } + p->rec_rate = rdch->speed; + p->rec_format = rdch->format; + CHN_UNLOCK(rdch); + } else { + p->rec_rate = 0; + p->rec_format = 0; } } - /* FALLTHROUGH */ - - case AIOGFMT: - { - snd_chan_param *p = (snd_chan_param *)arg; - - p->play_rate = wrch? wrch->speed : 0; - p->rec_rate = rdch? rdch->speed : 0; - p->play_format = wrch? wrch->format : 0; - p->rec_format = rdch? rdch->format : 0; - } break; case AIOGCAP: /* get capabilities */ @@ -554,10 +569,14 @@ struct pcmchan_caps *pcaps = NULL, *rcaps = NULL; dev_t pdev; - if (rdch) + if (rdch) { + CHN_LOCK(rdch); rcaps = chn_getcaps(rdch); - if (wrch) + } + if (wrch) { + CHN_LOCK(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, @@ -573,15 +592,23 @@ p->mixers = 1; /* default: one mixer */ p->inputs = pdev->si_drv1? mix_getdevs(pdev->si_drv1) : 0; p->left = p->right = 100; + if (rdch) + CHN_UNLOCK(rdch); + if (wrch) + CHN_UNLOCK(wrch); } break; case AIOSTOP: - if (*arg_i == AIOSYNC_PLAY && wrch) + if (*arg_i == AIOSYNC_PLAY && wrch) { + CHN_LOCK(wrch); *arg_i = chn_abort(wrch); - else if (*arg_i == AIOSYNC_CAPTURE && rdch) + CHN_UNLOCK(wrch); + } else if (*arg_i == AIOSYNC_CAPTURE && rdch) { + CHN_LOCK(rdch); *arg_i = chn_abort(rdch); - else { + CHN_UNLOCK(rdch); + } else { printf("AIOSTOP: bad channel 0x%x\n", *arg_i); *arg_i = 0; } @@ -596,9 +623,15 @@ * here follow the standard ioctls (filio.h etc.) */ case FIONREAD: /* get # bytes to read */ -/* if (rdch && rdch->bufhard.dl) - while (chn_rdfeed(rdch) == 0); -*/ *arg_i = rdch? sndbuf_getready(rdch->bufsoft) : 0; + if (rdch) { + CHN_LOCK(rdch); +/* if (rdch && rdch->bufhard.dl) + while (chn_rdfeed(rdch) == 0); +*/ + *arg_i = sndbuf_getready(rdch->bufsoft); + CHN_UNLOCK(rdch); + } else + *arg_i = 0; break; case FIOASYNC: /*set/clear async i/o */ @@ -607,15 +640,21 @@ case SNDCTL_DSP_NONBLOCK: case FIONBIO: /* set/clear non-blocking i/o */ - if (rdch) - rdch->flags &= ~CHN_F_NBIO; - if (wrch) - wrch->flags &= ~CHN_F_NBIO; - if (*arg_i) { - if (rdch) + if (rdch) { + CHN_LOCK(rdch); + if (*arg_i) rdch->flags |= CHN_F_NBIO; - if (wrch) + else + rdch->flags &= ~CHN_F_NBIO; + CHN_UNLOCK(rdch); + } + if (wrch) { + CHN_LOCK(wrch); + if (*arg_i) wrch->flags |= CHN_F_NBIO; + else + wrch->flags &= ~CHN_F_NBIO; + CHN_UNLOCK(wrch); } break; @@ -625,71 +664,93 @@ #define THE_REAL_SNDCTL_DSP_GETBLKSIZE _IOWR('P', 4, int) case THE_REAL_SNDCTL_DSP_GETBLKSIZE: case SNDCTL_DSP_GETBLKSIZE: - if (wrch) - *arg_i = sndbuf_getblksz(wrch->bufsoft); - else if (rdch) - *arg_i = sndbuf_getblksz(rdch->bufsoft); - else - *arg_i = 0; + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = sndbuf_getblksz(chn->bufsoft); + CHN_UNLOCK(chn); break ; case SNDCTL_DSP_SETBLKSIZE: RANGE(*arg_i, 16, 65536); - if (wrch) + if (wrch) { + CHN_LOCK(wrch); chn_setblocksize(wrch, 2, *arg_i); - if (rdch) + CHN_UNLOCK(wrch); + } + if (rdch) { + CHN_LOCK(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) + if (wrch) { + CHN_LOCK(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; case SOUND_PCM_READ_RATE: - *arg_i = wrch? wrch->speed : rdch->speed; + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = chn->speed; + CHN_UNLOCK(chn); break; case SNDCTL_DSP_STEREO: 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; @@ -700,47 +761,67 @@ 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 - *arg_i = ((wrch? wrch->format : rdch->format) & AFMT_STEREO)? 2 : 1; + } else { + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = (chn->format & AFMT_STEREO) ? 2 : 1; + CHN_UNLOCK(chn); + } break; case SOUND_PCM_READ_CHANNELS: - *arg_i = ((wrch? wrch->format : rdch->format) & AFMT_STEREO)? 2 : 1; + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = (chn->format & AFMT_STEREO) ? 2 : 1; + CHN_UNLOCK(chn); break; case SNDCTL_DSP_GETFMTS: /* returns a mask of supported fmts */ - *arg_i = wrch? chn_getformats(wrch) : chn_getformats(rdch); + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = chn_getformats(chn); + CHN_UNLOCK(chn); break ; case SNDCTL_DSP_SETFMT: /* sets _one_ format */ - /* XXX locking */ 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 - *arg_i = (wrch? wrch->format : rdch->format) & ~AFMT_STEREO; + } else { + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = chn->format & ~AFMT_STEREO; + CHN_UNLOCK(chn); + } break; case SNDCTL_DSP_SETFRAGMENT: - /* XXX locking */ DEB(printf("SNDCTL_DSP_SETFRAGMENT 0x%08x\n", *(int *)arg)); { u_int32_t fragln = (*arg_i) & 0x0000ffff; @@ -759,14 +840,18 @@ 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; @@ -785,10 +870,12 @@ 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; @@ -800,11 +887,13 @@ 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; @@ -815,11 +904,13 @@ 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; } @@ -831,11 +922,13 @@ 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; } @@ -848,32 +941,47 @@ break; case SOUND_PCM_READ_BITS: - *arg_i = ((wrch? wrch->format : rdch->format) & AFMT_16BIT)? 16 : 8; + chn = wrch ? wrch : rdch; + CHN_LOCK(chn); + *arg_i = (chn->format & AFMT_16BIT) ? 16 : 8; + CHN_UNLOCK(chn); break; 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; case SNDCTL_DSP_GETTRIGGER: *arg_i = 0; - if (wrch && wrch->flags & CHN_F_TRIGGERED) - *arg_i |= PCM_ENABLE_OUTPUT; - if (rdch && rdch->flags & CHN_F_TRIGGERED) - *arg_i |= PCM_ENABLE_INPUT; + if (wrch) { + CHN_LOCK(wrch); + if (wrch->flags & CHN_F_TRIGGERED) + *arg_i |= PCM_ENABLE_OUTPUT; + CHN_UNLOCK(wrch); + } + if (rdch) { + CHN_LOCK(rdch); + if (rdch->flags & CHN_F_TRIGGERED) + *arg_i |= PCM_ENABLE_INPUT; + CHN_UNLOCK(rdch); + } break; case SNDCTL_DSP_GETODELAY: @@ -881,16 +989,20 @@ 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; @@ -908,10 +1020,6 @@ 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;