[PATCH] drm/amdgpu: Fix incorrect return value

[Christian König]

Am 03.04.24 um 09:06 schrieb YiPeng Chai:
> [Why]
>    After calling amdgpu_vram_mgr_reserve_range
> multiple times with the same address, calling
> amdgpu_vram_mgr_query_page_status will always
> return -EBUSY.
>    From the second call to amdgpu_vram_mgr_reserve_range,
> the same address will be added to the reservations_pending
> list again and is never moved to the reserved_pages list
> because the address had been reserved.

Well that sounds like a really bad idea to me. Why is the function 
called multiple times with the same address in the first place ?

Apart from that a note on the coding style below.

>
> [How]
>    First add the address status check before calling
> amdgpu_vram_mgr_do_reserve, if the address is already
> reserved, do nothing; If the address is already in the
> reservations_pending list, directly reserve memory;
> only add new nodes for the addresses that are not in the
> reserved_pages list and reservations_pending list.
>
> Signed-off-by: YiPeng Chai <YiPeng.Chai at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 28 +++++++++++++-------
>   1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> index 1e36c428d254..0bf3f4092900 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> @@ -317,7 +317,6 @@ static void amdgpu_vram_mgr_do_reserve(struct ttm_resource_manager *man)
>   
>   		dev_dbg(adev->dev, "Reservation 0x%llx - %lld, Succeeded\n",
>   			rsv->start, rsv->size);
> -
>   		vis_usage = amdgpu_vram_mgr_vis_size(adev, block);
>   		atomic64_add(vis_usage, &mgr->vis_usage);
>   		spin_lock(&man->bdev->lru_lock);
> @@ -340,19 +339,30 @@ int amdgpu_vram_mgr_reserve_range(struct amdgpu_vram_mgr *mgr,
>   				  uint64_t start, uint64_t size)
>   {
>   	struct amdgpu_vram_reservation *rsv;
> +	int ret = 0;

Don't initialize local variables when it isn't necessary.

>   
> -	rsv = kzalloc(sizeof(*rsv), GFP_KERNEL);
> -	if (!rsv)
> -		return -ENOMEM;
> +	ret = amdgpu_vram_mgr_query_page_status(mgr, start);
> +	if (!ret)
> +		return 0;
> +
> +	if (ret == -ENOENT) {
> +		rsv = kzalloc(sizeof(*rsv), GFP_KERNEL);
> +		if (!rsv)
> +			return -ENOMEM;
>   
> -	INIT_LIST_HEAD(&rsv->allocated);
> -	INIT_LIST_HEAD(&rsv->blocks);
> +		INIT_LIST_HEAD(&rsv->allocated);
> +		INIT_LIST_HEAD(&rsv->blocks);
>   
> -	rsv->start = start;
> -	rsv->size = size;
> +		rsv->start = start;
> +		rsv->size = size;
> +
> +		mutex_lock(&mgr->lock);
> +		list_add_tail(&rsv->blocks, &mgr->reservations_pending);
> +		mutex_unlock(&mgr->lock);
> +
> +	}

You should probably not lock/unlock here.

Regards,
Christian.

>   
>   	mutex_lock(&mgr->lock);
> -	list_add_tail(&rsv->blocks, &mgr->reservations_pending);
>   	amdgpu_vram_mgr_do_reserve(&mgr->manager);
>   	mutex_unlock(&mgr->lock);
>