Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 23 Feb 2004 21:43:50 -0800 (PST)
From:      Don Lewis <truckman@FreeBSD.org>
To:        b_oshea@yahoo.com
Cc:        mat@cnd.mcgill.ca
Subject:   Re: 5.2.1-RC hangs occasionally when sound files are played using the pcm driver
Message-ID:  <200402240543.i1O5ho7E072966@gw.catspoiler.org>
In-Reply-To: <20040223182101.29562.qmail@web10506.mail.yahoo.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On 23 Feb, Brian O'Shea wrote:
> --- Don Lewis <truckman@FreeBSD.org> wrote:
>> 
>> The cause of deadlocks is more likely to be caught by WITNESS.  In this
> 
> With WITNESS the hang still occurrs, and still no panic.  It's hard to
> tell since the problem tends to happen at random times, but it seems like
> it happens more quickly with the kernel that has WITNESS and INVARIANTS
> enabled.

Can you see any kernel diagnostic messages, or are you running X?  If
you are running X, can you set up a serial console, or can you reproduce
the hang while switched to a text console?

Depending on the cause of the problem, you might need to build a kernel
with DDB support and break into DDB from the console while the machine
is hung to figure out the cause.

>> case it might be the result of a malloc() call while a mutex is held.
>> Even the version of the sound code in the most recent -CURRENT has some
>> problems in this area.  I've got a patch out for testing that will this
>> problem to some extent, but it might not be enough.
>> 
> 
> I'd be willing to test your patch, if that would help.  Where can I
> find it?

The patch below should be applied to a recent version of -CURRENT.

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;



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200402240543.i1O5ho7E072966>