Date: Mon, 23 Feb 2004 21:43:50 -0800 (PST) From: Don Lewis <truckman@FreeBSD.org> To: b_oshea@yahoo.com Cc: mat@cnd.mcgill.ca Subject: Re: 5.2.1-RC hangs occasionally when sound files are played using the pcm driver Message-ID: <200402240543.i1O5ho7E072966@gw.catspoiler.org> In-Reply-To: <20040223182101.29562.qmail@web10506.mail.yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23 Feb, Brian O'Shea wrote:
> --- Don Lewis <truckman@FreeBSD.org> 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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200402240543.i1O5ho7E072966>