Re: [PATCH v2] drm/gem: fix not to assign error value to gem name
On Fri, Jun 28, 2013 at 01:15:43PM +0900, 김승우 wrote: Being pedantic, ret == 0 is also an error - but a programming error leading to an object leak. BUG_ON(ret == 0) ? It seems that idr_alloc() with start id 1 does not return 0, so IMHO, ret == 0 can be ignored here. Yes, it is an impossible condition, hence the suggestion of a BUG_ON(). It is a paranoid check for whether idr_alloc() fails. But if you think it needs to consider programming error, I'll add some assertion. Please let me know. Successfully setting obj-name = 0 would lead to interesting confusion in userspace, and very difficult to debug. Given that, maybe it would be better to BUG in the kernel. If you do feel like adding it, submit it as a separate patch, so that it is not caught up with the bugfix. -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] drm/gem: fix not to assign error value to gem name
On Thu, Jun 27, 2013 at 08:58:33AM +0900, Seung-Woo Kim wrote: From: YoungJun Cho yj44@samsung.com If idr_alloc() is failed, obj-name can be error value. Also it cleans up duplicated flink processing code. This regression has been introduced in commit 2e928815c1886fe628ed54623aa98d0889cf5509 Author: Tejun Heo t...@kernel.org Date: Wed Feb 27 17:04:08 2013 -0800 drm: convert to idr_alloc() Signed-off-by: YoungJun Cho yj44@samsung.com Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Minor comment inline. --- change since v1: - Add a regression commit information in commit msg as Chris commented drivers/gpu/drm/drm_gem.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4321713..c9d7081 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -453,25 +453,21 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, spin_lock(dev-object_name_lock); if (!obj-name) { ret = idr_alloc(dev-object_name_idr, obj, 1, 0, GFP_NOWAIT); - obj-name = ret; - args-name = (uint64_t) obj-name; - spin_unlock(dev-object_name_lock); - idr_preload_end(); - if (ret 0) goto err; Being pedantic, ret == 0 is also an error - but a programming error leading to an object leak. BUG_ON(ret == 0) ? -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] drm/gem: fix not to assign error value to gem name
Hello, Chris, On 2013년 06월 27일 17:31, Chris Wilson wrote: On Thu, Jun 27, 2013 at 08:58:33AM +0900, Seung-Woo Kim wrote: From: YoungJun Cho yj44@samsung.com If idr_alloc() is failed, obj-name can be error value. Also it cleans up duplicated flink processing code. This regression has been introduced in commit 2e928815c1886fe628ed54623aa98d0889cf5509 Author: Tejun Heo t...@kernel.org Date: Wed Feb 27 17:04:08 2013 -0800 drm: convert to idr_alloc() Signed-off-by: YoungJun Cho yj44@samsung.com Signed-off-by: Seung-Woo Kim sw0312@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Minor comment inline. --- change since v1: - Add a regression commit information in commit msg as Chris commented drivers/gpu/drm/drm_gem.c | 18 +++--- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 4321713..c9d7081 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -453,25 +453,21 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data, spin_lock(dev-object_name_lock); if (!obj-name) { ret = idr_alloc(dev-object_name_idr, obj, 1, 0, GFP_NOWAIT); -obj-name = ret; -args-name = (uint64_t) obj-name; -spin_unlock(dev-object_name_lock); -idr_preload_end(); - if (ret 0) goto err; Being pedantic, ret == 0 is also an error - but a programming error leading to an object leak. BUG_ON(ret == 0) ? -Chris It seems that idr_alloc() with start id 1 does not return 0, so IMHO, ret == 0 can be ignored here. But if you think it needs to consider programming error, I'll add some assertion. Please let me know. Thanks and Regards, - Seung-Woo Kim -- Seung-Woo Kim Samsung Software RD Center -- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel