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

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

[Sui Jingfeng]

 else is not generally useful after return

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;
 }
 
 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;
 
 	return 0;
-- 
2.25.1

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

[Sui Jingfeng]

ping ?

On 2023/3/14 20:53, Sui Jingfeng wrote:
>   else is not generally useful after return
>
> 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;
>   }
>   
>   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;
>   
>   	return 0;

-- 
Jingfeng

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

[Thomas Zimmermann]

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

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.

>>
>> 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.

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 --]

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

[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.


But both case (1. and 2.) are same in the semantic, right?


> Best regards
> Thomas
>
>>>       return 0;
>>
>
-- 
Jingfeng

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

[Thomas Zimmermann]

[-- 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 --]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

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@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?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Sui Jingfeng]

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@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.

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

But I think the 'NULL' should also 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.


Their are also many other drivers have the same problem.

But I don't have their hardware to do the basic test.

> Maxime

-- 
Jingfeng

Re: [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@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

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

[Thomas Zimmermann]

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

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@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)

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

Re: [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@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