[PATCH 2/2] drm/amdgpu: drop unnecessary dereference

Pan, Xinhui Xinhui.Pan at amd.com
Mon Mar 11 05:45:29 UTC 2019


adev->gfx.ras_if is a pointer and it is NULL at the first time

-----Original Message-----
From: Quan, Evan <Evan.Quan at amd.com> 
Sent: 2019年3月11日 13:41
To: Pan, Xinhui <Xinhui.Pan at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: RE: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference

I cannot get your point. "ras_if" was already assigned to be same as "adev->gfx.ras_if" at the start of the APIs. 
////  struct ras_common_if *ras_if = adev->gfx.ras_if; ///// Why there is need to set " adev->gfx.ras_if = ras_if" again?

Regards,
Evan
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of 
> Pan, Xinhui
> Sent: 2019年3月11日 13:19
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan 
> <Evan.Quan at amd.com>
> Subject: RE: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
> 
> Well, I think you forgot setting adev->gfx.ras_if = ras_if  in the end.
> 
> struct ras_common_if **ras_if is pretty simple and easy to use.
> I prefer to keep the code as it is.
> 
> Thanks
> xinhui
> 
> 
> -----Original Message-----
> From: Evan Quan <evan.quan at amd.com>
> Sent: 2019年3月11日 12:31
> To: amd-gfx at lists.freedesktop.org
> Cc: Pan, Xinhui <Xinhui.Pan at amd.com>; Deucher, Alexander 
> <Alexander.Deucher at amd.com>; Quan, Evan <Evan.Quan at amd.com>
> Subject: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
> 
> It's unnecessary and confusing.
> 
> Change-Id: I77fe54a108b7ee2031851b3e11d63c4fb74c0d43
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c  | 26 
> +++++++++++++------------- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 26 
> +++++++++++++------------
> -  drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 26 +++++++++++++---------
> ----
>  3 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 3fb72bf420e0..31996d448817 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -3532,7 +3532,7 @@ static int gfx_v9_0_process_ras_data_cb(struct
> amdgpu_device *adev,  static int gfx_v9_0_ecc_late_init(void *handle)  {
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> -	struct ras_common_if **ras_if = &adev->gfx.ras_if;
> +	struct ras_common_if *ras_if = adev->gfx.ras_if;
>  	struct ras_ih_if ih_info = {
>  		.cb = gfx_v9_0_process_ras_data_cb,
>  	};
> @@ -3553,21 +3553,21 @@ static int gfx_v9_0_ecc_late_init(void *handle)
>  		return 0;
>  	}
> 
> -	if (*ras_if)
> +	if (ras_if)
>  		goto resume;
> 
> -	*ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> -	if (!*ras_if)
> +	ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> +	if (!ras_if)
>  		return -ENOMEM;
> 
> -	**ras_if = ras_block;
> +	*ras_if = ras_block;
> 
> -	r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> +	r = amdgpu_ras_feature_enable(adev, ras_if, 1);
>  	if (r)
>  		goto feature;
> 
> -	ih_info.head = **ras_if;
> -	fs_info.head = **ras_if;
> +	ih_info.head = *ras_if;
> +	fs_info.head = *ras_if;
> 
>  	r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
>  	if (r)
> @@ -3587,16 +3587,16 @@ static int gfx_v9_0_ecc_late_init(void 
> *handle)
> 
>  	return 0;
>  irq:
> -	amdgpu_ras_sysfs_remove(adev, *ras_if);
> +	amdgpu_ras_sysfs_remove(adev, ras_if);
>  sysfs:
> -	amdgpu_ras_debugfs_remove(adev, *ras_if);
> +	amdgpu_ras_debugfs_remove(adev, ras_if);
>  debugfs:
>  	amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
>  interrupt:
> -	amdgpu_ras_feature_enable(adev, *ras_if, 0);
> +	amdgpu_ras_feature_enable(adev, ras_if, 0);
>  feature:
> -	kfree(*ras_if);
> -	*ras_if = NULL;
> +	kfree(ras_if);
> +	ras_if = NULL;
>  	return -EINVAL;
>  }
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 5d1ac53f7ddb..229e614d1b76 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -714,7 +714,7 @@ static int gmc_v9_0_allocate_vm_inv_eng(struct
> amdgpu_device *adev)  static int gmc_v9_0_ecc_late_init(void *handle)  {
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> -	struct ras_common_if **ras_if = &adev->gmc.ras_if;
> +	struct ras_common_if *ras_if = adev->gmc.ras_if;
>  	struct ras_ih_if ih_info = {
>  		.cb = gmc_v9_0_process_ras_data_cb,
>  	};
> @@ -735,21 +735,21 @@ static int gmc_v9_0_ecc_late_init(void *handle)
>  		return 0;
>  	}
>  	/* handle resume path. */
> -	if (*ras_if)
> +	if (ras_if)
>  		goto resume;
> 
> -	*ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> -	if (!*ras_if)
> +	ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> +	if (!ras_if)
>  		return -ENOMEM;
> 
> -	**ras_if = ras_block;
> +	*ras_if = ras_block;
> 
> -	r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> +	r = amdgpu_ras_feature_enable(adev, ras_if, 1);
>  	if (r)
>  		goto feature;
> 
> -	ih_info.head = **ras_if;
> -	fs_info.head = **ras_if;
> +	ih_info.head = *ras_if;
> +	fs_info.head = *ras_if;
> 
>  	r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
>  	if (r)
> @@ -769,16 +769,16 @@ static int gmc_v9_0_ecc_late_init(void *handle)
> 
>  	return 0;
>  irq:
> -	amdgpu_ras_sysfs_remove(adev, *ras_if);
> +	amdgpu_ras_sysfs_remove(adev, ras_if);
>  sysfs:
> -	amdgpu_ras_debugfs_remove(adev, *ras_if);
> +	amdgpu_ras_debugfs_remove(adev, ras_if);
>  debugfs:
>  	amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
>  interrupt:
> -	amdgpu_ras_feature_enable(adev, *ras_if, 0);
> +	amdgpu_ras_feature_enable(adev, ras_if, 0);
>  feature:
> -	kfree(*ras_if);
> -	*ras_if = NULL;
> +	kfree(ras_if);
> +	ras_if = NULL;
>  	return -EINVAL;
>  }
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 3ac5abe937f4..521218053477 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -1501,7 +1501,7 @@ static int sdma_v4_0_process_ras_data_cb(struct
> amdgpu_device *adev,  static int sdma_v4_0_late_init(void *handle)  {
>  	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> -	struct ras_common_if **ras_if = &adev->sdma.ras_if;
> +	struct ras_common_if *ras_if = adev->sdma.ras_if;
>  	struct ras_ih_if ih_info = {
>  		.cb = sdma_v4_0_process_ras_data_cb,
>  	};
> @@ -1523,21 +1523,21 @@ static int sdma_v4_0_late_init(void *handle)
>  	}
> 
>  	/* handle resume path. */
> -	if (*ras_if)
> +	if (ras_if)
>  		goto resume;
> 
> -	*ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> -	if (!*ras_if)
> +	ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> +	if (!ras_if)
>  		return -ENOMEM;
> 
> -	**ras_if = ras_block;
> +	*ras_if = ras_block;
> 
> -	r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> +	r = amdgpu_ras_feature_enable(adev, ras_if, 1);
>  	if (r)
>  		goto feature;
> 
> -	ih_info.head = **ras_if;
> -	fs_info.head = **ras_if;
> +	ih_info.head = *ras_if;
> +	fs_info.head = *ras_if;
> 
>  	r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
>  	if (r)
> @@ -1563,16 +1563,16 @@ static int sdma_v4_0_late_init(void *handle)
> 
>  	return 0;
>  irq:
> -	amdgpu_ras_sysfs_remove(adev, *ras_if);
> +	amdgpu_ras_sysfs_remove(adev, ras_if);
>  sysfs:
> -	amdgpu_ras_debugfs_remove(adev, *ras_if);
> +	amdgpu_ras_debugfs_remove(adev, ras_if);
>  debugfs:
>  	amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
>  interrupt:
> -	amdgpu_ras_feature_enable(adev, *ras_if, 0);
> +	amdgpu_ras_feature_enable(adev, ras_if, 0);
>  feature:
> -	kfree(*ras_if);
> -	*ras_if = NULL;
> +	kfree(ras_if);
> +	ras_if = NULL;
>  	return -EINVAL;
>  }
> 
> --
> 2.21.0
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


More information about the amd-gfx mailing list