[PATCH 03/10] drm/amdgpu:fix NULL pointer access during drv remove

[Liu, Monk]

ok

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com] 
Sent: 2017年11月14日 19:49
To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 03/10] drm/amdgpu:fix NULL pointer access during drv remove

Am 14.11.2017 um 10:07 schrieb Monk Liu:
> NULL pointer is because original logic will step into
> set_pde_pte() even after the gart.ptr is freed due to there are twice 
> gart_unbind() on all gart area.
>
> also, there are other minor fixes:
> 1,since gart_init only create dummy page, the corresponding gart_fini 
> shouldn't do more like unbinding all GART, this is unnecessary because 
> in driver fini stage all GART unbinding had already been done during 
> each IP's SW_FINI (GMC's SW_FINI is the last one called), so remove 
> the step for the GART unbinding in gart_fini().
>
> 2,gart_fini() is already invoked during each GMC IP's gart_fini 
> routine,e.g. gmc_vx_0_gart_fini(), so no need to manually call it 
> during ttm_fini().
>
> 3,amdgpu_gem_force_release() should be put ahead of
> amdgpu_vm_manager_fini()
>
> Change-Id: Ib1f31a2cf6bfbb9b54c7cc2a8cf9e4e3160bcfa0
> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 15 +++++----------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c  |  1 -
>   drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c    |  2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c    |  2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c    |  2 +-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c    |  2 +-
>   6 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index 4b5ec15..c4eef4a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> @@ -255,10 +255,8 @@ int amdgpu_gart_init(struct amdgpu_device *adev)
>   #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
>   	/* Allocate pages table */
>   	adev->gart.pages = vzalloc(sizeof(void *) * adev->gart.num_cpu_pages);
> -	if (adev->gart.pages == NULL) {
> -		amdgpu_gart_fini(adev);
> +	if (adev->gart.pages == NULL)
>   		return -ENOMEM;
> -	}
>   #endif
>   
>   	return 0;
> @@ -273,14 +271,11 @@ int amdgpu_gart_init(struct amdgpu_device *adev)
>    */
>   void amdgpu_gart_fini(struct amdgpu_device *adev)
>   {
> -	if (adev->gart.ready) {
> -		/* unbind pages */
> -		amdgpu_gart_unbind(adev, 0, adev->gart.num_cpu_pages);
> -	}
> -	adev->gart.ready = false;
>   #ifdef CONFIG_DRM_AMDGPU_GART_DEBUGFS
> -	vfree(adev->gart.pages);
> -	adev->gart.pages = NULL;
> +	if (adev->gart.pages) {
> +		vfree(adev->gart.pages);
> +		adev->gart.pages = NULL;
> +	}

vfree() is NULL save, so extra checks are usually complained about by automated testers.

I would just drop this hunk, the rest of the patch looks good to me.

Christian.

>   #endif
>   	amdgpu_dummy_page_fini(adev);
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index e8401a9..615b849 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1653,7 +1653,6 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
>   	if (adev->gds.oa.total_size)
>   		ttm_bo_clean_mm(&adev->mman.bdev, AMDGPU_PL_OA);
>   	ttm_bo_device_release(&adev->mman.bdev);
> -	amdgpu_gart_fini(adev);
>   	amdgpu_ttm_global_fini(adev);
>   	adev->mman.initialized = false;
>   	DRM_INFO("amdgpu: ttm finalized\n"); diff --git 
> a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> index aac493b..6112dfc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v6_0.c
> @@ -894,9 +894,9 @@ static int gmc_v6_0_sw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> +	amdgpu_gem_force_release(adev);
>   	amdgpu_vm_manager_fini(adev);
>   	gmc_v6_0_gart_fini(adev);
> -	amdgpu_gem_force_release(adev);
>   	amdgpu_bo_fini(adev);
>   	release_firmware(adev->mc.fw);
>   	adev->mc.fw = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index dede91a..b57dc06 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -1067,9 +1067,9 @@ static int gmc_v7_0_sw_fini(void *handle)
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
>   	kfree(adev->mc.vm_fault_info);
> +	amdgpu_gem_force_release(adev);
>   	amdgpu_vm_manager_fini(adev);
>   	gmc_v7_0_gart_fini(adev);
> -	amdgpu_gem_force_release(adev);
>   	amdgpu_bo_fini(adev);
>   	release_firmware(adev->mc.fw);
>   	adev->mc.fw = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 847c046..5be4854 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1168,9 +1168,9 @@ static int gmc_v8_0_sw_fini(void *handle)
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
>   	kfree(adev->mc.vm_fault_info);
> +	amdgpu_gem_force_release(adev);
>   	amdgpu_vm_manager_fini(adev);
>   	gmc_v8_0_gart_fini(adev);
> -	amdgpu_gem_force_release(adev);
>   	amdgpu_bo_fini(adev);
>   	release_firmware(adev->mc.fw);
>   	adev->mc.fw = NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index d0acf26..bea0774 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -667,9 +667,9 @@ static int gmc_v9_0_sw_fini(void *handle)
>   {
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> +	amdgpu_gem_force_release(adev);
>   	amdgpu_vm_manager_fini(adev);
>   	gmc_v9_0_gart_fini(adev);
> -	amdgpu_gem_force_release(adev);
>   	amdgpu_bo_fini(adev);
>   
>   	return 0;