Re: [PATCH v2 1/4] drm/xe: Simplify function return using drmm_add_action_or_reset()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 08 Apr 2024, Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
> Instead of assigning the value of drmm_add_action_or_reset() to err and
> returning err in case of failure and 0 in case of success, simply return
> the result of drmm_add_action_or_reset().
>
> -v2:
> cleanup in xe_display too.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/xe/display/xe_display.c | 8 +-------
>  drivers/gpu/drm/xe/xe_device.c          | 6 +-----
>  drivers/gpu/drm/xe/xe_gsc_proxy.c       | 7 +------
>  drivers/gpu/drm/xe/xe_gt.c              | 6 +-----
>  drivers/gpu/drm/xe/xe_guc_pc.c          | 6 +-----
>  drivers/gpu/drm/xe/xe_hw_engine.c       | 6 +-----
>  6 files changed, 6 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 6ec375c1c4b6..63b27fbcdaca 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -101,8 +101,6 @@ static void display_destroy(struct drm_device *dev, void *dummy)
>   */
>  int xe_display_create(struct xe_device *xe)
>  {
> -	int err;
> -
>  	spin_lock_init(&xe->display.fb_tracking.lock);
>  
>  	xe->display.hotplug.dp_wq = alloc_ordered_workqueue("xe-dp", 0);
> @@ -110,11 +108,7 @@ int xe_display_create(struct xe_device *xe)
>  	drmm_mutex_init(&xe->drm, &xe->sb_lock);
>  	xe->enabled_irq_mask = ~0;
>  
> -	err = drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&xe->drm, display_destroy, NULL);

Up to the xe maintainers, but personally I prefer these as they were,
throughout the series.

The thing is, these functions will be maintained for a long time, and
there'll inevitably be a need to add more initializations and error
returns and cleanups. All of these "return foo()" style things make
future changes harder.


BR,
Jani.


>  }
>  
>  static void xe_display_fini_nommio(struct drm_device *dev, void *dummy)
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 9083f5e02dd9..ce8d632cb961 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -588,11 +588,7 @@ int xe_device_probe(struct xe_device *xe)
>  
>  	xe_hwmon_register(xe);
>  
> -	err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
>  
>  err_fini_display:
>  	xe_display_driver_remove(xe);
> diff --git a/drivers/gpu/drm/xe/xe_gsc_proxy.c b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> index 35e397b68dfc..1b908d238bd1 100644
> --- a/drivers/gpu/drm/xe/xe_gsc_proxy.c
> +++ b/drivers/gpu/drm/xe/xe_gsc_proxy.c
> @@ -403,7 +403,6 @@ static int proxy_channel_alloc(struct xe_gsc *gsc)
>  	struct xe_device *xe = gt_to_xe(gt);
>  	struct xe_bo *bo;
>  	void *csme;
> -	int err;
>  
>  	csme = kzalloc(GSC_PROXY_CHANNEL_SIZE, GFP_KERNEL);
>  	if (!csme)
> @@ -424,11 +423,7 @@ static int proxy_channel_alloc(struct xe_gsc *gsc)
>  	gsc->proxy.to_csme = csme;
>  	gsc->proxy.from_csme = csme + GSC_PROXY_BUFFER_SIZE;
>  
> -	err = drmm_add_action_or_reset(&xe->drm, proxy_channel_free, gsc);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&xe->drm, proxy_channel_free, gsc);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index cfa5da900461..2421a8d34324 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -560,11 +560,7 @@ int xe_gt_init(struct xe_gt *gt)
>  	if (err)
>  		return err;
>  
> -	err = drmm_add_action_or_reset(&gt_to_xe(gt)->drm, gt_fini, gt);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&gt_to_xe(gt)->drm, gt_fini, gt);
>  }
>  
>  static int do_gt_reset(struct xe_gt *gt)
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 521ae24f2314..509649d0e65e 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -937,9 +937,5 @@ int xe_guc_pc_init(struct xe_guc_pc *pc)
>  
>  	pc->bo = bo;
>  
> -	err = drmm_add_action_or_reset(&xe->drm, xe_guc_pc_fini, pc);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&xe->drm, xe_guc_pc_fini, pc);
>  }
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index a688bb2d96ce..455f375c1cbd 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -550,11 +550,7 @@ static int hw_engine_init(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  	if (xe->info.has_usm && hwe->class == XE_ENGINE_CLASS_COPY)
>  		gt->usm.reserved_bcs_instance = hwe->instance;
>  
> -	err = drmm_add_action_or_reset(&xe->drm, hw_engine_fini, hwe);
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return drmm_add_action_or_reset(&xe->drm, hw_engine_fini, hwe);
>  
>  err_kernel_lrc:
>  	xe_lrc_finish(&hwe->kernel_lrc);

-- 
Jani Nikula, Intel