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

From: Don Lewis <truckman_at_FreeBSD.org>
Date: Fri, 16 Jan 2004 01:31:31 -0800 (PST)
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