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

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