[Intel-gfx] [PATCH 0/5] Improvements to uc firmare management

[Intel-gfx] [PATCH 0/5] Improvements to uc firmare management

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Enhance the firmware table verification code to catch more potential
errors and to generally improve the code itself.

Track patch level version even on reduced version files to allow user
notification of missing bug fixes.

Detect another immediate failure case when loading GuC firmware.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>


John Harrison (5):
  drm/i915/guc: Decode another GuC load failure case
  drm/i915/guc: Print status register when waiting for GuC to load
  drm/i915/uc: Track patch level versions on reduced version firmware
    files
  drm/i915/uc: Split firmware table validation to a separate function
  drm/i915/uc: Reject doplicate entries in firmware table

 .../gpu/drm/i915/gt/uc/abi/guc_errors_abi.h   |   1 +
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c     |  12 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      | 222 ++++++++++++------
 3 files changed, 159 insertions(+), 76 deletions(-)

-- 
2.39.1

[Intel-gfx] [PATCH 1/5] drm/i915/guc: Decode another GuC load failure case

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Explain another potential firmware failure mode and early exit the
long wait if hit.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 1 +
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c       | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
index bcb1129b36102..dabeaf4f245f3 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
@@ -44,6 +44,7 @@ enum intel_guc_load_status {
 enum intel_bootrom_load_status {
 	INTEL_BOOTROM_STATUS_NO_KEY_FOUND                 = 0x13,
 	INTEL_BOOTROM_STATUS_AES_PROD_KEY_FOUND           = 0x1A,
+	INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE       = 0x2B,
 	INTEL_BOOTROM_STATUS_RSA_FAILED                   = 0x50,
 	INTEL_BOOTROM_STATUS_PAVPC_FAILED                 = 0x73,
 	INTEL_BOOTROM_STATUS_WOPCM_FAILED                 = 0x74,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 6fda3aec5c66a..0ff088a5e51a8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -129,6 +129,7 @@ static inline bool guc_load_done(struct intel_uncore *uncore, u32 *status, bool
 	case INTEL_BOOTROM_STATUS_RC6CTXCONFIG_FAILED:
 	case INTEL_BOOTROM_STATUS_MPUMAP_INCORRECT:
 	case INTEL_BOOTROM_STATUS_EXCEPTION:
+	case INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE:
 		*success = false;
 		return true;
 	}
@@ -219,6 +220,11 @@ static int guc_wait_ucode(struct intel_guc *guc)
 			guc_info(guc, "firmware signature verification failed\n");
 			ret = -ENOEXEC;
 			break;
+
+		case INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE:
+			guc_info(guc, "firmware production part check failure\n");
+			ret = -ENOEXEC;
+			break;
 		}
 
 		switch (ukernel) {
-- 
2.39.1

[Intel-gfx] [PATCH 2/5] drm/i915/guc: Print status register when waiting for GuC to load

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

If the GuC load is taking an excessively long time, the wait loop
currently prints the GT frequency. Extend that to include the GuC
status as well so we can see if the GuC is actually making progress or
not.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
index 0ff088a5e51a8..364d0d546ec82 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
@@ -191,8 +191,10 @@ static int guc_wait_ucode(struct intel_guc *guc)
 		if (!ret || !success)
 			break;
 
-		guc_dbg(guc, "load still in progress, count = %d, freq = %dMHz\n",
-			count, intel_rps_read_actual_frequency(&uncore->gt->rps));
+		guc_dbg(guc, "load still in progress, count = %d, freq = %dMHz, status = 0x%08X [0x%02X/%02X]\n",
+			count, intel_rps_read_actual_frequency(&uncore->gt->rps), status,
+			REG_FIELD_GET(GS_BOOTROM_MASK, status),
+			REG_FIELD_GET(GS_UKERNEL_MASK, status));
 	}
 	after = ktime_get();
 	delta = ktime_sub(after, before);
-- 
2.39.1

[Intel-gfx] [PATCH 3/5] drm/i915/uc: Track patch level versions on reduced version firmware files

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

When reduced version firmware files were added (matching major
component being the only strict requirement), the minor version was
still tracked and a notification reported if it was older. However,
the patch version should really be tracked as well for the same
reasons. The KMD can work without the change but if the effort has
been taken to release a new firmware with the change then there must
be a valid reason for doing so - important bug fix, security fix, etc.
And in that case it would be good to alert the user if they are
missing out on that new fix.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 41 +++++++++++++++++-------
 1 file changed, 30 insertions(+), 11 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 a82a53dbbc86d..6bb45d6b8da5f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -80,14 +80,14 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
  */
 #define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_maj, guc_mmp) \
 	fw_def(METEORLAKE,   0, guc_mmp(mtl,  70, 6, 5)) \
-	fw_def(DG2,          0, guc_maj(dg2,  70, 5)) \
-	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5)) \
+	fw_def(DG2,          0, guc_maj(dg2,  70, 5, 4)) \
+	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 4)) \
 	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 70, 1, 1)) \
 	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 69, 0, 3)) \
-	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5)) \
+	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5, 4)) \
 	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  70, 1, 1)) \
 	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  69, 0, 3)) \
-	fw_def(DG1,          0, guc_maj(dg1,  70, 5)) \
+	fw_def(DG1,          0, guc_maj(dg1,  70, 5, 4)) \
 	fw_def(ROCKETLAKE,   0, guc_mmp(tgl,  70, 1, 1)) \
 	fw_def(TIGERLAKE,    0, guc_mmp(tgl,  70, 1, 1)) \
 	fw_def(JASPERLAKE,   0, guc_mmp(ehl,  70, 1, 1)) \
@@ -141,7 +141,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
 	__stringify(patch_) ".bin"
 
 /* Minor for internal driver use, not part of file name */
-#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_) \
+#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_) \
 	__MAKE_UC_FW_PATH_MAJOR(prefix_, "guc", major_)
 
 #define MAKE_GUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \
@@ -197,9 +197,9 @@ struct __packed uc_fw_blob {
 	{ UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
 	  .legacy = true }
 
-#define GUC_FW_BLOB(prefix_, major_, minor_) \
-	UC_FW_BLOB_NEW(major_, minor_, 0, false, \
-		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_))
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
+		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))
 
 #define GUC_FW_BLOB_MMP(prefix_, major_, minor_, patch_) \
 	UC_FW_BLOB_OLD(major_, minor_, patch_, \
@@ -296,6 +296,7 @@ __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.ver.major = blob->major;
 		uc_fw->file_wanted.ver.minor = blob->minor;
+		uc_fw->file_wanted.ver.patch = blob->patch;
 		uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
 		found = true;
 		break;
@@ -776,6 +777,17 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC && !guc_check_version_range(uc_fw))
 		goto fail;
 
