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