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 <ge...@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;
> }
