Date: Sun, 22 Feb 2004 15:47:35 -0800 (PST) From: Don Lewis <truckman@FreeBSD.org> To: haro@kgt.co.jp Cc: mat@cnd.mcgill.ca Subject: Re: LOR with sound Message-ID: <200402222347.i1MNlZ7E068836@gw.catspoiler.org> In-Reply-To: <20040216.101505.74756126.haro@kgt.co.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
--0-1804289383-1077493664=:46511 Content-Type: TEXT/plain; charset=us-ascii On 16 Feb, Munehiro Matsuda wrote: > Hi all, > > I've gotten following LoR with sounde code, using kernel from Feb 15th > with patch from Mathew Kanner: > http://docs.freebsd.org/cgi/mid.cgi?20031215185606.GA63202 > >> lock order reversal >> 1st 0xc31ca140 pcm0:play:0 (pcm play channel) @ dev/sound/pcm/channel.c:1069 >> 2nd 0xc31b6ec0 pcm0:record:0 (pcm record channel) @ dev/sound/pcm/channel.c:1350 >> Stack backtrace: >> backtrace(0,ffffffff,c0761120,c0761148,c07303fc) at backtrace+0x12 >> witness_checkorder(c31b6ec0,9,c06cfeaf,546) at witness_checkorder+0x593 >> _mtx_lock_flags(c31b6ec0,0,c06cfea6,546,d29f2a70) at _mtx_lock_flags+0x67 >> chn_lock(c32add80,c,c407b000,c405b000,102) at chn_lock+0x1a >> sndbuf_remalloc(c31bab00,200,100,c32add80,100) at sndbuf_remalloc+0x8c >> chn_setblocksize(c32add80,200,0,20000,c31cf718) at chn_setblocksize+0x306 >> chn_tryspeed(c32add80,1f40) at chn_tryspeed+0x149 >> chn_tryformat(c32add80,10,10,d29f2c60,d29f2c60) at chn_tryformat+0xaf >> chn_setformat(c32add80,10,0,c1616a00,c32add80) at chn_setformat+0x15 >> dsp_ioctl(c075a6a0,c0045005,d29f2c60,3,c3733540) at dsp_ioctl+0x9f2 >> spec_ioctl(d29f2b88,d29f2c34,c0583df7,d29f2b88,c074fc40) at spec_ioctl+0x12d >> spec_vnoperate(d29f2b88) at spec_vnoperate+0x13 >> vn_ioctl(c3778f24,c0045005,d29f2c60,c371f180,c3733540) at vn_ioctl+0x17f >> ioctl(c3733540,d29f2d14,3,5,296) at ioctl+0x37c >> syscall(c068002f,2f,2f,0,0) at syscall+0x217 >> Xint0x80_syscall() at Xint0x80_syscall+0x1d >> --- syscall (54), eip = 0x284e7b1b, esp = 0xbfbf5dfc, ebp = 0xbfbf5e18 --- > > > Also, I'm getting following warning with sound related code path. > >> malloc() of "4096" with the following non-sleepable locks held: >> exclusive sleep mutex pcm0:play:0 (pcm play channel) r = 0 (0xc31ca140) locked @ dev/sound/pcm/channel.c:1069 >> malloc() of "4096" with the following non-sleepable locks held: >> exclusive sleep mutex pcm0 (sound cdev) r = 0 (0xc31ca2c0) locked @ dev/sound/pcm/dsp.c:213 >> malloc() of "4096" with the following non-sleepable locks held: >> exclusive sleep mutex pcm0:record:0 (pcm record channel) r = 0 (0xc31b6ec0) locked @ dev/sound/pcm/dsp.c:479 > > FYI, I use following sound card build into my note book. > > pcm0: <Yamaha DS-1E (YMF744)> port 0xfcac-0xfcaf,0xfc00-0xfc3f mem 0xfecf0000-0xfecf7fff irq 9 at device 9.0 on pci0 > pcm0: <Asahi Kasei AK4543 AC97 Codec> Here's a different patch to dsp_ioctl() that optimizes out some unneeded channel locking and unlocking, at the expense of bloating the source. It needs some review and testing. It should get rid of the complaints about the channel mutex lock order reversals and malloc() calls while the channel mutexes are held. The malloc() call while the "sound cdev" mutex held problem is not addresses by this patch. --0-1804289383-1077493664=:46511 Content-Type: TEXT/plain; name="dsp_ioctl.diff2" Content-Disposition: attachment; filename="dsp_ioctl.diff2" 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; --0-1804289383-1077493664=:46511--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200402222347.i1MNlZ7E068836>