[PATCH] drm/amdgpu: don't try to unreserve NULL pointer

[Christian König]

Am 18.09.2018 um 11:27 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Tuesday, September 18, 2018 4:41 PM
>> To: Zhu, Rex <Rex.Zhu at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: don't try to unreserve NULL pointer
>>
>> Am 18.09.2018 um 10:16 schrieb Zhu, Rex:
>>>> -----Original Message-----
>>>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>>>> Sent: Tuesday, September 18, 2018 3:14 PM
>>>> To: Zhu, Rex <Rex.Zhu at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: don't try to unreserve NULL pointer
>>>>
>>>> Am 18.09.2018 um 08:16 schrieb Zhu, Rex:
>>>>>> -----Original Message-----
>>>>>> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
>>>>>> Christian König
>>>>>> Sent: Tuesday, September 18, 2018 2:07 AM
>>>>>> To: amd-gfx at lists.freedesktop.org
>>>>>> Subject: [PATCH] drm/amdgpu: don't try to unreserve NULL pointer
>>>>>>
>>>>>> Don't try to unreserve a BO we doesn't allocated.
>>>>>>
>>>>>> Fixes: 07012fdd497e drm/amdgpu: don't allocate zero sized kernel
>>>>>> BOs
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 84d82d5382f9..c1387efc0c91 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -348,7 +348,8 @@ int amdgpu_bo_create_kernel(struct
>>>> amdgpu_device
>>>>>> *adev,
>>>>>> 	if (r)
>>>>>>     		return r;
>>>>>> -	amdgpu_bo_unreserve(*bo_ptr);
>>>>>> +	if (*bo_ptr)
>>>>>> +		amdgpu_bo_unreserve(*bo_ptr);
>>>>>>
>>>>>>     	return 0;
>>>>>>     }
>>>>> It is weird.
>>>>> If we return true for allocate bo with size  0.
>>>>> Does that mean we need to check all the bo_ptr before we use them.
>>>> No, allocating a BO with zero size doesn't make much sense and was
>>>> essentially undefined behavior previously.
>>>>
>>>> So now we get a defined behavior, but not necessary the one you
>> expected.
>>>> Is that only a rhetorical question or really a problem somewhere?
>>> Logically, the code is trick.
>> Yeah, that is not a bad argument.
>>
>> But it also makes the function more useful, e.g. we don't need size checks
>> any more whenever an optional BO is allocated.
> Yes, the problem is user don't need size check. But user have no way to check whether the bo is allocated successfully.
>
> Because in size 0 case, the create function also return true.
> And as you said below, check bo_ptr is not safe either(the *bo_ptr  may be not modified at all).

That is not correct. When size==0 we call amdgpu_bo_unref(bo_ptr), and 
that one sets bo_ptr to NULL.

When size==0 was possible before, the calling code would have needed to 
check bo_ptr later on anyway.

In other words what we had before:

calling_function()
{
     if (size) {
         r = amdgpu_bo_create_kernel(..., size, &bo);
         if (r)
             goto error_handling;
     }
     ....
     if (bo)
         ....
}


But now that looks like:
calling_function()
{
     r = amdgpu_bo_create_kernel(..., size, &bo);
     if (r)
         goto error_handling;
     ....
     if (bo) {
         ....
}

So we just removed the extra size check of the calling function. I think 
that is a valid usage.

Christian.


>
> Regards
> Rex
>
>
>
>
>>>    It also make the  code
>>> if (r)
>>> 	return r;
>>> redundant.
>> Actually that is not correct.
>>
>> When an error happens the *bo_ptr is not modified at all. So we could then
>> try to unreserve a BO which was never reserved.
> Yes, You ae right.
>
>
>> Christian.
>>
>>> Regards
>>> Rex
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best Regards
>>>>> Rex
>>>>>
>>>>>> --
>>>>>> 2.14.1
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx