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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