+	gt_info(gt, "%s firmware: wanted = %s / %d.%d.%d, got = %s / %d.%d.%d\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);
+
 	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) {
@@ -790,6 +802,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 		} else {
 			if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor)
 				old_ver = true;
+			else if ((uc_fw->file_selected.ver.minor == uc_fw->file_wanted.ver.minor) &&
+				 (uc_fw->file_selected.ver.patch < uc_fw->file_wanted.ver.patch))
+				old_ver = true;
 		}
 	}
 
@@ -797,12 +812,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) is recommended, but only %s (%d.%d) was found\n",
+		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.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.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);
 	}
-- 
2.39.1

[Intel-gfx] [PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The validation of the firmware table was being done inside the code
for scanning the table for the next available firmware blob. Which is
unnecessary. Potentially, it should be a selftest. But either way, the
first step is pulling it out into a separate function that can be
called just once rather than once per blob attempt per blob type.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 164 ++++++++++++++---------
 1 file changed, 99 insertions(+), 65 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 6bb45d6b8da5f..c589782467265 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -233,20 +233,22 @@ struct fw_blobs_by_type {
 	u32 count;
 };
 
+static const struct uc_fw_platform_requirement blobs_guc[] = {
+	INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
+};
+
+static const struct uc_fw_platform_requirement blobs_huc[] = {
+	INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
+};
+
+static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = {
+	[INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
+	[INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
+};
+
 static void
 __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 {
-	static const struct uc_fw_platform_requirement blobs_guc[] = {
-		INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
-	};
-	static const struct uc_fw_platform_requirement blobs_huc[] = {
-		INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
-	};
-	static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = {
-		[INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
-		[INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
-	};
-	static bool verified[INTEL_UC_FW_NUM_TYPES];
 	const struct uc_fw_platform_requirement *fw_blobs;
 	enum intel_platform p = INTEL_INFO(i915)->platform;
 	u32 fw_count;
@@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 			continue;
 
 		if (uc_fw->file_selected.path) {
+			/*
+			 * Continuing an earlier search after a found blob failed to load.
+			 * Once the previously chosen path has been found, clear it out
+			 * and let the search continue from there.
+			 */
 			if (uc_fw->file_selected.path == blob->path)
 				uc_fw->file_selected.path = NULL;
 
@@ -306,78 +313,103 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 		/* 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[uc_fw->type]) {
-		verified[uc_fw->type] = true;
+static void validate_fw_table_type(struct drm_i915_private *i915, enum intel_uc_fw_type type)
+{
+	const struct uc_fw_platform_requirement *fw_blobs;
+	u32 fw_count;
+	int i;
 
-		for (i = 1; i < fw_count; i++) {
-			/* Next platform is good: */
-			if (fw_blobs[i].p < fw_blobs[i - 1].p)
-				continue;
+	if (type >= ARRAY_SIZE(blobs_all)) {
+		drm_err(&i915->drm, "No blob array for %s\n", intel_uc_fw_type_repr(type));
+		return;
+	}
 
-			/* Next platform revision is good: */
-			if (fw_blobs[i].p == fw_blobs[i - 1].p &&
-			    fw_blobs[i].rev < fw_blobs[i - 1].rev)
-				continue;
+	fw_blobs = blobs_all[type].blobs;
+	fw_count = blobs_all[type].count;
 
-			/* Platform/revision must be in order: */
-			if (fw_blobs[i].p != fw_blobs[i - 1].p ||
-			    fw_blobs[i].rev != fw_blobs[i - 1].rev)
-				goto bad;
+	if (!fw_count)
+		return;
 
-			/* Next major version is good: */
-			if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
-				continue;
+	/* make sure the list is ordered as expected */
+	for (i = 1; i < fw_count; i++) {
+		/* Next platform is good: */
+		if (fw_blobs[i].p < fw_blobs[i - 1].p)
+			continue;
 
-			/* New must be before legacy: */
-			if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
-				goto bad;
+		/* Next platform revision is good: */
+		if (fw_blobs[i].p == fw_blobs[i - 1].p &&
+		    fw_blobs[i].rev < fw_blobs[i - 1].rev)
+			continue;
 
-			/* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y (GuC) */
-			if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
-				if (!fw_blobs[i - 1].blob.major)
-					continue;
+		/* Platform/revision must be in order: */
+		if (fw_blobs[i].p != fw_blobs[i - 1].p ||
+		    fw_blobs[i].rev != fw_blobs[i - 1].rev)
+			goto bad;
 
-				if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
-					continue;
-			}
+		/* Next major version is good: */
+		if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
+			continue;
 
-			/* Major versions must be in order: */
-			if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
-				goto bad;
+		/* New must be before legacy: */
+		if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
+			goto bad;
 
-			/* Next minor version is good: */
-			if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+		/* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y (GuC) */
+		if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
+			if (!fw_blobs[i - 1].blob.major)
 				continue;
 
-			/* Minor versions must be in order: */
-			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)
+			if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
 				continue;
+		}
+
+		/* Major versions must be in order: */
+		if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
+			goto bad;
+
+		/* Next minor version is good: */
+		if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+			continue;
+
+		/* Minor versions must be in order: */
+		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)
+			continue;
 
 bad:
-			drm_err(&i915->drm, "Invalid %s blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
-				intel_uc_fw_type_repr(uc_fw->type),
-				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,
-				fw_blobs[i - 1].blob.minor,
-				fw_blobs[i - 1].blob.patch,
-				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);
-
-			uc_fw->file_selected.path = NULL;
-		}
+		drm_err(&i915->drm, "Invalid %s blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
+			intel_uc_fw_type_repr(type),
+			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,
+			fw_blobs[i - 1].blob.minor,
+			fw_blobs[i - 1].blob.patch,
+			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);
 	}
 }
 
+static void validate_fw_table(struct drm_i915_private *i915)
+{
+	enum intel_uc_fw_type type;
+	static bool done;
+
+	if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST) || done)
+		return;
+	done = true;
+
+	for (type = 0; type < INTEL_UC_FW_NUM_TYPES; type++)
+		validate_fw_table_type(i915, type);
+}
+
 static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
 {
 	if (i915->params.enable_guc & ENABLE_GUC_MASK)
@@ -432,6 +464,8 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 {
 	struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915;
 
+	validate_fw_table(i915);
+
 	/*
 	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
 	 * before we're looked at the HW caps to see if we have uc support
-- 
2.39.1

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

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

It was noticed that duplicte entries in the firmware table could cause
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.

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",
+				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:
-- 
2.39.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Improvements to uc firmare management

[Patchwork]

== Series Details ==

Series: Improvements to uc firmare management
URL   : https://patchwork.freedesktop.org/series/116517/
State : warning

== Summary ==

Error: dim checkpatch failed
e2f47c7391c0 drm/i915/guc: Decode another GuC load failure case
a317c98847de drm/i915/guc: Print status register when waiting for GuC to load
b7ccc12b47a8 drm/i915/uc: Track patch level versions on reduced version firmware files
-:58: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#58: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:200:
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
+		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))

-:58: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#58: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:200:
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
+		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))

-:58: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'patch_' - possible side-effects?
#58: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:200:
+#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
+	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
+		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))

total: 0 errors, 0 warnings, 3 checks, 90 lines checked
aebc3a2f1989 drm/i915/uc: Split firmware table validation to a separate function
6ef9c0062ed6 drm/i915/uc: Reject doplicate entries in firmware table

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Improvements to uc firmare management

[Patchwork]

== Series Details ==

Series: Improvements to uc firmare management
URL   : https://patchwork.freedesktop.org/series/116517/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for Improvements to uc firmare management

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6768 bytes --]

== Series Details ==

Series: Improvements to uc firmare management
URL   : https://patchwork.freedesktop.org/series/116517/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13009 -> Patchwork_116517v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/index.html

Participating hosts (37 -> 37)
------------------------------

  Additional (1): fi-kbl-soraka 
  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_116517v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         NOTRUN -> [ABORT][1] ([i915#6687] / [i915#7978])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#2190])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) +3 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][4] -> [DMESG-FAIL][5] ([i915#5334])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][6] ([i915#1886])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@i915_selftest@live@hangcheck:
    - fi-skl-guc:         [PASS][7] -> [DMESG-WARN][8] ([i915#8073])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-2:         [PASS][9] -> [ABORT][10] ([i915#4983] / [i915#7913])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/bat-rpls-2/igt@i915_selftest@live@requests.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-rpls-2/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         NOTRUN -> [DMESG-FAIL][11] ([i915#6367])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-rpls-1/igt@i915_selftest@live@slpc.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][12] ([fdo#109271]) +16 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-adlp-9:         NOTRUN -> [SKIP][13] ([i915#3546]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - bat-atsm-1:         [DMESG-FAIL][14] ([i915#7699]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-atsm-1/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@mman:
    - bat-rpls-1:         [TIMEOUT][16] ([i915#6794]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/bat-rpls-1/igt@i915_selftest@live@mman.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-rpls-1/igt@i915_selftest@live@mman.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-rpls-1:         [ABORT][18] ([i915#6687] / [i915#7978]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/bat-rpls-1/igt@i915_suspend@basic-s3-without-i915.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-rpls-1/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1:
    - bat-dg2-8:          [FAIL][20] ([i915#7932]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073


Build changes
-------------

  * Linux: CI_DRM_13009 -> Patchwork_116517v1

  CI-20190529: 20190529
  CI_DRM_13009: 766ccc538115e757a747d12d1bf7a714d1424ae5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7256: 066fa5410180730b85f61e4f3073da9a2055dc49 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v1: 766ccc538115e757a747d12d1bf7a714d1424ae5 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

55c8ccc66f9a drm/i915/uc: Reject doplicate entries in firmware table
bf6562e42c5e drm/i915/uc: Split firmware table validation to a separate function
1f998cb2fd4f drm/i915/uc: Track patch level versions on reduced version firmware files
db9f3a63f45b drm/i915/guc: Print status register when waiting for GuC to load
ea5bc9597b73 drm/i915/guc: Decode another GuC load failure case

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/index.html

[-- Attachment #2: Type: text/html, Size: 8008 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for Improvements to uc firmare management

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 10763 bytes --]

== Series Details ==

Series: Improvements to uc firmare management
URL   : https://patchwork.freedesktop.org/series/116517/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13009_full -> Patchwork_116517v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Known issues
------------

  Here are the changes found in Patchwork_116517v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-deadline:
    - shard-apl:          [PASS][1] -> [FAIL][2] ([i915#2846])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-apl7/igt@gem_exec_fair@basic-deadline.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-apl7/igt@gem_exec_fair@basic-deadline.html
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#2846])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-glk3/igt@gem_exec_fair@basic-deadline.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk1/igt@gem_exec_fair@basic-deadline.html

  * igt@kms_big_fb@linear-16bpp-rotate-270:
    - shard-glk:          NOTRUN -> [SKIP][5] ([fdo#109271]) +42 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk6/igt@kms_big_fb@linear-16bpp-rotate-270.html

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic-y_tiled_gen12_mc_ccs:
    - shard-glk:          NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#3886]) +2 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk6/igt@kms_ccs@pipe-b-crc-sprite-planes-basic-y_tiled_gen12_mc_ccs.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [PASS][7] -> [FAIL][8] ([i915#2346])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
    - shard-glk:          [PASS][9] -> [FAIL][10] ([i915#2346])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-glk3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-snb:          [PASS][11] -> [INCOMPLETE][12] ([i915#7878])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-snb5/igt@kms_fbcon_fbt@fbc-suspend.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-snb7/igt@kms_fbcon_fbt@fbc-suspend.html

  * igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area:
    - shard-glk:          NOTRUN -> [SKIP][13] ([fdo#109271] / [i915#658])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk6/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@virtual-idle:
    - {shard-rkl}:        [FAIL][14] ([i915#7742]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-rkl-3/igt@drm_fdinfo@virtual-idle.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-rkl-7/igt@drm_fdinfo@virtual-idle.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][16] ([i915#2842]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-apl4/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-apl7/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-none@bcs0:
    - {shard-rkl}:        [FAIL][18] ([i915#2842]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-rkl-2/igt@gem_exec_fair@basic-none@bcs0.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-rkl-1/igt@gem_exec_fair@basic-none@bcs0.html

  * igt@gem_sync@basic-store-all:
    - shard-glk:          [TIMEOUT][20] -> [PASS][21] +5 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-glk2/igt@gem_sync@basic-store-all.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-glk6/igt@gem_sync@basic-store-all.html

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - {shard-rkl}:        [SKIP][22] ([i915#1397]) -> [PASS][23] +2 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-rkl-7/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-rkl-1/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html

  * igt@perf_pmu@idle@rcs0:
    - {shard-rkl}:        [FAIL][24] ([i915#4349]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13009/shard-rkl-6/igt@perf_pmu@idle@rcs0.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/shard-rkl-6/igt@perf_pmu@idle@rcs0.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3936]: https://gitlab.freedesktop.org/drm/intel/issues/3936
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4936]: https://gitlab.freedesktop.org/drm/intel/issues/4936
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5493]: https://gitlab.freedesktop.org/drm/intel/issues/5493
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7878]: https://gitlab.freedesktop.org/drm/intel/issues/7878
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8228]: https://gitlab.freedesktop.org/drm/intel/issues/8228
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292


Build changes
-------------

  * Linux: CI_DRM_13009 -> Patchwork_116517v1

  CI-20190529: 20190529
  CI_DRM_13009: 766ccc538115e757a747d12d1bf7a714d1424ae5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7256: 066fa5410180730b85f61e4f3073da9a2055dc49 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v1: 766ccc538115e757a747d12d1bf7a714d1424ae5 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v1/index.html

[-- Attachment #2: Type: text/html, Size: 7959 bytes --]

Re: [Intel-gfx] [PATCH 2/5] drm/i915/guc: Print status register when waiting for GuC to load

[Ceraolo Spurio, Daniele]

On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> If the GuC load is taking an excessively long time, the wait loop
> currently prints the GT frequency. Extend that to include the GuC
> status as well so we can see if the GuC is actually making progress or
> not.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

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

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> index 0ff088a5e51a8..364d0d546ec82 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -191,8 +191,10 @@ static int guc_wait_ucode(struct intel_guc *guc)
>   		if (!ret || !success)
>   			break;
>   
> -		guc_dbg(guc, "load still in progress, count = %d, freq = %dMHz\n",
> -			count, intel_rps_read_actual_frequency(&uncore->gt->rps));
> +		guc_dbg(guc, "load still in progress, count = %d, freq = %dMHz, status = 0x%08X [0x%02X/%02X]\n",
> +			count, intel_rps_read_actual_frequency(&uncore->gt->rps), status,
> +			REG_FIELD_GET(GS_BOOTROM_MASK, status),
> +			REG_FIELD_GET(GS_UKERNEL_MASK, status));
>   	}
>   	after = ktime_get();
>   	delta = ktime_sub(after, before);

Re: [Intel-gfx] [PATCH 1/5] drm/i915/guc: Decode another GuC load failure case

[Ceraolo Spurio, Daniele]

On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> Explain another potential firmware failure mode and early exit the
> long wait if hit.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>


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

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h | 1 +
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c       | 6 ++++++
>   2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> index bcb1129b36102..dabeaf4f245f3 100644
> --- a/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> +++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_errors_abi.h
> @@ -44,6 +44,7 @@ enum intel_guc_load_status {
>   enum intel_bootrom_load_status {
>   	INTEL_BOOTROM_STATUS_NO_KEY_FOUND                 = 0x13,
>   	INTEL_BOOTROM_STATUS_AES_PROD_KEY_FOUND           = 0x1A,
> +	INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE       = 0x2B,
>   	INTEL_BOOTROM_STATUS_RSA_FAILED                   = 0x50,
>   	INTEL_BOOTROM_STATUS_PAVPC_FAILED                 = 0x73,
>   	INTEL_BOOTROM_STATUS_WOPCM_FAILED                 = 0x74,
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> index 6fda3aec5c66a..0ff088a5e51a8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> @@ -129,6 +129,7 @@ static inline bool guc_load_done(struct intel_uncore *uncore, u32 *status, bool
>   	case INTEL_BOOTROM_STATUS_RC6CTXCONFIG_FAILED:
>   	case INTEL_BOOTROM_STATUS_MPUMAP_INCORRECT:
>   	case INTEL_BOOTROM_STATUS_EXCEPTION:
> +	case INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE:
>   		*success = false;
>   		return true;
>   	}
> @@ -219,6 +220,11 @@ static int guc_wait_ucode(struct intel_guc *guc)
>   			guc_info(guc, "firmware signature verification failed\n");
>   			ret = -ENOEXEC;
>   			break;
> +
> +		case INTEL_BOOTROM_STATUS_PROD_KEY_CHECK_FAILURE:
> +			guc_info(guc, "firmware production part check failure\n");
> +			ret = -ENOEXEC;
> +			break;
>   		}
>   
>   		switch (ukernel) {

Re: [Intel-gfx] [PATCH 3/5] drm/i915/uc: Track patch level versions on reduced version firmware files

[Ceraolo Spurio, Daniele]

On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> When reduced version firmware files were added (matching major
> component being the only strict requirement), the minor version was
> still tracked and a notification reported if it was older. However,
> the patch version should really be tracked as well for the same
> reasons. The KMD can work without the change but if the effort has
> been taken to release a new firmware with the change then there must
> be a valid reason for doing so - important bug fix, security fix, etc.
> And in that case it would be good to alert the user if they are
> missing out on that new fix.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 41 +++++++++++++++++-------
>   1 file changed, 30 insertions(+), 11 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 a82a53dbbc86d..6bb45d6b8da5f 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -80,14 +80,14 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>    */
>   #define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_maj, guc_mmp) \
>   	fw_def(METEORLAKE,   0, guc_mmp(mtl,  70, 6, 5)) \
> -	fw_def(DG2,          0, guc_maj(dg2,  70, 5)) \
> -	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5)) \
> +	fw_def(DG2,          0, guc_maj(dg2,  70, 5, 4)) \
> +	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 4)) \
>   	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 70, 1, 1)) \
>   	fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 69, 0, 3)) \
> -	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5)) \
> +	fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5, 4)) \
>   	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  69, 0, 3)) \
> -	fw_def(DG1,          0, guc_maj(dg1,  70, 5)) \
> +	fw_def(DG1,          0, guc_maj(dg1,  70, 5, 4)) \
>   	fw_def(ROCKETLAKE,   0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(TIGERLAKE,    0, guc_mmp(tgl,  70, 1, 1)) \
>   	fw_def(JASPERLAKE,   0, guc_mmp(ehl,  70, 1, 1)) \

AFAICS in linux-firmware we don't have any 70.5.4 binaries, the newest 
ones are 70.5.1.

> @@ -141,7 +141,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw,
>   	__stringify(patch_) ".bin"
>   
>   /* Minor for internal driver use, not part of file name */
> -#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_) \
> +#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_) \
>   	__MAKE_UC_FW_PATH_MAJOR(prefix_, "guc", major_)
>   
>   #define MAKE_GUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \
> @@ -197,9 +197,9 @@ struct __packed uc_fw_blob {
>   	{ UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>   	  .legacy = true }
>   
> -#define GUC_FW_BLOB(prefix_, major_, minor_) \
> -	UC_FW_BLOB_NEW(major_, minor_, 0, false, \
> -		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_))
> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
> +	UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
> +		       MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))
>   
>   #define GUC_FW_BLOB_MMP(prefix_, major_, minor_, patch_) \
>   	UC_FW_BLOB_OLD(major_, minor_, patch_, \
> @@ -296,6 +296,7 @@ __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.ver.major = blob->major;
>   		uc_fw->file_wanted.ver.minor = blob->minor;
> +		uc_fw->file_wanted.ver.patch = blob->patch;
>   		uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
>   		found = true;
>   		break;
> @@ -776,6 +777,17 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC && !guc_check_version_range(uc_fw))
>   		goto fail;
>   
> +	gt_info(gt, "%s firmware: wanted = %s / %d.%d.%d, got = %s / %d.%d.%d\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);

Some of the info here is redundant from print_fw_ver(). Can we have a 
single print with all the info we need?
Something like:

GuC firmware i915/mtl_guc_70.bin (v70.6.5, expected v70.6.4 or newer)

Otherwise, I'd suggest demoting this to gt_dbg to avoid printing the 
same thing twice at info verbosity

Daniele

> +
>   	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) {
> @@ -790,6 +802,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   		} else {
>   			if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor)
>   				old_ver = true;
> +			else if ((uc_fw->file_selected.ver.minor == uc_fw->file_wanted.ver.minor) &&
> +				 (uc_fw->file_selected.ver.patch < uc_fw->file_wanted.ver.patch))
> +				old_ver = true;
>   		}
>   	}
>   
> @@ -797,12 +812,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) is recommended, but only %s (%d.%d) was found\n",
> +		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.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.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);
>   	}

