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

[Sui Jingfeng]

Hi,

On 2023/6/26 21:20, Sui Jingfeng wrote:
> Hi,
>
> On 2023/6/26 20:32, Maxime Ripard wrote:
>> 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.
>>
>> Could we add a comment maybe to make it more obvious?
>
> It could, but what should we to do ?
>
>
> It seems that it is true the  ->vmap() callback can succeed with ret=0,
>
> But I think this break the *convention*,
>
> the vmap() function at mm/vmalloc.c already said "Return: the address 
> of the area or %NULL on failure."
>
>
> The kernel's vmap() function return NULL on failure(may because the 
> space is not enough on 32-bit ARM SoCs).
>
> But the drm core's vmap hook just don't honor this.
>
>
> Take the drm/tegra as an example:
>
>
> ```
>
> static void *tegra_bo_mmap(struct host1x_bo *bo)
> {
>     struct tegra_bo *obj = host1x_to_tegra_bo(bo);
>     struct iosys_map map;
>     int ret;
>
>     if (obj->vaddr) {
>         return obj->vaddr;
>     } else if (obj->gem.import_attach) {
>         ret = dma_buf_vmap_unlocked(obj->gem.import_attach->dmabuf, 
> &map);
>         return ret ? NULL : map.vaddr;
>     } else {
>         return vmap(obj->pages, obj->num_pages, VM_MAP,
>                 pgprot_writecombine(PAGE_KERNEL));
>     }
> }
>
>
> static int tegra_gem_prime_vmap(struct dma_buf *buf, struct iosys_map 
> *map)
> {
>     struct drm_gem_object *gem = buf->priv;
>     struct tegra_bo *bo = to_tegra_bo(gem);
>     void *vaddr;
>
>     vaddr = tegra_bo_mmap(&bo->base);
>     if (IS_ERR(vaddr))
>         return PTR_ERR(vaddr);
>
>     iosys_map_set_vaddr(map, vaddr);
>
>     return 0;
> }
>
> ```
>
>
> On one of the all code path, 

> tegra_gem_prime_vmap() call vmap() fucntion to give the vmap the 
> caller wanted.

tegra_gem_prime_vmap() call vmap() function to map the memory the into 
kernel address space.


>
> but the tegra_gem_prime_vmap() function *only* think 'error' as 
> error(by calling IS_ERR(vaddr)),


The tegra_gem_prime_vmap() function only return a error code if 
IS_ERR(vaddr) is *true*.


>
> But I think the 'NULL' should also be counted as error.
>
I think the 'NULL'(vmap() return NULL) should be counted as error.
> For my patch(this patch),

> but the ultimate results is same for the case before apply this patch 
> and after apply this patch.
>
For my patch, the ultimate binary generated is same, 'before apply it' 
and 'after apply it',
> Their are also many other drivers have the same problem.
>
There are drivers have the same problem.


>
>> Maxime
>
-- 
Jingfeng