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@xxxxxxx>
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
