Re: panic on 5.2 BETA: blockable sleep lock

On 27 Nov, Stefan Ehmann wrote:
> On Wed, 2003-11-26 at 08:33, Don Lewis wrote:
>> The problem is that selrecord() wants to lock a MTX_DEF mutex, which can
>> cause a context switch if the mutex is already locked by another thread.
>> This is contrary to what bktr_poll() wants to accomplish by calling
>> critical_enter().
> 
> Strange enough that does not seem to happen with a kernel built without
> INVARIANTS and WITNESS. Does this make any sense or is this just by
> chance?

You might try the patch below with WITNESS enabled.  I don't have the
hardware, so I can't test it.  It compiles for me, but for all I know it
could delete all your files if you run it.

Index: sys/dev/bktr/bktr_core.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/bktr/bktr_core.c,v
retrieving revision 1.131
diff -u -r1.131 bktr_core.c
--- sys/dev/bktr/bktr_core.c	9 Nov 2003 09:17:21 -0000	1.131
+++ sys/dev/bktr/bktr_core.c	27 Nov 2003 23:58:19 -0000
_at__at_ -526,6 +526,9 _at__at_
 	}
 #endif	/* FreeBSD or BSDi */
 
+#ifdef USE_VBIMUTEX
+	mtx_init(&bktr->vbimutex, "bktr vbi lock", NULL, MTX_DEF);
+#endif
 
 /* If this is a module, save the current contiguous memory */
 #if defined(BKTR_FREEBSD_MODULE)
_at__at_ -807,6 +810,7 _at__at_
 	 * both Odd and Even VBI data is captured. Therefore we do this
 	 * in the Even field interrupt handler.
 	 */
+	LOCK_VBI(bktr);
 	if (  (bktr->vbiflags & VBI_CAPTURE)
 	    &&(bktr->vbiflags & VBI_OPEN)
             &&(field==EVEN_F)) {
_at__at_ -826,6 +830,7 _at__at_
 
 
 	}
+	UNLOCK_VBI(bktr);
 
 	/*
 	 *  Register the completed field
_at__at_ -1066,8 +1071,13 _at__at_
 int
 vbi_open( bktr_ptr_t bktr )
 {
-	if (bktr->vbiflags & VBI_OPEN)		/* device is busy */
+
+	LOCK_VBI(bktr);
+
+	if (bktr->vbiflags & VBI_OPEN) {	/* device is busy */
+		UNLOCK_VBI(bktr);
 		return( EBUSY );
+	}
 
 	bktr->vbiflags |= VBI_OPEN;
 
_at__at_ -1081,6 +1091,8 _at__at_
 	bzero((caddr_t) bktr->vbibuffer, VBI_BUFFER_SIZE);
 	bzero((caddr_t) bktr->vbidata,  VBI_DATA_SIZE);
 
+	UNLOCK_VBI(bktr);
+
 	return( 0 );
 }
 
_at__at_ -1166,8 +1178,12 _at__at_
 vbi_close( bktr_ptr_t bktr )
 {
 
+	LOCK_VBI(bktr);
+
 	bktr->vbiflags &= ~VBI_OPEN;
 
+	UNLOCK_VBI(bktr);
+
 	return( 0 );
 }
 
_at__at_ -1232,19 +1248,32 _at__at_
 int
 vbi_read(bktr_ptr_t bktr, struct uio *uio, int ioflag)
 {
-	int             readsize, readsize2;
+	int             readsize, readsize2, start;
 	int             status;
 
+	/*
+	 * XXX - vbi_read() should be protected against being re-entered
+	 * while it is unlocked for the uiomove.
+	 */
+	LOCK_VBI(bktr);
 
 	while(bktr->vbisize == 0) {
 		if (ioflag & IO_NDELAY) {
-			return EWOULDBLOCK;
+			status = EWOULDBLOCK;
+			goto out;
 		}
 
 		bktr->vbi_read_blocked = TRUE;
+#ifdef USE_VBIMUTEX
+		if ((status = msleep(VBI_SLEEP, &bktr->vbimutex, VBIPRI, "vbi",
+		    0))) {
+			goto out;
+		}
+#else
 		if ((status = tsleep(VBI_SLEEP, VBIPRI, "vbi", 0))) {
-			return status;
+			goto out;
 		}
+#endif
 	}
 
 	/* Now we have some data to give to the user */
_at__at_ -1262,19 +1291,28 _at__at_
 		/* We need to wrap around */
 
 		readsize2 = VBI_BUFFER_SIZE - bktr->vbistart;
-               	status = uiomove((caddr_t)bktr->vbibuffer + bktr->vbistart, readsize2, uio);
-		status += uiomove((caddr_t)bktr->vbibuffer, (readsize - readsize2), uio);
+		start =  bktr->vbistart;
+		UNLOCK_VBI(bktr);
+               	status = uiomove((caddr_t)bktr->vbibuffer + start, readsize2, uio);
+		if (status == 0)
+			status = uiomove((caddr_t)bktr->vbibuffer, (readsize - readsize2), uio);
 	} else {
+		UNLOCK_VBI(bktr);
 		/* We do not need to wrap around */
 		status = uiomove((caddr_t)bktr->vbibuffer + bktr->vbistart, readsize, uio);
 	}
 
