Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/uc: Fix issues with overriding firmware files
On 9/13/2022 5:58 PM, john.c.harri...@intel.com wrote:
From: John Harrison
The earlier update to support reduced versioning of firmware files
introduced an issue with the firmware override module parameter. If an
invalid file was specified then an infinite loop could occur trying to
find a backup firmware.
The fix is that if an explicit override has been set, then don't scan
for backup options because there is no point anyway. The user wanted X
and if X is not available, that's their problem.
This patch also fixes up the scanning loop code so that if an invalid
file is passed in, it will exit rather than loop forever. So if the
impossible situation did somehow occur in the future, it wouldn't be
such a big problem.
v2: Also remove ANSI colour codes that accidentally got left in an
error message in the original patch.
With the commit message updated to include what you mentioned in your
reply, this is:
Reviewed-by: Daniele Ceraolo Spurio
Daniele
Fixes: 665ae9c9ca79 ("drm/i915/uc: Support for version reduced and multiple
firmware files")
Cc: Daniele Ceraolo Spurio
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Matthew Brost
Cc: Umesh Nerlige Ramappa
Cc: Matthew Auld
Cc: Alan Previn
Cc: Matt Roper
Cc: Lucas De Marchi
Cc: Vinay Belgaumkar
Cc: "Thomas Hellström"
Cc: Venkata Sandeep Dhanalakota
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index af425916cdf64..1169e2a09da24 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -232,6 +232,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
u32 fw_count;
u8 rev = INTEL_REVID(i915);
int i;
+ bool found;
/*
* The only difference between the ADL GuC FWs is the HWConfig support.
@@ -246,6 +247,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
fw_blobs = blobs_all[uc_fw->type].blobs;
fw_count = blobs_all[uc_fw->type].count;
+ found = false;
for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
const struct uc_fw_blob *blob = _blobs[i].blob;
@@ -266,9 +268,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
uc_fw->file_wanted.path = blob->path;
uc_fw->file_wanted.major_ver = blob->major;
uc_fw->file_wanted.minor_ver = blob->minor;
+ found = true;
break;
}
+ if (!found && uc_fw->file_selected.path) {
+ /* Failed to find a match for the last attempt?! */
+ uc_fw->file_selected.path = NULL;
+ }
+
/* make sure the list is ordered as expected */
if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified) {
verified = true;
@@ -322,7 +330,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
continue;
bad:
- drm_err(>drm, "\x1B[35;1mInvalid FW blob order: %s r%u
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
+ drm_err(>drm, "Invalid FW blob order: %s r%u
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
intel_platform_name(fw_blobs[i - 1].p),
fw_blobs[i - 1].rev,
fw_blobs[i - 1].blob.legacy ? "L" : "v",
fw_blobs[i - 1].blob.major,
@@ -553,10 +561,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
err = firmware_request_nowarn(, uc_fw->file_selected.path, dev);
memcpy(_ideal, _fw->file_wanted, sizeof(file_ideal));
- if (!err || intel_uc_fw_is_overridden(uc_fw))
- goto done;
+
+ /* Any error is terminal if overriding. Don't bother searching for
older versions */
+ if (err && intel_uc_fw_is_overridden(uc_fw))
+ goto fail;
while (err == -ENOENT) {
+ old_ver = true;
+
__uc_fw_auto_select(i915, uc_fw);
if (!uc_fw->file_selected.path) {
/*
@@ -576,8 +588,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
if (err)
goto fail;
- old_ver = true;
-done:
if (uc_fw->loaded_via_gsc)
err = check_gsc_manifest(fw, uc_fw);
else
Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/uc: Fix issues with overriding firmware files
On 9/13/2022 17:58, john.c.harri...@intel.com wrote:
From: John Harrison
The earlier update to support reduced versioning of firmware files
introduced an issue with the firmware override module parameter. If an
invalid file was specified then an infinite loop could occur trying to
find a backup firmware.
The above not entirely correct - got myself confused while typing it up.
A self test would specify an invalid file name (invalid meaning not in
the table) both with and without setting the override flag. The
*non-override* case would cause the infinite loop. I.e. a situation that
is impossible to hit outside of the selftest because either the file
name has come from the table in first place or it came from an override.
However, the override case was still broken in that it would bypass some
of the later processing.
The fix is that if an explicit override has been set, then don't scan
for backup options because there is no point anyway. The user wanted X
and if X is not available, that's their problem.
This patch also fixes up the scanning loop code so that if an invalid
file is passed in, it will exit rather than loop forever. So if the
impossible situation did somehow occur in the future, it wouldn't be
such a big problem.
It also flips the logic on the override early exit to be negative rather
than positive so as not to skip code that still needs to be run.
John.
v2: Also remove ANSI colour codes that accidentally got left in an
error message in the original patch.
Fixes: 665ae9c9ca79 ("drm/i915/uc: Support for version reduced and multiple
firmware files")
Cc: Daniele Ceraolo Spurio
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Matthew Brost
Cc: Umesh Nerlige Ramappa
Cc: Matthew Auld
Cc: Alan Previn
Cc: Matt Roper
Cc: Lucas De Marchi
Cc: Vinay Belgaumkar
Cc: "Thomas Hellström"
Cc: Venkata Sandeep Dhanalakota
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index af425916cdf64..1169e2a09da24 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -232,6 +232,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
u32 fw_count;
u8 rev = INTEL_REVID(i915);
int i;
+ bool found;
/*
* The only difference between the ADL GuC FWs is the HWConfig support.
@@ -246,6 +247,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
fw_blobs = blobs_all[uc_fw->type].blobs;
fw_count = blobs_all[uc_fw->type].count;
+ found = false;
for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
const struct uc_fw_blob *blob = _blobs[i].blob;
@@ -266,9 +268,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
uc_fw->file_wanted.path = blob->path;
uc_fw->file_wanted.major_ver = blob->major;
uc_fw->file_wanted.minor_ver = blob->minor;
+ found = true;
break;
}
+ if (!found && uc_fw->file_selected.path) {
+ /* Failed to find a match for the last attempt?! */
+ uc_fw->file_selected.path = NULL;
+ }
+
/* make sure the list is ordered as expected */
if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified) {
verified = true;
@@ -322,7 +330,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
continue;
bad:
- drm_err(>drm, "\x1B[35;1mInvalid FW blob order: %s r%u
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
+ drm_err(>drm, "Invalid FW blob order: %s r%u
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
intel_platform_name(fw_blobs[i - 1].p),
fw_blobs[i - 1].rev,
fw_blobs[i - 1].blob.legacy ? "L" : "v",
fw_blobs[i - 1].blob.major,
@@ -553,10 +561,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
err = firmware_request_nowarn(, uc_fw->file_selected.path, dev);
memcpy(_ideal, _fw->file_wanted, sizeof(file_ideal));
- if (!err || intel_uc_fw_is_overridden(uc_fw))
- goto done;
+
+ /* Any error is terminal if overriding. Don't bother searching for
older versions */
+ if (err && intel_uc_fw_is_overridden(uc_fw))
+ goto fail;
while (err == -ENOENT) {
+ old_ver = true;
+
__uc_fw_auto_select(i915, uc_fw);
if (!uc_fw->file_selected.path) {
/*
@@ -576,8 +588,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
if (err)
goto fail;
-
[Intel-gfx] [PATCH v2 1/1] drm/i915/uc: Fix issues with overriding firmware files
From: John Harrison
The earlier update to support reduced versioning of firmware files
introduced an issue with the firmware override module parameter. If an
invalid file was specified then an infinite loop could occur trying to
find a backup firmware.
The fix is that if an explicit override has been set, then don't scan
for backup options because there is no point anyway. The user wanted X
and if X is not available, that's their problem.
This patch also fixes up the scanning loop code so that if an invalid
file is passed in, it will exit rather than loop forever. So if the
impossible situation did somehow occur in the future, it wouldn't be
such a big problem.
v2: Also remove ANSI colour codes that accidentally got left in an
error message in the original patch.
Fixes: 665ae9c9ca79 ("drm/i915/uc: Support for version reduced and multiple
firmware files")
Cc: Daniele Ceraolo Spurio
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Matthew Brost
Cc: Umesh Nerlige Ramappa
Cc: Matthew Auld
Cc: Alan Previn
Cc: Matt Roper
Cc: Lucas De Marchi
Cc: Vinay Belgaumkar
Cc: "Thomas Hellström"
Cc: Venkata Sandeep Dhanalakota
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +++-
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index af425916cdf64..1169e2a09da24 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -232,6 +232,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
u32 fw_count;
u8 rev = INTEL_REVID(i915);
int i;
+ bool found;
/*
* The only difference between the ADL GuC FWs is the HWConfig support.
@@ -246,6 +247,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
fw_blobs = blobs_all[uc_fw->type].blobs;
fw_count = blobs_all[uc_fw->type].count;
+ found = false;
for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
const struct uc_fw_blob *blob = _blobs[i].blob;
@@ -266,9 +268,15 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
uc_fw->file_wanted.path = blob->path;
uc_fw->file_wanted.major_ver = blob->major;
uc_fw->file_wanted.minor_ver = blob->minor;
+ found = true;
break;
}
+ if (!found && uc_fw->file_selected.path) {
+ /* Failed to find a match for the last attempt?! */
+ uc_fw->file_selected.path = NULL;
+ }
+
/* make sure the list is ordered as expected */
if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified) {
verified = true;
@@ -322,7 +330,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct
intel_uc_fw *uc_fw)
continue;
bad:
- drm_err(>drm, "\x1B[35;1mInvalid FW blob order:
%s r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
+ drm_err(>drm, "Invalid FW blob order: %s r%u
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
intel_platform_name(fw_blobs[i - 1].p),
fw_blobs[i - 1].rev,
fw_blobs[i - 1].blob.legacy ? "L" : "v",
fw_blobs[i - 1].blob.major,
@@ -553,10 +561,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
err = firmware_request_nowarn(, uc_fw->file_selected.path, dev);
memcpy(_ideal, _fw->file_wanted, sizeof(file_ideal));
- if (!err || intel_uc_fw_is_overridden(uc_fw))
- goto done;
+
+ /* Any error is terminal if overriding. Don't bother searching for
older versions */
+ if (err && intel_uc_fw_is_overridden(uc_fw))
+ goto fail;
while (err == -ENOENT) {
+ old_ver = true;
+
__uc_fw_auto_select(i915, uc_fw);
if (!uc_fw->file_selected.path) {
/*
@@ -576,8 +588,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
if (err)
goto fail;
- old_ver = true;
-done:
if (uc_fw->loaded_via_gsc)
err = check_gsc_manifest(fw, uc_fw);
else
--
2.37.3
