fix the prime lock deadlock

fix the prime lock deadlock

[Dave Airlie]

The first two patches are cleanups, the second two attempt
to fix the deadlock on the prime lock, in the failure paths.

Patch 3 is based on a proposal from Daniel Vetter.

I've ran these under virgl and I can't reproduce the deadlock
there.

Dave.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/gem: consolidate handle deletion code.

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This code was duplicated, make it decidely
less duplicated.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_gem.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 3c2d4ab..12d0dc7 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -239,6 +239,21 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 	drm_gem_object_unreference_unlocked(obj);
 }
 
+static void
+drm_gem_handle_delete_tail(struct drm_file *filp,
+			   struct drm_gem_object *obj)
+{
+	struct drm_device *dev = obj->dev;
+
+	if (drm_core_check_feature(dev, DRIVER_PRIME))
+		drm_gem_remove_prime_handles(obj, filp);
+	drm_vma_node_revoke(&obj->vma_node, filp->filp);
+
+	if (dev->driver->gem_close_object)
+		dev->driver->gem_close_object(obj, filp);
+	drm_gem_object_handle_unreference_unlocked(obj);
+}
+
 /**
  * drm_gem_handle_delete - deletes the given file-private handle
  * @filp: drm file-private structure to use for the handle look up
@@ -276,14 +291,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 	idr_remove(&filp->object_idr, handle);
 	spin_unlock(&filp->table_lock);
 
-	if (drm_core_check_feature(dev, DRIVER_PRIME))
-		drm_gem_remove_prime_handles(obj, filp);
-	drm_vma_node_revoke(&obj->vma_node, filp->filp);
-
-	if (dev->driver->gem_close_object)
-		dev->driver->gem_close_object(obj, filp);
-	drm_gem_object_handle_unreference_unlocked(obj);
-
+	drm_gem_handle_delete_tail(filp, obj);
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_handle_delete);
@@ -708,17 +716,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 {
 	struct drm_file *file_priv = data;
 	struct drm_gem_object *obj = ptr;
-	struct drm_device *dev = obj->dev;
-
-	if (drm_core_check_feature(dev, DRIVER_PRIME))
-		drm_gem_remove_prime_handles(obj, file_priv);
-	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
-
-	if (dev->driver->gem_close_object)
-		dev->driver->gem_close_object(obj, file_priv);
-
-	drm_gem_object_handle_unreference_unlocked(obj);
 
+	drm_gem_handle_delete_tail(file_priv, obj);
 	return 0;
 }
 
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/prime: cleanup goto mess in drm_gem_prime_handle_to_fd

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

I think I might have been responsible for some of this, but it's messy
to decode, this doesn't change anything, but cleans up the goto's
and exit paths.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_prime.c | 73 ++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 9f935f5..d22ce83 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -420,47 +420,42 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
 	if (dmabuf) {
 		get_dma_buf(dmabuf);
-		goto out_have_handle;
-	}
-
-	mutex_lock(&dev->object_name_lock);
-	/* re-export the original imported object */
-	if (obj->import_attach) {
-		dmabuf = obj->import_attach->dmabuf;
-		get_dma_buf(dmabuf);
-		goto out_have_obj;
-	}
-
-	if (obj->dma_buf) {
-		get_dma_buf(obj->dma_buf);
-		dmabuf = obj->dma_buf;
-		goto out_have_obj;
-	}
+	} else {
+		mutex_lock(&dev->object_name_lock);
+		/* re-export the original imported object */
+		if (obj->import_attach) {
+			dmabuf = obj->import_attach->dmabuf;
+			get_dma_buf(dmabuf);
+		} else if (obj->dma_buf) {
+			get_dma_buf(obj->dma_buf);
+			dmabuf = obj->dma_buf;
+		} else {
+			dmabuf = export_and_register_object(dev, obj, flags);
+			if (IS_ERR(dmabuf)) {
+				/* normally the created dma-buf takes ownership of the ref,
+				 * but if that fails then drop the ref
+				 */
+				ret = PTR_ERR(dmabuf);
+				mutex_unlock(&dev->object_name_lock);
+				goto out;
+			}
+		}
 
-	dmabuf = export_and_register_object(dev, obj, flags);
-	if (IS_ERR(dmabuf)) {
-		/* normally the created dma-buf takes ownership of the ref,
-		 * but if that fails then drop the ref
+		/*
+		 * If we've exported this buffer then cheat and add it to the import list
+		 * so we get the correct handle back. We must do this under the
+		 * protection of dev->object_name_lock to ensure that a racing gem close
+		 * ioctl doesn't miss to remove this buffer handle from the cache.
 		 */
-		ret = PTR_ERR(dmabuf);
+		ret = drm_prime_add_buf_handle(&file_priv->prime,
+					       dmabuf, handle);
 		mutex_unlock(&dev->object_name_lock);
-		goto out;
+		if (ret) {
+			dma_buf_put(dmabuf);
+			goto out;
+		}
 	}
 
-out_have_obj:
-	/*
-	 * If we've exported this buffer then cheat and add it to the import list
-	 * so we get the correct handle back. We must do this under the
-	 * protection of dev->object_name_lock to ensure that a racing gem close
-	 * ioctl doesn't miss to remove this buffer handle from the cache.
-	 */
-	ret = drm_prime_add_buf_handle(&file_priv->prime,
-				       dmabuf, handle);
-	mutex_unlock(&dev->object_name_lock);
-	if (ret)
-		goto fail_put_dmabuf;
-
-out_have_handle:
 	ret = dma_buf_fd(dmabuf, flags);
 	/*
 	 * We must _not_ remove the buffer from the handle cache since the newly
@@ -469,16 +464,12 @@ out_have_handle:
 	 * Closing the handle will clean out the cache anyway, so we don't leak.
 	 */
 	if (ret < 0) {
-		goto fail_put_dmabuf;
+		dma_buf_put(dmabuf);
 	} else {
 		*prime_fd = ret;
 		ret = 0;
 	}
 
-	goto out;
-
-fail_put_dmabuf:
-	dma_buf_put(dmabuf);
 out:
 	drm_gem_object_unreference_unlocked(obj);
 out_unlock:
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/prime: cleanup goto mess in drm_gem_prime_handle_to_fd

[Daniel Vetter]

On Thu, Oct 15, 2015 at 11:51:39AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> I think I might have been responsible for some of this, but it's messy
> to decode, this doesn't change anything, but cleans up the goto's
> and exit paths.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>

Before coffee, but looks good. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_prime.c | 73 ++++++++++++++++++++-------------------------
>  1 file changed, 32 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 9f935f5..d22ce83 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -420,47 +420,42 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>  	if (dmabuf) {
>  		get_dma_buf(dmabuf);
> -		goto out_have_handle;
> -	}
> -
> -	mutex_lock(&dev->object_name_lock);
> -	/* re-export the original imported object */
> -	if (obj->import_attach) {
> -		dmabuf = obj->import_attach->dmabuf;
> -		get_dma_buf(dmabuf);
> -		goto out_have_obj;
> -	}
> -
> -	if (obj->dma_buf) {
> -		get_dma_buf(obj->dma_buf);
> -		dmabuf = obj->dma_buf;
> -		goto out_have_obj;
> -	}
> +	} else {
> +		mutex_lock(&dev->object_name_lock);
> +		/* re-export the original imported object */
> +		if (obj->import_attach) {
> +			dmabuf = obj->import_attach->dmabuf;
> +			get_dma_buf(dmabuf);
> +		} else if (obj->dma_buf) {
> +			get_dma_buf(obj->dma_buf);
> +			dmabuf = obj->dma_buf;
> +		} else {
> +			dmabuf = export_and_register_object(dev, obj, flags);
> +			if (IS_ERR(dmabuf)) {
> +				/* normally the created dma-buf takes ownership of the ref,
> +				 * but if that fails then drop the ref
> +				 */
> +				ret = PTR_ERR(dmabuf);
> +				mutex_unlock(&dev->object_name_lock);
> +				goto out;
> +			}
> +		}
>  
> -	dmabuf = export_and_register_object(dev, obj, flags);
> -	if (IS_ERR(dmabuf)) {
> -		/* normally the created dma-buf takes ownership of the ref,
> -		 * but if that fails then drop the ref
> +		/*
> +		 * If we've exported this buffer then cheat and add it to the import list
> +		 * so we get the correct handle back. We must do this under the
> +		 * protection of dev->object_name_lock to ensure that a racing gem close
> +		 * ioctl doesn't miss to remove this buffer handle from the cache.
>  		 */
> -		ret = PTR_ERR(dmabuf);
> +		ret = drm_prime_add_buf_handle(&file_priv->prime,
> +					       dmabuf, handle);
>  		mutex_unlock(&dev->object_name_lock);
> -		goto out;
> +		if (ret) {
> +			dma_buf_put(dmabuf);
> +			goto out;
> +		}
>  	}
>  
> -out_have_obj:
> -	/*
> -	 * If we've exported this buffer then cheat and add it to the import list
> -	 * so we get the correct handle back. We must do this under the
> -	 * protection of dev->object_name_lock to ensure that a racing gem close
> -	 * ioctl doesn't miss to remove this buffer handle from the cache.
> -	 */
> -	ret = drm_prime_add_buf_handle(&file_priv->prime,
> -				       dmabuf, handle);
> -	mutex_unlock(&dev->object_name_lock);
> -	if (ret)
> -		goto fail_put_dmabuf;
> -
> -out_have_handle:
>  	ret = dma_buf_fd(dmabuf, flags);
>  	/*
>  	 * We must _not_ remove the buffer from the handle cache since the newly
> @@ -469,16 +464,12 @@ out_have_handle:
>  	 * Closing the handle will clean out the cache anyway, so we don't leak.
>  	 */
>  	if (ret < 0) {
> -		goto fail_put_dmabuf;
> +		dma_buf_put(dmabuf);
>  	} else {
>  		*prime_fd = ret;
>  		ret = 0;
>  	}
>  
> -	goto out;
> -
> -fail_put_dmabuf:
> -	dma_buf_put(dmabuf);
>  out:
>  	drm_gem_object_unreference_unlocked(obj);
>  out_unlock:
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/gem: cleanup prime lock issue with drm_gem_handle_create_tail

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

This patch cleans up one locking order issue with a deadlock on
the prime lock.

It avoids calling the prime delete function from drm_gem_handle_create_tail,
and instead open codes the exit paths, it also splits the common code
out for the exit path which can use it.

This means the callee has to call the prime cleanup paths, which is fine
since this is only the prime code and it holds the lock already.

This is based on an patch/idea by Daniel Vetter, but I've cleaned it up
and reworked it.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_gem.c   | 55 +++++++++++++++++++++++++++------------------
 drivers/gpu/drm/drm_prime.c |  3 +++
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 12d0dc7..49354d7 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -239,14 +239,12 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 	drm_gem_object_unreference_unlocked(obj);
 }
 
+/* use for deletion and failure in allocation */
 static void
-drm_gem_handle_delete_tail(struct drm_file *filp,
-			   struct drm_gem_object *obj)
+drm_gem_handle_cleanup(struct drm_file *filp,
+		       struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
-
-	if (drm_core_check_feature(dev, DRIVER_PRIME))
-		drm_gem_remove_prime_handles(obj, filp);
 	drm_vma_node_revoke(&obj->vma_node, filp->filp);
 
 	if (dev->driver->gem_close_object)
@@ -254,6 +252,17 @@ drm_gem_handle_delete_tail(struct drm_file *filp,
 	drm_gem_object_handle_unreference_unlocked(obj);
 }
 
+static void
+drm_gem_handle_delete_tail_prime(struct drm_file *filp,
+				 struct drm_gem_object *obj)
+{
+	struct drm_device *dev = obj->dev;
+
+	if (drm_core_check_feature(dev, DRIVER_PRIME))
+		drm_gem_remove_prime_handles(obj, filp);
+	drm_gem_handle_cleanup(filp, obj);
+}
+
 /**
  * drm_gem_handle_delete - deletes the given file-private handle
  * @filp: drm file-private structure to use for the handle look up
@@ -291,7 +300,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 	idr_remove(&filp->object_idr, handle);
 	spin_unlock(&filp->table_lock);
 
-	drm_gem_handle_delete_tail(filp, obj);
+	drm_gem_handle_delete_tail_prime(filp, obj);
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_handle_delete);
@@ -333,6 +342,17 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 
 	WARN_ON(!mutex_is_locked(&dev->object_name_lock));
 
+	ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
+	if (ret) {
+		goto out_unlock;
+	}
+
+	if (dev->driver->gem_open_object) {
+		ret = dev->driver->gem_open_object(obj, file_priv);
+		if (ret)
+			goto out_vma;
+	}
+
 	/*
 	 * Get the user-visible handle using idr.  Preload and perform
 	 * allocation under our spinlock.
@@ -347,26 +367,17 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 	idr_preload_end();
 	mutex_unlock(&dev->object_name_lock);
 	if (ret < 0) {
-		drm_gem_object_handle_unreference_unlocked(obj);
+		drm_gem_handle_cleanup(file_priv, obj);
 		return ret;
 	}
 	*handlep = ret;
 
-	ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
-	if (ret) {
-		drm_gem_handle_delete(file_priv, *handlep);
-		return ret;
-	}
-
-	if (dev->driver->gem_open_object) {
-		ret = dev->driver->gem_open_object(obj, file_priv);
-		if (ret) {
-			drm_gem_handle_delete(file_priv, *handlep);
-			return ret;
-		}
-	}
-
 	return 0;
+out_vma:
+	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
+out_unlock:
+	mutex_unlock(&dev->object_name_lock);
+	return ret;
 }
 
 /**
@@ -717,7 +728,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 	struct drm_file *file_priv = data;
 	struct drm_gem_object *obj = ptr;
 
-	drm_gem_handle_delete_tail(file_priv, obj);
+	drm_gem_handle_delete_tail_prime(file_priv, obj);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d22ce83..88c004e 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -590,6 +590,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
 	/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */
 	ret = drm_gem_handle_create_tail(file_priv, obj, handle);
+	if (ret)
+		drm_prime_remove_buf_handle_locked(&file_priv->prime,
+						   obj->dma_buf);
 	drm_gem_object_unreference_unlocked(obj);
 	if (ret)
 		goto out_put;
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/gem: cleanup prime lock issue with drm_gem_handle_create_tail

[Daniel Vetter]

On Thu, Oct 15, 2015 at 11:51:40AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> This patch cleans up one locking order issue with a deadlock on
> the prime lock.
> 
> It avoids calling the prime delete function from drm_gem_handle_create_tail,
> and instead open codes the exit paths, it also splits the common code
> out for the exit path which can use it.
> 
> This means the callee has to call the prime cleanup paths, which is fine
> since this is only the prime code and it holds the lock already.
> 
> This is based on an patch/idea by Daniel Vetter, but I've cleaned it up
> and reworked it.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_gem.c   | 55 +++++++++++++++++++++++++++------------------
>  drivers/gpu/drm/drm_prime.c |  3 +++
>  2 files changed, 36 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 12d0dc7..49354d7 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -239,14 +239,12 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
>  	drm_gem_object_unreference_unlocked(obj);
>  }
>  
> +/* use for deletion and failure in allocation */
>  static void
> -drm_gem_handle_delete_tail(struct drm_file *filp,
> -			   struct drm_gem_object *obj)
> +drm_gem_handle_cleanup(struct drm_file *filp,
> +		       struct drm_gem_object *obj)
>  {
>  	struct drm_device *dev = obj->dev;
> -
> -	if (drm_core_check_feature(dev, DRIVER_PRIME))
> -		drm_gem_remove_prime_handles(obj, filp);
>  	drm_vma_node_revoke(&obj->vma_node, filp->filp);
>  
>  	if (dev->driver->gem_close_object)
> @@ -254,6 +252,17 @@ drm_gem_handle_delete_tail(struct drm_file *filp,
>  	drm_gem_object_handle_unreference_unlocked(obj);
>  }
>  
> +static void
> +drm_gem_handle_delete_tail_prime(struct drm_file *filp,
> +				 struct drm_gem_object *obj)
> +{
> +	struct drm_device *dev = obj->dev;
> +
> +	if (drm_core_check_feature(dev, DRIVER_PRIME))
> +		drm_gem_remove_prime_handles(obj, filp);
> +	drm_gem_handle_cleanup(filp, obj);
> +}
> +
>  /**
>   * drm_gem_handle_delete - deletes the given file-private handle
>   * @filp: drm file-private structure to use for the handle look up
> @@ -291,7 +300,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
>  	idr_remove(&filp->object_idr, handle);
>  	spin_unlock(&filp->table_lock);
>  
> -	drm_gem_handle_delete_tail(filp, obj);
> +	drm_gem_handle_delete_tail_prime(filp, obj);
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_handle_delete);
> @@ -333,6 +342,17 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  
>  	WARN_ON(!mutex_is_locked(&dev->object_name_lock));
>  
> +	ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> +	if (ret) {
> +		goto out_unlock;
> +	}
> +
> +	if (dev->driver->gem_open_object) {
> +		ret = dev->driver->gem_open_object(obj, file_priv);
> +		if (ret)
> +			goto out_vma;
> +	}
> +
>  	/*
>  	 * Get the user-visible handle using idr.  Preload and perform
>  	 * allocation under our spinlock.
> @@ -347,26 +367,17 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  	idr_preload_end();
>  	mutex_unlock(&dev->object_name_lock);
>  	if (ret < 0) {
> -		drm_gem_object_handle_unreference_unlocked(obj);
> +		drm_gem_handle_cleanup(file_priv, obj);
>  		return ret;
>  	}
>  	*handlep = ret;
>  
> -	ret = drm_vma_node_allow(&obj->vma_node, file_priv->filp);
> -	if (ret) {
> -		drm_gem_handle_delete(file_priv, *handlep);
> -		return ret;
> -	}
> -
> -	if (dev->driver->gem_open_object) {
> -		ret = dev->driver->gem_open_object(obj, file_priv);
> -		if (ret) {
> -			drm_gem_handle_delete(file_priv, *handlep);
> -			return ret;
> -		}
> -	}
> -
>  	return 0;
> +out_vma:
> +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> +out_unlock:
> +	mutex_unlock(&dev->object_name_lock);
> +	return ret;
>  }
>  
>  /**
> @@ -717,7 +728,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  	struct drm_file *file_priv = data;
>  	struct drm_gem_object *obj = ptr;
>  
> -	drm_gem_handle_delete_tail(file_priv, obj);
> +	drm_gem_handle_delete_tail_prime(file_priv, obj);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index d22ce83..88c004e 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -590,6 +590,9 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>  	/* drm_gem_handle_create_tail unlocks dev->object_name_lock. */
>  	ret = drm_gem_handle_create_tail(file_priv, obj, handle);
> +	if (ret)
> +		drm_prime_remove_buf_handle_locked(&file_priv->prime,
> +						   obj->dma_buf);

Why is this needed? We haven't yet added the handle the buffer lookup
table at this point. the create_handle_tail changes look good, so with
this hunk here removed the patch is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>  	drm_gem_object_unreference_unlocked(obj);
>  	if (ret)
>  		goto out_put;
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/prime: cleanup prime/gem locking interaction

[Dave Airlie]

From: Dave Airlie <airlied@redhat.com>

calling drm_gem_handle_delete would cause an attempt to retake
the prime lock.

move code around so we never need to do that. This patch allocates
the member structure early, so there is no failure path that
requires calling drm_gem_handle_delete.

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/drm_prime.c | 49 ++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 88c004e..6991398 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -71,20 +71,14 @@ struct drm_prime_attachment {
 	enum dma_data_direction dir;
 };
 
-static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
-				    struct dma_buf *dma_buf, uint32_t handle)
+static void drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
+				     struct drm_prime_member *member,
+				     struct dma_buf *dma_buf, uint32_t handle)
 {
-	struct drm_prime_member *member;
-
-	member = kmalloc(sizeof(*member), GFP_KERNEL);
-	if (!member)
-		return -ENOMEM;
-
 	get_dma_buf(dma_buf);
 	member->dma_buf = dma_buf;
 	member->handle = handle;
 	list_add(&member->entry, &prime_fpriv->head);
-	return 0;
 }
 
 static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv,
@@ -409,6 +403,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 	struct drm_gem_object *obj;
 	int ret = 0;
 	struct dma_buf *dmabuf;
+	struct drm_prime_member *member;
 
 	mutex_lock(&file_priv->prime.lock);
 	obj = drm_gem_object_lookup(dev, file_priv, handle);
@@ -417,6 +412,12 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		goto out_unlock;
 	}
 
+	member = kmalloc(sizeof(*member), GFP_KERNEL);
+	if (!member) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
 	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
 	if (dmabuf) {
 		get_dma_buf(dmabuf);
@@ -437,6 +438,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 				 */
 				ret = PTR_ERR(dmabuf);
 				mutex_unlock(&dev->object_name_lock);
+				kfree(member);
 				goto out;
 			}
 		}
@@ -447,13 +449,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
 		 * protection of dev->object_name_lock to ensure that a racing gem close
 		 * ioctl doesn't miss to remove this buffer handle from the cache.
 		 */
-		ret = drm_prime_add_buf_handle(&file_priv->prime,
-					       dmabuf, handle);
+		drm_prime_add_buf_handle(&file_priv->prime,
+					 member, dmabuf, handle);
 		mutex_unlock(&dev->object_name_lock);
-		if (ret) {
-			dma_buf_put(dmabuf);
-			goto out;
-		}
 	}
 
 	ret = dma_buf_fd(dmabuf, flags);
