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

[Thomas Zimmermann]

Hi

Am 26.06.23 um 14:32 schrieb Maxime Ripard:
> Hi,
> 
> On Tue, Jun 20, 2023 at 06:18:31PM +0200, Thomas Zimmermann wrote:
>> 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 at 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.
> 
> Reading that patch, it wasn't obvious to me at all and could have made
> the same patch.

The vmap callback could return any errno code; plus a 0 with a NULL 
pointer means -ENOMEM.  Doing this here simplifies the callers of 
drm_gem_vmap and makes them more robust.  We'd otherwise duplicate the 
test for NULL in each caller.

> 
> Could we add a comment maybe to make it more obvious?

A one-liner that states the given rational might make sense.

Best regards
Thomas

> 
> Maxime

-- 
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230627/cc40c549/attachment-0001.sig>