Re: [Updated PATCH v2] drm: modify drm_global_item_ref to avoid two times of writing ref->object

[Huang Rui <ray.huang-5C7GfCeVMHo@public.gmane.org>]

On Wed, Sep 07, 2016 at 09:12:29AM +0200, Christian König wrote:
> Am 07.09.2016 um 07:24 schrieb Huang Rui:
> >In previous drm_global_item_ref, there are two times of writing
> >ref->object if item->refcount is 0. So this patch does a minor update
> >to put alloc and init ref firstly, and then to modify the item of glob
> >array. Use "else" to avoid two times of writing ref->object. It can
> >make the code logic more clearly.
> >
> >Signed-off-by: Huang Rui <ray.huang@amd.com>
> 
> Well when you update your patch, even when it's just fixing a small
> typo, please increase the version number.
> 
> That makes it much easier to track the different instances of a patch.
> 

OK.

> A few additional notes below.
> 
> >---
> >
> >Changes from V1 -> V2:
> >- Add kfree exceptional handle to avoid memory leak.
> >- Improve code style.
> >
> >---
> >  drivers/gpu/drm/drm_global.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c
> >index 3d2e91c..b181e81 100644
> >--- a/drivers/gpu/drm/drm_global.c
> >+++ b/drivers/gpu/drm/drm_global.c
> >@@ -65,30 +65,33 @@ void drm_global_release(void)
> >  int drm_global_item_ref(struct drm_global_reference *ref)
> >  {
> >-	int ret;
> >+	int ret = 0;
> >  	struct drm_global_item *item = &glob[ref->global_type];
> >  	mutex_lock(&item->mutex);
> >  	if (item->refcount == 0) {
> >-		item->object = kzalloc(ref->size, GFP_KERNEL);
> >-		if (unlikely(item->object == NULL)) {
> >+		ref->object = kzalloc(ref->size, GFP_KERNEL);
> >+		if (unlikely(ref->object == NULL)) {
> >  			ret = -ENOMEM;
> >-			goto out_err;
> >+			goto out;
> >  		}
> >-
> >-		ref->object = item->object;
> >  		ret = ref->init(ref);
> >  		if (unlikely(ret != 0))
> >  			goto out_err;
> >+		item->object = ref->object;
> >+	} else {
> >+		ref->object = item->object;
> >  	}
> >+
> >  	++item->refcount;
> >-	ref->object = item->object;
> >-	mutex_unlock(&item->mutex);
> >-	return 0;
> >+	goto out;
> 
> A goto in the not error path is a bit unusual. Since out_err is only
> used once couldn't you just move the code into the if and then goto
> to out as well?
> 
> Alternative you could just duplicate the mutex release in the
> non-error path.
> 

Actually, I also have a little concern with "goto" in non-error path. But
as Sean's suggestion, use "goto" can avoid a duplicate mutex release, you
know.

@Sean, if you don't have concern with adding a mutex release below, I will
update it in V3.

---
        ++item->refcount;
        mutex_unlock(&item->mutex);
        return 0;

out_err:
        kfree(ref->object);
        ref->object = NULL;
out:
        mutex_unlock(&item->mutex);
        return ret;
---


Thanks,
Rui
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx