[v1 2/4] drm/amdgpu: enhance error handling of psp_sw_init()

[Lazar, Lijo]

On 2/7/2025 11:58 AM, Jiang Liu wrote:
> Enhance error handling in function psp_sw_init() by:
> 1) bail out when failed to allocate memory
> 2) release allocated resource on error
> 3) introduce helper function psp_bo_init()
> 
> Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 84 ++++++++++++++++---------
>  1 file changed, 54 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 4e9766a1d421..0d1eb7b8e59b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -423,6 +423,50 @@ static bool psp_get_runtime_db_entry(struct amdgpu_device *adev,
>  	return ret;
>  }
>  
> +static int psp_bo_init(struct amdgpu_device *adev, struct psp_context *psp)
> +{
> +	int ret;
> +
> +	ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
> +				      (amdgpu_sriov_vf(adev) || adev->debug_use_vram_fw_buf) ?
> +				      AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> +				      &psp->fw_pri_bo,
> +				      &psp->fw_pri_mc_addr,
> +				      &psp->fw_pri_buf);
> +	if (ret)
> +		goto failed1;

Better keep it as return ret, will avoid another label.

> +
> +	ret = amdgpu_bo_create_kernel(adev, PSP_FENCE_BUFFER_SIZE, PAGE_SIZE,
> +				      AMDGPU_GEM_DOMAIN_VRAM |
> +				      AMDGPU_GEM_DOMAIN_GTT,
> +				      &psp->fence_buf_bo,
> +				      &psp->fence_buf_mc_addr,
> +				      &psp->fence_buf);
> +	if (ret)
> +		goto failed2;
> +
> +	ret = amdgpu_bo_create_kernel(adev, PSP_CMD_BUFFER_SIZE, PAGE_SIZE,
> +				      AMDGPU_GEM_DOMAIN_VRAM |
> +				      AMDGPU_GEM_DOMAIN_GTT,
> +				      &psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
> +				      (void **)&psp->cmd_buf_mem);
> +	if (ret)
> +		goto failed3;
> +
> +	return 0;
> +
> +failed3:
> +	amdgpu_bo_free_kernel(&psp->fence_buf_bo,
> +			      &psp->fence_buf_mc_addr, &psp->fence_buf);
> +	psp->fence_buf_bo = NULL;

This NULL assignment is not required, same below as well.

> +failed2:
> +	amdgpu_bo_free_kernel(&psp->fw_pri_bo,
> +			      &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
> +	psp->fw_pri_bo = NULL;
> +failed1:
> +	return ret;
> +}
> +
>  static int psp_sw_init(struct amdgpu_ip_block *ip_block)
>  {
>  	struct amdgpu_device *adev = ip_block->adev;
> @@ -435,7 +479,7 @@ static int psp_sw_init(struct amdgpu_ip_block *ip_block)
>  	psp->cmd = kzalloc(sizeof(struct psp_gfx_cmd_resp), GFP_KERNEL);
>  	if (!psp->cmd) {
>  		dev_err(adev->dev, "Failed to allocate memory to command buffer!\n");
> -		ret = -ENOMEM;
> +		return -ENOMEM;
>  	}
>  
>  	adev->psp.xgmi_context.supports_extended_data =
> @@ -482,50 +526,27 @@ static int psp_sw_init(struct amdgpu_ip_block *ip_block)
>  		ret = psp_memory_training_init(psp);
>  		if (ret) {
>  			dev_err(adev->dev, "Failed to initialize memory training!\n");
> -			return ret;
> +			goto failed1;
>  		}
>  
>  		ret = psp_mem_training(psp, PSP_MEM_TRAIN_COLD_BOOT);
>  		if (ret) {
>  			dev_err(adev->dev, "Failed to process memory training!\n");
> -			return ret;
> +			goto failed2;
>  		}
>  	}
>  
> -	ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
> -				      (amdgpu_sriov_vf(adev) || adev->debug_use_vram_fw_buf) ?
> -				      AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
> -				      &psp->fw_pri_bo,
> -				      &psp->fw_pri_mc_addr,
> -				      &psp->fw_pri_buf);
> -	if (ret)
> -		return ret;
> -
> -	ret = amdgpu_bo_create_kernel(adev, PSP_FENCE_BUFFER_SIZE, PAGE_SIZE,
> -				      AMDGPU_GEM_DOMAIN_VRAM |
> -				      AMDGPU_GEM_DOMAIN_GTT,
> -				      &psp->fence_buf_bo,
> -				      &psp->fence_buf_mc_addr,
> -				      &psp->fence_buf);
> -	if (ret)
> -		goto failed1;
> -
> -	ret = amdgpu_bo_create_kernel(adev, PSP_CMD_BUFFER_SIZE, PAGE_SIZE,
> -				      AMDGPU_GEM_DOMAIN_VRAM |
> -				      AMDGPU_GEM_DOMAIN_GTT,
> -				      &psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
> -				      (void **)&psp->cmd_buf_mem);
> +	ret = psp_bo_init(adev, psp);
>  	if (ret)
>  		goto failed2;
>  
>  	return 0;
>  
>  failed2:
> -	amdgpu_bo_free_kernel(&psp->fence_buf_bo,
> -			      &psp->fence_buf_mc_addr, &psp->fence_buf);
> +	psp_memory_training_fini(psp);
>  failed1:
> -	amdgpu_bo_free_kernel(&psp->fw_pri_bo,
> -			      &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
> +	kfree(psp->cmd);
> +	psp->cmd = NULL;
>  	return ret;
>  }
>  
> @@ -554,10 +575,13 @@ static int psp_sw_fini(struct amdgpu_ip_block *ip_block)
>  
>  	amdgpu_bo_free_kernel(&psp->fw_pri_bo,
>  			      &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
> +	psp->fw_pri_bo = NULL;
>  	amdgpu_bo_free_kernel(&psp->fence_buf_bo,
>  			      &psp->fence_buf_mc_addr, &psp->fence_buf);
> +	psp->fence_buf_bo = NULL;
>  	amdgpu_bo_free_kernel(&psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
>  			      (void **)&psp->cmd_buf_mem);
> +	psp->cmd_buf_bo = NULL;

This is already taken care by amdgpu_bo_free_kernel

Thanks,
Lijo

>  
>  	return 0;
>  }