| Message ID | CAOw6vbKxmKmdJrMytkbTvkK7XFQmOgVuswOF-FV8QtwtuHLdTw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 06, 2016 at 10:33:18PM +0800, Sean Paul wrote: > 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? Yep, right. The original code seems to missed a kfree exceptional handle. > > I think perhaps you can simplify (or maybe streamline is a better > word) things better by unifying the unlock and return. Something like: > Thanks, will update it in V2. Rui
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);