Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/uc: Fix issues with overriding firmware files

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

On 9/13/2022 5:58 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The earlier update to support reduced versioning of firmware files
> introduced an issue with the firmware override module parameter. If an
> invalid file was specified then an infinite loop could occur trying to
> find a backup firmware.
>
> The fix is that if an explicit override has been set, then don't scan
> for backup options because there is no point anyway. The user wanted X
> and if X is not available, that's their problem.
>
> This patch also fixes up the scanning loop code so that if an invalid
> file is passed in, it will exit rather than loop forever. So if the
> impossible situation did somehow occur in the future, it wouldn't be
> such a big problem.
>
> v2: Also remove ANSI colour codes that accidentally got left in an
> error message in the original patch.

With the commit message updated to include what you mentioned in your 
reply, this is:

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

Daniele

>
> Fixes: 665ae9c9ca79 ("drm/i915/uc: Support for version reduced and multiple
> firmware files")
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 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 af425916cdf64..1169e2a09da24 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -232,6 +232,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   	u32 fw_count;
>   	u8 rev = INTEL_REVID(i915);
>   	int i;
> +	bool found;
>   
>   	/*
>   	 * The only difference between the ADL GuC FWs is the HWConfig support.
> @@ -246,6 +247,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   	fw_blobs = blobs_all[uc_fw->type].blobs;
>   	fw_count = blobs_all[uc_fw->type].count;
>   
> +	found = false;
>   	for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
>   		const struct uc_fw_blob *blob = &fw_blobs[i].blob;
>   
> @@ -266,9 +268,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   		uc_fw->file_wanted.path = blob->path;
>   		uc_fw->file_wanted.major_ver = blob->major;
>   		uc_fw->file_wanted.minor_ver = blob->minor;
> +		found = true;
>   		break;
>   	}
>   
> +	if (!found && uc_fw->file_selected.path) {
> +		/* Failed to find a match for the last attempt?! */
> +		uc_fw->file_selected.path = NULL;
> +	}
> +
>   	/* make sure the list is ordered as expected */
>   	if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified) {
>   		verified = true;
> @@ -322,7 +330,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   				continue;
>   
>   bad:
> -			drm_err(&i915->drm, "\x1B[35;1mInvalid FW blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
> +			drm_err(&i915->drm, "Invalid FW blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
>   				intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 1].rev,
>   				fw_blobs[i - 1].blob.legacy ? "L" : "v",
>   				fw_blobs[i - 1].blob.major,
> @@ -553,10 +561,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   
>   	err = firmware_request_nowarn(&fw, uc_fw->file_selected.path, dev);
>   	memcpy(&file_ideal, &uc_fw->file_wanted, sizeof(file_ideal));
> -	if (!err || intel_uc_fw_is_overridden(uc_fw))
> -		goto done;
> +
> +	/* Any error is terminal if overriding. Don't bother searching for older versions */
> +	if (err && intel_uc_fw_is_overridden(uc_fw))
> +		goto fail;
>   
>   	while (err == -ENOENT) {
> +		old_ver = true;
> +
>   		__uc_fw_auto_select(i915, uc_fw);
>   		if (!uc_fw->file_selected.path) {
>   			/*
> @@ -576,8 +588,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   	if (err)
>   		goto fail;
>   
> -	old_ver = true;
> -done:
>   	if (uc_fw->loaded_via_gsc)
>   		err = check_gsc_manifest(fw, uc_fw);
>   	else