Re: [Intel-gfx] [PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function

[Ceraolo Spurio, Daniele]

On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The validation of the firmware table was being done inside the code
> for scanning the table for the next available firmware blob. Which is
> unnecessary. Potentially, it should be a selftest. But either way, the
> first step is pulling it out into a separate function that can be
> called just once rather than once per blob attempt per blob type.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 164 ++++++++++++++---------
>   1 file changed, 99 insertions(+), 65 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 6bb45d6b8da5f..c589782467265 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -233,20 +233,22 @@ struct fw_blobs_by_type {
>   	u32 count;
>   };
>   
> +static const struct uc_fw_platform_requirement blobs_guc[] = {
> +	INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
> +};
> +
> +static const struct uc_fw_platform_requirement blobs_huc[] = {
> +	INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
> +};
> +
> +static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = {
> +	[INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
> +	[INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
> +};
> +
>   static void
>   __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   {
> -	static const struct uc_fw_platform_requirement blobs_guc[] = {
> -		INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
> -	};
> -	static const struct uc_fw_platform_requirement blobs_huc[] = {
> -		INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
> -	};
> -	static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = {
> -		[INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
> -		[INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
> -	};
> -	static bool verified[INTEL_UC_FW_NUM_TYPES];
>   	const struct uc_fw_platform_requirement *fw_blobs;
>   	enum intel_platform p = INTEL_INFO(i915)->platform;
>   	u32 fw_count;
> @@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   			continue;
>   
>   		if (uc_fw->file_selected.path) {
> +			/*
> +			 * Continuing an earlier search after a found blob failed to load.
> +			 * Once the previously chosen path has been found, clear it out
> +			 * and let the search continue from there.
> +			 */
>   			if (uc_fw->file_selected.path == blob->path)
>   				uc_fw->file_selected.path = NULL;
>   
> @@ -306,78 +313,103 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   		/* 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[uc_fw->type]) {
> -		verified[uc_fw->type] = true;
> +static void validate_fw_table_type(struct drm_i915_private *i915, enum intel_uc_fw_type type)
> +{
> +	const struct uc_fw_platform_requirement *fw_blobs;
> +	u32 fw_count;
> +	int i;
>   
> -		for (i = 1; i < fw_count; i++) {
> -			/* Next platform is good: */
> -			if (fw_blobs[i].p < fw_blobs[i - 1].p)
> -				continue;
> +	if (type >= ARRAY_SIZE(blobs_all)) {
> +		drm_err(&i915->drm, "No blob array for %s\n", intel_uc_fw_type_repr(type));
> +		return;
> +	}
>   
> -			/* Next platform revision is good: */
> -			if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> -			    fw_blobs[i].rev < fw_blobs[i - 1].rev)
> -				continue;
> +	fw_blobs = blobs_all[type].blobs;
> +	fw_count = blobs_all[type].count;
>   
> -			/* Platform/revision must be in order: */
> -			if (fw_blobs[i].p != fw_blobs[i - 1].p ||
> -			    fw_blobs[i].rev != fw_blobs[i - 1].rev)
> -				goto bad;
> +	if (!fw_count)
> +		return;
>   
> -			/* Next major version is good: */
> -			if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
> -				continue;
> +	/* make sure the list is ordered as expected */
> +	for (i = 1; i < fw_count; i++) {
> +		/* Next platform is good: */
> +		if (fw_blobs[i].p < fw_blobs[i - 1].p)
> +			continue;
>   
> -			/* New must be before legacy: */
> -			if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
> -				goto bad;
> +		/* Next platform revision is good: */
> +		if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> +		    fw_blobs[i].rev < fw_blobs[i - 1].rev)
> +			continue;
>   
> -			/* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y (GuC) */
> -			if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
> -				if (!fw_blobs[i - 1].blob.major)
> -					continue;
> +		/* Platform/revision must be in order: */
> +		if (fw_blobs[i].p != fw_blobs[i - 1].p ||
> +		    fw_blobs[i].rev != fw_blobs[i - 1].rev)
> +			goto bad;
>   
> -				if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
> -					continue;
> -			}
> +		/* Next major version is good: */
> +		if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
> +			continue;
>   
> -			/* Major versions must be in order: */
> -			if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
> -				goto bad;
> +		/* New must be before legacy: */
> +		if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
> +			goto bad;
>   
> -			/* Next minor version is good: */
> -			if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
> +		/* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y (GuC) */
> +		if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
> +			if (!fw_blobs[i - 1].blob.major)
>   				continue;
>   
> -			/* Minor versions must be in order: */
> -			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)
> +			if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
>   				continue;
> +		}
> +
> +		/* Major versions must be in order: */
> +		if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
> +			goto bad;
> +
> +		/* Next minor version is good: */
> +		if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
> +			continue;
> +
> +		/* Minor versions must be in order: */
> +		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)
> +			continue;
>   
>   bad:
> -			drm_err(&i915->drm, "Invalid %s blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
> -				intel_uc_fw_type_repr(uc_fw->type),
> -				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,
> -				fw_blobs[i - 1].blob.minor,
> -				fw_blobs[i - 1].blob.patch,
> -				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);
> -
> -			uc_fw->file_selected.path = NULL;
> -		}
> +		drm_err(&i915->drm, "Invalid %s blob order: %s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
> +			intel_uc_fw_type_repr(type),
> +			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,
> +			fw_blobs[i - 1].blob.minor,
> +			fw_blobs[i - 1].blob.patch,
> +			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);

Confirming that this big diff was just an indent change was painful :/

>   	}
>   }
>   
> +static void validate_fw_table(struct drm_i915_private *i915)
> +{
> +	enum intel_uc_fw_type type;
> +	static bool done;
> +
> +	if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST) || done)
> +		return;
> +	done = true;
> +
> +	for (type = 0; type < INTEL_UC_FW_NUM_TYPES; type++)
> +		validate_fw_table_type(i915, type);
> +}
> +
>   static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
>   {
>   	if (i915->params.enable_guc & ENABLE_GUC_MASK)
> @@ -432,6 +464,8 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>   {
>   	struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915;
>   
> +	validate_fw_table(i915);

Personal preference: IMO since we're calling intel_uc_fw_init_early per 
FW type it would've been cleaner to restrict validate_fw_table() to a 
single blob type. This would have the negative side effect of having to 
track the "done" status per FW type, so I can see it's not a clean 
improvement. Not a blocker.

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

Daniele

> +
>   	/*
>   	 * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
>   	 * before we're looked at the HW caps to see if we have uc support

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

[Ceraolo Spurio, Daniele]

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:

Re: [Intel-gfx] [PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function

[John Harrison]

On 4/18/2023 16:14, Ceraolo Spurio, Daniele wrote:
> On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The validation of the firmware table was being done inside the code
>> for scanning the table for the next available firmware blob. Which is
>> unnecessary. Potentially, it should be a selftest. But either way, the
>> first step is pulling it out into a separate function that can be
>> called just once rather than once per blob attempt per blob type.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 164 ++++++++++++++---------
>>   1 file changed, 99 insertions(+), 65 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 6bb45d6b8da5f..c589782467265 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -233,20 +233,22 @@ struct fw_blobs_by_type {
>>       u32 count;
>>   };
>>   +static const struct uc_fw_platform_requirement blobs_guc[] = {
>> +    INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
>> +};
>> +
>> +static const struct uc_fw_platform_requirement blobs_huc[] = {
>> +    INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
>> HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
>> +};
>> +
>> +static const struct fw_blobs_by_type 
>> blobs_all[INTEL_UC_FW_NUM_TYPES] = {
>> +    [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
>> +    [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
>> +};
>> +
>>   static void
>>   __uc_fw_auto_select(struct drm_i915_private *i915, struct 
>> intel_uc_fw *uc_fw)
>>   {
>> -    static const struct uc_fw_platform_requirement blobs_guc[] = {
>> -        INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, 
>> GUC_FW_BLOB_MMP)
>> -    };
>> -    static const struct uc_fw_platform_requirement blobs_huc[] = {
>> -        INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
>> HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
>> -    };
>> -    static const struct fw_blobs_by_type 
>> blobs_all[INTEL_UC_FW_NUM_TYPES] = {
>> -        [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
>> -        [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
>> -    };
>> -    static bool verified[INTEL_UC_FW_NUM_TYPES];
>>       const struct uc_fw_platform_requirement *fw_blobs;
>>       enum intel_platform p = INTEL_INFO(i915)->platform;
>>       u32 fw_count;
>> @@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private 
>> *i915, struct intel_uc_fw *uc_fw)
>>               continue;
>>             if (uc_fw->file_selected.path) {
>> +            /*
>> +             * Continuing an earlier search after a found blob 
>> failed to load.
>> +             * Once the previously chosen path has been found, clear 
>> it out
>> +             * and let the search continue from there.
>> +             */
>>               if (uc_fw->file_selected.path == blob->path)
>>                   uc_fw->file_selected.path = NULL;
>>   @@ -306,78 +313,103 @@ __uc_fw_auto_select(struct drm_i915_private 
>> *i915, struct intel_uc_fw *uc_fw)
>>           /* 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[uc_fw->type]) {
>> -        verified[uc_fw->type] = true;
>> +static void validate_fw_table_type(struct drm_i915_private *i915, 
>> enum intel_uc_fw_type type)
>> +{
>> +    const struct uc_fw_platform_requirement *fw_blobs;
>> +    u32 fw_count;
>> +    int i;
>>   -        for (i = 1; i < fw_count; i++) {
>> -            /* Next platform is good: */
>> -            if (fw_blobs[i].p < fw_blobs[i - 1].p)
>> -                continue;
>> +    if (type >= ARRAY_SIZE(blobs_all)) {
>> +        drm_err(&i915->drm, "No blob array for %s\n", 
>> intel_uc_fw_type_repr(type));
>> +        return;
>> +    }
>>   -            /* Next platform revision is good: */
>> -            if (fw_blobs[i].p == fw_blobs[i - 1].p &&
>> -                fw_blobs[i].rev < fw_blobs[i - 1].rev)
>> -                continue;
>> +    fw_blobs = blobs_all[type].blobs;
>> +    fw_count = blobs_all[type].count;
>>   -            /* Platform/revision must be in order: */
>> -            if (fw_blobs[i].p != fw_blobs[i - 1].p ||
>> -                fw_blobs[i].rev != fw_blobs[i - 1].rev)
>> -                goto bad;
>> +    if (!fw_count)
>> +        return;
>>   -            /* Next major version is good: */
>> -            if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
>> -                continue;
>> +    /* make sure the list is ordered as expected */
>> +    for (i = 1; i < fw_count; i++) {
>> +        /* Next platform is good: */
>> +        if (fw_blobs[i].p < fw_blobs[i - 1].p)
>> +            continue;
>>   -            /* New must be before legacy: */
>> -            if (!fw_blobs[i].blob.legacy && fw_blobs[i - 
>> 1].blob.legacy)
>> -                goto bad;
>> +        /* Next platform revision is good: */
>> +        if (fw_blobs[i].p == fw_blobs[i - 1].p &&
>> +            fw_blobs[i].rev < fw_blobs[i - 1].rev)
>> +            continue;
>>   -            /* New to legacy also means 0.0 to X.Y (HuC), or X.0 
>> to X.Y (GuC) */
>> -            if (fw_blobs[i].blob.legacy && !fw_blobs[i - 
>> 1].blob.legacy) {
>> -                if (!fw_blobs[i - 1].blob.major)
>> -                    continue;
>> +        /* Platform/revision must be in order: */
>> +        if (fw_blobs[i].p != fw_blobs[i - 1].p ||
>> +            fw_blobs[i].rev != fw_blobs[i - 1].rev)
>> +            goto bad;
>>   -                if (fw_blobs[i].blob.major == fw_blobs[i - 
>> 1].blob.major)
>> -                    continue;
>> -            }
>> +        /* Next major version is good: */
>> +        if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
>> +            continue;
>>   -            /* Major versions must be in order: */
>> -            if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
>> -                goto bad;
>> +        /* New must be before legacy: */
>> +        if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
>> +            goto bad;
>>   -            /* Next minor version is good: */
>> -            if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
>> +        /* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y 
>> (GuC) */
>> +        if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
>> +            if (!fw_blobs[i - 1].blob.major)
>>                   continue;
>>   -            /* Minor versions must be in order: */
>> -            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)
>> +            if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
>>                   continue;
>> +        }
>> +
>> +        /* Major versions must be in order: */
>> +        if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
>> +            goto bad;
>> +
>> +        /* Next minor version is good: */
>> +        if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
>> +            continue;
>> +
>> +        /* Minor versions must be in order: */
>> +        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)
>> +            continue;
>>     bad:
>> -            drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
>> %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
>> -                intel_uc_fw_type_repr(uc_fw->type),
>> -                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,
>> -                fw_blobs[i - 1].blob.minor,
>> -                fw_blobs[i - 1].blob.patch,
>> -                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);
>> -
>> -            uc_fw->file_selected.path = NULL;
>> -        }
>> +        drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
>> %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
>> +            intel_uc_fw_type_repr(type),
>> +            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,
>> +            fw_blobs[i - 1].blob.minor,
>> +            fw_blobs[i - 1].blob.patch,
>> +            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);
>
> Confirming that this big diff was just an indent change was painful :/
Yeah, not a lot one can do about that. If you pull the patches to a 
local tree then you can run 'git show -b'. Can't really post that to 
patchwork though.

