Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc: Make unexpected firmware versions an error in debug builds

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

On 4/20/2023 6:15 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> If the DEBUG_GEM config option is set then escalate the 'unexpected
> firmware version' message from a notice to an error. This will ensure
> that the CI system treats such occurences as a failure and logs a bug
> about it (or fails the pre-merge testing).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 34 ++++++++++++++----------
>   1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index bc4011d55667c..c5b21d17ca437 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -17,6 +17,12 @@
>   #include "i915_drv.h"
>   #include "i915_reg.h"
>   
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> +#define UNEXPECTED	gt_err
> +#else
> +#define UNEXPECTED	gt_notice

I tried to find an alternative word that had the same number of 
characters as "gt_notice" so that we could avoid the indent in the diff, 
but nothing came to mind, so:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> +#endif
> +
>   static inline struct intel_gt *
>   ____uc_fw_to_gt(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
>   {
> @@ -828,10 +834,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   	if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
>   		/* Check the file's major version was as it claimed */
>   		if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
> -			gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> -				  intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
> -				  uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
> -				  uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
> +			UNEXPECTED(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> +				   intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
> +				   uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
> +				   uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
>   			if (!intel_uc_fw_is_overridden(uc_fw)) {
>   				err = -ENOEXEC;
>   				goto fail;
> @@ -849,16 +855,16 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   		/* Preserve the version that was really wanted */
>   		memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>   
> -		gt_notice(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
> -			  intel_uc_fw_type_repr(uc_fw->type),
> -			  uc_fw->file_wanted.path,
> -			  uc_fw->file_wanted.ver.major,
> -			  uc_fw->file_wanted.ver.minor,
> -			  uc_fw->file_wanted.ver.patch,
> -			  uc_fw->file_selected.path,
> -			  uc_fw->file_selected.ver.major,
> -			  uc_fw->file_selected.ver.minor,
> -			  uc_fw->file_selected.ver.patch);
> +		UNEXPECTED(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
> +			   intel_uc_fw_type_repr(uc_fw->type),
> +			   uc_fw->file_wanted.path,
> +			   uc_fw->file_wanted.ver.major,
> +			   uc_fw->file_wanted.ver.minor,
> +			   uc_fw->file_wanted.ver.patch,
> +			   uc_fw->file_selected.path,
> +			   uc_fw->file_selected.ver.major,
> +			   uc_fw->file_selected.ver.minor,
> +			   uc_fw->file_selected.ver.patch);
>   		gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
>   			INTEL_UC_FIRMWARE_URL);
>   	}