Am 07.09.2016 um 09:54 schrieb Huang Rui:
> 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 at 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.
I would certainly prefer the duplicate mutex release.
>
> @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:
BTW: I would name those labels error_free and error_unlock to make clear
that they are for error handling and what they are supposed to do.
Regards,
Christian.
> mutex_unlock(&item->mutex);
> return ret;
> ---
>
>
> Thanks,
> Rui