>
>>       }
>>   }
>>   +static void validate_fw_table(struct drm_i915_private *i915)
>> +{
>> +    enum intel_uc_fw_type type;
>> +    static bool done;
>> +
>> +    if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST) || done)
>> +        return;
>> +    done = true;
>> +
>> +    for (type = 0; type < INTEL_UC_FW_NUM_TYPES; type++)
>> +        validate_fw_table_type(i915, type);
>> +}
>> +
>>   static const char *__override_guc_firmware_path(struct 
>> drm_i915_private *i915)
>>   {
>>       if (i915->params.enable_guc & ENABLE_GUC_MASK)
>> @@ -432,6 +464,8 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>> *uc_fw,
>>   {
>>       struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, 
>> type)->i915;
>>   +    validate_fw_table(i915);
>
> Personal preference: IMO since we're calling intel_uc_fw_init_early 
> per FW type it would've been cleaner to restrict validate_fw_table() 
> to a single blob type. This would have the negative side effect of 
> having to track the "done" status per FW type, so I can see it's not a 
> clean improvement. Not a blocker.
That's step 2 - move the validate call out of the per blob type init 
sequence completely. Either just pushing it further up the call stack or 
moving it sideways to a selftest. First step was just to separate the 
code itself out as cleanly as possible.

John.

>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Daniele
>
>> +
>>       /*
>>        * we use FIRMWARE_UNINITIALIZED to detect checks against 
>> uc_fw->status
>>        * before we're looked at the HW caps to see if we have uc support
>

Re: [Intel-gfx] [PATCH 3/5] drm/i915/uc: Track patch level versions on reduced version firmware files

[John Harrison]

On 4/18/2023 15:46, Ceraolo Spurio, Daniele wrote:
> On 4/14/2023 5:57 PM, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> When reduced version firmware files were added (matching major
>> component being the only strict requirement), the minor version was
>> still tracked and a notification reported if it was older. However,
>> the patch version should really be tracked as well for the same
>> reasons. The KMD can work without the change but if the effort has
>> been taken to release a new firmware with the change then there must
>> be a valid reason for doing so - important bug fix, security fix, etc.
>> And in that case it would be good to alert the user if they are
>> missing out on that new fix.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 41 +++++++++++++++++-------
>>   1 file changed, 30 insertions(+), 11 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 a82a53dbbc86d..6bb45d6b8da5f 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -80,14 +80,14 @@ void intel_uc_fw_change_status(struct intel_uc_fw 
>> *uc_fw,
>>    */
>>   #define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_maj, guc_mmp) \
>>       fw_def(METEORLAKE,   0, guc_mmp(mtl,  70, 6, 5)) \
>> -    fw_def(DG2,          0, guc_maj(dg2,  70, 5)) \
>> -    fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5)) \
>> +    fw_def(DG2,          0, guc_maj(dg2,  70, 5, 4)) \
>> +    fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 4)) \
>>       fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 70, 1, 1)) \
>>       fw_def(ALDERLAKE_P,  0, guc_mmp(adlp, 69, 0, 3)) \
>> -    fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5)) \
>> +    fw_def(ALDERLAKE_S,  0, guc_maj(tgl,  70, 5, 4)) \
>>       fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  70, 1, 1)) \
>>       fw_def(ALDERLAKE_S,  0, guc_mmp(tgl,  69, 0, 3)) \
>> -    fw_def(DG1,          0, guc_maj(dg1,  70, 5)) \
>> +    fw_def(DG1,          0, guc_maj(dg1,  70, 5, 4)) \
>>       fw_def(ROCKETLAKE,   0, guc_mmp(tgl,  70, 1, 1)) \
>>       fw_def(TIGERLAKE,    0, guc_mmp(tgl,  70, 1, 1)) \
>>       fw_def(JASPERLAKE,   0, guc_mmp(ehl,  70, 1, 1)) \
>
> AFAICS in linux-firmware we don't have any 70.5.4 binaries, the newest 
> ones are 70.5.1.
You would be correct. Hence the CI results all say:
<5> [27.947440] i915 0000:00:02.0: [drm] GT0: GuC firmware 
i915/adlp_guc_70.bin (70.5.4) is recommended, but only 
i915/adlp_guc_70.bin (70.5.1) was found

