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

On Mon, Sep 5, 2016 at 3:00 AM, Huang Rui ray.huang@amd.com wrote:

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

drivers/gpu/drm/drm_global.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c index 3d2e91c..3abe738 100644 --- a/drivers/gpu/drm/drm_global.c +++ b/drivers/gpu/drm/drm_global.c @@ -70,25 +70,25 @@ int drm_global_item_ref(struct drm_global_reference *ref)

    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;
            }
  

•           ref->object = item->object;
            ret = ref->init(ref);
            if (unlikely(ret != 0))
                    goto out_err;
  
  

•   }
  

•           item->object = ref->object;
  

•   } else
  

•           ref->object = item->object;
  

You should add the braces to all blocks if they're present in any block.

    ++item->refcount;

•   ref->object = item->object;
    mutex_unlock(&item->mutex);
    return 0;
  

out_err: mutex_unlock(&item->mutex);

•   item->object = NULL;
  

•   ref->object = NULL;
  

It seems like you're leaking memory here. Do you need a kfree(ref->object) before this?

I think perhaps you can simplify (or maybe streamline is a better word) things better by unifying the unlock and return. Something like:

diff --git a/drivers/gpu/drm/drm_global.c b/drivers/gpu/drm/drm_global.c index 3d2e91c..5b255e4 100644 --- a/drivers/gpu/drm/drm_global.c +++ b/drivers/gpu/drm/drm_global.c @@ -65,30 +65,31 @@ 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; - + } else { + ref->object = item->object; } - ++item->refcount; - ref->object = item->object; - mutex_unlock(&item->mutex); - return 0; + + item->refcount++; + goto out; + out_err: + kfree(ref->object); + ref->object = NULL; +out: mutex_unlock(&item->mutex); - item->object = NULL; return ret; } EXPORT_SYMBOL(drm_global_item_ref);

    return ret;

} EXPORT_SYMBOL(drm_global_item_ref); -- 2.7.4

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel