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

[Sui Jingfeng]

Hi,

On 2023/6/27 17:00, Thomas Zimmermann wrote:
> 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.
>
Yeah, I'm agree.

But I think this is a short-term solution.

We probably should duplicate the test for NULL in each driver in a long 
term.

Because we should to keep the align with TTM and SHMEM.


I means that TTM and SHMEM memory manager are good example to follow.

In TTM, Nearly all mapping related function will return -ENOMEM.


```

int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map *map)
{
         // ...

         if (!vaddr_iomem)
             return -ENOMEM;

         iosys_map_set_vaddr_iomem(map, vaddr_iomem);

     } else {
         // ...
         vaddr = vmap(ttm->pages, ttm->num_pages, 0, prot);
         if (!vaddr)
             return -ENOMEM;

         iosys_map_set_vaddr(map, vaddr);
     }

     return 0;
}

```


The drm_gem_shmem_vmap() function in the SHMEM helper,

also return -ENOMEM.


After reading the code, It seems that

this is necessary to consolidate the standard behavior,

avoid the various device drivers violate each other.


> Best regards
> Thomas
>
>>
>> Maxime
>
-- 
Jingfeng