Re: [PATCH] drm/amd/amdgpu: Fix potential ioremap() memory leaks in amdgpu_device_init()

[Christian König <christian.koenig@xxxxxxx>]

Am 24.02.24 um 07:38 schrieb Srinivasan Shanmugam:
> This ensures that the memory mapped by ioremap for adev->rmmio, is
> properly handled in amdgpu_device_init(). If the function exits early
> due to an error, the memory is unmapped. If the function completes
> successfully, the memory remains mapped.
>
> Fixes the below:
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4337 amdgpu_device_init() warn: 'adev->rmmio' from ioremap() not released on lines: 4035,4045,4051,4058,4068,4337

Hui? How do you got that warning?

> Cc: Christian König <christian.koenig@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 1ef892bea488..68bf5e910cb8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4031,8 +4031,10 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	 * early on during init and before calling to RREG32.
>   	 */
>   	adev->reset_domain = amdgpu_reset_create_reset_domain(SINGLE_DEVICE, "amdgpu-reset-dev");
> -	if (!adev->reset_domain)
> +	if (!adev->reset_domain) {
> +		iounmap(adev->rmmio);
>   		return -ENOMEM;
> +	}

Please use a goto label and error handling instead. Apart from that 
looks good to me.

Regards,
Christian.

>   
>   	/* detect hw virtualization here */
>   	amdgpu_detect_virtualization(adev);
> @@ -4042,20 +4044,25 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	r = amdgpu_device_get_job_timeout_settings(adev);
>   	if (r) {
>   		dev_err(adev->dev, "invalid lockup_timeout parameter syntax\n");
> +		iounmap(adev->rmmio);
>   		return r;
>   	}
>   
>   	/* early init functions */
>   	r = amdgpu_device_ip_early_init(adev);
> -	if (r)
> +	if (r) {
> +		iounmap(adev->rmmio);
>   		return r;
> +	}
>   
>   	amdgpu_device_set_mcbp(adev);
>   
>   	/* Get rid of things like offb */
>   	r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, &amdgpu_kms_driver);
> -	if (r)
> +	if (r) {
> +		iounmap(adev->rmmio);
>   		return r;
> +	}
>   
>   	/* Enable TMZ based on IP_VERSION */
>   	amdgpu_gmc_tmz_set(adev);
> @@ -4064,8 +4071,10 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	/* Need to get xgmi info early to decide the reset behavior*/
>   	if (adev->gmc.xgmi.supported) {
>   		r = adev->gfxhub.funcs->get_xgmi_info(adev);
> -		if (r)
> +		if (r) {
> +			iounmap(adev->rmmio);
>   			return r;
> +		}
>   	}
>   
>   	/* enable PCIE atomic ops */
> @@ -4334,6 +4343,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   failed:
>   	amdgpu_vf_error_trans_all(adev);
>   
> +	iounmap(adev->rmmio);
>   	return r;
>   }
>