Re: [PATCH] drm/drm_gem.c: remove surplus else after return clause

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3014 bytes --]

Hi

Am 20.06.23 um 18:06 schrieb Sui Jingfeng:
> Hi,
> 
> On 2023/6/20 22:43, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 20.06.23 um 06:08 schrieb Sui Jingfeng:
>>> ping ?
>>>
>>> On 2023/3/14 20:53, Sui Jingfeng wrote:
>>>>   else is not generally useful after return
>>
>> No indention please.
>>
> OK, will be fixed at the next version.
>>>>
>>>> Signed-off-by: Sui Jingfeng <15330273260@189.cn>
>>>> ---
>>>>   drivers/gpu/drm/drm_gem.c | 7 ++++---
>>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>>> index a6208e2c089b..364e3733af98 100644
>>>> --- a/drivers/gpu/drm/drm_gem.c
>>>> +++ b/drivers/gpu/drm/drm_gem.c
>>>> @@ -1150,8 +1150,8 @@ int drm_gem_pin(struct drm_gem_object *obj)
>>>>   {
>>>>       if (obj->funcs->pin)
>>>>           return obj->funcs->pin(obj);
>>>> -    else
>>>> -        return 0;
>>>> +
>>>> +    return 0;
>>
>> This change is ok.
>>
>>>>   }
>>>>   void drm_gem_unpin(struct drm_gem_object *obj)
>>>> @@ -1172,7 +1172,8 @@ int drm_gem_vmap(struct drm_gem_object *obj, 
>>>> struct iosys_map *map)
>>>>       ret = obj->funcs->vmap(obj, map);
>>>>       if (ret)
>>>>           return ret;
>>>> -    else if (iosys_map_is_null(map))
>>>> +
>>>> +    if (iosys_map_is_null(map))
>>>>           return -ENOMEM;
>>
>> This is not correct. Calling iosys_map_is_null() is part of handling 
>> the return value from vmap, so the else is fine.
>>
> Are you serious ?
> 
> 
> 1. Before apply this patch:
> 
> 
> If the 'ret' is 0,  it stand for obj->funcs->vmap() is successful, then 
> if (iosys_map_is_null(map)) will be run.
> 
> If the 'ret' is NOT 0, then it return immediately.
> 
> 
> 2. After apply this patch:
> 
> 
> If the 'ret' is NOT 0, it stand for obj->funcs->vmap() failed, then it 
> return immediately.
> 
> If the 'ret' is 0, it stand for obj->funcs->vmap() is successful, then 
> the check if (iosys_map_is_null(map))
> 
> will be run!
> 
> 
> I feel strange about the core here, I think the check ' if 
> (iosys_map_is_null(map))' is not needed,
> 
> the implement should responsible to handle all of possible errors.

The ->vmap() callback can succeed with ret=0, but we still have no 
memory. Then we return -ENOMEM. The call to _is_null(map) is part of the 
error handling for ->vmap(). That is a bit strange, but it as always 
worked like that. Keeping all error handling in the same if-else block 
make all this more obvious.

Best regards
Thomas

> 
> 
> But both case (1. and 2.) are same in the semantic, right?
> 
> 
>> Best regards
>> Thomas
>>
>>>>       return 0;
>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]