Refine the locking of the DRM. Most significant is covering the driver

    ioctls with dev_lock, which is a major step toward being able to remove
    Giant. Covers some new pieces (dev->unique*) in the core, and avoids
    one call down into system internals with the drm lock held, which is
    usually bad (FreeBSD LOR #23, #27).

8 files changed, 130 insertions, 93 deletions

diff --git a/bsd-core/drmP.h b/bsd-core/drmP.h
index 17af0d18..43e22039 100644
--- a/bsd-core/drmP.h
+++ b/bsd-core/drmP.h
@@ -197,6 +197,7 @@ MALLOC_DECLARE(M_DRM);
 #define DRM_SPINUNINIT(l)
 #define DRM_SPINLOCK(l)	
 #define DRM_SPINUNLOCK(u)
+#define DRM_SPINLOCK_ASSERT(l)
 #define DRM_CURRENTPID		curproc->p_pid
 #define DRM_LOCK()
 #define DRM_UNLOCK()
@@ -353,9 +354,7 @@ do {								\
 		DRM_ERROR("filp doesn't match curproc\n");	\
 		return EINVAL;					\
 	}							\
-	DRM_LOCK();						\
 	_priv = drm_find_file_by_proc(dev, DRM_CURPROC);	\
-	DRM_UNLOCK();						\
 	if (_priv == NULL) {					\
 		DRM_ERROR("can't find authenticator\n");	\
 		return EINVAL;					\
@@ -375,6 +374,7 @@ do {									\
 #define DRM_GETSAREA()					\
 do {								\
 	drm_map_list_entry_t *listentry;			\
+	DRM_SPINLOCK_ASSERT(&dev->dev_lock);			\
 	TAILQ_FOREACH(listentry, dev->maplist, link) {		\
 		drm_local_map_t *map = listentry->map;		\
 		if (map->type == _DRM_SHM &&			\
@@ -388,11 +388,13 @@ do {								\
 #if defined(__FreeBSD__) && __FreeBSD_version > 500000
 #define DRM_WAIT_ON( ret, queue, timeout, condition )		\
 for ( ret = 0 ; !ret && !(condition) ; ) {			\
+	DRM_UNLOCK();						\
 	mtx_lock(&dev->irq_lock);				\
 	if (!(condition))					\
-	   ret = msleep(&(queue), &dev->irq_lock, 	\
+	   ret = msleep(&(queue), &dev->irq_lock, 		\
 			 PZERO | PCATCH, "drmwtq", (timeout));	\
-	mtx_unlock(&dev->irq_lock);			\
+	mtx_unlock(&dev->irq_lock);				\
+	DRM_LOCK();						\
 }
 #else
 #define DRM_WAIT_ON( ret, queue, timeout, condition )	\
@@ -527,10 +529,6 @@ typedef struct drm_device_dma {
 		_DRM_DMA_USE_AGP = 0x01,
 		_DRM_DMA_USE_SG  = 0x02
 	} flags;
-
-				/* DMA support */
-	drm_buf_t	  *this_buffer;	/* Buffer being sent		   */
-	drm_buf_t	  *next_buffer; /* Selected buffer to send	   */
 } drm_device_dma_t;
 
 typedef struct drm_agp_mem {
diff --git a/bsd-core/drm_auth.c b/bsd-core/drm_auth.c
index fb556f33..dfd820da 100644
--- a/bsd-core/drm_auth.c
+++ b/bsd-core/drm_auth.c
@@ -116,12 +116,18 @@ static int drm_remove_magic(drm_device_t *dev, drm_magic_t magic)
 
 int drm_getmagic(DRM_IOCTL_ARGS)
 {
+	DRM_DEVICE;
 	static drm_magic_t sequence = 0;
 	drm_auth_t auth;
 	drm_file_t *priv;
-	DRM_DEVICE;
 
-	DRM_GET_PRIV_WITH_RETURN(priv, filp);
+	DRM_LOCK();
+	priv = drm_find_file_by_proc(dev, p);
+	DRM_UNLOCK();
+	if (priv == NULL) {
+		DRM_ERROR("can't find authenticator\n");
+		return EINVAL;
+	}
 
 				/* Find unique magic */
 	if (priv->magic) {
diff --git a/bsd-core/drm_context.c b/bsd-core/drm_context.c
index d785d361..714edccc 100644
--- a/bsd-core/drm_context.c
+++ b/bsd-core/drm_context.c
@@ -229,15 +229,12 @@ int drm_context_switch_complete(drm_device_t *dev, int new)
 int drm_resctx(DRM_IOCTL_ARGS)
 {
 	drm_ctx_res_t res;
-	drm_ctx_t ctx;
 	int i;
 
 	DRM_COPY_FROM_USER_IOCTL( res, (drm_ctx_res_t *)data, sizeof(res) );
 
 	if ( res.count >= DRM_RESERVED_CONTEXTS ) {
-		bzero(&ctx, sizeof(ctx));
 		for ( i = 0 ; i < DRM_RESERVED_CONTEXTS ; i++ ) {
-			ctx.handle = i;
 			if ( DRM_COPY_TO_USER( &res.contexts[i],
 					   &i, sizeof(i) ) )
 				return DRM_ERR(EFAULT);
@@ -269,8 +266,11 @@ int drm_addctx(DRM_IOCTL_ARGS)
 		return DRM_ERR(ENOMEM);
 	}
 
-	if (dev->context_ctor && ctx.handle != DRM_KERNEL_CONTEXT)
+	if (dev->context_ctor && ctx.handle != DRM_KERNEL_CONTEXT) {
+		DRM_LOCK();
 		dev->context_ctor(dev, ctx.handle);
+		DRM_UNLOCK();
+	}
 
 	DRM_COPY_TO_USER_IOCTL( (drm_ctx_t *)data, ctx, sizeof(ctx) );
 
@@ -330,8 +330,11 @@ int drm_rmctx(DRM_IOCTL_ARGS)
 
 	DRM_DEBUG( "%d\n", ctx.handle );
 	if ( ctx.handle != DRM_KERNEL_CONTEXT ) {
-		if (dev->context_dtor)
+		if (dev->context_dtor) {
+			DRM_LOCK();
 			dev->context_dtor(dev, ctx.handle);
+			DRM_UNLOCK();
+		}
 
 		drm_ctxbitmap_free(dev, ctx.handle);
 	}
diff --git a/bsd-core/drm_drv.c b/bsd-core/drm_drv.c
index 7880f76c..fb9313af 100644
--- a/bsd-core/drm_drv.c
+++ b/bsd-core/drm_drv.c
@@ -809,9 +809,17 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags,
 	drm_ioctl_desc_t *ioctl;
 	int (*func)(DRM_IOCTL_ARGS);
 	int nr = DRM_IOCTL_NR(cmd);
+	int is_driver_ioctl = 0;
 	drm_file_t *priv;
+	DRMFILE filp = (DRMFILE)(uintptr_t)DRM_CURRENTPID;
 
-	DRM_GET_PRIV_WITH_RETURN(priv, (DRMFILE)(uintptr_t)DRM_CURRENTPID);
+	DRM_LOCK();
+	priv = drm_find_file_by_proc(dev, p);
+	DRM_UNLOCK();
+	if (priv == NULL) {
+		DRM_ERROR("can't find authenticator\n");
+		return EINVAL;
+	}
 
 	atomic_inc( &dev->counts[_DRM_STAT_IOCTLS] );
 	++priv->ioctl_count;
@@ -868,6 +876,7 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags,
 			return EINVAL;
 		}
 		ioctl = &dev->driver_ioctls[nr];
+		is_driver_ioctl = 1;
 	}
 	func = ioctl->func;
 
@@ -879,7 +888,11 @@ int drm_ioctl(struct cdev *kdev, u_long cmd, caddr_t data, int flags,
 	    !priv->authenticated))
 		return EACCES;
 
-	retcode = func(kdev, cmd, data, flags, p, (void *)(uintptr_t)DRM_CURRENTPID);
+	if (is_driver_ioctl)
+		DRM_LOCK();
+	retcode = func(kdev, cmd, data, flags, p, filp);
+	if (is_driver_ioctl)
+		DRM_UNLOCK();
 
 	if (retcode != 0)
 		DRM_DEBUG("    returning %d\n", retcode);
diff --git a/bsd-core/drm_ioctl.c b/bsd-core/drm_ioctl.c
index 829a99fe..d40a7c90 100644
--- a/bsd-core/drm_ioctl.c
+++ b/bsd-core/drm_ioctl.c
@@ -64,40 +64,53 @@ int drm_setunique(DRM_IOCTL_ARGS)
 	DRM_DEVICE;
 	drm_unique_t u;
 	int domain, bus, slot, func, ret;
-
-	if (dev->unique_len || dev->unique)
-		return DRM_ERR(EBUSY);
+	char *busid;
 
 	DRM_COPY_FROM_USER_IOCTL( u, (drm_unique_t *)data, sizeof(u) );
 
+	/* Check and copy in the submitted Bus ID */
 	if (!u.unique_len || u.unique_len > 1024)
 		return DRM_ERR(EINVAL);
 
-	dev->unique_len = u.unique_len;
-	dev->unique = malloc(u.unique_len + 1, M_DRM, M_NOWAIT);
-
-	if (dev->unique == NULL)
+	busid = malloc(u.unique_len + 1, M_DRM, M_WAITOK);
+	if (busid == NULL)
 		return DRM_ERR(ENOMEM);
 
-	if (DRM_COPY_FROM_USER(dev->unique, u.unique, dev->unique_len))
+	if (DRM_COPY_FROM_USER(busid, u.unique, u.unique_len)) {
+		free(busid, M_DRM);
 		return DRM_ERR(EFAULT);
-
-	dev->unique[dev->unique_len] = '\0';
+	}
+	busid[u.unique_len] = '\0';
 
 	/* Return error if the busid submitted doesn't match the device's actual
 	 * busid.
 	 */
-	ret = sscanf(dev->unique, "PCI:%d:%d:%d", &bus, &slot, &func);
-	if (ret != 3)
+	ret = sscanf(busid, "PCI:%d:%d:%d", &bus, &slot, &func);
+	if (ret != 3) {
+		free(busid, M_DRM);
 		return DRM_ERR(EINVAL);
+	}
 	domain = bus >> 8;
 	bus &= 0xff;
 	
 	if ((domain != dev->pci_domain) ||
 	    (bus != dev->pci_bus) ||
 	    (slot != dev->pci_slot) ||
-	    (func != dev->pci_func))
+	    (func != dev->pci_func)) {
+		free(busid, M_DRM);
 		return DRM_ERR(EINVAL);
+	}
+
+	/* Actually set the device's busid now. */
+	DRM_LOCK();
+	if (dev->unique_len || dev->unique) {
+		DRM_UNLOCK();
+		return DRM_ERR(EBUSY);
+	}
+
+	dev->unique_len = u.unique_len;
+	dev->unique = busid;
+	DRM_UNLOCK();
 
 	return 0;
 }
@@ -107,17 +120,25 @@ static int
 drm_set_busid(drm_device_t *dev)
 {
 
-	if (dev->unique != NULL)
+	DRM_LOCK();
+
+	if (dev->unique != NULL) {
+		DRM_UNLOCK();
 		return EBUSY;
+	}
 
 	dev->unique_len = 20;
 	dev->unique = malloc(dev->unique_len + 1, M_DRM, M_NOWAIT);
-	if (dev->unique == NULL)
+	if (dev->unique == NULL) {
+		DRM_UNLOCK();
 		return ENOMEM;
+	}
 
 	snprintf(dev->unique, dev->unique_len, "pci:%04x:%02x:%02x.%1x",
 	    dev->pci_domain, dev->pci_bus, dev->pci_slot, dev->pci_func);
 
+	DRM_UNLOCK();
+
 	return 0;
 }
 
@@ -264,9 +285,6 @@ int drm_setversion(DRM_IOCTL_ARGS)
 		if (sv.drm_dd_major != dev->driver_major ||
 		    sv.drm_dd_minor < 0 || sv.drm_dd_minor > dev->driver_minor)
 			return EINVAL;
-#ifdef DRIVER_SETVERSION
-		DRIVER_SETVERSION(dev, &sv);
-#endif
 	}
 	return 0;
 }
diff --git a/bsd-core/drm_irq.c b/bsd-core/drm_irq.c
index eb32bac4..422f7222 100644
--- a/bsd-core/drm_irq.c
+++ b/bsd-core/drm_irq.c
@@ -73,25 +73,22 @@ int drm_irq_install(drm_device_t *dev)
 	if (dev->irq == 0 || dev->dev_private == NULL)
 		return DRM_ERR(EINVAL);
 
+	DRM_DEBUG( "%s: irq=%d\n", __FUNCTION__, dev->irq );
+
 	DRM_LOCK();
 	if (dev->irq_enabled) {
 		DRM_UNLOCK();
 		return DRM_ERR(EBUSY);
 	}
 	dev->irq_enabled = 1;
-	DRM_UNLOCK();
-
-	DRM_DEBUG( "%s: irq=%d\n", __FUNCTION__, dev->irq );
 
 	dev->context_flag = 0;
 
-	dev->dma->next_buffer = NULL;
-	dev->dma->this_buffer = NULL;
-
 	DRM_SPININIT(dev->irq_lock, "DRM IRQ lock");
 
 				/* Before installing handler */
 	dev->irq_preinstall(dev);
+	DRM_UNLOCK();
 
 				/* Install handler */
 #ifdef __FreeBSD__
@@ -125,7 +122,9 @@ int drm_irq_install(drm_device_t *dev)
 #endif
 
 				/* After installing handler */
+	DRM_LOCK();
 	dev->irq_postinstall(dev);
+	DRM_UNLOCK();
 
 	return 0;
 err:
@@ -143,9 +142,6 @@ err:
 	return retcode;
 }
 
-/* XXX: This function needs to be called with the device lock held.  In some
- * cases it isn't, so far.
- */
 int drm_irq_uninstall(drm_device_t *dev)
 {
 	int irqrid;
@@ -162,8 +158,10 @@ int drm_irq_uninstall(drm_device_t *dev)
 	dev->irq_uninstall(dev);
 
 #ifdef __FreeBSD__
+	DRM_UNLOCK();
 	bus_teardown_intr(dev->device, dev->irqr, dev->irqh);
 	bus_release_resource(dev->device, SYS_RES_IRQ, irqrid, dev->irqr);
+	DRM_LOCK();
 #elif defined(__NetBSD__) || defined(__OpenBSD__)
 	pci_intr_disestablish(&dev->pa.pa_pc, dev->irqh);
 #endif
@@ -242,7 +240,9 @@ int drm_wait_vblank(DRM_IOCTL_ARGS)
 #endif
 		ret = EINVAL;
 	} else {
+		DRM_LOCK();
 		ret = dev->vblank_wait(dev, &vblwait.request.sequence);
+		DRM_UNLOCK();
 
 		microtime(&now);
 		vblwait.reply.tval_sec = now.tv_sec;
diff --git a/bsd-core/drm_scatter.c b/bsd-core/drm_scatter.c
index 03e82439..7a035af5 100644
--- a/bsd-core/drm_scatter.c
+++ b/bsd-core/drm_scatter.c
@@ -36,7 +36,6 @@
 void drm_sg_cleanup(drm_sg_mem_t *entry)
 {
 	free(entry->virtual, M_DRM);
-
 	free(entry->busaddr, M_DRM);
 	free(entry, M_DRM);
 }
@@ -56,7 +55,7 @@ int drm_sg_alloc(DRM_IOCTL_ARGS)
 	DRM_COPY_FROM_USER_IOCTL(request, (drm_scatter_gather_t *)data,
 			     sizeof(request) );
 
-	entry = malloc(sizeof(*entry), M_DRM, M_NOWAIT | M_ZERO);
+	entry = malloc(sizeof(*entry), M_DRM, M_WAITOK | M_ZERO);
 	if ( !entry )
 		return ENOMEM;
 
@@ -66,16 +65,15 @@ int drm_sg_alloc(DRM_IOCTL_ARGS)
 	entry->pages = pages;
 
 	entry->busaddr = malloc(pages * sizeof(*entry->busaddr), M_DRM,
-	    M_NOWAIT | M_ZERO);
+	    M_WAITOK | M_ZERO);
 	if ( !entry->busaddr ) {
-		free(entry, M_DRM);
+		drm_sg_cleanup(entry);
 		return ENOMEM;
 	}
 
 	entry->virtual = malloc(pages << PAGE_SHIFT, M_DRM, M_WAITOK | M_ZERO);
 	if ( !entry->virtual ) {
-		free(entry->busaddr, M_DRM);
-		free(entry, M_DRM);
+		drm_sg_cleanup(entry);
 		return ENOMEM;
 	}
 
@@ -90,12 +88,16 @@ int drm_sg_alloc(DRM_IOCTL_ARGS)
 			   request,
 			   sizeof(request) );
 
+	DRM_LOCK();
+	if (dev->sg) {
+		DRM_UNLOCK();
+		drm_sg_cleanup(entry);
+		return EINVAL;
+	}
 	dev->sg = entry;
+	DRM_UNLOCK();
 
 	return 0;
-
-	drm_sg_cleanup(entry);
-	return ENOMEM;
 }
 
 int drm_sg_free(DRM_IOCTL_ARGS)
@@ -107,8 +109,10 @@ int drm_sg_free(DRM_IOCTL_ARGS)
 	DRM_COPY_FROM_USER_IOCTL( request, (drm_scatter_gather_t *)data,
 			     sizeof(request) );
 
+	DRM_LOCK();
 	entry = dev->sg;
 	dev->sg = NULL;
+	DRM_UNLOCK();
 
 	if ( !entry || entry->handle != request.handle )
 		return EINVAL;
diff --git a/bsd-core/drm_vm.c b/bsd-core/drm_vm.c
index f3558b9e..1b5d4d3f 100644
--- a/bsd-core/drm_vm.c
+++ b/bsd-core/drm_vm.c
@@ -26,35 +26,6 @@
 #include "drm.h"
 
 #if defined(__FreeBSD__) && __FreeBSD_version >= 500102
-static int drm_dma_mmap(struct cdev *kdev, vm_offset_t offset,
-    vm_paddr_t *paddr, int prot)
-#elif defined(__FreeBSD__)
-static int drm_dma_mmap(dev_t kdev, vm_offset_t offset, int prot)
-#elif defined(__NetBSD__) || defined(__OpenBSD__)
-static paddr_t drm_dma_mmap(dev_t kdev, vm_offset_t offset, int prot)
-#endif
-{
-	DRM_DEVICE;
-	drm_device_dma_t *dma = dev->dma;
-	unsigned long physical;
-	unsigned long page;
-
-	if (dma == NULL || dma->pagelist == NULL)
-		return -1;
-
-	page	 = offset >> PAGE_SHIFT;
-	physical = dma->pagelist[page];
-
-	DRM_DEBUG("0x%08lx (page %lu) => 0x%08lx\n", (long)offset, page, physical);
-#if defined(__FreeBSD__) && __FreeBSD_version >= 500102
-	*paddr = physical;
-	return 0;
-#else
-	return atop(physical);
-#endif
-}
-
-#if defined(__FreeBSD__) && __FreeBSD_version >= 500102
 int drm_mmap(struct cdev *kdev, vm_offset_t offset, vm_paddr_t *paddr, 
     int prot)
 #elif defined(__FreeBSD__)
@@ -67,20 +38,40 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot)
 	drm_local_map_t *map = NULL;
 	drm_map_list_entry_t *listentry = NULL;
 	drm_file_t *priv;
+	drm_map_type_t type;
 
-	DRM_GET_PRIV_WITH_RETURN(priv, (DRMFILE)(uintptr_t)DRM_CURRENTPID);
+	DRM_LOCK();
+	priv = drm_find_file_by_proc(dev, DRM_CURPROC);
+	DRM_UNLOCK();
+	if (priv == NULL) {
+		DRM_ERROR("can't find authenticator\n");
+		return EINVAL;
+	}
 
 	if (!priv->authenticated)
 		return DRM_ERR(EACCES);
 
-	if (dev->dma
-	    && offset >= 0
-	    && offset < ptoa(dev->dma->page_count))
+	DRM_SPINLOCK(&dev->dma_lock);
+	if (dev->dma && offset >= 0 && offset < ptoa(dev->dma->page_count)) {
+		drm_device_dma_t *dma = dev->dma;
+
+		if (dma->pagelist != NULL) {
+			unsigned long page = offset >> PAGE_SHIFT;
+			unsigned long phys = dma->pagelist[page];
+
 #if defined(__FreeBSD__) && __FreeBSD_version >= 500102
-		return drm_dma_mmap(kdev, offset, paddr, prot);
+			*paddr = phys;
+			DRM_SPINUNLOCK(&dev->dma_lock);
+			return 0;
 #else
-		return drm_dma_mmap(kdev, offset, prot);
-#endif
+			return atop(phys);
+#endif		
+		} else {
+			DRM_SPINUNLOCK(&dev->dma_lock);
+			return -1;
+		}
+	}
+	DRM_SPINUNLOCK(&dev->dma_lock);
 
 				/* A sequential search of a linked list is
 				   fine here because: 1) there will only be
@@ -89,23 +80,27 @@ paddr_t drm_mmap(dev_t kdev, off_t offset, int prot)
 				   once, so it doesn't have to be optimized
 				   for performance, even if the list was a
 				   bit longer. */
+	DRM_LOCK();
 	TAILQ_FOREACH(listentry, dev->maplist, link) {
 		map = listentry->map;
-/*		DRM_DEBUG("considering 0x%x..0x%x\n", map->offset, map->offset + map->size - 1);*/
-		if (offset >= map->offset
-		    && offset < map->offset + map->size) break;
+		if (offset >= map->offset && offset < map->offset + map->size)
+			break;
 	}
 	
 	if (!listentry) {
+		DRM_UNLOCK();
 		DRM_DEBUG("can't find map\n");
 		return -1;
 	}
 	if (((map->flags&_DRM_RESTRICTED) && DRM_SUSER(DRM_CURPROC))) {
+		DRM_UNLOCK();
 		DRM_DEBUG("restricted map\n");
 		return -1;
 	}
+	type = map->type;
+	DRM_UNLOCK();
 
-	switch (map->type) {
+	switch (type) {
 	case _DRM_FRAME_BUFFER:
 	case _DRM_REGISTERS:
 	case _DRM_AGP: