Re: [PATCH] drm/amdgpu: Fix the iounmap error of rmmio

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

Am 15.03.24 um 06:17 schrieb Ma Jun:
> Setting the rmmio pointer to NULL to fix the following
> iounmap error and calltrace.
> iounmap: bad address 00000000d0b3631f
>
> Fixes: 923f7a82d2e1 ("drm/amd/amdgpu: Fix potential ioremap() memory leaks in amdgpu_device_init()")
> Signed-off-by: Ma Jun <Jun.Ma2@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 39dd76e57154..d65a6aabefbb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4383,6 +4383,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   
>   unmap_memory:
>   	iounmap(adev->rmmio);
> +	adev->rmmio = NULL;

Well that doesn't looks correct to me. You seem to be working around 
broken initialisation code here.

>   	return r;
>   }
>   
> @@ -4514,9 +4515,11 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>   #endif
>   
>   	if (drm_dev_enter(adev_to_drm(adev), &idx)) {

Ok, well that alone doesn't look correct to me. The MMIO regions needs 
to be unmapped independent if the driver is disconnected or not.

> +		if (adev->rmmio) {

That looks just like a hack to me.

Regards,
Christian.

> +			iounmap(adev->rmmio);
> +			adev->rmmio = NULL;
> +		}
>   
> -		iounmap(adev->rmmio);
> -		adev->rmmio = NULL;
>   		amdgpu_doorbell_fini(adev);
>   		drm_dev_exit(idx);
>   	}