@@ -560,6 +558,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 {
 	struct dma_buf *dma_buf;
 	struct drm_gem_object *obj;
+	struct drm_prime_member *member;
 	int ret;
 
 	dma_buf = dma_buf_get(prime_fd);
@@ -573,6 +572,11 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 	if (ret == 0)
 		goto out_put;
 
+	member = kmalloc(sizeof(*member), GFP_KERNEL);
+	if (!member) {
+		ret = -ENOMEM;
+		goto out_put;
+	}
 	/* never seen this one, need to import */
 	mutex_lock(&dev->object_name_lock);
 	obj = dev->driver->gem_prime_import(dev, dma_buf);
@@ -595,12 +599,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 						   obj->dma_buf);
 	drm_gem_object_unreference_unlocked(obj);
 	if (ret)
-		goto out_put;
+		goto out_member;
 
-	ret = drm_prime_add_buf_handle(&file_priv->prime,
-			dma_buf, *handle);
-	if (ret)
-		goto fail;
+	drm_prime_add_buf_handle(&file_priv->prime,
+				 member, dma_buf, *handle);
 
 	mutex_unlock(&file_priv->prime.lock);
 
@@ -608,13 +610,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
 
 	return 0;
 
-fail:
-	/* hmm, if driver attached, we are relying on the free-object path
-	 * to detach.. which seems ok..
-	 */
-	drm_gem_handle_delete(file_priv, *handle);
 out_unlock:
 	mutex_unlock(&dev->object_name_lock);
