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 -- Chris Wilson, Intel Open Source Technology Centre
