On 3/12/2023 12:56, Alexandre Oliva wrote:
If two or more suitable entries with the same filename are found in
__uc_fw_auto_select's fw_blobs, and that filename fails to load in the
first attempt and in the retry, when __uc_fw_auto_select is called for
the third time, the coincidence of strings will cause it to clear
file_selected.path at the first hit, so it will return the second hit
over and over again, indefinitely.
Of course this doesn't occur with the pristine blob lists, but a
modified version could run into this, e.g., patching in a duplicate
entry, or (as in our case) disarming blob loading by remapping their
names to "/*(DEBLOBBED)*/", given a toolchain that unifies identical
string literals.
Not sure what you mean by disarming?
I think what you are saying is that you made a change similar to this?
#define __MAKE_UC_FW_PATH_MMP(prefix_, name_, major_, minor_,
patch_) "i915/invalid_file_name.bin"
So all entries in the table have the exact same filename. And with the
toolchain unification comment, that means not just a matching string but
the same string pointer. Thus, the search code is getting confused.
I'm not sure that is really a valid use case that the driver code should
be expected to support. I'm not even sure what the purpose of your
testing is? Even without the infinite loop, the driver is not going to
load because you have removed the firmware files?
However, I think you are saying that the problem would also exist if
there was some kind of genuine duplication in the table? Can you give an
example of a genuine use case problem? If the same string is used for
different platforms, I believe that should be fine. E.g. there are
already a bunch of different platforms that all use the same TGL
firmware file. Even with the string unification, that should not be an
issue because the search is within a platform only. So there can only be
a problem if a single platform specifies the same filename multiple
times? Which would be a bug in the table because why? It would be
redundant entries that have no purpose.
Note that I'm not saying we don't want to take your change. But I would
like to understand if there is a genuine issue that maybe needs a better
fix. E.g. should the table verification code be enhanced to just reject
the table entirely if there are such errors present.
Also, is this string unification thing a part of the current gcc
toolchain? Or are you saying that is a new feature that is not generally
available yet? Or maybe only exists in some non-gcc toolchain?
Thanks,
John.
Of course I'm ready to carry a patchlet to avoid this problem
triggered by our (GNU Linux-libre's) intentional changes, but I
figured you might be interested in fail-safing it even in accidental
backporting circumstances. I realize it's not entirely foolproof: if
the same string appears in two entries separated by a different one,
the infinite loop might still occur. Catching that even more unlikely
situation seemed too expensive.
Link: https://www.fsfla.org/pipermail/linux-libre/2023-March/003506.html
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx # 6.[12].x
Signed-off-by: Alexandre Oliva <lxoliva@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
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 9d6f571097e6..2b7564a3ed82 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -259,7 +259,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
uc_fw->file_selected.path = NULL;
continue;
- }
+ } else if (uc_fw->file_wanted.path == blob->path)
+ /* Avoid retrying forever when neighbor
+ entries point to the same path. */
+ continue;
uc_fw->file_selected.path = blob->path;
uc_fw->file_wanted.path = blob->path;
