Re: sound/pcm/* bugs (was: Re: page fault panic tracked down (selwakeuppri()) - really sound/pcm/*)

On  7 Jan, Stefan Ehmann wrote:
> On Wed, 2004-01-07 at 21:15, Don Lewis wrote:
>> Try the following patch.  I won't guarantee that you still won't see
>> panics, but at least it should panic cleanly instead of stomping on some
>> innocent block of memory that corrupts data or some critical structure
>> that will trigger a mysterious and hard to debug panic at some later
>> time.
>> 
>> The reason for changing the KASSERT to a test and an explicit call to
>> panic() is to make sure that the error is always caught because the
>> sound code is typically not compiled with INVARIANTS, so the KASSERT
>> will typically be ignored.  The vchan stuff really needs a proper fix,
>> but I don't understand the sound code well enough.
> 
> Nice patch - unfortunately sound doesn't work any longer with vchans
> enabled (which is set to 4 at boot time here).
> 
> I get "pcm0:virtual:0: play interrupt timeout, channel dead" whenever I
> try to play anything.
> 
> However, if I do "hw.snd.pcm0.vchans: 4 -> 0" it works nice again.
> 
> I'll probably try later if I'm able to get a panic with vchans disabled
> (Especially those that had - at least not in an obvious way - any sound
> stuff in backtrace)
>  
> If not, maybe the panic gets triggered if for some reason the dsp device
> isn't ready again after a song played and a new virtual dsp device is
> opened and vchan code gets in action (combined with other bad things).
> 
> It can't be vchans alone because I just had three mpg123s open and they
> just played fine.

Some other wierdness that I noticed is that if one of the
chn_setblocksize() is called for one of the vchans, vchan_setblocksize()
will get called, which will call chn_notify(parent, CHN_N_BLOCKSIZE).
When this happens, the parent will interate over all of its children
looking for the one with the minimum bufhard blksz.  It will then call
chn_setblocksize() on itself, and chn_setblocksize() will call
sndbuf_remalloc() on its bufsoft, which will set reallocate the buffer
with the size (blkcnt * blksz).  If this channel is the parent vchan and
the new size of bufsoft is smaller than the size of bufhard (which never
gets reallocated), feed_vchan_s16() will write past the end of bufsoft
and things will go boom sometime later.

Try the patch below in place of my previous patch.  As you might guess,
I'm grasping at straws.


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	8 Jan 2004 09:57:26 -0000
_at__at_ -1009,7 +1009,16 _at__at_
 
 	bufsz = blkcnt * blksz;
 
-	ret = sndbuf_remalloc(bs, blkcnt, blksz);
+	/*
+	 * XXX - Don't allow bufsoft to shrink, feed_vchan_s16() expects
+	 *       bufhard and bufsoft to be the same size, at least when
+	 *       called from chn_wrfeed().  Just set blkcnt and blksz
+	 *       instead.  Nothing seems to want to make the buffers
+	 *       larger.
+	 */
+	/* ret = sndbuf_remalloc(bs, blkcnt, blksz); */
+	sndbuf_setblkcnt(bs, blkcnt);
+	sndbuf_setblksz(bs, blksz);
 	if (ret)
 		goto out;
 
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	7 Jan 2004 20:01:38 -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/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	7 Jan 2004 19:58:57 -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("tmp buffer size %d < count %d", sndbuf_getsize(src),
+		    count);
 	count &= ~1;
 	bzero(b, count);