Hello Chris, On Jul 1, 2013 8:53 PM, "Chris Wilson" <<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> wrote: > > On Mon, Jul 01, 2013 at 08:14:42PM +0900, Seung-Woo Kim wrote: > > Hello Chris, > > > > On 2013Гт 07Пљ 01РЯ 19:57, Chris Wilson wrote: > > > On Mon, Jul 01, 2013 at 07:49:10PM +0900, Seung-Woo Kim wrote: > > >> + > > >> +out_close: > > >> + if (dev->driver->postclose) > > >> + dev->driver->postclose(dev, priv); > > >> +out_free: > > >> kfree(priv); > > >> filp->private_data = NULL; > > >> return ret; > > > > > > Looks like we are also missing: > > > > > > if (drm_core_check_feature(dev, DRIVER_PRIME)) > > > drm_prime_destroy_file_private(&file_priv->prime); > > > > Currently, file_priv->prime is just initialized, and > > drm_prime_destroy_file_private() just checks the list is empty and at > > the open time, prime list is always empty. So IMHO, it seems unnecessary > > to call drm_prime_destroy_file_private(). > > > > If this is necessary, drm_gem_release() is also needed because the pair > > function of drm_gem_open() is drm_gem_release(). > > Hmm, could be a slight ordering bug in drm_release(), but yes I agree > that we should also call drm_gem_release() here in drm_open_helper. It > is better for the code to be symmetric in cleaning up anything that we > create so that we are correct for future changes. We might as well fix it > correctly, rather than having to rediscover this bug at some later date. > -Chris > Thank you for quick reviews. We'll post V3 patch with this also. Best regards YJ > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > dri-devel mailing list > <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a>