+	LOCK_VBI(bktr);
+
 	/* Update the number of bytes left to read */
 	bktr->vbisize -= readsize;
 
 	/* Update vbistart */
 	bktr->vbistart += readsize;
 	bktr->vbistart = bktr->vbistart % VBI_BUFFER_SIZE; /* wrap around if needed */
+
+out:
+	UNLOCK_VBI(bktr);
 
 	return( status );
 
Index: sys/dev/bktr/bktr_os.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/bktr/bktr_os.c,v
retrieving revision 1.39
diff -u -r1.39 bktr_os.c
--- sys/dev/bktr/bktr_os.c	2 Sep 2003 17:30:34 -0000	1.39
+++ sys/dev/bktr/bktr_os.c	27 Nov 2003 23:49:39 -0000
_at__at_ -499,6 +499,9 _at__at_
 		printf("bktr%d: i2c_attach: can't attach\n",
 		     device_get_unit(dev));
 #endif
+#ifdef USE_VBIMUTEX
+        mtx_destroy(&bktr->vbimutex);
+#endif
 
 	/* Note: We do not free memory for RISC programs, grab buffer, vbi buffers */
 	/* The memory is retained by the bktr_mem module so we can unload and */
_at__at_ -830,6 +833,7 _at__at_
 		return (ENXIO);
 	}
 
+	LOCK_VBI(bktr);
 	DISABLE_INTR(s);
 
 	if (events & (POLLIN | POLLRDNORM)) {
_at__at_ -845,6 +849,7 _at__at_
 	}
 
 	ENABLE_INTR(s);
+	UNLOCK_VBI(bktr);
 
 	return (revents);
 }
Index: sys/dev/bktr/bktr_os.h
===================================================================
RCS file: /home/ncvs/src/sys/dev/bktr/bktr_os.h,v
retrieving revision 1.6
diff -u -r1.6 bktr_os.h
--- sys/dev/bktr/bktr_os.h	18 Dec 2001 00:27:15 -0000	1.6
+++ sys/dev/bktr/bktr_os.h	27 Nov 2003 23:38:51 -0000
_at__at_ -61,9 +61,10 _at__at_
 /************************************/
 #if defined(__FreeBSD__)
 #if (__FreeBSD_version >=500000)
+#define USE_VBIMUTEX
 #define	DECLARE_INTR_MASK(s)	/* no need to declare 's' */
-#define DISABLE_INTR(s)		critical_enter()
-#define	ENABLE_INTR(s)		critical_exit()
+#define DISABLE_INTR(s)
+#define ENABLE_INTR(s)
 #else
 #define DECLARE_INTR_MASK(s)	intrmask_t s
 #define DISABLE_INTR(s)		s=spltty()
_at__at_ -75,4 +76,10 _at__at_
 #define ENABLE_INTR(s)		enable_intr()
 #endif
 
-
+#ifdef USE_VBIMUTEX
+#define LOCK_VBI(bktr)		mtx_lock(&bktr->vbimutex)
+#define UNLOCK_VBI(bktr)	mtx_unlock(&bktr->vbimutex)
+#else
+#define LOCK_VBI(bktr)
+#define UNLOCK_VBI(bktr)
+#endif
Index: sys/dev/bktr/bktr_reg.h
===================================================================
RCS file: /home/ncvs/src/sys/dev/bktr/bktr_reg.h,v
retrieving revision 1.45
diff -u -r1.45 bktr_reg.h
--- sys/dev/bktr/bktr_reg.h	12 Aug 2003 09:45:34 -0000	1.45
+++ sys/dev/bktr/bktr_reg.h	27 Nov 2003 23:31:53 -0000
_at__at_ -542,6 +542,9 _at__at_
     dev_t           tunerdev_alias;	/* alias /dev/tuner to /dev/tuner0 */
     dev_t           vbidev_alias;	/* alias /dev/vbi to /dev/vbi0 */
     #endif
+    #if (__FreeBSD_version >= 500000)
+    struct mtx      vbimutex;  /* Mutex protecting vbi buffer */
+    #endif
     #if (__FreeBSD_version >= 310000)
     bus_space_tag_t	memt;	/* Bus space register access functions */
     bus_space_handle_t	memh;	/* Bus space register access functions */