drm: reference framebuffers which are on the idr

Since otherwise looking and reference-counting around
drm_framebuffer_lookup will be an unmanageable mess. With this change,
an object can either be found in the idr and will stay around once we
incremented the reference counter. Or it will be gone for good and
can't be looked up using its id any more.

Atomicity is guaranteed by the dev->mode_config.fb_lock. The
newly-introduce fpriv->fbs_lock looks a bit redundant, but the next
patch will shuffle the locking order between these two locks and all
the modeset locks taken in modeset_lock_all, so we'll need it.

Also, since userspace could do really funky stuff and race e.g. a
getresources with an rmfb, we need to make sure that the kernel
doesn't fall over trying to look-up an inexistent fb, or causing
confusion by having two fbs around with the same id. Simply reset the
framebuffer id to 0, which marks it as reaped. Any lookups of that id
will fail, so the object is really gone for good from userspace's pov.

Note that we still need to protect the "remove framebuffer from all
use-cases" and the final unreference with the modeset-lock, since most
framebuffer use-sites don't implement proper reference counting yet.
We can only lift this once _all_ users are converted.

With this change, two references are held on alife, but unused
framebuffers:
- The reference for the idr lookup, created in this patch.
- For user-created framebuffers the fpriv->fbs reference, for
  driver-private fbs the driver is supposed to hold it's own last
  reference.

Note that the dev->mode_config.fb_list itself does _not_ hold a
reference onto the framebuffers (this list is essentially only used
for debugfs files). Hence if there's anything left there when the
driver has cleaned up all it's modeset resources, this is a ref-leak.
WARN about it.

Now we only need to fix up all other places to properly reference
count framebuffers.

v2: Fix spelling fail in a comment spotted by Rob Clark.

Reviewed-by: Rob Clark <rob@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

1 files changed, 80 insertions, 38 deletions

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3eddfabeba9..a50f7553b31 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -356,6 +356,9 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	if (ret)
 		goto out;
 
+	/* Grab the idr reference. */
+	drm_framebuffer_reference(fb);
+
 	dev->mode_config.num_fb++;
 	list_add(&fb->head, &dev->mode_config.fb_list);
 out:
@@ -372,6 +375,23 @@ static void drm_framebuffer_free(struct kref *kref)
 	fb->funcs->destroy(fb);
 }
 
+static struct drm_framebuffer *__drm_framebuffer_lookup(struct drm_device *dev,
+							uint32_t id)
+{
+	struct drm_mode_object *obj = NULL;
+	struct drm_framebuffer *fb;
+
+	mutex_lock(&dev->mode_config.idr_mutex);
+	obj = idr_find(&dev->mode_config.crtc_idr, id);
+	if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id))
+		fb = NULL;
+	else
+		fb = obj_to_fb(obj);
+	mutex_unlock(&dev->mode_config.idr_mutex);
+
+	return fb;
+}
+
 /**
  * drm_framebuffer_lookup - look up a drm framebuffer and grab a reference
  * @dev: drm device
@@ -384,22 +404,12 @@ static void drm_framebuffer_free(struct kref *kref)
 struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
 					       uint32_t id)
 {
-	struct drm_mode_object *obj = NULL;
 	struct drm_framebuffer *fb;
 
 	mutex_lock(&dev->mode_config.fb_lock);
-
-	mutex_lock(&dev->mode_config.idr_mutex);
-	obj = idr_find(&dev->mode_config.crtc_idr, id);
-	if (!obj || (obj->type != DRM_MODE_OBJECT_FB) || (obj->id != id))
-		fb = NULL;
-	else
-		fb = obj_to_fb(obj);
-	mutex_unlock(&dev->mode_config.idr_mutex);
-
+	fb = __drm_framebuffer_lookup(dev, id);
 	if (fb)
 		kref_get(&fb->refcount);
-
 	mutex_unlock(&dev->mode_config.fb_lock);
 
 	return fb;
@@ -430,6 +440,24 @@ void drm_framebuffer_reference(struct drm_framebuffer *fb)
 }
 EXPORT_SYMBOL(drm_framebuffer_reference);
 
+static void drm_framebuffer_free_bug(struct kref *kref)
+{
+	BUG();
+}
+
+/* dev->mode_config.fb_lock must be held! */
+static void __drm_framebuffer_unregister(struct drm_device *dev,
+					 struct drm_framebuffer *fb)
+{
+	mutex_lock(&dev->mode_config.idr_mutex);
+	idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
+	mutex_unlock(&dev->mode_config.idr_mutex);
+
+	fb->base.id = 0;
+
+	kref_put(&fb->refcount, drm_framebuffer_free_bug);
+}
+
 /**
  * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
  * @fb: fb to unregister
@@ -441,6 +469,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference);
  */
 void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = fb->dev;