I was just testing that the code worked ;)...

Although it does beg the question that maybe we should bump the print 
from a 'notice' to an 'err' if CONFIG_DEBUG_GEM is defined or something? 
Ensure that CI is actually testing against the correct firmware versions.


>
>> @@ -141,7 +141,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw 
>> *uc_fw,
>>       __stringify(patch_) ".bin"
>>     /* Minor for internal driver use, not part of file name */
>> -#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_) \
>> +#define MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_) \
>>       __MAKE_UC_FW_PATH_MAJOR(prefix_, "guc", major_)
>>     #define MAKE_GUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \
>> @@ -197,9 +197,9 @@ struct __packed uc_fw_blob {
>>       { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \
>>         .legacy = true }
>>   -#define GUC_FW_BLOB(prefix_, major_, minor_) \
>> -    UC_FW_BLOB_NEW(major_, minor_, 0, false, \
>> -               MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_))
>> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
>> +    UC_FW_BLOB_NEW(major_, minor_, patch_, false, \
>> +               MAKE_GUC_FW_PATH_MAJOR(prefix_, major_, minor_, patch_))
>>     #define GUC_FW_BLOB_MMP(prefix_, major_, minor_, patch_) \
>>       UC_FW_BLOB_OLD(major_, minor_, patch_, \
>> @@ -296,6 +296,7 @@ __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.ver.major = blob->major;
>>           uc_fw->file_wanted.ver.minor = blob->minor;
>> +        uc_fw->file_wanted.ver.patch = blob->patch;
>>           uc_fw->loaded_via_gsc = blob->loaded_via_gsc;
>>           found = true;
>>           break;
>> @@ -776,6 +777,17 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>>       if (uc_fw->type == INTEL_UC_FW_TYPE_GUC && 
>> !guc_check_version_range(uc_fw))
>>           goto fail;
>>   +    gt_info(gt, "%s firmware: wanted = %s / %d.%d.%d, got = %s / 
>> %d.%d.%d\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);
>
> Some of the info here is redundant from print_fw_ver(). Can we have a 
> single print with all the info we need?
> Something like:
Hmm. I think this was just my test code that got left in by accident. As 
you say, we print the actual version in use later on when we do the load 
itself. And if the wanted does not match then we print the 'is 
recommended' message below. So this line is basically redundant. I'll 
remove it.

John.

>
> GuC firmware i915/mtl_guc_70.bin (v70.6.5, expected v70.6.4 or newer)
>
> Otherwise, I'd suggest demoting this to gt_dbg to avoid printing the 
> same thing twice at info verbosity
>
> Daniele
>
>> +
>>       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) {
>> @@ -790,6 +802,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>>           } else {
>>               if (uc_fw->file_selected.ver.minor < 
>> uc_fw->file_wanted.ver.minor)
>>                   old_ver = true;
>> +            else if ((uc_fw->file_selected.ver.minor == 
>> uc_fw->file_wanted.ver.minor) &&
>> +                 (uc_fw->file_selected.ver.patch < 
>> uc_fw->file_wanted.ver.patch))
>> +                old_ver = true;
>>           }
>>       }
>>   @@ -797,12 +812,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) is recommended, but 
>> only %s (%d.%d) was found\n",
>> +        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.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.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);
>>       }
>

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

