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

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

Am 19.03.24 um 09:07 schrieb Ma Jun:
> This patch causes the following iounmap erorr and calltrace
> iounmap: bad address 00000000d0b3631f

> So just revert it and amdgpu_device_fini_sw() will cleanup the
> rmmio mapping.

I think we can improve the wording here. Something like this:

The original patch was unjustified because amdgpu_device_fini_sw() will 
always cleanup the
rmmio mapping.

> This reverts commit 923f7a82d2e12a99744a940954f3829ab18a9dc7.

You Signed-off-by tag is missing.

With that fixed the patch is Reviewed-by: Christian König 
<christian.koenig@xxxxxxx>

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 70261eb9b0bb..3204b8f6edeb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4042,10 +4042,8 @@ 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) {
> -		r = -ENOMEM;
> -		goto unmap_memory;
> -	}
> +	if (!adev->reset_domain)
> +		return -ENOMEM;
>   
>   	/* detect hw virtualization here */
>   	amdgpu_detect_virtualization(adev);
> @@ -4055,7 +4053,7 @@ 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");
> -		goto unmap_memory;
> +		return r;
>   	}
>   
>   	amdgpu_device_set_mcbp(adev);
> @@ -4063,12 +4061,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	/* early init functions */
>   	r = amdgpu_device_ip_early_init(adev);
>   	if (r)
> -		goto unmap_memory;
> +		return r;
>   
>   	/* Get rid of things like offb */
>   	r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, &amdgpu_kms_driver);
>   	if (r)
> -		goto unmap_memory;
> +		return r;
>   
>   	/* Enable TMZ based on IP_VERSION */
>   	amdgpu_gmc_tmz_set(adev);
> @@ -4078,7 +4076,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	if (adev->gmc.xgmi.supported) {
>   		r = adev->gfxhub.funcs->get_xgmi_info(adev);
>   		if (r)
> -			goto unmap_memory;
> +			return r;
>   	}
>   
>   	/* enable PCIE atomic ops */
> @@ -4347,8 +4345,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   failed:
>   	amdgpu_vf_error_trans_all(adev);
>   
> -unmap_memory:
> -	iounmap(adev->rmmio);
>   	return r;
>   }
>