<div dir="ltr"><div>Hi Kamil,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Apr 19, 2024 at 5:05 PM Kamil Konieczny <<a href="mailto:kamil.konieczny@linux.intel.com">kamil.konieczny@linux.intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Peter,<br>
On 2024-04-11 at 21:53:34 +0200, Peter Senna Tschudin wrote:<br>
> Currently trying to run `gem_exec_capture --run-subtest<br>
> many-4K-incremental` or `gem_exec_capture --run-subtest many-4K-zero`<br>
> will fail with:<br>
> <br>
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Test<br>
>  assertion failure function gem_engine_properties_configure, file<br>
>  ../lib/i915/gem_engine_topology.c:577:<br>
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Failed assertion: ret == 1<br>
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: Last errno: 9, Bad file descriptor<br>
>  (gem_exec_capture:81999) i915/gem_engine_topology-CRITICAL: error: -1 != 1<br>
> <br>
> This problem happens inside the macro find_first_available_engine()<br>
> when:<br>
>  1. for_each_ctx_engine() allocates an struct intel_engine_data 'ed'<br>
>     inside a for loop. The core of the issue is that ed only exists<br>
>     inside the for loop. As soon as the for loop ends, ed is out of scope<br>
>     and after it's lifetime.<br>
>  2. intel_get_current_engine() sets '*e' to an address of ed. This is ok<br>
>     while inside the for loop, and is undefined behavior after the for<br>
>     loop ends.<br>
>  3. configure_hangs() uses '*e' after the lifetime of 'ed' has ended<br>
>     leading to undefined behavior<br>
>  4. After the call to find_first_available_engine() __captureN() will<br>
>     fail as it expects '*e' to be valid. This is also undefined<br>
>     behavior.<br>
> <br>
> Additionally, this patch sets 'e' to NULL early on at the macro, and at<br>
> the end check if it is still NULL. If so it will skip the test to<br>
> prevent downstream code from crashing.<br>
<br>
Strictly speaking we are not preventing crash but catch a case<br>
when test cannot proceed (as it needs capable engine),<br>
so then we skip it. Btw I plan on re-testing it on Monday and<br>
merge it next week with that small description correction.<br></blockquote><div><br></div><div>I guess that we need to reduce the test size to avoid the CI timeout we see after my fix. I made a patch for it:</div><div><br></div><div><a href="https://patchwork.freedesktop.org/series/131845/">https://patchwork.freedesktop.org/series/131845/</a><br></div><div><br></div><div>Thank you,</div><div><br></div><div>Peter</div><div><br></div><div>[...]</div><div><br></div></div></div>