[Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Alexandre Oliva]

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.

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@lists.freedesktop.org
Cc: stable@vger.kernel.org # 6.[12].x
Signed-off-by: Alexandre Oliva <lxoliva@fsfla.org>
---
 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;
-- 
2.25.1


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

[Intel-gfx] ✗ Fi.CI.BUILD: warning for avoid infinite retries in GuC/HuC loading

[Patchwork]

== Series Details ==

Series: avoid infinite retries in GuC/HuC loading
URL   : https://patchwork.freedesktop.org/series/115386/
State : warning

== Summary ==

Error: git fetch origin failed

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for avoid infinite retries in GuC/HuC loading

[Patchwork]

== Series Details ==

Series: avoid infinite retries in GuC/HuC loading
URL   : https://patchwork.freedesktop.org/series/115386/
State : warning

== Summary ==

Error: git fetch origin failed

[Intel-gfx] ✓ Fi.CI.BAT: success for avoid infinite retries in GuC/HuC loading

[Patchwork]

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

== Series Details ==

Series: avoid infinite retries in GuC/HuC loading
URL   : https://patchwork.freedesktop.org/series/115386/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12884 -> Patchwork_115386v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (35 -> 34)
------------------------------

  Missing    (1): bat-dg1-6 

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

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

### IGT changes ###

