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

[Daniel Vetter <daniel@ffwll.ch>]

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