Re: [PATCH v2 2/2] drm/xe: Check valid domain is passed in xe_force_wake_ref

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 07.06.2024 07:22, Himal Prasad Ghimiray wrote:
> Assert in case of XE_FORCEWAKE_ALL is passed.

We assert that XE_FORCEWAKE_ALL is *not* passed ;)

> 
> v2
> - use domain != XE_FORCEWAKE_ALL (Michal)
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_force_wake.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/xe/xe_force_wake.h
> index 651ea1e62c63..346785df3b49 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.h
> +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> @@ -24,7 +24,7 @@ static inline int
>  xe_force_wake_ref(struct xe_force_wake *fw,
>  		  enum xe_force_wake_domains domain)
>  {
> -	xe_gt_assert(fw->gt, domain);
> +	xe_gt_assert(fw->gt, domain != XE_FORCEWAKE_ALL);

IMO we still need to document our assumptions about enum values (as
kernel-doc for enum xe_force_wake_domains) that all of them but ALL are
single BIT values, so below trick to convert it back to index must work
by design

then we can also add code more consistent with such description:

	xe_gt_assert(fw->gt, is_power_of_2(domain));
	return fw->domains[ilog2(domain)].ref;

but this patch is still a good move, so

Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

>  	return fw->domains[ffs(domain) - 1].ref;
>  }
>