#### Issues hit ####

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

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

  
#### Possible fixes ####

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][3] ([i915#4983]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/bat-rpls-1/igt@i915_selftest@live@reset.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/bat-rpls-1/igt@i915_selftest@live@reset.html

  
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996


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

  * Linux: CI_DRM_12884 -> Patchwork_115386v1

  CI-20190529: 20190529
  CI_DRM_12884: 1d4054731cfcb1cb9810d309b70535ae0b90ecf0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7208: f327c5d77b6ea6adff1ef6d08f21f232dfe093e3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115386v1: 1d4054731cfcb1cb9810d309b70535ae0b90ecf0 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

131470d79f4d avoid infinite retries in GuC/HuC loading

== Logs ==

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

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

[Intel-gfx] ✓ Fi.CI.IGT: success for avoid infinite retries in GuC/HuC loading

[Patchwork]

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

== Series Details ==

Series: avoid infinite retries in GuC/HuC loading
URL   : https://patchwork.freedesktop.org/series/115386/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12884_full -> Patchwork_115386v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (8 -> 8)
------------------------------

  No changes in participating hosts

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

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

### IGT changes ###

#### Suppressed ####

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

  * igt@gem_mmap_offset@clear@smem0:
    - {shard-dg1}:        NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-dg1-18/igt@gem_mmap_offset@clear@smem0.html

  * igt@kms_ccs@pipe-c-bad-aux-stride-yf_tiled_ccs:
    - {shard-rkl}:        [SKIP][2] ([fdo#109315]) -> [SKIP][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@kms_ccs@pipe-c-bad-aux-stride-yf_tiled_ccs.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_ccs@pipe-c-bad-aux-stride-yf_tiled_ccs.html

  * igt@kms_ccs@pipe-c-crc-sprite-planes-basic-4_tiled_dg2_mc_ccs:
    - {shard-rkl}:        [SKIP][4] ([i915#4098]) -> [SKIP][5] +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@kms_ccs@pipe-c-crc-sprite-planes-basic-4_tiled_dg2_mc_ccs.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_ccs@pipe-c-crc-sprite-planes-basic-4_tiled_dg2_mc_ccs.html

  * igt@kms_ccs@pipe-d-missing-ccs-buffer-y_tiled_gen12_mc_ccs:
    - {shard-rkl}:        [SKIP][6] ([i915#1845] / [i915#4098]) -> [SKIP][7] +4 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@kms_ccs@pipe-d-missing-ccs-buffer-y_tiled_gen12_mc_ccs.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_ccs@pipe-d-missing-ccs-buffer-y_tiled_gen12_mc_ccs.html

  * igt@kms_plane@plane-panning-top-left:
    - {shard-rkl}:        NOTRUN -> [SKIP][8]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-3/igt@kms_plane@plane-panning-top-left.html

  * igt@kms_plane@plane-position-hole:
    - {shard-tglu}:       NOTRUN -> [SKIP][9] +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-9/igt@kms_plane@plane-position-hole.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [PASS][10] -> [FAIL][11] ([i915#2842])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-apl4/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-apl2/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-apl:          [PASS][12] -> [ABORT][13] ([i915#5566])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-apl6/igt@gen9_exec_parse@allowed-all.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-apl1/igt@gen9_exec_parse@allowed-all.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-glk:          [PASS][14] -> [FAIL][15] ([i915#2346])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-glk2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-glk3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [PASS][16] -> [FAIL][17] ([i915#2346])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2:
    - shard-glk:          [PASS][18] -> [FAIL][19] ([i915#79])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-glk3/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-glk1/igt@kms_flip@flip-vs-expired-vblank@c-hdmi-a2.html

  
#### Possible fixes ####

  * igt@device_reset@unbind-reset-rebind:
    - {shard-rkl}:        [FAIL][20] ([i915#4778]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@device_reset@unbind-reset-rebind.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@device_reset@unbind-reset-rebind.html

  * igt@fbdev@write:
    - {shard-tglu}:       [SKIP][22] ([i915#2582]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@fbdev@write.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-7/igt@fbdev@write.html

  * {igt@gem_barrier_race@remote-request@rcs0}:
    - {shard-tglu}:       [ABORT][24] ([i915#8211]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@gem_barrier_race@remote-request@rcs0.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-7/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_ctx_persistence@smoketest:
    - {shard-tglu}:       [FAIL][26] ([i915#5099]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-6/igt@gem_ctx_persistence@smoketest.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-3/igt@gem_ctx_persistence@smoketest.html

  * igt@gem_exec_balancer@fairslice:
    - {shard-rkl}:        [SKIP][28] ([i915#6259]) -> [PASS][29]
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@gem_exec_balancer@fairslice.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@gem_exec_balancer@fairslice.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-rkl}:        [FAIL][30] ([i915#2842]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-4/igt@gem_exec_fair@basic-pace-share@rcs0.html
    - shard-apl:          [FAIL][32] ([i915#2842]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-apl6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-apl7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - {shard-rkl}:        [SKIP][34] ([fdo#109313]) -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-6/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-5/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html

  * igt@gem_exec_reloc@basic-wc-cpu:
    - {shard-rkl}:        [SKIP][36] ([i915#3281]) -> [PASS][37] +4 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-6/igt@gem_exec_reloc@basic-wc-cpu.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-5/igt@gem_exec_reloc@basic-wc-cpu.html

  * igt@gem_exec_suspend@basic-s4-devices@smem:
    - {shard-tglu}:       [ABORT][38] ([i915#7975]) -> [PASS][39]
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-10/igt@gem_exec_suspend@basic-s4-devices@smem.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-5/igt@gem_exec_suspend@basic-s4-devices@smem.html

  * igt@gem_set_tiling_vs_pwrite:
    - {shard-rkl}:        [SKIP][40] ([i915#3282]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-1/igt@gem_set_tiling_vs_pwrite.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-5/igt@gem_set_tiling_vs_pwrite.html

  * igt@gen9_exec_parse@batch-zero-length:
    - {shard-rkl}:        [SKIP][42] ([i915#2527]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-1/igt@gen9_exec_parse@batch-zero-length.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-5/igt@gen9_exec_parse@batch-zero-length.html

  * igt@i915_pm_dc@dc6-dpms:
    - {shard-tglu}:       [FAIL][44] ([i915#3989] / [i915#454]) -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-7/igt@i915_pm_dc@dc6-dpms.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-2/igt@i915_pm_dc@dc6-dpms.html

  * igt@i915_pm_dc@dc9-dpms:
    - {shard-tglu}:       [SKIP][46] ([i915#4281]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-6/igt@i915_pm_dc@dc9-dpms.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-4/igt@i915_pm_dc@dc9-dpms.html

  * igt@i915_pm_rpm@dpms-lpsp:
    - {shard-tglu}:       [SKIP][48] ([i915#1397]) -> [PASS][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@i915_pm_rpm@dpms-lpsp.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-3/igt@i915_pm_rpm@dpms-lpsp.html

  * igt@i915_pm_rpm@i2c:
    - {shard-tglu}:       [SKIP][50] ([i915#3547]) -> [PASS][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@i915_pm_rpm@i2c.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-3/igt@i915_pm_rpm@i2c.html

  * igt@i915_pm_rpm@system-suspend-modeset:
    - {shard-rkl}:        [SKIP][52] ([fdo#109308]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@i915_pm_rpm@system-suspend-modeset.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@i915_pm_rpm@system-suspend-modeset.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-0:
    - {shard-rkl}:        [SKIP][54] ([i915#1845] / [i915#4098]) -> [PASS][55] +15 similar issues
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_big_fb@x-tiled-32bpp-rotate-0.html

  * igt@kms_big_fb@x-tiled-32bpp-rotate-180:
    - {shard-tglu}:       [SKIP][56] ([i915#1845]) -> [PASS][57] +17 similar issues
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-10/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-5/igt@kms_big_fb@x-tiled-32bpp-rotate-180.html

  * igt@kms_ccs@pipe-d-bad-pixel-format-y_tiled_gen12_rc_ccs:
    - {shard-tglu}:       [SKIP][58] ([i915#1845] / [i915#7651]) -> [PASS][59] +26 similar issues
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@kms_ccs@pipe-d-bad-pixel-format-y_tiled_gen12_rc_ccs.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-7/igt@kms_ccs@pipe-d-bad-pixel-format-y_tiled_gen12_rc_ccs.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - {shard-tglu}:       [SKIP][60] ([i915#1849]) -> [PASS][61] +6 similar issues
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff.html

  * igt@kms_frontbuffer_tracking@psr-modesetfrombusy:
    - {shard-rkl}:        [SKIP][62] ([fdo#109315]) -> [PASS][63] +3 similar issues
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@kms_frontbuffer_tracking@psr-modesetfrombusy.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-modesetfrombusy.html

  * igt@kms_frontbuffer_tracking@psr-rgb565-draw-render:
    - {shard-rkl}:        [SKIP][64] ([i915#1849] / [i915#4098]) -> [PASS][65] +6 similar issues
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@kms_frontbuffer_tracking@psr-rgb565-draw-render.html
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-rgb565-draw-render.html

  * {igt@kms_plane@invalid-pixel-format-settings}:
    - {shard-tglu}:       [SKIP][66] ([i915#8152]) -> [PASS][67]
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-tglu-9/igt@kms_plane@invalid-pixel-format-settings.html
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-tglu-3/igt@kms_plane@invalid-pixel-format-settings.html

  * igt@kms_psr@primary_mmap_gtt:
    - {shard-rkl}:        [SKIP][68] ([i915#1072]) -> [PASS][69]
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@kms_psr@primary_mmap_gtt.html
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@kms_psr@primary_mmap_gtt.html

  * igt@prime_vgem@basic-fence-flip:
    - {shard-rkl}:        [SKIP][70] ([fdo#109295] / [i915#3708] / [i915#4098]) -> [PASS][71]
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-2/igt@prime_vgem@basic-fence-flip.html
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-read:
    - {shard-rkl}:        [SKIP][72] ([fdo#109295] / [i915#3291] / [i915#3708]) -> [PASS][73]
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-1/igt@prime_vgem@basic-fence-read.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-5/igt@prime_vgem@basic-fence-read.html

  * igt@syncobj_timeline@invalid-multi-wait-all-unsubmitted-submitted-signaled:
    - {shard-rkl}:        [SKIP][74] ([i915#2575]) -> [PASS][75] +1 similar issue
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12884/shard-rkl-5/igt@syncobj_timeline@invalid-multi-wait-all-unsubmitted-submitted-signaled.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115386v1/shard-rkl-6/igt@syncobj_timeline@invalid-multi-wait-all-unsubmitted-submitted-signaled.html

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

  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [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#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [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#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [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#2435]: https://gitlab.freedesktop.org/drm/intel/issues/2435
  [i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436
  [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#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#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [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#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3547]: https://gitlab.freedesktop.org/drm/intel/issues/3547
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [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#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3778]: https://gitlab.freedesktop.org/drm/intel/issues/3778
  [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#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#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4778]: https://gitlab.freedesktop.org/drm/intel/issues/4778
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5099]: https://gitlab.freedesktop.org/drm/intel/issues/5099
  [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#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
  [i915#6259]: https://gitlab.freedesktop.org/drm/intel/issues/6259
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
  [i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
  [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981
  [i915#8152]: https://gitlab.freedesktop.org/drm/intel/issues/8152
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8228]: https://gitlab.freedesktop.org/drm/intel/issues/8228
  [i915#8247]: https://gitlab.freedesktop.org/drm/intel/issues/8247
  [i915#8282]: https://gitlab.freedesktop.org/drm/intel/issues/8282


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

  * Linux: CI_DRM_12884 -> Patchwork_115386v1

  CI-20190529: 20190529
  CI_DRM_12884: 1d4054731cfcb1cb9810d309b70535ae0b90ecf0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7208: f327c5d77b6ea6adff1ef6d08f21f232dfe093e3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115386v1: 1d4054731cfcb1cb9810d309b70535ae0b90ecf0 @ 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_115386v1/index.html

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

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Rodrigo Vivi]

On Sun, Mar 12, 2023 at 04:56:23PM -0300, 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.
> 
> 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@lists.freedesktop.org
> Cc: stable@vger.kernel.org # 6.[12].x
> Signed-off-by: Alexandre Oliva <lxoliva@fsfla.org>

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>

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

Since __uc_fw_auto_select is also called from another place,
intel_uc_fw_init_early
out of the intel_uc_fw_fetch infinite loop,
I'm afraid this proposal below could have some side-effect.

I hope Daniele and John have a better understanding and can provide
some guidance or acks here.

>  				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;
> -- 
> 2.25.1
> 
> 
> -- 
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Alexandre Oliva]

On Mar 22, 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:

> On Sun, Mar 12, 2023 at 04:56:23PM -0300, Alexandre Oliva wrote:
>> 
> Since __uc_fw_auto_select is also called from another place,
> intel_uc_fw_init_early
> out of the intel_uc_fw_fetch infinite loop,

That other place is conceptually, sort of, the first iteration of the
infinite loop.  Before that first separate early-init call, *uc_fw is
returned by devm_drm_dev_alloc to i915_driver_create, so
zero-initialized I presume, in both the guc and the huc cases.

Only if this first call finds a matching entry (setting both
file_{selected,wanted}.path), and the selected entry fails to load, do
we even enter the loop (provided that other conditionals are satisfied)
and look for other entries, using file_selected.path to find how far the
previous call got (and, with the proposed patch, file_wanted.path to
avoid retrying the path we've just tried).

> I hope Daniele and John have a better understanding and can provide
> some guidance or acks here.

I surely appreciate additional eyes and minds that are more acquainted
with the code at hand than I am.  Thanks,

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[John Harrison]

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@lists.freedesktop.org
> Cc: stable@vger.kernel.org # 6.[12].x
> Signed-off-by: Alexandre Oliva <lxoliva@fsfla.org>
> ---
>   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;

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Alexandre Oliva]

Hello, John,

On Mar 24, 2023, John Harrison <john.c.harrison@intel.com> wrote:

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

Our users find loading nonfree firmware harmful.

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

Yeah, that's the jist of it.  The name we use is "/*(DEBLOBBED)*/", so
that it can't possibly be satisfied.

> So all entries in the table have the exact same filename.

*nod*

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

Exactly

> I'm not sure that is really a valid use case that the driver code
> should be expected to support.

It's most certainly not.  As I wrote, I'd be happy to keep on carrying
the patch that adjusts the code to cope with our changes.  I just
thought the same issue could come up by, say, mistakenly applying a
patch twice to add support for a new card, a circumstance in which one
might not have the card readily available to try it out.

> Even without the infinite loop, the driver is not
> going to load because you have removed the firmware files?

Oh, no, the driver loads just fine even without those blobs, and that's
much nicer of you than other drivers for hardware that doesn't really
require blobs, but that insist on bailing out if the firmware can't be
loaded.  i915 hasn't been hostile like that.

When you override the firmware filenames, and it fails to load, the
driver makes it a (reasonable IMHO) hard fail, but when it just fails to
find the regular firmware files, it's nice that it proceeds that does
the best it can.

> However, I think you are saying that the problem would also exist if
> there was some kind of genuine duplication in the table?

Yes.  Not the kind you mention, for different platforms, but an actual
duplicate entry, such as what you might get if you applied a patch that
added an entry for a new card, and then applied it again, resolving the
conflicts in a way that retained the duplicate entries.

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

Agreed.

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

Table verification might wish to detect and report duplicate filenames
for the same platform, to catch even alternating duplicates (e.g. "a",
then "b", then "a" again), but it would be kind if you didn't make that
a hard error, otherwise we'd have to tweak it to cope with our own
"/*(DEBLOBBED)*/" duplicates.

Another approach, that would probably be more efficient as the table
grows, is to store in uc_fw a pointer to or index of the current or next
entry to be searched, so that the code doesn't have to iterate over the
table at every try (O(n^2)), and instead takes it from exactly where it
left off, running overall a single time over the whole table (O(n)), at
the cost of a pointer or index in uc_fw.  Then, duplicates in the table
wouldn't matter at all.

> Also, is this string unification thing a part of the current gcc
> toolchain?

Yeah, compilers and linkers have been unifying (read-only) string
literals for a very long time.

Thanks,

-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[John Harrison]

On 3/26/2023 02:46, Alexandre Oliva wrote:
> Hello, John,
>
> On Mar 24, 2023, John Harrison <john.c.harrison@intel.com> wrote:
>
>> 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?
> Our users find loading nonfree firmware harmful.
>
>> 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"
> Yeah, that's the jist of it.  The name we use is "/*(DEBLOBBED)*/", so
> that it can't possibly be satisfied.
>
>> So all entries in the table have the exact same filename.
> *nod*
>
>> 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.
> Exactly
>
>> I'm not sure that is really a valid use case that the driver code
>> should be expected to support.
> It's most certainly not.  As I wrote, I'd be happy to keep on carrying
> the patch that adjusts the code to cope with our changes.  I just
> thought the same issue could come up by, say, mistakenly applying a
> patch twice to add support for a new card, a circumstance in which one
> might not have the card readily available to try it out.
Not following this argument. You can't add support for a card that you 
don't have access to. GuC firmware is produced internally by Intel so it 
isn't going to be added by some third party person. And internally, we 
have CI systems up and running for each platform before the patches to 
support that platform land in the upstream tree. So any such error most 
certainly should be caught by pre-merge CI.

>> Even without the infinite loop, the driver is not
>> going to load because you have removed the firmware files?
> Oh, no, the driver loads just fine even without those blobs, and that's
> much nicer of you than other drivers for hardware that doesn't really
> require blobs, but that insist on bailing out if the firmware can't be
> loaded.  i915 hasn't been hostile like that.
That situation won't last...

> When you override the firmware filenames, and it fails to load, the
> driver makes it a (reasonable IMHO) hard fail, but when it just fails to
> find the regular firmware files, it's nice that it proceeds that does
> the best it can.
>
>> However, I think you are saying that the problem would also exist if
>> there was some kind of genuine duplication in the table?
> Yes.  Not the kind you mention, for different platforms, but an actual
> duplicate entry, such as what you might get if you applied a patch that
> added an entry for a new card, and then applied it again, resolving the
> conflicts in a way that retained the duplicate entries.
I would consider that a bug that should never make it past either 
pre-merge CI or code review.

Also, that is what we have the table verification code for - ensuring 
that bugs don't creep in to the table. So if you have spotted a hole in 
that verification then I do think it needs plugging.

Unfortunately for you, I think that is the best way forward for 
i915/Intel. Enhancing the verification step to ensure that such bugs 
can't happen before it gets to do the search. However, I think there are 
easier ways for you to modify the driver to prevent firmware loading. 
E.g. rather than modifying the table, just force an early exit from the 
loading code itself. And if you really do need to remove the firmware 
files from the compiled binary completely, then replacing them with 
unique names would also work - '/*(DEBLOBBED_1)*/', '/*(DEBLOBBED_2)*/', 
etc.


>
>> 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.
> Agreed.
>
>> 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.
> Table verification might wish to detect and report duplicate filenames
> for the same platform, to catch even alternating duplicates (e.g. "a",
> then "b", then "a" again), but it would be kind if you didn't make that
> a hard error, otherwise we'd have to tweak it to cope with our own
> "/*(DEBLOBBED)*/" duplicates.
>
> Another approach, that would probably be more efficient as the table
> grows, is to store in uc_fw a pointer to or index of the current or next
> entry to be searched, so that the code doesn't have to iterate over the
> table at every try (O(n^2)), and instead takes it from exactly where it
> left off, running overall a single time over the whole table (O(n)), at
> the cost of a pointer or index in uc_fw.  Then, duplicates in the table
> wouldn't matter at all.
>
>> Also, is this string unification thing a part of the current gcc
>> toolchain?
> Yeah, compilers and linkers have been unifying (read-only) string
> literals for a very long time.
That's what I would have assumed. Which is why I was confused that you 
were saying 'if you use a toolchain that does this'. It seemed that you 
were implying that most don't and this was a special situation.

John.

>
> Thanks,
>

Re: [Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[Alexandre Oliva]

On Mar 31, 2023, John Harrison <john.c.harrison@intel.com> wrote:

>>> I'm not sure that is really a valid use case that the driver code
>>> should be expected to support.

>> It's most certainly not.  As I wrote, I'd be happy to keep on carrying
>> the patch that adjusts the code to cope with our changes.  I just
>> thought the same issue could come up by, say, mistakenly applying a
>> patch twice to add support for a new card, a circumstance in which one
>> might not have the card readily available to try it out.

> Not following this argument.

I was talking about downstream backporting, e.g. random users or
small-distro maintainers attempting to backport support for certain
cards without realizing it's already there.

>> Oh, no, the driver loads just fine even without those blobs, and that's
>> much nicer of you than other drivers for hardware that doesn't really
>> require blobs, but that insist on bailing out if the firmware can't be
>> loaded.  i915 hasn't been hostile like that.
> That situation won't last...

:-(

> I would consider that a bug that should never make it past either
> pre-merge CI or code review.

Agreed.

> And if you really do need to remove the firmware files from the
> compiled binary completely, then replacing them with unique names
> would also work - '/*(DEBLOBBED_1)*/', '/*(DEBLOBBED_2)*/', etc.

That is not doable with our current deblob-check implementation,
unfortunately.  There are long-term plans to switch to a different
approach, but we're not there yet.  So I guess we'll have to use custom
code to disable blob loading on i915, while that's still possible, when
the stricter table checking hits.

>>> Also, is this string unification thing a part of the current gcc
>>> toolchain?
>> Yeah, compilers and linkers have been unifying (read-only) string
>> literals for a very long time.
> That's what I would have assumed. Which is why I was confused that you
> were saying 'if you use a toolchain that does this'. It seemed that
> you were implying that most don't and this was a special situation.

Oh, no, sorry, it's just that compilers can't be dependent on for string
literal unification.  It's an optimization, not a language-imposed
requirement.


Thanks for considering the patch, and for the heads up about darker days
coming for users of Intel video cards! :-(


-- 
Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
   Free Software Activist                       GNU Toolchain Engineer
Disinformation flourishes because many people care deeply about injustice
but very few check the facts.  Ask me about <https://stallmansupport.org>