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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 12-04-2024 20:11, Jani Nikula wrote:
> On Fri, 12 Apr 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote:
>> Oh... nit on commit message "Incase" is not a proper word. Same on other
>> places in this series. Please use "In case" when re-spinning this series.
> Or just "remove sysfs only once on action add failure" or something.
Noted.
>
>> 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@intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@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);
>>> 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
>>>