[John Harrison]

On 4/18/2023 16:24, Ceraolo Spurio, Daniele wrote:
> 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.
The validation code is rejecting duplicates. Whether the driver loads or 
not after a failed validation is another matter.

I was basically assuming that CI will fail on the error message and thus 
prevent such code ever being merged. But yeah, I guess we don't run CI 
on backports to stable kernels and such. Although, I would hope that 
anyone pushing patches to a stable kernel would run some testing on it 
first!

Any thoughts on a good way to fail the load? We don't want to just 
pretend that firmware is not wanted/required on the platform and just 
load the i915 module without the firmware. Also, what about the longer 
plan of moving the validation to a selftest. You can't fail the load at 
all then.

John.

>
>>
>> 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:
>

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

[John Harrison]

On 4/19/2023 10:02, John Harrison wrote:
> On 4/18/2023 16:24, Ceraolo Spurio, Daniele wrote:
>> 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.
> The validation code is rejecting duplicates. Whether the driver loads 
> or not after a failed validation is another matter.
>
> I was basically assuming that CI will fail on the error message and 
> thus prevent such code ever being merged. But yeah, I guess we don't 
> run CI on backports to stable kernels and such. Although, I would hope 
> that anyone pushing patches to a stable kernel would run some testing 
> on it first!
>
> Any thoughts on a good way to fail the load? We don't want to just 
> pretend that firmware is not wanted/required on the platform and just 
> load the i915 module without the firmware. Also, what about the longer 
> plan of moving the validation to a selftest. You can't fail the load 
> at all then.
Actually, forgot we already have a INTEL_UC_FIRMWARE_ERROR status. That 
works fine for aborting the load. So just go with that and drop the plan 
to move to a selftest?