+out_member:
+	kfree(member);
 out_put:
 	dma_buf_put(dma_buf);
 	mutex_unlock(&file_priv->prime.lock);
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/prime: cleanup prime/gem locking interaction

[Daniel Vetter]

On Thu, Oct 15, 2015 at 11:51:41AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> calling drm_gem_handle_delete would cause an attempt to retake
> the prime lock.
> 
> move code around so we never need to do that. This patch allocates
> the member structure early, so there is no failure path that
> requires calling drm_gem_handle_delete.
> 
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 49 ++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 88c004e..6991398 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -71,20 +71,14 @@ struct drm_prime_attachment {
>  	enum dma_data_direction dir;
>  };
>  
> -static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> -				    struct dma_buf *dma_buf, uint32_t handle)
> +static void drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> +				     struct drm_prime_member *member,
> +				     struct dma_buf *dma_buf, uint32_t handle)
>  {
> -	struct drm_prime_member *member;
> -
> -	member = kmalloc(sizeof(*member), GFP_KERNEL);
> -	if (!member)
> -		return -ENOMEM;
> -
>  	get_dma_buf(dma_buf);
>  	member->dma_buf = dma_buf;
>  	member->handle = handle;
>  	list_add(&member->entry, &prime_fpriv->head);
> -	return 0;
>  }
>  
>  static struct dma_buf *drm_prime_lookup_buf_by_handle(struct drm_prime_file_private *prime_fpriv,
> @@ -409,6 +403,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  	struct drm_gem_object *obj;
>  	int ret = 0;
>  	struct dma_buf *dmabuf;
> +	struct drm_prime_member *member;
>  
>  	mutex_lock(&file_priv->prime.lock);
>  	obj = drm_gem_object_lookup(dev, file_priv, handle);
> @@ -417,6 +412,12 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  		goto out_unlock;
>  	}
>  
> +	member = kmalloc(sizeof(*member), GFP_KERNEL);
> +	if (!member) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
>  	dmabuf = drm_prime_lookup_buf_by_handle(&file_priv->prime, handle);
>  	if (dmabuf) {
>  		get_dma_buf(dmabuf);
> @@ -437,6 +438,7 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  				 */
>  				ret = PTR_ERR(dmabuf);
>  				mutex_unlock(&dev->object_name_lock);
> +				kfree(member);
>  				goto out;
>  			}
>  		}
> @@ -447,13 +449,9 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>  		 * protection of dev->object_name_lock to ensure that a racing gem close
>  		 * ioctl doesn't miss to remove this buffer handle from the cache.
>  		 */
> -		ret = drm_prime_add_buf_handle(&file_priv->prime,
> -					       dmabuf, handle);
> +		drm_prime_add_buf_handle(&file_priv->prime,
> +					 member, dmabuf, handle);
>  		mutex_unlock(&dev->object_name_lock);
> -		if (ret) {
> -			dma_buf_put(dmabuf);
> -			goto out;
> -		}
>  	}
>  
>  	ret = dma_buf_fd(dmabuf, flags);
> @@ -560,6 +558,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  {
>  	struct dma_buf *dma_buf;
>  	struct drm_gem_object *obj;
> +	struct drm_prime_member *member;
>  	int ret;
>  
>  	dma_buf = dma_buf_get(prime_fd);
> @@ -573,6 +572,11 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  	if (ret == 0)
>  		goto out_put;
>  
> +	member = kmalloc(sizeof(*member), GFP_KERNEL);
> +	if (!member) {
> +		ret = -ENOMEM;
> +		goto out_put;
> +	}
>  	/* never seen this one, need to import */
>  	mutex_lock(&dev->object_name_lock);
>  	obj = dev->driver->gem_prime_import(dev, dma_buf);
> @@ -595,12 +599,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  						   obj->dma_buf);
>  	drm_gem_object_unreference_unlocked(obj);
>  	if (ret)
> -		goto out_put;
> +		goto out_member;
>  
> -	ret = drm_prime_add_buf_handle(&file_priv->prime,
> -			dma_buf, *handle);
> -	if (ret)
> -		goto fail;
> +	drm_prime_add_buf_handle(&file_priv->prime,
> +				 member, dma_buf, *handle);

	member = NULL;
>  
>  	mutex_unlock(&file_priv->prime.lock);
>  
> @@ -608,13 +610,10 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>  
>  	return 0;
>  
> -fail:
> -	/* hmm, if driver attached, we are relying on the free-object path
> -	 * to detach.. which seems ok..
> -	 */
> -	drm_gem_handle_delete(file_priv, *handle);
>  out_unlock:
>  	mutex_unlock(&dev->object_name_lock);
> +out_member:
> +	kfree(member);
>  out_put:

and put the kfree (since it can deal with NULL) below the out_put here as
a random bikeshed. Anyway, looks good as is.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	dma_buf_put(dma_buf);
>  	mutex_unlock(&file_priv->prime.lock);
> -- 
> 2.5.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel