Re: [PATCH] drm/xe: Check return values of functions in xe_gt_shutdown()

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

On 23-09-2024 17:27, Jani Nikula wrote:
> On Mon, 23 Sep 2024, apoorva.singh@intel.com wrote:
>> From: Apoorva Singh <apoorva.singh@intel.com>
>>
>> Check the return values of the functions xe_force_wake_get()
>> and xe_force_wake_put() to prevent mistakenly treating them as
>> void returns.
>>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Signed-off-by: Apoorva Singh <apoorva.singh@intel.com>
>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_gt.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 274737417b0f..eaeaae1df198 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -890,9 +890,9 @@ int xe_gt_suspend(struct xe_gt *gt)
>>   
>>   void xe_gt_shutdown(struct xe_gt *gt)
>>   {
>> -	xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +	XE_WARN_ON(xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL));
>>   	do_gt_reset(gt);
>> -	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +	XE_WARN_ON(xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL));
> 
> Up to the xe maintainers, but personally I very much dislike "hiding"
> functional stuff inside *WARN_ON(). It's harder to read. IMO it should
> be only for checks, and functions without side effects.

usage of XE_WARN_ON for force_wake_get/force_wake_put will be removed in 
all the places. Below series covers it
https://lore.kernel.org/intel-xe/20240924121641.1045763-1-himal.prasad.ghimiray@intel.com/T/#t

@Apoorva,
If these cleanup is not of urgency lets hold on till above series gets 
concluded and merged.

If this is required to be addressed urgently and current version is not 
acceptable something like below will help

int err;
err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
xe_gt_WARN(gt, err, "Acknowledgment for domain awake timedout");
do_gt_reset(gt);
err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
xe_gt_WARN(gt, err, "Acknowledgment for domain sleep timedout");

BR
Himal

> 
> And while we're at it, unrelated to this patch, what's the point of
> XE_WARN_ON?
> 
> It's defined to be just WARN_ON. But for multi-GPU systems you'd benefit
> from using drm_WARN_ON() with device info. But for that you'd need to
> pass drm device, and a simple redefinition doesn't even work.
> 
> There are a little over 100 uses, but more seem to crop up all the time,
> and people love to cargo cult this kind of stuff. It's not too late to
> change course now, but it's a royal PITA to change this when you have 1k
> of them.


> 
> 
> BR,
> Jani.
> 
> 
>>   }
>>   
>>   /**
>