+
+	mutex_lock(&dev->mode_config.fb_lock);
+	/* Mark fb as reaped and drop idr ref. */
+	__drm_framebuffer_unregister(dev, fb);
+	mutex_unlock(&dev->mode_config.fb_lock);
 }
 EXPORT_SYMBOL(drm_framebuffer_unregister_private);
 
@@ -464,14 +498,6 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
 
-	/*
-	 * This could be moved to drm_framebuffer_remove(), but for
-	 * debugging is nice to keep around the list of fb's that are
-	 * no longer associated w/ a drm_file but are not unreferenced
-	 * yet.  (i915 and omapdrm have debugfs files which will show
-	 * this.)
-	 */
-	drm_mode_object_put(dev, &fb->base);
 	mutex_lock(&dev->mode_config.fb_lock);
 	list_del(&fb->head);
 	dev->mode_config.num_fb--;
@@ -1181,9 +1207,15 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 		drm_property_destroy(dev, property);
 	}
 
-	/* Single-threaded teardown context, so it's not requied to grab the
+	/*
+	 * Single-threaded teardown context, so it's not required to grab the
 	 * fb_lock to protect against concurrent fb_list access. Contrary, it
-	 * would actually deadlock with the drm_framebuffer_cleanup function. */
+	 * would actually deadlock with the drm_framebuffer_cleanup function.
+	 *
+	 * Also, if there are any framebuffers left, that's a driver leak now,
+	 * so politely WARN about this.
+	 */
+	WARN_ON(!list_empty(&dev->mode_config.fb_list));
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
 		drm_framebuffer_remove(fb);
 	}
@@ -2464,39 +2496,41 @@ int drm_mode_rmfb(struct drm_device *dev,
 	struct drm_framebuffer *fb = NULL;
 	struct drm_framebuffer *fbl = NULL;
 	uint32_t *id = data;
-	int ret = 0;
 	int found = 0;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	drm_modeset_lock_all(dev);
-	fb = drm_framebuffer_lookup(dev, *id);
-	if (!fb) {
-		ret = -EINVAL;
-		goto out;
-	}
-	/* fb is protect by the mode_config lock, so drop the ref immediately */
-	drm_framebuffer_unreference(fb);
-
 	mutex_lock(&file_priv->fbs_lock);
+	mutex_lock(&dev->mode_config.fb_lock);
+	fb = __drm_framebuffer_lookup(dev, *id);
+	if (!fb)
+		goto fail_lookup;
+
 	list_for_each_entry(fbl, &file_priv->fbs, filp_head)
 		if (fb == fbl)
 			found = 1;
-	if (!found) {
-		ret = -EINVAL;
-		mutex_unlock(&file_priv->fbs_lock);
-		goto out;
-	}
+	if (!found)
+		goto fail_lookup;
+
+	/* Mark fb as reaped, we still have a ref from fpriv->fbs. */
+	__drm_framebuffer_unregister(dev, fb);
 
 	list_del_init(&fb->filp_head);
+	mutex_unlock(&dev->mode_config.fb_lock);
 	mutex_unlock(&file_priv->fbs_lock);
 
+	drm_modeset_lock_all(dev);
 	drm_framebuffer_remove(fb);
-out:
 	drm_modeset_unlock_all(dev);
 
-	return ret;
+	return 0;
+
+fail_lookup:
+	mutex_unlock(&dev->mode_config.fb_lock);
+	mutex_unlock(&file_priv->fbs_lock);
+
+	return -EINVAL;
 }
 
 /**
@@ -2639,7 +2673,15 @@ void drm_fb_release(struct drm_file *priv)
 	drm_modeset_lock_all(dev);
 	mutex_lock(&priv->fbs_lock);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
+
+		mutex_lock(&dev->mode_config.fb_lock);
+		/* Mark fb as reaped, we still have a ref from fpriv->fbs. */
+		__drm_framebuffer_unregister(dev, fb);
+		mutex_unlock(&dev->mode_config.fb_lock);
+
 		list_del_init(&fb->filp_head);
+
+		/* This will also drop the fpriv->fbs reference. */
 		drm_framebuffer_remove(fb);
 	}
 	mutex_unlock(&priv->fbs_lock);