Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper
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 -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper
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 -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/3] drm: fix error routines in drm_open_helper
Hello Chris, On Jul 1, 2013 8:53 PM, Chris Wilson ch...@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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
