[PATCH v3 2/7] drm/xe: Incase of action add failure, remove sysfs only once

[Lucas De Marchi]

On Fri, Apr 12, 2024 at 01:32:40PM +0530, Himal Prasad Ghimiray wrote:
>The drmm_add_action_or_reset function automatically invokes the action
>(sysfs removal) in the event of a failure; therefore, there's no
>necessity to call it within the return check.
>
>Modify the return type of xe_gt_ccs_mode_sysfs_init to int, allowing the
>caller to pass errors up the call chain. Should sysfs creation or
>drmm_add_action_or_reset fail, error propagation will prompt a driver
>load abort.
>
>Fixes: f3bc5bb4d53d ("drm/xe: Allow userspace to configure CCS mode")
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
>Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
>---
> drivers/gpu/drm/xe/xe_gt.c          |  5 ++++-
> drivers/gpu/drm/xe/xe_gt_ccs_mode.c | 17 +++++++----------
> drivers/gpu/drm/xe/xe_gt_ccs_mode.h |  2 +-
> 3 files changed, 12 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>index 2421a8d34324..dca0e9fb3315 100644
>--- a/drivers/gpu/drm/xe/xe_gt.c
>+++ b/drivers/gpu/drm/xe/xe_gt.c
>@@ -379,7 +379,9 @@ static int gt_fw_domain_init(struct xe_gt *gt)
> 			 err);
>
> 	/* Initialize CCS mode sysfs after early initialization of HW engines */
>-	xe_gt_ccs_mode_sysfs_init(gt);
>+	err = xe_gt_ccs_mode_sysfs_init(gt);
>+	if (err)
>+		goto err_gt_sysfs_create;
>
> 	/*
> 	 * Stash hardware-reported version.  Since this register does not exist
>@@ -395,6 +397,7 @@ static int gt_fw_domain_init(struct xe_gt *gt)
>
> err_force_wake:
> 	dump_pat_on_error(gt);
>+err_gt_sysfs_create:
> 	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);

gt_fw_domain_init() is a mess: this function is not really about
gt_fw_domain_init() and the error unwinding is wrong. But let's not add
to it.

this jump here in the middle, leaving dump_pat_on_error() out doesn't
seem good as it breaks the unwind orde. Let's just jump to err_force_wake
and clean it up further later?

otherwise, lgtm.

	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi

> err_hw_fence_irq:
> 	for (i = 0; i < XE_ENGINE_CLASS_MAX; ++i)
>diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>index 529fc286cd06..67a5bd71770d 100644
>--- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>+++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
>@@ -167,25 +167,22 @@ static void xe_gt_ccs_mode_sysfs_fini(struct drm_device *drm, void *arg)
>  * and it is expected that there are no open drm clients while doing so.
>  * The number of available compute slices is exposed to user through a per-gt
>  * 'num_cslices' sysfs interface.
>+ *
>+ * Returns: Returns error value for failure and 0 for success.
>  */
>-void xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt)
>+int xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt)
> {
> 	struct xe_device *xe = gt_to_xe(gt);
> 	int err;
>
> 	if (!xe_gt_ccs_mode_enabled(gt))
>-		return;
>+		return 0;
>
> 	err = sysfs_create_files(gt->sysfs, gt_ccs_mode_attrs);
> 	if (err) {
>-		drm_warn(&xe->drm, "Sysfs creation for ccs_mode failed err: %d\n", err);
>-		return;
>+		drm_err(&xe->drm, "Sysfs creation for ccs_mode failed err: %d\n", err);
>+		return err;
> 	}
>
>-	err = drmm_add_action_or_reset(&xe->drm, xe_gt_ccs_mode_sysfs_fini, gt);
>-	if (err) {
>-		sysfs_remove_files(gt->sysfs, gt_ccs_mode_attrs);
>-		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset failed, err: %d\n",
>-			 __func__, err);
>-	}
>+	return drmm_add_action_or_reset(&xe->drm, xe_gt_ccs_mode_sysfs_fini, gt);
> }
>diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>index f39975aaaab0..f8779852cf0d 100644
>--- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>+++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.h
>@@ -12,7 +12,7 @@
> #include "xe_platform_types.h"
>
> void xe_gt_apply_ccs_mode(struct xe_gt *gt);
>-void xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt);
>+int xe_gt_ccs_mode_sysfs_init(struct xe_gt *gt);
>
> static inline bool xe_gt_ccs_mode_enabled(const struct xe_gt *gt)
> {
>-- 
>2.25.1
>