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

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

[Christian König]

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@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;
 }
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[James Zhu]

On 2018-09-17 02:07 PM, Christian König wrote:
> 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@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)
Can we put this check inside ttm_bo_unreserve?
James Zhu
> +		amdgpu_bo_unreserve(*bo_ptr);
>   
>   	return 0;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

Am 17.09.2018 um 20:41 schrieb James Zhu:
>
>
> On 2018-09-17 02:07 PM, Christian König wrote:
>> 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@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)
> Can we put this check inside ttm_bo_unreserve?

No, calling amdgpu_bo_unreserve() with a NULL pointer is certainly a bug.

Christian.

> James Zhu
>> +        amdgpu_bo_unreserve(*bo_ptr);
>>         return 0;
>>   }
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Huang Rui]

On Mon, Sep 17, 2018 at 08:07:24PM +0200, Christian König wrote:
> 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@amd.com>

If we set size as 0 while create bo, the bo_ptr will be NULL after that.

Acked-by: Huang Rui <ray.huang@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;
>  }
> -- 
> 2.14.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhu, Rex]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Tuesday, September 18, 2018 2:07 AM
> To: amd-gfx@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@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.

Best Regards
Rex
 
> --
> 2.14.1
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

Am 18.09.2018 um 08:16 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>> Christian König
>> Sent: Tuesday, September 18, 2018 2:07 AM
>> To: amd-gfx@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@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?

Regards,
Christian.

>
> Best Regards
> Rex
>   
>> --
>> 2.14.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhu, Rex]

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, September 18, 2018 3:14 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@lists.freedesktop.org> On Behalf Of
> >> Christian König
> >> Sent: Tuesday, September 18, 2018 2:07 AM
> >> To: amd-gfx@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@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. It also make the  code
if (r)
	return r; 
redundant.

Regards
Rex

> Regards,
> Christian.
> 
> >
> > Best Regards
> > Rex
> >
> >> --
> >> 2.14.1
> >>
> >> _______________________________________________
> >> amd-gfx mailing list
> >> amd-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

Am 18.09.2018 um 10:16 schrieb Zhu, Rex:
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, September 18, 2018 3:14 PM
>> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@lists.freedesktop.org> On Behalf Of
>>>> Christian König
>>>> Sent: Tuesday, September 18, 2018 2:07 AM
>>>> To: amd-gfx@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@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.

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

Christian.

>
> Regards
> Rex
>
>> Regards,
>> Christian.
>>
>>> Best Regards
>>> Rex
>>>
>>>> --
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhu, Rex]

> -----Original Message-----
> From: Koenig, Christian
> Sent: Tuesday, September 18, 2018 4:41 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@gmail.com>
> >> Sent: Tuesday, September 18, 2018 3:14 PM
> >> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@lists.freedesktop.org> On Behalf Of
> >>>> Christian König
> >>>> Sent: Tuesday, September 18, 2018 2:07 AM
> >>>> To: amd-gfx@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@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).

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@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@amd.com>; amd-gfx@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@gmail.com>
>>>> Sent: Tuesday, September 18, 2018 3:14 PM
>>>> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@lists.freedesktop.org> On Behalf Of
>>>>>> Christian König
>>>>>> Sent: Tuesday, September 18, 2018 2:07 AM
>>>>>> To: amd-gfx@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@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@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhu, Rex]

Thanks. 
So we still need to check the bo valid before use for the case that if we don't know the size when allocate.

Best Regards
Rex

> -----Original Message-----
> From: Koenig, Christian
> Sent: Tuesday, September 18, 2018 5:34 PM
> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: don't try to unreserve NULL pointer
> 
> 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@amd.com>; amd-gfx@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@gmail.com>
> >>>> Sent: Tuesday, September 18, 2018 3:14 PM
> >>>> To: Zhu, Rex <Rex.Zhu@amd.com>; amd-gfx@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@lists.freedesktop.org> On Behalf
> >>>>>> Of Christian König
> >>>>>> Sent: Tuesday, September 18, 2018 2:07 AM
> >>>>>> To: amd-gfx@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@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@lists.freedesktop.org
> >>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx