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

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

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.

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.


>  }
>  
>  /**

-- 
Jani Nikula, Intel