| Message ID | 1372675750-21312-1-git-send-email-sw0312.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); put_pid(file_priv->pid); after out_free. -Chris
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(). > > put_pid(file_priv->pid); Yes, you are rignt. put_pid is also needed. After discussion about above part, I'll post v3 for this. Thanks and Regards, - Seung-Woo Kim > > after out_free. > -Chris >
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
Hello Chris, On Jul 1, 2013 8:53 PM, "Chris Wilson" <chris@chris-wilson.co.uk> 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 > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 429e07d..0470261 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -271,6 +271,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, priv->uid = current_euid(); priv->pid = get_pid(task_pid(current)); priv->minor = idr_find(&drm_minors_idr, minor_id); + if (!priv->minor) { + ret = -ENODEV; + goto out_free; + } + priv->ioctl_count = 0; /* for compatibility root is always authenticated */ priv->authenticated = capable(CAP_SYS_ADMIN); @@ -304,7 +309,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, if (!priv->minor->master) { mutex_unlock(&dev->struct_mutex); ret = -ENOMEM; - goto out_free; + goto out_close; } priv->is_master = 1; @@ -322,7 +327,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_lock(&dev->struct_mutex); @@ -333,7 +338,7 @@ static int drm_open_helper(struct inode *inode, struct file *filp, drm_master_put(&priv->minor->master); drm_master_put(&priv->master); mutex_unlock(&dev->struct_mutex); - goto out_free; + goto out_close; } } mutex_unlock(&dev->struct_mutex); @@ -367,7 +372,11 @@ static int drm_open_helper(struct inode *inode, struct file *filp, #endif return 0; - out_free: + +out_close: + if (dev->driver->postclose) + dev->driver->postclose(dev, priv); +out_free: kfree(priv); filp->private_data = NULL; return ret;