Date: Fri, 16 Jan 2004 01:31:31 -0800 (PST) From: Don Lewis <truckman@FreeBSD.org> To: shoesoft@gmx.net Cc: mat@cnd.mcgill.ca Subject: Re: sound/pcm/* bugs (was: Re: page fault panic tracked down (selwakeuppri()) - really sound/pcm/*) Message-ID: <200401160931.i0G9VV7E044831@gw.catspoiler.org> In-Reply-To: <1074123385.733.11.camel@shoeserv.freebsd>
next in thread | previous in thread | raw e-mail | index | archive | help
On 15 Jan, Stefan Ehmann wrote:
> On Wed, 2004-01-14 at 22:28, Don Lewis wrote:
>> Sigh ... I had hoped that your earlier testing had meant that locking
>> code was clean. Here's another patch, but I won't guarantee that you
>> won't run into more lock assertion problems. I guess that I should set
>> up some sound hardware here so that I can actually test some of this
>> stuff.
>
> This time the system survived sysctl but as soon as sound playback
> starts the assertion in channel.c:978 gets triggered.
>
> Looks like setting up sound would be a good idea since there are
> probably more traps ahead.
Judging by all the arrows I just dug out of my back, I'd say you are
correct. Really enabling the lock assertion checks turned up quite a
few things, and I found some more locking problems by inspection.
The following patch seems to be at least somewhat stable on my machine.
I did get one last panic in feed_vchan_s16() that I haven't been able to
reproduce since I added some more sanity checks. Since it was caught in
the interrupt routine, it's not easy to track down to its root cause. I
think there is still a bug somewhere, but it's not easy to trigger.
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 16 Jan 2004 07:16:46 -0000
@@ -215,6 +215,9 @@
sndbuf_acquire(bs, NULL, sndbuf_getfree(bs));
amt = sndbuf_getfree(b);
+ if (amt > sndbuf_getsize(b))
+ panic("chn_wrfeed(): bufhard free %d > size %d",
+ amt, sndbuf_getsize(b));
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,14 +756,27 @@
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;
c->flags = 0;
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 +798,7 @@
out:
+ CHN_UNLOCK(c);
if (ret) {
if (c->devinfo) {
if (CHANNEL_FREE(c->methods, c->devinfo))
@@ -1009,12 +1019,8 @@
bufsz = blkcnt * blksz;
- ret = sndbuf_remalloc(bs, blkcnt, blksz);
- if (ret)
- goto out;
-
/* 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);
@@ -1032,11 +1038,23 @@
sndbuf_setblksz(b, CHANNEL_SETBLOCKSIZE(c->methods, c->devinfo, blksz));
+ /*
+ * Force bufsoft blksz to match bufhard in case CHANNEL_SETBLOCKSIZE()
+ * forces something larger than requested.
+ */
+ ret = sndbuf_remalloc(bs, blkcnt, sndbuf_getblksz(b));
+ if (ret)
+ goto out;
+
irqhz = (sndbuf_getbps(b) * sndbuf_getspd(b)) / sndbuf_getblksz(b);
DEB(printf("got %d, irqhz == %d\n", sndbuf_getblksz(b), irqhz));
chn_resetbuf(c);
out:
+ if (sndbuf_getsize(bs) < sndbuf_getsize(b) ||
+ sndbuf_getsize(bs) < sndbuf_getfree(b))
+ panic("chn_setblocksize(): bufsoft %d smaller than bufhard size %d or free %d, ret = %d",
+ sndbuf_getsize(bs), sndbuf_getsize(bs), sndbuf_getfree(b), ret);
return ret;
}
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 16 Jan 2004 01:35:12 -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)
Index: sys/dev/sound/pcm/dsp.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/sound/pcm/dsp.c,v
retrieving revision 1.67
diff -u -r1.67 dsp.c
--- sys/dev/sound/pcm/dsp.c 11 Nov 2003 05:38:28 -0000 1.67
+++ sys/dev/sound/pcm/dsp.c 16 Jan 2004 05:52:50 -0000
@@ -487,11 +487,13 @@
* 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 */
@@ -518,10 +520,16 @@
{
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;
@@ -548,10 +556,24 @@
{
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;
+ if (wrch) {
+ CHN_LOCK(wrch);
+ 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_LOCK(rdch);
+ p->rec_rate = rdch->speed;
+ p->rec_format = rdch->format;
+ CHN_UNLOCK(rdch);
+ } else {
+ p->rec_rate = 0;
+ p->rec_format = 0;
+ }
}
break;
@@ -592,11 +614,15 @@
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;
}
@@ -613,7 +639,13 @@
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);
+ *arg_i = sndbuf_getready(rdch->bufsoft);
+ CHN_UNLOCK(rdch);
+ } else
+ *arg_i = 0;
break;
case FIOASYNC: /*set/clear async i/o */
@@ -622,15 +654,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;
@@ -640,11 +678,15 @@
#define THE_REAL_SNDCTL_DSP_GETBLKSIZE _IOWR('P', 4, int)
case THE_REAL_SNDCTL_DSP_GETBLKSIZE:
case SNDCTL_DSP_GETBLKSIZE:
- if (wrch)
+ if (wrch) {
+ CHN_LOCK(wrch);
*arg_i = sndbuf_getblksz(wrch->bufsoft);
- else if (rdch)
+ CHN_UNLOCK(wrch);
+ } else if (rdch) {
+ CHN_LOCK(rdch);
*arg_i = sndbuf_getblksz(rdch->bufsoft);
- else
+ CHN_UNLOCK(rdch);
+ } else
*arg_i = 0;
break ;
@@ -664,10 +706,16 @@
case SNDCTL_DSP_RESET:
DEB(printf("dsp reset\n"));
- if (wrch)
+ if (wrch) {
+ CHN_LOCK(wrch);
chn_abort(wrch);
- if (rdch)
+ CHN_UNLOCK(wrch);
+ }
+ if (rdch) {
+ CHN_LOCK(rdch);
chn_abort(rdch);
+ CHN_UNLOCK(rdch);
+ }
break;
case SNDCTL_DSP_SYNC:
@@ -700,7 +748,15 @@
break;
case SOUND_PCM_READ_RATE:
- *arg_i = wrch? wrch->speed : rdch->speed;
+ if (wrch) {
+ CHN_LOCK(wrch);
+ *arg_i = wrch->speed;
+ CHN_UNLOCK(wrch);
+ } else {
+ CHN_LOCK(rdch);
+ *arg_i = rdch->speed;
+ CHN_UNLOCK(rdch);
+ }
break;
case SNDCTL_DSP_STEREO:
@@ -747,11 +803,27 @@
break;
case SOUND_PCM_READ_CHANNELS:
- *arg_i = ((wrch? wrch->format : rdch->format) & AFMT_STEREO)? 2 : 1;
+ if (wrch) {
+ CHN_LOCK(wrch);
+ *arg_i = (wrch->format & AFMT_STEREO)? 2 : 1;
+ CHN_UNLOCK(wrch);
+ } else {
+ CHN_LOCK(rdch);
+ *arg_i = (rdch->format & AFMT_STEREO)? 2 : 1;
+ CHN_UNLOCK(rdch);
+ }
break;
case SNDCTL_DSP_GETFMTS: /* returns a mask of supported fmts */
- *arg_i = wrch? chn_getformats(wrch) : chn_getformats(rdch);
+ if (wrch) {
+ CHN_LOCK(wrch);
+ *arg_i = chn_getformats(wrch);
+ CHN_UNLOCK(wrch);
+ } else {
+ CHN_LOCK(rdch);
+ *arg_i = chn_getformats(rdch);
+ CHN_UNLOCK(rdch);
+ }
break ;
case SNDCTL_DSP_SETFMT: /* sets _one_ format */
@@ -824,9 +896,10 @@
{
audio_buf_info *a = (audio_buf_info *)arg;
if (rdch) {
- struct snd_dbuf *bs = rdch->bufsoft;
+ struct snd_dbuf *bs;
CHN_LOCK(rdch);
+ bs = rdch->bufsoft;
a->bytes = sndbuf_getready(bs);
a->fragments = a->bytes / sndbuf_getblksz(bs);
a->fragstotal = sndbuf_getblkcnt(bs);
@@ -841,9 +914,10 @@
{
audio_buf_info *a = (audio_buf_info *)arg;
if (wrch) {
- struct snd_dbuf *bs = wrch->bufsoft;
+ struct snd_dbuf *bs;
CHN_LOCK(wrch);
+ bs = wrch->bufsoft;
chn_wrupdate(wrch);
a->bytes = sndbuf_getfree(bs);
a->fragments = a->bytes / sndbuf_getblksz(bs);
@@ -897,7 +971,15 @@
break;
case SOUND_PCM_READ_BITS:
- *arg_i = ((wrch? wrch->format : rdch->format) & AFMT_16BIT)? 16 : 8;
+ if (wrch) {
+ CHN_LOCK(wrch);
+ *arg_i = (wrch->format & AFMT_16BIT)? 16 : 8;
+ CHN_UNLOCK(wrch);
+ } else {
+ CHN_LOCK(rdch);
+ *arg_i = (rdch->format & AFMT_16BIT)? 16 : 8;
+ CHN_UNLOCK(rdch);
+ }
break;
case SNDCTL_DSP_SETTRIGGER:
@@ -923,18 +1005,28 @@
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:
if (wrch) {
- struct snd_dbuf *b = wrch->bufhard;
- struct snd_dbuf *bs = wrch->bufsoft;
+ struct snd_dbuf *b;
+ struct snd_dbuf *bs;
CHN_LOCK(wrch);
+ b = wrch->bufhard;
+ bs = wrch->bufsoft;
chn_wrupdate(wrch);
*arg_i = sndbuf_getready(b) + sndbuf_getready(bs);
CHN_UNLOCK(wrch);
Index: sys/dev/sound/pcm/sound.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/sound/pcm/sound.c,v
retrieving revision 1.86
diff -u -r1.86 sound.c
--- sys/dev/sound/pcm/sound.c 8 Dec 2003 01:08:03 -0000 1.86
+++ sys/dev/sound/pcm/sound.c 16 Jan 2004 06:40:28 -0000
@@ -96,7 +96,7 @@
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 | MTX_RECURSE | MTX_DUPOK);
return m;
#else
return (void *)0xcafebabe;
@@ -334,7 +334,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++) {
Index: sys/dev/sound/pcm/vchan.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/sound/pcm/vchan.c,v
retrieving revision 1.13
diff -u -r1.13 vchan.c
--- sys/dev/sound/pcm/vchan.c 7 Sep 2003 16:28:03 -0000 1.13
+++ sys/dev/sound/pcm/vchan.c 16 Jan 2004 06:43:54 -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(): tmp buffer size %d < count %d",
+ sndbuf_getsize(src), count);
count &= ~1;
bzero(b, count);
@@ -151,7 +153,9 @@
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_LOCK(parent);
chn_notify(parent, CHN_N_FORMAT);
+ CHN_UNLOCK(parent);
return 0;
}
@@ -162,7 +166,9 @@
struct pcm_channel *parent = ch->parent;
ch->spd = speed;
+ CHN_LOCK(parent);
chn_notify(parent, CHN_N_RATE);
+ CHN_UNLOCK(parent);
return speed;
}
@@ -174,11 +180,13 @@
int prate, crate;
ch->blksz = blocksize;
+ CHN_LOCK(parent);
chn_notify(parent, CHN_N_BLOCKSIZE);
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;
@@ -195,7 +203,9 @@
return 0;
ch->run = (go == PCMTRIG_START)? 1 : 0;
+ CHN_LOCK(parent);
chn_notify(parent, CHN_N_TRIGGER);
+ CHN_UNLOCK(parent);
return 0;
}
@@ -268,12 +278,14 @@
/* XXX gross ugly hack, murder death kill */
if (first && !err) {
+ CHN_LOCK(parent);
err = chn_reset(parent, AFMT_STEREO | AFMT_S16_LE);
if (err)
printf("chn_reset: %d\n", err);
err = chn_setspeed(parent, 44100);
if (err)
printf("chn_setspeed: %d\n", err);
+ CHN_UNLOCK(parent);
}
return err;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200401160931.i0G9VV7E044831>