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

On 14 Jan, Stefan Ehmann wrote:
> On Wed, 2004-01-14 at 13:45, Don Lewis wrote:
>> On 14 Jan, Stefan Ehmann wrote:
>> > No luck - again...
>> > 
>> > panic: mutex pcm0:fake not owned at
>> > /usr/src/sys/dev/sound/pcm/channel.c:834
>> > 
>> > at boottime
>> 
>> I suspect something new got built with INVARIANTS and a working
>> CHN_LOCKASSERT() for the first time. Try adding a call to CHN_LOCK()
>> after the call to chn_lockinit() in chn_init() and a call to
>> CHN_UNLOCK() just after the out: label.  These got deleted in rev 1.85,
>> though the CHN_UNLOCK() call was in the wrong place in 1.84.
> 
> That seems to fix it - boots fine now.
> 
> Looks like we need some further locks. When I try to set vchans to 4
> with sysctl I get a similar panic.
> 
> panic: mutex pcm0:play:0 not owned at
> /usr/src/sys/dev/sound/pcm/channel.c:706
> 
> #15 0xc04e5b57 in panic () at /usr/src/sys/kern/kern_shutdown.c:550
> #16 0xc04dc4fc in _mtx_assert (m=0xc37ba280, what=0, 
>     file=0xc07ee3e9 "/usr/src/sys/dev/sound/pcm/channel.c", line=706)
>     at /usr/src/sys/kern/kern_mutex.c:673
> #17 0xc07e436f in chn_reset (c=0x2c2, fmt=268435472)
>     at /usr/src/sys/dev/sound/pcm/channel.c:706
> #18 0xc07ed1fa in vchan_create (parent=0xc37a8880)
>     at /usr/src/sys/dev/sound/pcm/vchan.c:273

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.

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	14 Jan 2004 12:00:47 -0000
_at__at_ -740,6 +740,7 _at__at_
 	int ret;
 
 	chn_lockinit(c, dir);
+	CHN_LOCK(c);
 
 	b = NULL;
 	bs = NULL;
_at__at_ -789,6 +790,7 _at__at_
 
 
 out:
+	CHN_UNLOCK(c);
 	if (ret) {
 		if (c->devinfo) {
 			if (CHANNEL_FREE(c->methods, c->devinfo))
_at__at_ -1009,12 +1011,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_ -1031,6 +1029,14 _at__at_
 	DEB(printf("%s: hard blksz requested %d (maxsize %d), ", __func__, blksz, sndbuf_getmaxsize(b)));
 
 	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));
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/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	14 Jan 2004 07:14:05 -0000
_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	14 Jan 2004 21:23:44 -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);
 
_at__at_ -268,12 +270,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;