On 6 Jan, To: shoesoft_at_gmx.net wrote: > On 6 Jan, Stefan Ehmann wrote: >> Finally found out in the handbook why debugging of the modules didn't >> work and now got it right it seems. >> >> #0 doadump () at /usr/src/sys/kern/kern_shutdown.c:240 >> #1 0xc04e5178 in boot (howto=256) at >> /usr/src/sys/kern/kern_shutdown.c:372 >> #2 0xc04e5507 in panic () at /usr/src/sys/kern/kern_shutdown.c:550 >> #3 0xc07eb648 in feed_vchan_s16 (f=0xc3967c00, c=0x0, b=0xc37d0000 "", >> count=2048, source=0xc3741500) at >> /usr/src/sys/dev/sound/pcm/vchan.c:80 >> #4 0xc07e1c6d in sndbuf_feed (from=0xc3741500, to=0xc3741600, >> channel=0xc37a8880, feeder=0xc3967c00, count=3279164672) at >> feeder_if.h:60 >> #5 0xc07e2225 in chn_wrfeed (c=0xc37a8880) >> at /usr/src/sys/dev/sound/pcm/channel.c:221 >> #6 0xc07e227c in chn_wrintr (c=0x800) >> at /usr/src/sys/dev/sound/pcm/channel.c:237 >> #7 0xc07e2990 in chn_intr (c=0x800) >> at /usr/src/sys/dev/sound/pcm/channel.c:483 >> #8 0xc07fba2f in csa_intr (p=0xc37a8700) >> at /usr/src/sys/dev/sound/pci/csapcm.c:623 >> #9 0xc07fa724 in csa_intr (arg=0xc37a8600) >> at /usr/src/sys/dev/sound/pci/csa.c:532 >> #10 0xc04d1612 in ithread_loop (arg=0xc1737b00) >> at /usr/src/sys/kern/kern_intr.c:544 >> #11 0xc04d0604 in fork_exit (callout=0xc04d1480 <ithread_loop>, arg=0x0, >> frame=0x0) at /usr/src/sys/kern/kern_fork.c:796 > > This code is really, really hard to follow, but it looks like there are > some assumptions about buffer sizes that could be violated and cause bad > things to happen. > > In chn_wrfeed(), we have the following bits of code: > > struct snd_dbuf *b = c->bufhard; > struct snd_dbuf *bs = c->bufsoft; > unsigned int ret, amt; > > amt = sndbuf_getfree(b); > sndbuf_feed(bs, b, c, c->feeder, amt) > > sndbuf_feed() looks like: > > int > sndbuf_feed(struct snd_dbuf *from, struct snd_dbuf *to, struct pcm_channel *chan > nel, struct pcm_feeder *feeder, unsigned int count) > { > if (sndbuf_getfree(to) < count) > return EINVAL; > > count = FEEDER_FEED(feeder, channel, to->tmpbuf, count, from); > > [ snip ] > > In this case, FEEDER_FEED() invokes feed_vchan_s16(), which looks like: > > static int > feed_vchan_s16(struct pcm_feeder *f, struct pcm_channel *c, u_int8_t *b, u_int32 > _t count, void *source) > { > /* we're going to abuse things a bit */ > struct snd_dbuf *src = source; > [ snip ] > KASSERT(sndbuf_getsize(src) >= count, ("bad bufsize")); > count &= ~1; > bzero(b, count); > [ snip ] > dst = (int16_t *)b; > tmp = (int16_t *)sndbuf_getbuf(src); > bzero(tmp, count); > > > We start off by calculation the amount to copy as the amount of free > space in bufhard. In sndbuf_feed() "to" points to bufhard and we > validate the amount of data to copy is will fit into the available free > space. In feed_vchan_16, the pointer to the destination buffer is in > "b", and source/src points to bufsoft. We get a pointer to the actual > buffer for bufsoft and then bzero the desired number of bytes. But what > if the bufsoft buffer is smaller than the bufhard buffer? It doesn't > look like the buffer sizes are forced to match. When chn_setblocksize() > is called, it calls sndbuf_remalloc on the channel's bufsoft, which will > free and rellocate the buffer, but it does not do the same for bufhard. > If bufsoft shrinks, feed_vchan_s16() is likely stomp the wrong thing on > the heap. > > When the sound channel is closed, dsp_close() calls chn_reset(), which > calls chn_setblocksize(c, 0, 0) ... However it happens, in feed_vchan_s16() count is 2048, while src->bufsize and src->maxsize are both 688, src->blksz is 344, and src->blkcnt is 2. > It looks like there will be one final interrupt after dsp_close(). > dsp_close() calls chn_flush(), which flushes the remaining buffered > data. Any interrupts which happen after the final call to chn_sleep() > won't be serviced until after the channel mutex is unlocked and the > dsp_close() code has had a chance to put things in a bad state. > chn_flush() should probably wait for the final interrupt, possibly using > a flag set by the interrupt handler to signal that the handler has seen > the buffer drain (and the handler should not call chn_dmaupdate(), I > think). After thinking about this some more, I think the easiest fix would be to skip the calls to chn_wrintr() and chn_rdintr() in chn_intr() if the CHN_F_RUNNING flag is not set. This will avoid running the channel interrupt code when the channel is more likely to be in a bad state. > The dsp_read() and dsp_write() routines should lock the channels to > protect the flag manipulations. This turns out to be OK since the channels are locked, I just didn't see it at first. I found another bug, though. Holding a mutex across a malloc() call is not allowed because malloc() can sleep, and sleeping while holding a mutex is not allowed. sndbuf_resize() sndbuf_remalloc() both call malloc() to allocate buffers, and sndbuf_alloc() and sndbuf_setup() call sndbuf_resize(). The problem is that chn_setblocksize() calls sndbuf_remalloc() while the channel mutex is held. We can't just unlock the mutex around the sndbuf_remalloc() call because the channel interrupt service routine could run while the buffer is being re-allocated. It would be best if the channel were to be shut down when re-allocating the buffer. This is likely to be messy since chn_setblocksize() is called from lots of different places.Received on Tue Jan 06 2004 - 20:01:48 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:36 UTC