John.


>
> John.
>
>>
>>>
>>> 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:
>>
>

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

[Ceraolo Spurio, Daniele]

On 4/19/2023 10:12 AM, John Harrison wrote:
> On 4/19/2023 10:02, John Harrison wrote:
>> On 4/18/2023 16:24, Ceraolo Spurio, Daniele wrote:
>>> 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.
>> The validation code is rejecting duplicates. Whether the driver loads 
>> or not after a failed validation is another matter.
>>
>> I was basically assuming that CI will fail on the error message and 
>> thus prevent such code ever being merged. But yeah, I guess we don't 
>> run CI on backports to stable kernels and such. Although, I would 
>> hope that anyone pushing patches to a stable kernel would run some 
>> testing on it first!
>>
>> Any thoughts on a good way to fail the load? We don't want to just 
>> pretend that firmware is not wanted/required on the platform and just 
>> load the i915 module without the firmware. Also, what about the 
>> longer plan of moving the validation to a selftest. You can't fail 
>> the load at all then.
> Actually, forgot we already have a INTEL_UC_FIRMWARE_ERROR status. 
> That works fine for aborting the load. So just go with that and drop 
> the plan to move to a selftest?
>
> John.

I do actually like the idea of moving this code to a mock selftest. 
Maybe just add a comment above the tables making clear that duplicated 
entries are not allowed and will break the loading flow?

Daniele

>
>
>>
>> John.
>>
>>>
>>>>
>>>> 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:
>>>
>>
>

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

[John Harrison]

On 4/19/2023 10:33, Ceraolo Spurio, Daniele wrote:
> On 4/19/2023 10:12 AM, John Harrison wrote:
>> On 4/19/2023 10:02, John Harrison wrote:
>>> On 4/18/2023 16:24, Ceraolo Spurio, Daniele wrote:
>>>> 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.
>>> The validation code is rejecting duplicates. Whether the driver 
>>> loads or not after a failed validation is another matter.
>>>
>>> I was basically assuming that CI will fail on the error message and 
>>> thus prevent such code ever being merged. But yeah, I guess we don't 
>>> run CI on backports to stable kernels and such. Although, I would 
>>> hope that anyone pushing patches to a stable kernel would run some 
>>> testing on it first!
>>>
>>> Any thoughts on a good way to fail the load? We don't want to just 
>>> pretend that firmware is not wanted/required on the platform and 
>>> just load the i915 module without the firmware. Also, what about the 
>>> longer plan of moving the validation to a selftest. You can't fail 
>>> the load at all then.
>> Actually, forgot we already have a INTEL_UC_FIRMWARE_ERROR status. 
>> That works fine for aborting the load. So just go with that and drop 
>> the plan to move to a selftest?
>>
>> John.
>
> I do actually like the idea of moving this code to a mock selftest. 
> Maybe just add a comment above the tables making clear that duplicated 
> entries are not allowed and will break the loading flow?
The point is about accidental breakages. The 'issue' was not an actual 
failure but that a failure could potentially occur if, for example, a 
patch got applied twice due to some backport error/confusion. Adding a 
comment doesn't help with that.

Given that it is trivial to return FIRMWARE_ERROR on a validation 
failure, I'm inclined to go with that for now. The validation is still 
only being done once (and moving it to _early time means we don't 
actually need the static bool at all) and the driver is guaranteed to 
not try to process a broken table and get confused. It is also instantly 
evident if a backport is broken for any reason without worrying about 
what kind of explicit testing may or may not be run.

John.


>
> Daniele
>
>>
>>
>>>
>>> John.
>>>
>>>>
>>>>>
>>>>> 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:
>>>>
>>>
>>
>