diff mbox

[1/3] drm: Balance error path for GEM handle allocation

Message ID 1451902261-25380-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson Jan. 4, 2016, 10:10 a.m. UTC
The current error path for failure when establishing a handle for a GEM
object is unbalance, e.g. we call object_close() without calling first
object_open(). Use the typical onion structure to only undo what has
been set up prior to the error.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

Chris Wilson Jan. 4, 2016, 10:18 a.m. UTC | #1
On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> The current error path for failure when establishing a handle for a GEM
> object is unbalance, e.g. we call object_close() without calling first
> object_open(). Use the typical onion structure to only undo what has
> been set up prior to the error.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e10bba4468b..a08176debc0e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  	spin_unlock(&file_priv->table_lock);
>  	idr_preload_end();
>  	mutex_unlock(&dev->object_name_lock);
> -	if (ret < 0) {
> -		drm_gem_object_handle_unreference_unlocked(obj);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto err_unref;
> +
>  	*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 (ret)
> +		goto err_remove;
>  
>  	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;
> -		}
> +		if (ret)
> +			goto err_revoke;
>  	}
>  
>  	return 0;
> +
> +err_revoke:
> +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> +err_remove:
> +	spin_lock(&file_priv->table_lock);
> +	idr_remove(&file_priv->object_idr, *handlep);

For bonus points, we could *handlep = 0; here.

> +	spin_unlock(&file_priv->table_lock);
> +err_unref:
> +	drm_gem_object_handle_unreference_unlocked(obj);
> +	return ret;
>  }
Ville Syrjala Jan. 4, 2016, 3:24 p.m. UTC | #2
On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> The current error path for failure when establishing a handle for a GEM
> object is unbalance, e.g. we call object_close() without calling first
> object_open(). Use the typical onion structure to only undo what has
> been set up prior to the error.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2e10bba4468b..a08176debc0e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  	spin_unlock(&file_priv->table_lock);
>  	idr_preload_end();
>  	mutex_unlock(&dev->object_name_lock);
> -	if (ret < 0) {
> -		drm_gem_object_handle_unreference_unlocked(obj);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto err_unref;
> +
>  	*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 (ret)
> +		goto err_remove;
>  
>  	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;
> -		}
> +		if (ret)
> +			goto err_revoke;
>  	}
>  
>  	return 0;
> +
> +err_revoke:
> +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> +err_remove:
> +	spin_lock(&file_priv->table_lock);
> +	idr_remove(&file_priv->object_idr, *handlep);
> +	spin_unlock(&file_priv->table_lock);
> +err_unref:
> +	drm_gem_object_handle_unreference_unlocked(obj);
> +	return ret;

First I misread this as drm_gem_object_unreference_unlocked()
and though we'd leak the handle_count++, but it's
drm_gem_object_handle_unreference_unlocked() which does the
handle_count-- we need.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  /**
> -- 
> 2.6.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjala Jan. 4, 2016, 3:33 p.m. UTC | #3
On Mon, Jan 04, 2016 at 10:18:50AM +0000, Chris Wilson wrote:
> On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > The current error path for failure when establishing a handle for a GEM
> > object is unbalance, e.g. we call object_close() without calling first
> > object_open(). Use the typical onion structure to only undo what has
> > been set up prior to the error.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e10bba4468b..a08176debc0e 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> >  	spin_unlock(&file_priv->table_lock);
> >  	idr_preload_end();
> >  	mutex_unlock(&dev->object_name_lock);
> > -	if (ret < 0) {
> > -		drm_gem_object_handle_unreference_unlocked(obj);
> > -		return ret;
> > -	}
> > +	if (ret < 0)
> > +		goto err_unref;
> > +
> >  	*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 (ret)
> > +		goto err_remove;
> >  
> >  	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;
> > -		}
> > +		if (ret)
> > +			goto err_revoke;
> >  	}
> >  
> >  	return 0;
> > +
> > +err_revoke:
> > +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > +err_remove:
> > +	spin_lock(&file_priv->table_lock);
> > +	idr_remove(&file_priv->object_idr, *handlep);
> 
> For bonus points, we could *handlep = 0; here.

For these sort of things I usually prefer to not clobber any
return parameters unless the whole operation suceeds.
 
> 
> > +	spin_unlock(&file_priv->table_lock);
> > +err_unref:
> > +	drm_gem_object_handle_unreference_unlocked(obj);
> > +	return ret;
> >  }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjala Jan. 4, 2016, 3:37 p.m. UTC | #4
On Mon, Jan 04, 2016 at 05:24:11PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > The current error path for failure when establishing a handle for a GEM
> > object is unbalance, e.g. we call object_close() without calling first
> > object_open(). Use the typical onion structure to only undo what has
> > been set up prior to the error.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 2e10bba4468b..a08176debc0e 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> >  	spin_unlock(&file_priv->table_lock);
> >  	idr_preload_end();
> >  	mutex_unlock(&dev->object_name_lock);
> > -	if (ret < 0) {
> > -		drm_gem_object_handle_unreference_unlocked(obj);
> > -		return ret;
> > -	}
> > +	if (ret < 0)
> > +		goto err_unref;
> > +
> >  	*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 (ret)
> > +		goto err_remove;
> >  
> >  	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;
> > -		}
> > +		if (ret)
> > +			goto err_revoke;
> >  	}
> >  
> >  	return 0;
> > +
> > +err_revoke:
> > +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > +err_remove:
> > +	spin_lock(&file_priv->table_lock);
> > +	idr_remove(&file_priv->object_idr, *handlep);
> > +	spin_unlock(&file_priv->table_lock);
> > +err_unref:
> > +	drm_gem_object_handle_unreference_unlocked(obj);
> > +	return ret;
> 
> First I misread this as drm_gem_object_unreference_unlocked()
> and though we'd leak the handle_count++, but it's
> drm_gem_object_handle_unreference_unlocked() which does the
> handle_count-- we need.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

BTW while looking at the code I spotted that drm_gem_handle_delete()
contains an open coded copy of drm_gem_object_release_handle(). Might
make sense to eliminate that duplication.

> 
> >  }
> >  
> >  /**
> > -- 
> > 2.6.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Jan. 4, 2016, 3:37 p.m. UTC | #5
On Mon, Jan 04, 2016 at 05:33:45PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 04, 2016 at 10:18:50AM +0000, Chris Wilson wrote:
> > On Mon, Jan 04, 2016 at 10:10:59AM +0000, Chris Wilson wrote:
> > > The current error path for failure when establishing a handle for a GEM
> > > object is unbalance, e.g. we call object_close() without calling first
> > > object_open(). Use the typical onion structure to only undo what has
> > > been set up prior to the error.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/drm_gem.c | 29 +++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 2e10bba4468b..a08176debc0e 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -343,27 +343,32 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> > >  	spin_unlock(&file_priv->table_lock);
> > >  	idr_preload_end();
> > >  	mutex_unlock(&dev->object_name_lock);
> > > -	if (ret < 0) {
> > > -		drm_gem_object_handle_unreference_unlocked(obj);
> > > -		return ret;
> > > -	}
> > > +	if (ret < 0)
> > > +		goto err_unref;
> > > +
> > >  	*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 (ret)
> > > +		goto err_remove;
> > >  
> > >  	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;
> > > -		}
> > > +		if (ret)
> > > +			goto err_revoke;
> > >  	}
> > >  
> > >  	return 0;
> > > +
> > > +err_revoke:
> > > +	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
> > > +err_remove:
> > > +	spin_lock(&file_priv->table_lock);
> > > +	idr_remove(&file_priv->object_idr, *handlep);
> > 
> > For bonus points, we could *handlep = 0; here.
> 
> For these sort of things I usually prefer to not clobber any
> return parameters unless the whole operation suceeds.

True, resetting handlep back to the INVALID_HANDLE value was just a step
in the right direction ;-)
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2e10bba4468b..a08176debc0e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -343,27 +343,32 @@  drm_gem_handle_create_tail(struct drm_file *file_priv,
 	spin_unlock(&file_priv->table_lock);
 	idr_preload_end();
 	mutex_unlock(&dev->object_name_lock);
-	if (ret < 0) {
-		drm_gem_object_handle_unreference_unlocked(obj);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_unref;
+
 	*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 (ret)
+		goto err_remove;
 
 	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;
-		}
+		if (ret)
+			goto err_revoke;
 	}
 
 	return 0;
+
+err_revoke:
+	drm_vma_node_revoke(&obj->vma_node, file_priv->filp);
+err_remove:
+	spin_lock(&file_priv->table_lock);
+	idr_remove(&file_priv->object_idr, *handlep);
+	spin_unlock(&file_priv->table_lock);
+err_unref:
+	drm_gem_object_handle_unreference_unlocked(obj);
+	return ret;
 }
 
 /**