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 _at__at_ -215,6 +215,9 _at__at_ 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++; _at__at_ -746,13 +749,6 _at__at_ 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) _at__at_ -760,14 +756,27 _at__at_ 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; _at__at_ -789,6 +798,7 _at__at_ out: + CHN_UNLOCK(c); if (ret) { if (c->devinfo) { if (CHANNEL_FREE(c->methods, c->devinfo)) _at__at_ -1009,12 +1019,8 _at__at_ 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); _at__at_ -1032,11 +1038,23 _at__at_ 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 _at__at_ -103,7 +103,7 _at__at_ #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 _at__at_ -487,11 +487,13 _at__at_ * 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 */ _at__at_ -518,10 +520,16 _at__at_ { 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; _at__at_ -548,10 +556,24 _at__at_ { 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; _at__at_ -592,11 +614,15 _at__at_ 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; } _at__at_ -613,7 +639,13 _at__at_ 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 */ _at__at_ -622,15 +654,21 _at__at_ 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; _at__at_ -640,11 +678,15 _at__at_ #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 ; _at__at_ -664,10 +706,16 _at__at_ 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: _at__at_ -700,7 +748,15 _at__at_ 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: _at__at_ -747,11 +803,27 _at__at_ 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 */ _at__at_ -824,9 +896,10 _at__at_ { 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); _at__at_ -841,9 +914,10 _at__at_ { 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); _at__at_ -897,7 +971,15 _at__at_ 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: _at__at_ -923,18 +1005,28 _at__at_ 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 _at__at_ -96,7 +96,7 _at__at_ 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; _at__at_ -334,7 +334,7 _at__at_ 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 _at__at_ -77,7 +77,9 _at__at_ 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); _at__at_ -151,7 +153,9 _at__at_ 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; } _at__at_ -162,7 +166,9 _at__at_ struct pcm_channel *parent = ch->parent; ch->spd = speed; + CHN_LOCK(parent); chn_notify(parent, CHN_N_RATE); + CHN_UNLOCK(parent); return speed; } _at__at_ -174,11 +180,13 _at__at_ 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; _at__at_ -195,7 +203,9 _at__at_ return 0; ch->run = (go == PCMTRIG_START)? 1 : 0; + CHN_LOCK(parent); chn_notify(parent, CHN_N_TRIGGER); + CHN_UNLOCK(parent); return 0; } _at__at_ -268,12 +278,14 _at__at_ /* 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;Received on Fri Jan 16 2004 - 00:32:13 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:38 UTC