drm/irq: remove cargo-culted locking from irq_install/uninstall
The dev->struct_mutex locking in drm_irq.c only protects
dev->irq_enabled. Which isn't really much at all and only prevents
especially nasty ums userspace from concurrently installing the
interrupt handling a few times. Or at least trying.
There are tons of unlocked readers of dev->irqs_enabled in the vblank
wait code (and by extension also in the pageflip code since that uses
the same vblank timestamp engine).
Real modesetting drivers should ensure that nothing can go haywire
with a sane setup teardown sequence. So we only really need this for
the drm_control ioctl, everywhere else this will just paper over
nastiness.
Note that drm/i915 is a bit specially due to the gem+ums combination.
So there we also need to properly protect the entervt and leavevt
ioctls. But it's definitely saner to do everything in one go than to
drop the lock in-between.
Finally there's the gpu reset code in drm/i915. That one's just race
(concurrent userspace calls to for vblank waits of pageflips could
spuriously fail). So wrap it up in with a nice comment since fixing
this is more involved.
v2: Rebase and fix commit message (Thierry)
Reviewed-by: Thierry Reding <treding@nvidia.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 580f9ed..06a53f8 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -254,20 +254,13 @@
if (drm_dev_to_irq(dev) == 0)
return -EINVAL;
- mutex_lock(&dev->struct_mutex);
-
/* Driver must have been initialized */
- if (!dev->dev_private) {
- mutex_unlock(&dev->struct_mutex);
+ if (!dev->dev_private)
return -EINVAL;
- }
- if (dev->irq_enabled) {
- mutex_unlock(&dev->struct_mutex);
+ if (dev->irq_enabled)
return -EBUSY;
- }
dev->irq_enabled = true;
- mutex_unlock(&dev->struct_mutex);
DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
@@ -288,9 +281,7 @@
sh_flags, irqname, dev);
if (ret < 0) {
- mutex_lock(&dev->struct_mutex);
dev->irq_enabled = false;
- mutex_unlock(&dev->struct_mutex);
return ret;
}
@@ -302,9 +293,7 @@
ret = dev->driver->irq_postinstall(dev);
if (ret < 0) {
- mutex_lock(&dev->struct_mutex);
dev->irq_enabled = false;
- mutex_unlock(&dev->struct_mutex);
if (!drm_core_check_feature(dev, DRIVER_MODESET))
vga_client_register(dev->pdev, NULL, NULL, NULL);
free_irq(drm_dev_to_irq(dev), dev);
@@ -330,10 +319,8 @@
if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
return -EINVAL;
- mutex_lock(&dev->struct_mutex);
irq_enabled = dev->irq_enabled;
dev->irq_enabled = false;
- mutex_unlock(&dev->struct_mutex);
/*
* Wake up any waiters so they don't hang.
@@ -381,6 +368,7 @@
struct drm_file *file_priv)
{
struct drm_control *ctl = data;
+ int ret = 0;
/* if we haven't irq we fallback for compatibility reasons -
* this used to be a separate function in drm_dma.h
@@ -399,9 +387,17 @@
if (dev->if_version < DRM_IF_VERSION(1, 2) &&
ctl->irq != drm_dev_to_irq(dev))
return -EINVAL;
- return drm_irq_install(dev);
+ mutex_lock(&dev->struct_mutex);
+ ret = drm_irq_install(dev);
+ mutex_unlock(&dev->struct_mutex);
+
+ return ret;
case DRM_UNINST_HANDLER:
- return drm_irq_uninstall(dev);
+ mutex_lock(&dev->struct_mutex);
+ ret = drm_irq_uninstall(dev);
+ mutex_unlock(&dev->struct_mutex);
+
+ return ret;
default:
return -EINVAL;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 82f4d1f..87ce105 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -746,6 +746,11 @@
return ret;
}
+ /*
+ * FIXME: This is horribly race against concurrent pageflip and
+ * vblank wait ioctls since they can observe dev->irqs_disabled
+ * being false when they shouldn't be able to.
+ */
drm_irq_uninstall(dev);
drm_irq_install(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2871ce7..d2cc19a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4522,16 +4522,15 @@
}
BUG_ON(!list_empty(&dev_priv->gtt.base.active_list));
- mutex_unlock(&dev->struct_mutex);
ret = drm_irq_install(dev);
if (ret)
goto cleanup_ringbuffer;
+ mutex_unlock(&dev->struct_mutex);
return 0;
cleanup_ringbuffer:
- mutex_lock(&dev->struct_mutex);
i915_gem_cleanup_ringbuffer(dev);
dev_priv->ums.mm_suspended = 1;
mutex_unlock(&dev->struct_mutex);
@@ -4546,7 +4545,9 @@
if (drm_core_check_feature(dev, DRIVER_MODESET))
return 0;
+ mutex_lock(&dev->struct_mutex);
drm_irq_uninstall(dev);
+ mutex_unlock(&dev->struct_mutex);
return i915_gem_suspend(dev);
}