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

[Christian König]

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