Re: [Intel-gfx] [PATCH 5/5] drm/i915/uc: Reject doplicate entries in firmware table

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

Typo doplicate in patch title

On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> It was noticed that duplicte entries in the firmware table could cause

typo duplicte

> an infinite loop in the firmware loading code if that entry failed to
> load. Duplicate entries are a bug anyway and so should never happen.
> Ensure they don't by tweaking the table validation code to reject
> duplicates.

Here you're not really rejecting anything though, just printing an error 
(and even that only if the SELFTEST kconfig is selected). This would 
allow our CI to catch issues with patches sent to our ML, but IIRC the 
reported bug was on a kernel fork. We could disable the FW loading is 
the table for that particular blob type is in an invalid state, as it 
wouldn't be safe to attempt a load in that case anyway.

>
> For full m/m/p files, that can be done by simply tweaking the patch
> level check to reject matching values. For reduced version entries,
> the filename itself must be compared.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 27 +++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 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 c589782467265..44829247ef6bc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -319,7 +319,7 @@ static void validate_fw_table_type(struct drm_i915_private *i915, enum intel_uc_
>   {
>   	const struct uc_fw_platform_requirement *fw_blobs;
>   	u32 fw_count;
> -	int i;
> +	int i, j;
>   
>   	if (type >= ARRAY_SIZE(blobs_all)) {
>   		drm_err(&i915->drm, "No blob array for %s\n", intel_uc_fw_type_repr(type));
> @@ -334,6 +334,27 @@ static void validate_fw_table_type(struct drm_i915_private *i915, enum intel_uc_
>   
>   	/* make sure the list is ordered as expected */
>   	for (i = 1; i < fw_count; i++) {
> +		/* Versionless file names must be unique per platform: */
> +		for (j = i + 1; j < fw_count; j++) {
> +			/* Same platform? */
> +			if (fw_blobs[i].p != fw_blobs[j].p)
> +				continue;
> +
> +			if (fw_blobs[i].blob.path != fw_blobs[j].blob.path)
> +				continue;
> +
> +			drm_err(&i915->drm, "Diplicaate %s blobs: %s r%u %s%d.%d.%d [%s] matches %s r%u %s%d.%d.%d [%s]\n",

Typo Diplicaate

Daniele

> +				intel_uc_fw_type_repr(type),
> +				intel_platform_name(fw_blobs[j].p), fw_blobs[j].rev,
> +				fw_blobs[j].blob.legacy ? "L" : "v",
> +				fw_blobs[j].blob.major, fw_blobs[j].blob.minor,
> +				fw_blobs[j].blob.patch, fw_blobs[j].blob.path,
> +				intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
> +				fw_blobs[i].blob.legacy ? "L" : "v",
> +				fw_blobs[i].blob.major, fw_blobs[i].blob.minor,
> +				fw_blobs[i].blob.patch, fw_blobs[i].blob.path);
> +		}
> +
>   		/* Next platform is good: */
>   		if (fw_blobs[i].p < fw_blobs[i - 1].p)
>   			continue;
> @@ -377,8 +398,8 @@ static void validate_fw_table_type(struct drm_i915_private *i915, enum intel_uc_
>   		if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
>   			goto bad;
>   
> -		/* Patch versions must be in order: */
> -		if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
> +		/* Patch versions must be in order and unique: */
> +		if (fw_blobs[i].blob.patch < fw_blobs[i - 1].blob.patch)
>   			continue;
>   
>   bad: