[PATCH 0/6] Improvements to uc firmare management

[PATCH 0/6] 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.

Treat more problems as fatal errors, at least for DEBUG builds.

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


John Harrison (6):
  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: Enhancements to firmware table validation
  drm/i915/uc: Reject duplicate entries in firmware table
  drm/i915/uc: Make unexpected firmware versions an error in debug
    builds

 .../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      | 224 +++++++++++-------
 3 files changed, 154 insertions(+), 83 deletions(-)

-- 
2.39.1

[Intel-gfx] [PATCH 0/6] 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.

Treat more problems as fatal errors, at least for DEBUG builds.

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


John Harrison (6):
  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: Enhancements to firmware table validation
  drm/i915/uc: Reject duplicate entries in firmware table
  drm/i915/uc: Make unexpected firmware versions an error in debug
    builds

 .../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      | 224 +++++++++++-------
 3 files changed, 154 insertions(+), 83 deletions(-)

-- 
2.39.1

[PATCH 1/6] 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>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 1/6] 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>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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

[PATCH 2/6] 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>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 2/6] 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>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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

[PATCH 3/6] 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.

v2: Use correct patch version number and drop redunant debug print
(review by Daniele / CI results).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 30 +++++++++++++++---------
 1 file changed, 19 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..c9cd9bb47577f 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, 1)) \
+	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 1)) \
 	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, 1)) \
 	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, 1)) \
 	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;
@@ -790,6 +791,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 +801,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 3/6] 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.

v2: Use correct patch version number and drop redunant debug print
(review by Daniele / CI results).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 30 +++++++++++++++---------
 1 file changed, 19 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..c9cd9bb47577f 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, 1)) \
+	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 1)) \
 	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, 1)) \
 	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, 1)) \
 	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;
@@ -790,6 +791,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 +801,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

[PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[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. So pull it out into a separate function that is only
called once per blob type at init time.

Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
It was mentioned that potential issues with backports would not be
caught by regular pre-merge CI as that only occurs on tip not stable
branches. Making the validation unconditional and failing driver load
on detecting of a problem ensures that such backports will also be
validated correctly.

v2: Change to unconditionally fail module load on a validation error
(review feedback/discussion with Daniele).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
 1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
+	}
 
-			/* 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 true;
 
-			/* 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);
+		return false;
 	}
+
+	return true;
 }
 
 static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
@@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 	uc_fw->type = type;
 
 	if (HAS_GT_UC(i915)) {
+		if (!validate_fw_table_type(i915, type)) {
+			intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
+			return;
+		}
+
 		__uc_fw_auto_select(i915, uc_fw);
 		__uc_fw_user_override(i915, uc_fw);
 	}
-- 
2.39.1

[Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[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. So pull it out into a separate function that is only
called once per blob type at init time.

Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
It was mentioned that potential issues with backports would not be
caught by regular pre-merge CI as that only occurs on tip not stable
branches. Making the validation unconditional and failing driver load
on detecting of a problem ensures that such backports will also be
validated correctly.

v2: Change to unconditionally fail module load on a validation error
(review feedback/discussion with Daniele).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
 1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
+	}
 
-			/* 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 true;
 
-			/* 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);
+		return false;
 	}
+
+	return true;
 }
 
 static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
@@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 	uc_fw->type = type;
 
 	if (HAS_GT_UC(i915)) {
+		if (!validate_fw_table_type(i915, type)) {
+			intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
+			return;
+		}
+
 		__uc_fw_auto_select(i915, uc_fw);
 		__uc_fw_user_override(i915, uc_fw);
 	}
-- 
2.39.1

[PATCH 5/6] drm/i915/uc: Reject duplicate entries in firmware table

[John.C.Harrison]

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

It was noticed that duplicate 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 eb52e8db9ae0b..bc4011d55667c 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 bool 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 bool 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 bool 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] [PATCH 5/6] drm/i915/uc: Reject duplicate entries in firmware table

[John.C.Harrison]

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

It was noticed that duplicate 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 eb52e8db9ae0b..bc4011d55667c 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 bool 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 bool 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 bool 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

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

[John.C.Harrison]

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

If the DEBUG_GEM config option is set then escalate the 'unexpected
firmware version' message from a notice to an error. This will ensure
that the CI system treats such occurences as a failure and logs a bug
about it (or fails the pre-merge testing).

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

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index bc4011d55667c..c5b21d17ca437 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -17,6 +17,12 @@
 #include "i915_drv.h"
 #include "i915_reg.h"
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
+#define UNEXPECTED	gt_err
+#else
+#define UNEXPECTED	gt_notice
+#endif
+
 static inline struct intel_gt *
 ____uc_fw_to_gt(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
 {
@@ -828,10 +834,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
 		/* Check the file's major version was as it claimed */
 		if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
-			gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
-				  intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-				  uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
-				  uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
+			UNEXPECTED(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
+				   intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
+				   uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
+				   uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
 			if (!intel_uc_fw_is_overridden(uc_fw)) {
 				err = -ENOEXEC;
 				goto fail;
@@ -849,16 +855,16 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 		/* Preserve the version that was really wanted */
 		memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
 
-		gt_notice(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
-			  intel_uc_fw_type_repr(uc_fw->type),
-			  uc_fw->file_wanted.path,
-			  uc_fw->file_wanted.ver.major,
-			  uc_fw->file_wanted.ver.minor,
-			  uc_fw->file_wanted.ver.patch,
-			  uc_fw->file_selected.path,
-			  uc_fw->file_selected.ver.major,
-			  uc_fw->file_selected.ver.minor,
-			  uc_fw->file_selected.ver.patch);
+		UNEXPECTED(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
+			   intel_uc_fw_type_repr(uc_fw->type),
+			   uc_fw->file_wanted.path,
+			   uc_fw->file_wanted.ver.major,
+			   uc_fw->file_wanted.ver.minor,
+			   uc_fw->file_wanted.ver.patch,
+			   uc_fw->file_selected.path,
+			   uc_fw->file_selected.ver.major,
+			   uc_fw->file_selected.ver.minor,
+			   uc_fw->file_selected.ver.patch);
 		gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
 			INTEL_UC_FIRMWARE_URL);
 	}
-- 
2.39.1

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

[John.C.Harrison]

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

If the DEBUG_GEM config option is set then escalate the 'unexpected
firmware version' message from a notice to an error. This will ensure
that the CI system treats such occurences as a failure and logs a bug
about it (or fails the pre-merge testing).

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

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index bc4011d55667c..c5b21d17ca437 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -17,6 +17,12 @@
 #include "i915_drv.h"
 #include "i915_reg.h"
 
+#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
+#define UNEXPECTED	gt_err
+#else
+#define UNEXPECTED	gt_notice
+#endif
+
 static inline struct intel_gt *
 ____uc_fw_to_gt(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
 {
@@ -828,10 +834,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
 		/* Check the file's major version was as it claimed */
 		if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
-			gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
-				  intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-				  uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
-				  uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
+			UNEXPECTED(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
+				   intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
+				   uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
+				   uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
 			if (!intel_uc_fw_is_overridden(uc_fw)) {
 				err = -ENOEXEC;
 				goto fail;
@@ -849,16 +855,16 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 		/* Preserve the version that was really wanted */
 		memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
 
-		gt_notice(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
-			  intel_uc_fw_type_repr(uc_fw->type),
-			  uc_fw->file_wanted.path,
-			  uc_fw->file_wanted.ver.major,
-			  uc_fw->file_wanted.ver.minor,
-			  uc_fw->file_wanted.ver.patch,
-			  uc_fw->file_selected.path,
-			  uc_fw->file_selected.ver.major,
-			  uc_fw->file_selected.ver.minor,
-			  uc_fw->file_selected.ver.patch);
+		UNEXPECTED(gt, "%s firmware %s (%d.%d.%d) is recommended, but only %s (%d.%d.%d) was found\n",
+			   intel_uc_fw_type_repr(uc_fw->type),
+			   uc_fw->file_wanted.path,
+			   uc_fw->file_wanted.ver.major,
+			   uc_fw->file_wanted.ver.minor,
+			   uc_fw->file_wanted.ver.patch,
+			   uc_fw->file_selected.path,
+			   uc_fw->file_selected.ver.major,
+			   uc_fw->file_selected.ver.minor,
+			   uc_fw->file_selected.ver.patch);
 		gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
 			INTEL_UC_FIRMWARE_URL);
 	}
-- 
2.39.1

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

[Patchwork]

== Series Details ==

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

== Summary ==

Error: dim checkpatch failed
5f4add3122b0 drm/i915/guc: Decode another GuC load failure case
2b35e460865b drm/i915/guc: Print status register when waiting for GuC to load
5fcf4600dbe0 drm/i915/uc: Track patch level versions on reduced version firmware files
-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'patch_' - possible side-effects?
#61: 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, 73 lines checked
0ad23ab7f74c drm/i915/uc: Enhancements to firmware table validation
8d3a53230905 drm/i915/uc: Reject duplicate entries in firmware table
0eae0615810a drm/i915/uc: Make unexpected firmware versions an error in debug builds

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

[Patchwork]

== Series Details ==

Series: Improvements to uc firmare management (rev2)
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: failure for Improvements to uc firmare management (rev2)

[Patchwork]

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

== Series Details ==

Series: Improvements to uc firmare management (rev2)
URL   : https://patchwork.freedesktop.org/series/116517/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13036 -> Patchwork_116517v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_116517v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_116517v2, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

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

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

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116517v2:

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-apl-guc:         NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v2/fi-apl-guc/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * 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_116517v2/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_116517v2/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

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

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

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

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-rpls-1:         NOTRUN -> [SKIP][7] ([i915#7828])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v2/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

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

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-rpls-1:         NOTRUN -> [SKIP][10] ([i915#1845])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v2/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

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

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][13] ([i915#4983] / [i915#7981]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13036/bat-rpls-1/igt@i915_selftest@live@reset.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v2/bat-rpls-1/igt@i915_selftest@live@reset.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
  [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073


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

  * Linux: CI_DRM_13036 -> Patchwork_116517v2

  CI-20190529: 20190529
  CI_DRM_13036: f373f2faeb2f25a27dc83bd1326b43315be1a6a4 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7263: a6bd8f415c4ec41b5a014c7db47e46c81ffd0074 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v2: f373f2faeb2f25a27dc83bd1326b43315be1a6a4 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

f4f328619600 drm/i915/uc: Make unexpected firmware versions an error in debug builds
6b2de09e0554 drm/i915/uc: Reject duplicate entries in firmware table
21490bd9fc18 drm/i915/uc: Enhancements to firmware table validation
680e844808c0 drm/i915/uc: Track patch level versions on reduced version firmware files
cea29917fe2d drm/i915/guc: Print status register when waiting for GuC to load
d47587460f75 drm/i915/guc: Decode another GuC load failure case

== Logs ==

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

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

Error: dim checkpatch failed
c9c1858ce1b5 drm/i915/guc: Decode another GuC load failure case
78137da72e02 drm/i915/guc: Print status register when waiting for GuC to load
b5a9b016719f drm/i915/uc: Track patch level versions on reduced version firmware files
-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'patch_' - possible side-effects?
#61: 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, 73 lines checked
1280d8f010c6 drm/i915/uc: Enhancements to firmware table validation
a4d2663e570b drm/i915/uc: Reject duplicate entries in firmware table
cd22dd08e779 drm/i915/uc: Make unexpected firmware versions an error in debug builds

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

[Patchwork]

== Series Details ==

Series: Improvements to uc firmare management (rev3)
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: failure for Improvements to uc firmare management (rev3)

[Patchwork]

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

== Series Details ==

Series: Improvements to uc firmare management (rev3)
URL   : https://patchwork.freedesktop.org/series/116517/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13043 -> Patchwork_116517v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_116517v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_116517v3, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

Participating hosts (36 -> 36)
------------------------------

  Additional (1): bat-mtlp-8 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116517v3:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-hdmi-a1:
    - fi-elk-e7500:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13043/fi-elk-e7500/igt@kms_flip@basic-flip-vs-wf_vblank@a-hdmi-a1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v3/fi-elk-e7500/igt@kms_flip@basic-flip-vs-wf_vblank@a-hdmi-a1.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@hugepages:
    - {bat-mtlp-8}:       NOTRUN -> [DMESG-FAIL][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v3/bat-mtlp-8/igt@i915_selftest@live@hugepages.html

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

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

### IGT changes ###

#### Warnings ####

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

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

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
  [i915#8346]: https://gitlab.freedesktop.org/drm/intel/issues/8346
  [i915#8368]: https://gitlab.freedesktop.org/drm/intel/issues/8368
  [i915#8369]: https://gitlab.freedesktop.org/drm/intel/issues/8369
  [i915#8379]: https://gitlab.freedesktop.org/drm/intel/issues/8379


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

  * Linux: CI_DRM_13043 -> Patchwork_116517v3

  CI-20190529: 20190529
  CI_DRM_13043: 2fa9c266135355c9993507d7c27cc6722956bfec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7264: 2f0a07378e58e5c7d7b589b39ace7e3a2317f6b2 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v3: 2fa9c266135355c9993507d7c27cc6722956bfec @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

9124e14cc346 drm/i915/uc: Make unexpected firmware versions an error in debug builds
285925456608 drm/i915/uc: Reject duplicate entries in firmware table
9d456229f40e drm/i915/uc: Enhancements to firmware table validation
f88b23c6bd08 drm/i915/uc: Track patch level versions on reduced version firmware files
0230c2d1ec3d drm/i915/guc: Print status register when waiting for GuC to load
99d1c102d0f1 drm/i915/guc: Decode another GuC load failure case

== Logs ==

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

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

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

[Patchwork]

== Series Details ==

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

== Summary ==

Error: dim checkpatch failed
767fe16369d4 drm/i915/guc: Decode another GuC load failure case
95f43703948d drm/i915/guc: Print status register when waiting for GuC to load
739f30be54fc drm/i915/uc: Track patch level versions on reduced version firmware files
-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'major_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'minor_' - possible side-effects?
#61: 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_))

-:61: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'patch_' - possible side-effects?
#61: 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, 73 lines checked
e4b70f34344b drm/i915/uc: Enhancements to firmware table validation
1f175721171c drm/i915/uc: Reject duplicate entries in firmware table
4d4fd1a1efe2 drm/i915/uc: Make unexpected firmware versions an error in debug builds

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

[Patchwork]

== Series Details ==

Series: Improvements to uc firmare management (rev4)
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 (rev4)

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_13056 -> Patchwork_116517v4
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-kbl-soraka:      [PASS][1] -> [DMESG-FAIL][2] ([i915#5334] / [i915#7872])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][3] -> [FAIL][4] ([i915#7932]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-glk-j4005:       [DMESG-FAIL][5] ([i915#5334]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_addfb_basic@unused-modifier:
    - fi-kbl-soraka:      [INCOMPLETE][7] -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/fi-kbl-soraka/igt@kms_addfb_basic@unused-modifier.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/fi-kbl-soraka/igt@kms_addfb_basic@unused-modifier.html

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

  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#8176]: https://gitlab.freedesktop.org/drm/intel/issues/8176
  [i915#8361]: https://gitlab.freedesktop.org/drm/intel/issues/8361


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

  * Linux: CI_DRM_13056 -> Patchwork_116517v4

  CI-20190529: 20190529
  CI_DRM_13056: 308c0163d9e46238948942260e6d2abcad3d8bff @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7267: a267f0236e06fc282e3dc3b8c7d76f9ed6088d9b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v4: 308c0163d9e46238948942260e6d2abcad3d8bff @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

73551022be0b drm/i915/uc: Make unexpected firmware versions an error in debug builds
88e712f9b743 drm/i915/uc: Reject duplicate entries in firmware table
167ae011abe2 drm/i915/uc: Enhancements to firmware table validation
840667ca3eca drm/i915/uc: Track patch level versions on reduced version firmware files
bc3e7163bbb3 drm/i915/guc: Print status register when waiting for GuC to load
5f41dc0364f0 drm/i915/guc: Decode another GuC load failure case

== Logs ==

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

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

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

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_13056_full -> Patchwork_116517v4_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116517v4_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@reload-with-fault-injection:
    - {shard-dg1}:        NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@i915_module_load@reload-with-fault-injection.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-pwrite:
    - {shard-dg1}:        [PASS][2] -> [SKIP][3] +5 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-dg1-16/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-pwrite.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - {shard-dg1}:        [SKIP][4] ([fdo#111825]) -> [SKIP][5] +19 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-dg1-17/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-mmap-gtt:
    - {shard-dg1}:        [SKIP][6] ([i915#4833]) -> [SKIP][7] +5 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-dg1-16/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-mmap-gtt.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-spr-indfb-draw-mmap-gtt.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-blt:
    - {shard-dg1}:        NOTRUN -> [SKIP][8] +50 similar issues
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-blt.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-wc:
    - {shard-dg1}:        [SKIP][9] ([i915#3458]) -> [SKIP][10] +8 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-dg1-16/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-wc.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-12/igt@kms_frontbuffer_tracking@psr-1p-primscrn-cur-indfb-draw-mmap-wc.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][11] -> [FAIL][12] ([i915#2842])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk5/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk5/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#2842])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-apl1/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-apl6/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@i915_pm_rps@reset:
    - shard-snb:          [PASS][15] -> [INCOMPLETE][16] ([i915#7790])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-snb4/igt@i915_pm_rps@reset.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-snb4/igt@i915_pm_rps@reset.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-glk:          [PASS][17] -> [DMESG-FAIL][18] ([i915#5334])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk1/igt@i915_selftest@live@gt_heartbeat.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk1/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@migrate:
    - shard-glk:          [PASS][19] -> [DMESG-FAIL][20] ([i915#7384])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk1/igt@i915_selftest@live@migrate.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk1/igt@i915_selftest@live@migrate.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [PASS][21] -> [FAIL][22] ([i915#2346])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
    - shard-glk:          [PASS][23] -> [FAIL][24] ([i915#2346])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_dither@fb-8bpc-vs-panel-6bpc@pipe-a-hdmi-a-1:
    - shard-glk:          NOTRUN -> [SKIP][25] ([fdo#109271])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk9/igt@kms_dither@fb-8bpc-vs-panel-6bpc@pipe-a-hdmi-a-1.html

  * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-a-vga-1:
    - shard-snb:          NOTRUN -> [SKIP][26] ([fdo#109271]) +36 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-snb5/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-a-vga-1.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@most-busy-idle-check-all@rcs0:
    - {shard-rkl}:        [FAIL][27] ([i915#7742]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-rkl-3/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-rkl-7/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html

  * igt@gem_barrier_race@remote-request@rcs0:
    - {shard-tglu}:       [ABORT][29] ([i915#8211]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-tglu-6/igt@gem_barrier_race@remote-request@rcs0.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-tglu-6/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_exec_fair@basic-deadline:
    - {shard-rkl}:        [FAIL][31] ([i915#2846]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-rkl-6/igt@gem_exec_fair@basic-deadline.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-rkl-1/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-share@rcs0:
    - shard-glk:          [FAIL][33] ([i915#2842]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk2/igt@gem_exec_fair@basic-none-share@rcs0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk5/igt@gem_exec_fair@basic-none-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-tglu}:       [FAIL][35] ([i915#2842]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-tglu-3/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-tglu-4/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-snb:          [FAIL][37] ([i915#8295]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-snb1/igt@gem_ppgtt@blt-vs-render-ctxn.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-snb5/igt@gem_ppgtt@blt-vs-render-ctxn.html

  * igt@i915_pm_dc@dc6-dpms:
    - {shard-tglu}:       [FAIL][39] ([i915#3989] / [i915#454]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-tglu-3/igt@i915_pm_dc@dc6-dpms.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-tglu-9/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_rpm@dpms-lpsp:
    - {shard-rkl}:        [SKIP][41] ([i915#1397]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-rkl-4/igt@i915_pm_rpm@dpms-lpsp.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-rkl-7/igt@i915_pm_rpm@dpms-lpsp.html

  * igt@kms_cursor_legacy@forked-move@pipe-b:
    - {shard-dg1}:        [INCOMPLETE][43] ([i915#8011] / [i915#8347]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-dg1-14/igt@kms_cursor_legacy@forked-move@pipe-b.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-dg1-16/igt@kms_cursor_legacy@forked-move@pipe-b.html

  * igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][45] ([i915#79]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-glk7/igt@kms_flip@2x-flip-vs-expired-vblank@ac-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1:
    - shard-apl:          [FAIL][47] ([i915#79]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13056/shard-apl1/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116517v4/shard-apl6/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-dp1.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#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [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#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
  [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#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
  [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#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [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#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
  [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [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#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4387]: https://gitlab.freedesktop.org/drm/intel/issues/4387
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4879]: https://gitlab.freedesktop.org/drm/intel/issues/4879
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
  [i915#4885]: https://gitlab.freedesktop.org/drm/intel/issues/4885
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [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#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5493]: https://gitlab.freedesktop.org/drm/intel/issues/5493
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6590]: https://gitlab.freedesktop.org/drm/intel/issues/6590
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7178]: https://gitlab.freedesktop.org/drm/intel/issues/7178
  [i915#7384]: https://gitlab.freedesktop.org/drm/intel/issues/7384
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7733]: https://gitlab.freedesktop.org/drm/intel/issues/7733
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7790]: https://gitlab.freedesktop.org/drm/intel/issues/7790
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8153]: https://gitlab.freedesktop.org/drm/intel/issues/8153
  [i915#8154]: https://gitlab.freedesktop.org/drm/intel/issues/8154
  [i915#8155]: https://gitlab.freedesktop.org/drm/intel/issues/8155
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292
  [i915#8295]: https://gitlab.freedesktop.org/drm/intel/issues/8295
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347
  [i915#8381]: https://gitlab.freedesktop.org/drm/intel/issues/8381


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

  * Linux: CI_DRM_13056 -> Patchwork_116517v4

  CI-20190529: 20190529
  CI_DRM_13056: 308c0163d9e46238948942260e6d2abcad3d8bff @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7267: a267f0236e06fc282e3dc3b8c7d76f9ed6088d9b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116517v4: 308c0163d9e46238948942260e6d2abcad3d8bff @ 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_116517v4/index.html

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

Re: [PATCH 3/6] drm/i915/uc: Track patch level versions on reduced version firmware files

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 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.
>
> v2: Use correct patch version number and drop redunant debug print
> (review by Daniele / CI results).
>
> 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_uc_fw.c | 30 +++++++++++++++---------
>   1 file changed, 19 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..c9cd9bb47577f 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, 1)) \
> +	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 1)) \
>   	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, 1)) \
>   	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, 1)) \
>   	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;
> @@ -790,6 +791,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 +801,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 3/6] drm/i915/uc: Track patch level versions on reduced version firmware files

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 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.
>
> v2: Use correct patch version number and drop redunant debug print
> (review by Daniele / CI results).
>
> 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_uc_fw.c | 30 +++++++++++++++---------
>   1 file changed, 19 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..c9cd9bb47577f 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, 1)) \
> +	fw_def(ALDERLAKE_P,  0, guc_maj(adlp, 70, 5, 1)) \
>   	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, 1)) \
>   	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, 1)) \
>   	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;
> @@ -790,6 +791,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 +801,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: [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 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. So pull it out into a separate function that is only
> called once per blob type at init time.
>
> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
> It was mentioned that potential issues with backports would not be
> caught by regular pre-merge CI as that only occurs on tip not stable
> branches. Making the validation unconditional and failing driver load
> on detecting of a problem ensures that such backports will also be
> validated correctly.
>
> v2: Change to unconditionally fail module load on a validation error
> (review feedback/discussion with Daniele).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
> +	}
>   
> -			/* 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 true;
>   
> -			/* 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);
> +		return false;
>   	}
> +
> +	return true;
>   }
>   
>   static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>   	uc_fw->type = type;
>   
>   	if (HAS_GT_UC(i915)) {
> +		if (!validate_fw_table_type(i915, type)) {
> +			intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);

In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR includes 
the fact that the fw has been selected, which in turns implies that 
file_selected.path is valid. this means that even with enable_guc=0 the 
wants/uses_guc macro might end up returning true, which is not something 
we want.

Daniele

> +			return;
> +		}
> +
>   		__uc_fw_auto_select(i915, uc_fw);
>   		__uc_fw_user_override(i915, uc_fw);
>   	}

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 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. So pull it out into a separate function that is only
> called once per blob type at init time.
>
> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
> It was mentioned that potential issues with backports would not be
> caught by regular pre-merge CI as that only occurs on tip not stable
> branches. Making the validation unconditional and failing driver load
> on detecting of a problem ensures that such backports will also be
> validated correctly.
>
> v2: Change to unconditionally fail module load on a validation error
> (review feedback/discussion with Daniele).
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
> +	}
>   
> -			/* 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 true;
>   
> -			/* 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);
> +		return false;
>   	}
> +
> +	return true;
>   }
>   
>   static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>   	uc_fw->type = type;
>   
>   	if (HAS_GT_UC(i915)) {
> +		if (!validate_fw_table_type(i915, type)) {
> +			intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);

In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR includes 
the fact that the fw has been selected, which in turns implies that 
file_selected.path is valid. this means that even with enable_guc=0 the 
wants/uses_guc macro might end up returning true, which is not something 
we want.

Daniele

> +			return;
> +		}
> +
>   		__uc_fw_auto_select(i915, uc_fw);
>   		__uc_fw_user_override(i915, uc_fw);
>   	}

Re: [PATCH 5/6] drm/i915/uc: Reject duplicate entries in firmware table

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> It was noticed that duplicate 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 eb52e8db9ae0b..bc4011d55667c 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 bool 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 bool 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

> +				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,

nit: we could avoid printing the platform twice because you're 
explicitly checking that it is the same earlier on. Not a blocked.
With the typo fixed:

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

Daniele

> +				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 bool 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/6] drm/i915/uc: Reject duplicate entries in firmware table

[Ceraolo Spurio, Daniele]

On 4/20/2023 6:15 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> It was noticed that duplicate 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 eb52e8db9ae0b..bc4011d55667c 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 bool 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 bool 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

> +				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,

nit: we could avoid printing the platform twice because you're 
explicitly checking that it is the same earlier on. Not a blocked.
With the typo fixed:

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

Daniele

> +				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 bool 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: [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[John Harrison]

On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>> called once per blob type at init time.
>>
>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>> It was mentioned that potential issues with backports would not be
>> caught by regular pre-merge CI as that only occurs on tip not stable
>> branches. Making the validation unconditional and failing driver load
>> on detecting of a problem ensures that such backports will also be
>> validated correctly.
>>
>> v2: Change to unconditionally fail module load on a validation error
>> (review feedback/discussion with Daniele).
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>> +    }
>>   -            /* 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 true;
>>   -            /* 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);
>> +        return false;
>>       }
>> +
>> +    return true;
>>   }
>>     static const char *__override_guc_firmware_path(struct 
>> drm_i915_private *i915)
>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>> *uc_fw,
>>       uc_fw->type = type;
>>         if (HAS_GT_UC(i915)) {
>> +        if (!validate_fw_table_type(i915, type)) {
>> +            intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
>
> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
> includes the fact that the fw has been selected, which in turns 
> implies that file_selected.path is valid. this means that even with 
> enable_guc=0 the wants/uses_guc macro might end up returning true, 
> which is not something we want.
>
> Daniele
Suggestions for a better plan? Add an another status enum? Nothing 
earlier in the sequence seems appropriate. And the init_early stack does 
not support returning error codes.

John.


>
>> +            return;
>> +        }
>> +
>>           __uc_fw_auto_select(i915, uc_fw);
>>           __uc_fw_user_override(i915, uc_fw);
>>       }
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[John Harrison]

On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>> called once per blob type at init time.
>>
>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>> It was mentioned that potential issues with backports would not be
>> caught by regular pre-merge CI as that only occurs on tip not stable
>> branches. Making the validation unconditional and failing driver load
>> on detecting of a problem ensures that such backports will also be
>> validated correctly.
>>
>> v2: Change to unconditionally fail module load on a validation error
>> (review feedback/discussion with Daniele).
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 +++++++++++++----------
>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>> +    }
>>   -            /* 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 true;
>>   -            /* 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);
>> +        return false;
>>       }
>> +
>> +    return true;
>>   }
>>     static const char *__override_guc_firmware_path(struct 
>> drm_i915_private *i915)
>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>> *uc_fw,
>>       uc_fw->type = type;
>>         if (HAS_GT_UC(i915)) {
>> +        if (!validate_fw_table_type(i915, type)) {
>> +            intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
>
> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
> includes the fact that the fw has been selected, which in turns 
> implies that file_selected.path is valid. this means that even with 
> enable_guc=0 the wants/uses_guc macro might end up returning true, 
> which is not something we want.
>
> Daniele
Suggestions for a better plan? Add an another status enum? Nothing 
earlier in the sequence seems appropriate. And the init_early stack does 
not support returning error codes.

John.


>
>> +            return;
>> +        }
>> +
>>           __uc_fw_auto_select(i915, uc_fw);
>>           __uc_fw_user_override(i915, uc_fw);
>>       }
>

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

[Ceraolo Spurio, Daniele]

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

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

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

Daniele

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

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

[John Harrison]

On 4/28/2023 17:19, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 6:15 PM, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> If the DEBUG_GEM config option is set then escalate the 'unexpected
>> firmware version' message from a notice to an error. This will ensure
>> that the CI system treats such occurences as a failure and logs a bug
>> about it (or fails the pre-merge testing).
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 34 ++++++++++++++----------
>>   1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> index bc4011d55667c..c5b21d17ca437 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -17,6 +17,12 @@
>>   #include "i915_drv.h"
>>   #include "i915_reg.h"
>>   +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>> +#define UNEXPECTED    gt_err
>> +#else
>> +#define UNEXPECTED    gt_notice
>
> I tried to find an alternative word that had the same number of 
> characters as "gt_notice" so that we could avoid the indent in the 
> diff, but nothing came to mind, so:
Yup, already went through that! And it didn't seem worth it to 
abbreviate by dropping a single random vowel.

John.


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

Re: [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[Ceraolo Spurio, Daniele]

On 4/28/2023 5:16 PM, John Harrison wrote:
> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>> called once per blob type at init time.
>>>
>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>> It was mentioned that potential issues with backports would not be
>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>> branches. Making the validation unconditional and failing driver load
>>> on detecting of a problem ensures that such backports will also be
>>> validated correctly.
>>>
>>> v2: Change to unconditionally fail module load on a validation error
>>> (review feedback/discussion with Daniele).
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>> +++++++++++++----------
>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>> +    }
>>>   -            /* 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 true;
>>>   -            /* 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);
>>> +        return false;
>>>       }
>>> +
>>> +    return true;
>>>   }
>>>     static const char *__override_guc_firmware_path(struct 
>>> drm_i915_private *i915)
>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>>> *uc_fw,
>>>       uc_fw->type = type;
>>>         if (HAS_GT_UC(i915)) {
>>> +        if (!validate_fw_table_type(i915, type)) {
>>> +            intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
>>
>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>> includes the fact that the fw has been selected, which in turns 
>> implies that file_selected.path is valid. this means that even with 
>> enable_guc=0 the wants/uses_guc macro might end up returning true, 
>> which is not something we want.
>>
>> Daniele
> Suggestions for a better plan? Add an another status enum? Nothing 
> earlier in the sequence seems appropriate. And the init_early stack 
> does not support returning error codes.

I think the question here is: what are you expecting to happen in case 
of error and on what platforms? let's say we have an invalid table entry 
for ADLP, would the expectation be that all GuC platforms won't boot, or 
just ADLP? And is that only if we have enable_guc set to a positive 
value, or even if enable_guc=0?

Daniele

>
> John.
>
>
>>
>>> +            return;
>>> +        }
>>> +
>>>           __uc_fw_auto_select(i915, uc_fw);
>>>           __uc_fw_user_override(i915, uc_fw);
>>>       }
>>
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[Ceraolo Spurio, Daniele]

On 4/28/2023 5:16 PM, John Harrison wrote:
> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>> called once per blob type at init time.
>>>
>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>> It was mentioned that potential issues with backports would not be
>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>> branches. Making the validation unconditional and failing driver load
>>> on detecting of a problem ensures that such backports will also be
>>> validated correctly.
>>>
>>> v2: Change to unconditionally fail module load on a validation error
>>> (review feedback/discussion with Daniele).
>>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>> +++++++++++++----------
>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>> +    }
>>>   -            /* 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 true;
>>>   -            /* 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);
>>> +        return false;
>>>       }
>>> +
>>> +    return true;
>>>   }
>>>     static const char *__override_guc_firmware_path(struct 
>>> drm_i915_private *i915)
>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>>> *uc_fw,
>>>       uc_fw->type = type;
>>>         if (HAS_GT_UC(i915)) {
>>> +        if (!validate_fw_table_type(i915, type)) {
>>> +            intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_ERROR);
>>
>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>> includes the fact that the fw has been selected, which in turns 
>> implies that file_selected.path is valid. this means that even with 
>> enable_guc=0 the wants/uses_guc macro might end up returning true, 
>> which is not something we want.
>>
>> Daniele
> Suggestions for a better plan? Add an another status enum? Nothing 
> earlier in the sequence seems appropriate. And the init_early stack 
> does not support returning error codes.

I think the question here is: what are you expecting to happen in case 
of error and on what platforms? let's say we have an invalid table entry 
for ADLP, would the expectation be that all GuC platforms won't boot, or 
just ADLP? And is that only if we have enable_guc set to a positive 
value, or even if enable_guc=0?

Daniele

>
> John.
>
>
>>
>>> +            return;
>>> +        }
>>> +
>>>           __uc_fw_auto_select(i915, uc_fw);
>>>           __uc_fw_user_override(i915, uc_fw);
>>>       }
>>
>

Re: [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[John Harrison]

On 4/28/2023 17:26, Ceraolo Spurio, Daniele wrote:
> On 4/28/2023 5:16 PM, John Harrison wrote:
>> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>>> called once per blob type at init time.
>>>>
>>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>>> It was mentioned that potential issues with backports would not be
>>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>>> branches. Making the validation unconditional and failing driver load
>>>> on detecting of a problem ensures that such backports will also be
>>>> validated correctly.
>>>>
>>>> v2: Change to unconditionally fail module load on a validation error
>>>> (review feedback/discussion with Daniele).
>>>>
>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>>> +++++++++++++----------
>>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>>> +    }
>>>>   -            /* 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 true;
>>>>   -            /* 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);
>>>> +        return false;
>>>>       }
>>>> +
>>>> +    return true;
>>>>   }
>>>>     static const char *__override_guc_firmware_path(struct 
>>>> drm_i915_private *i915)
>>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>>>> *uc_fw,
>>>>       uc_fw->type = type;
>>>>         if (HAS_GT_UC(i915)) {
>>>> +        if (!validate_fw_table_type(i915, type)) {
>>>> +            intel_uc_fw_change_status(uc_fw, 
>>>> INTEL_UC_FIRMWARE_ERROR);
>>>
>>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>>> includes the fact that the fw has been selected, which in turns 
>>> implies that file_selected.path is valid. this means that even with 
>>> enable_guc=0 the wants/uses_guc macro might end up returning true, 
>>> which is not something we want.
>>>
>>> Daniele
>> Suggestions for a better plan? Add an another status enum? Nothing 
>> earlier in the sequence seems appropriate. And the init_early stack 
>> does not support returning error codes.
>
> I think the question here is: what are you expecting to happen in case 
> of error and on what platforms? let's say we have an invalid table 
> entry for ADLP, would the expectation be that all GuC platforms won't 
> boot, or just ADLP? And is that only if we have enable_guc set to a 
> positive value, or even if enable_guc=0?
The intention is to totally break driver load on any table error.

The reason being that someone is back porting a firmware update to ADL-P 
but breaks DG2 in the process. However, the are only intending to change 
ADL-P and so don't test on DG2. They therefore don't realise that the 
driver is now broken for someone else. Whereas, if we make any table 
error a fatal load failure irrespective of tested platform, enable_guc 
or other module params, etc. then it is guaranteed to be caught no 
matter what platform they test on.

John.

>
> Daniele
>
>>
>> John.
>>
>>
>>>
>>>> +            return;
>>>> +        }
>>>> +
>>>>           __uc_fw_auto_select(i915, uc_fw);
>>>>           __uc_fw_user_override(i915, uc_fw);
>>>>       }
>>>
>>
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[John Harrison]

On 4/28/2023 17:26, Ceraolo Spurio, Daniele wrote:
> On 4/28/2023 5:16 PM, John Harrison wrote:
>> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>>> called once per blob type at init time.
>>>>
>>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>>> It was mentioned that potential issues with backports would not be
>>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>>> branches. Making the validation unconditional and failing driver load
>>>> on detecting of a problem ensures that such backports will also be
>>>> validated correctly.
>>>>
>>>> v2: Change to unconditionally fail module load on a validation error
>>>> (review feedback/discussion with Daniele).
>>>>
>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>>> +++++++++++++----------
>>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>>> +    }
>>>>   -            /* 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 true;
>>>>   -            /* 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);
>>>> +        return false;
>>>>       }
>>>> +
>>>> +    return true;
>>>>   }
>>>>     static const char *__override_guc_firmware_path(struct 
>>>> drm_i915_private *i915)
>>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
>>>> *uc_fw,
>>>>       uc_fw->type = type;
>>>>         if (HAS_GT_UC(i915)) {
>>>> +        if (!validate_fw_table_type(i915, type)) {
>>>> +            intel_uc_fw_change_status(uc_fw, 
>>>> INTEL_UC_FIRMWARE_ERROR);
>>>
>>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>>> includes the fact that the fw has been selected, which in turns 
>>> implies that file_selected.path is valid. this means that even with 
>>> enable_guc=0 the wants/uses_guc macro might end up returning true, 
>>> which is not something we want.
>>>
>>> Daniele
>> Suggestions for a better plan? Add an another status enum? Nothing 
>> earlier in the sequence seems appropriate. And the init_early stack 
>> does not support returning error codes.
>
> I think the question here is: what are you expecting to happen in case 
> of error and on what platforms? let's say we have an invalid table 
> entry for ADLP, would the expectation be that all GuC platforms won't 
> boot, or just ADLP? And is that only if we have enable_guc set to a 
> positive value, or even if enable_guc=0?
The intention is to totally break driver load on any table error.

The reason being that someone is back porting a firmware update to ADL-P 
but breaks DG2 in the process. However, the are only intending to change 
ADL-P and so don't test on DG2. They therefore don't realise that the 
driver is now broken for someone else. Whereas, if we make any table 
error a fatal load failure irrespective of tested platform, enable_guc 
or other module params, etc. then it is guaranteed to be caught no 
matter what platform they test on.

John.

>
> Daniele
>
>>
>> John.
>>
>>
>>>
>>>> +            return;
>>>> +        }
>>>> +
>>>>           __uc_fw_auto_select(i915, uc_fw);
>>>>           __uc_fw_user_override(i915, uc_fw);
>>>>       }
>>>
>>
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[John Harrison]

On 4/28/2023 17:30, John Harrison wrote:
> On 4/28/2023 17:26, Ceraolo Spurio, Daniele wrote:
>> On 4/28/2023 5:16 PM, John Harrison wrote:
>>> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>>>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>>>> called once per blob type at init time.
>>>>>
>>>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>>>> It was mentioned that potential issues with backports would not be
>>>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>>>> branches. Making the validation unconditional and failing driver load
>>>>> on detecting of a problem ensures that such backports will also be
>>>>> validated correctly.
>>>>>
>>>>> v2: Change to unconditionally fail module load on a validation error
>>>>> (review feedback/discussion with Daniele).
>>>>>
>>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>>>> +++++++++++++----------
>>>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>>>> +    }
>>>>>   -            /* 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 true;
>>>>>   -            /* 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);
>>>>> +        return false;
>>>>>       }
>>>>> +
>>>>> +    return true;
>>>>>   }
>>>>>     static const char *__override_guc_firmware_path(struct 
>>>>> drm_i915_private *i915)
>>>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct 
>>>>> intel_uc_fw *uc_fw,
>>>>>       uc_fw->type = type;
>>>>>         if (HAS_GT_UC(i915)) {
>>>>> +        if (!validate_fw_table_type(i915, type)) {
>>>>> +            intel_uc_fw_change_status(uc_fw, 
>>>>> INTEL_UC_FIRMWARE_ERROR);
>>>>
>>>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>>>> includes the fact that the fw has been selected, which in turns 
>>>> implies that file_selected.path is valid. this means that even with 
>>>> enable_guc=0 the wants/uses_guc macro might end up returning true, 
>>>> which is not something we want.
>>>>
>>>> Daniele
>>> Suggestions for a better plan? Add an another status enum? Nothing 
>>> earlier in the sequence seems appropriate. And the init_early stack 
>>> does not support returning error codes.
>>
>> I think the question here is: what are you expecting to happen in 
>> case of error and on what platforms? let's say we have an invalid 
>> table entry for ADLP, would the expectation be that all GuC platforms 
>> won't boot, or just ADLP? And is that only if we have enable_guc set 
>> to a positive value, or even if enable_guc=0?
> The intention is to totally break driver load on any table error.
>
> The reason being that someone is back porting a firmware update to 
> ADL-P but breaks DG2 in the process. However, the are only intending 
> to change ADL-P and so don't test on DG2. They therefore don't realise 
> that the driver is now broken for someone else. Whereas, if we make 
> any table error a fatal load failure irrespective of tested platform, 
> enable_guc or other module params, etc. then it is guaranteed to be 
> caught no matter what platform they test on.
>
Well, I guess if you are testing on a platform that does'nt use GuC/HuC 
at all (or have enalble_guc=0) then none of this code would even run? 
But then, if you are patching the firmware loading code then it is 
reasonable to expect a test run on at least one firmware enabled platform.

John.

> John.
>
>>
>> Daniele
>>
>>>
>>> John.
>>>
>>>
>>>>
>>>>> +            return;
>>>>> +        }
>>>>> +
>>>>>           __uc_fw_auto_select(i915, uc_fw);
>>>>>           __uc_fw_user_override(i915, uc_fw);
>>>>>       }
>>>>
>>>
>>
>

Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc: Enhancements to firmware table validation

[Ceraolo Spurio, Daniele]

On 4/28/2023 5:32 PM, John Harrison wrote:
> On 4/28/2023 17:30, John Harrison wrote:
>> On 4/28/2023 17:26, Ceraolo Spurio, Daniele wrote:
>>> On 4/28/2023 5:16 PM, John Harrison wrote:
>>>> On 4/28/2023 17:04, Ceraolo Spurio, Daniele wrote:
>>>>> On 4/20/2023 6:15 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. So pull it out into a separate function that is only
>>>>>> called once per blob type at init time.
>>>>>>
>>>>>> Also, drop the CONFIG_SELFTEST requirement and make errors terminal.
>>>>>> It was mentioned that potential issues with backports would not be
>>>>>> caught by regular pre-merge CI as that only occurs on tip not stable
>>>>>> branches. Making the validation unconditional and failing driver 
>>>>>> load
>>>>>> on detecting of a problem ensures that such backports will also be
>>>>>> validated correctly.
>>>>>>
>>>>>> v2: Change to unconditionally fail module load on a validation error
>>>>>> (review feedback/discussion with Daniele).
>>>>>>
>>>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 157 
>>>>>> +++++++++++++----------
>>>>>>   1 file changed, 92 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 c9cd9bb47577f..eb52e8db9ae0b 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,76 +313,91 @@ __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 bool 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 false;
>>>>>> +    }
>>>>>>   -            /* 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 true;
>>>>>>   -            /* 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);
>>>>>> +        return false;
>>>>>>       }
>>>>>> +
>>>>>> +    return true;
>>>>>>   }
>>>>>>     static const char *__override_guc_firmware_path(struct 
>>>>>> drm_i915_private *i915)
>>>>>> @@ -443,6 +465,11 @@ void intel_uc_fw_init_early(struct 
>>>>>> intel_uc_fw *uc_fw,
>>>>>>       uc_fw->type = type;
>>>>>>         if (HAS_GT_UC(i915)) {
>>>>>> +        if (!validate_fw_table_type(i915, type)) {
>>>>>> +            intel_uc_fw_change_status(uc_fw, 
>>>>>> INTEL_UC_FIRMWARE_ERROR);
>>>>>
>>>>> In our hierarchy of firmware statuses, INTEL_UC_FIRMWARE_ERROR 
>>>>> includes the fact that the fw has been selected, which in turns 
>>>>> implies that file_selected.path is valid. this means that even 
>>>>> with enable_guc=0 the wants/uses_guc macro might end up returning 
>>>>> true, which is not something we want.
>>>>>
>>>>> Daniele
>>>> Suggestions for a better plan? Add an another status enum? Nothing 
>>>> earlier in the sequence seems appropriate. And the init_early stack 
>>>> does not support returning error codes.
>>>
>>> I think the question here is: what are you expecting to happen in 
>>> case of error and on what platforms? let's say we have an invalid 
>>> table entry for ADLP, would the expectation be that all GuC 
>>> platforms won't boot, or just ADLP? And is that only if we have 
>>> enable_guc set to a positive value, or even if enable_guc=0?
>> The intention is to totally break driver load on any table error.
>>
>> The reason being that someone is back porting a firmware update to 
>> ADL-P but breaks DG2 in the process. However, the are only intending 
>> to change ADL-P and so don't test on DG2. They therefore don't 
>> realise that the driver is now broken for someone else. Whereas, if 
>> we make any table error a fatal load failure irrespective of tested 
>> platform, enable_guc or other module params, etc. then it is 
>> guaranteed to be caught no matter what platform they test on.
>>
> Well, I guess if you are testing on a platform that does'nt use 
> GuC/HuC at all (or have enalble_guc=0) then none of this code would 
> even run? But then, if you are patching the firmware loading code then 
> it is reasonable to expect a test run on at least one firmware enabled 
> platform.

I was thinking we could add a new status code, but things then might 
become complicated because both the submission mode (GuC vs execlists) 
and the FW fetching are tied to the "wants" flag, so I'm not sure how 
easy it would be to keep one but skip the other. An alternative option 
could be to set the path to a known "invalid" value and fail the fetch 
if we get that path (or just fail the fetch if path is NULL and/or 
status is error).

Daniele

>
> John.
>
>> John.
>>
>>>
>>> Daniele
>>>
>>>>
>>>> John.
>>>>
>>>>
>>>>>
>>>>>> +            return;
>>>>>> +        }
>>>>>> +
>>>>>>           __uc_fw_auto_select(i915, uc_fw);
>>>>>>           __uc_fw_user_override(i915, uc_fw);
>>>>>>       }
>>>>>
>>>>
>>>
>>
>