* [PATCH] drm/gem: fix not to assign error value to gem name @ 2013-06-26 1:42 Seung-Woo Kim 2013-06-26 8:55 ` Chris Wilson 0 siblings, 1 reply; 7+ messages in thread From: Seung-Woo Kim @ 2013-06-26 1:42 UTC (permalink / raw) To: dri-devel, airlied; +Cc: kyungmin.park, sw0312.kim, yj44.cho From: YoungJun Cho <yj44.cho@samsung.com> If idr_alloc() is failed, obj->name can be error value. Also it cleans up duplicated flink processing code. Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/gpu/drm/drm_gem.c | 18 +++++++----------- 1 files changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index cf919e3..239ef30 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; - ret = 0; + + obj->name = ret; /* Allocate a reference for the name table. */ drm_gem_object_reference(obj); - } else { - args->name = (uint64_t) obj->name; - spin_unlock(&dev->object_name_lock); - idr_preload_end(); - ret = 0; } + args->name = (uint64_t) obj->name; + ret = 0; + err: + spin_unlock(&dev->object_name_lock); + idr_preload_end(); drm_gem_object_unreference_unlocked(obj); return ret; } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/gem: fix not to assign error value to gem name 2013-06-26 1:42 [PATCH] drm/gem: fix not to assign error value to gem name Seung-Woo Kim @ 2013-06-26 8:55 ` Chris Wilson 2013-06-26 9:07 ` YoungJun Cho 2013-06-26 23:58 ` [PATCH v2] " Seung-Woo Kim 0 siblings, 2 replies; 7+ messages in thread From: Chris Wilson @ 2013-06-26 8:55 UTC (permalink / raw) To: Seung-Woo Kim; +Cc: kyungmin.park, yj44.cho, dri-devel On Wed, Jun 26, 2013 at 10:42:39AM +0900, Seung-Woo Kim wrote: > From: YoungJun Cho <yj44.cho@samsung.com> > > If idr_alloc() is failed, obj->name can be error value. Also > it cleans up duplicated flink processing code. You should mention that it is a regression from commit 2e928815c (drm: convert to idr_alloc()) > Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/gem: fix not to assign error value to gem name 2013-06-26 8:55 ` Chris Wilson @ 2013-06-26 9:07 ` YoungJun Cho 2013-06-26 23:58 ` [PATCH v2] " Seung-Woo Kim 1 sibling, 0 replies; 7+ messages in thread From: YoungJun Cho @ 2013-06-26 9:07 UTC (permalink / raw) To: Chris Wilson; +Cc: kyungmin.park, Seung-Woo Kim, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 942 bytes --] On Jun 26, 2013 5:56 PM, "Chris Wilson" <chris@chris-wilson.co.uk> wrote: > > On Wed, Jun 26, 2013 at 10:42:39AM +0900, Seung-Woo Kim wrote: > > From: YoungJun Cho <yj44.cho@samsung.com> > > > > If idr_alloc() is failed, obj->name can be error value. Also > > it cleans up duplicated flink processing code. > > You should mention that it is a regression from > commit 2e928815c (drm: convert to idr_alloc()) > > > Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> > > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > Thank you for comments! I'll update commit msg. Best regards YJ _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel [-- Attachment #1.2: Type: text/html, Size: 1635 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] drm/gem: fix not to assign error value to gem name 2013-06-26 8:55 ` Chris Wilson 2013-06-26 9:07 ` YoungJun Cho @ 2013-06-26 23:58 ` Seung-Woo Kim 2013-06-27 8:31 ` Chris Wilson 1 sibling, 1 reply; 7+ messages in thread From: Seung-Woo Kim @ 2013-06-26 23:58 UTC (permalink / raw) To: dri-devel, airlied; +Cc: kyungmin.park, sw0312.kim, yj44.cho From: YoungJun Cho <yj44.cho@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 <tj@kernel.org> Date: Wed Feb 27 17:04:08 2013 -0800 drm: convert to idr_alloc() Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- 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; - ret = 0; + + obj->name = ret; /* Allocate a reference for the name table. */ drm_gem_object_reference(obj); - } else { - args->name = (uint64_t) obj->name; - spin_unlock(&dev->object_name_lock); - idr_preload_end(); - ret = 0; } + args->name = (uint64_t) obj->name; + ret = 0; + err: + spin_unlock(&dev->object_name_lock); + idr_preload_end(); drm_gem_object_unreference_unlocked(obj); return ret; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/gem: fix not to assign error value to gem name 2013-06-26 23:58 ` [PATCH v2] " Seung-Woo Kim @ 2013-06-27 8:31 ` Chris Wilson 2013-06-28 4:15 ` 김승우 0 siblings, 1 reply; 7+ messages in thread From: Chris Wilson @ 2013-06-27 8:31 UTC (permalink / raw) To: Seung-Woo Kim; +Cc: kyungmin.park, yj44.cho, dri-devel On Thu, Jun 27, 2013 at 08:58:33AM +0900, Seung-Woo Kim wrote: > From: YoungJun Cho <yj44.cho@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 <tj@kernel.org> > Date: Wed Feb 27 17:04:08 2013 -0800 > > drm: convert to idr_alloc() > > Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> > Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Reviewed-by: Chris Wilson <chris@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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/gem: fix not to assign error value to gem name 2013-06-27 8:31 ` Chris Wilson @ 2013-06-28 4:15 ` 김승우 2013-06-28 8:08 ` Chris Wilson 0 siblings, 1 reply; 7+ messages in thread From: 김승우 @ 2013-06-28 4:15 UTC (permalink / raw) To: Chris Wilson, dri-devel, airlied, kyungmin.park, yj44.cho, Seung-Woo Kim 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.cho@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 <tj@kernel.org> >> Date: Wed Feb 27 17:04:08 2013 -0800 >> >> drm: convert to idr_alloc() >> >> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com> >> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Reviewed-by: Chris Wilson <chris@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 R&D Center -- _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/gem: fix not to assign error value to gem name 2013-06-28 4:15 ` 김승우 @ 2013-06-28 8:08 ` Chris Wilson 0 siblings, 0 replies; 7+ messages in thread From: Chris Wilson @ 2013-06-28 8:08 UTC (permalink / raw) To: 김승우; +Cc: kyungmin.park, yj44.cho, dri-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-06-28 8:08 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-26 1:42 [PATCH] drm/gem: fix not to assign error value to gem name Seung-Woo Kim 2013-06-26 8:55 ` Chris Wilson 2013-06-26 9:07 ` YoungJun Cho 2013-06-26 23:58 ` [PATCH v2] " Seung-Woo Kim 2013-06-27 8:31 ` Chris Wilson 2013-06-28 4:15 ` 김승우 2013-06-28 8:08 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.