| Message ID | 1451902261-25380-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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
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
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
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 --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; } /**
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(-)