All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.