On Thu, Oct 15, 2015 at 11:51:41AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxxxx>
> dma_buf_put(dma_buf);
> mutex_unlock(&file_priv->prime.lock);
> --
> 2.5.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
