Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/i915/selftests: rq->hwsp_seqno overwrite on retire in perf_mi_noop() --
commit 00425305585429060b5834586fddd6fa9a9e06c4 Author: Wentao Liang <[email protected]> drm/i915/selftests: read the noop seqno before dropping the request This commit moves the read of rq->hwsp_seqno[] in perf_mi_noop() before i915_request_put(rq) so that the request is not freed before the seqno slots are sampled. > diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > index 5ffa5e30f4192..cfe73b51557a1 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c > @@ -351,13 +351,19 @@ static int perf_mi_noop(void *arg) > > if (i915_request_wait(rq, 0, HZ / 5) < 0) > err = -EIO; > + > + /* > + * The request can be freed as soon as its last > + * reference is dropped, so read the seqno first. > + */ > + if (!err) > + cycles[i] = > + (rq->hwsp_seqno[4] - rq->hwsp_seqno[3]) > - > + (rq->hwsp_seqno[3] - rq->hwsp_seqno[2]); [Severity: Medium] If i915_request_retire(rq) has already run (or runs concurrently), will reading rq->hwsp_seqno[2..4] here still read out of bounds of rq->fence.seqno instead of reading the timeline HWSP? At the start of i915_request_retire(), before signaling rq->fence or dropping the request reference, it calls i915_request_mark_complete(): drivers/gpu/drm/i915/i915_request.c:i915_request_retire() { ... trace_i915_request_retire(rq); i915_request_mark_complete(rq); ... } drivers/gpu/drm/i915/i915_request.h:i915_request_mark_complete() { WRITE_ONCE(rq->hwsp_seqno, /* decouple from HWSP */ (u32 *)&rq->fence.seqno); } Holding a reference via i915_request_get(rq) keeps the request object alive, but once i915_request_mark_complete(rq) executes, rq->hwsp_seqno points to rq->fence.seqno inside struct dma_fence rather than the timeline HWSP where write_timestamp() wrote slots 2, 3, and 4. Indexing rq->hwsp_seqno[2..4] then reads past the 64-bit rq->fence.seqno field into adjacent fields of struct dma_fence. Should perf_mi_noop() save the timeline HWSP pointer (for example from rq->hwsp_seqno or ce->timeline->hwsp_seqno) before calling i915_request_add(rq), similar to __live_rc6_ctx() in drivers/gpu/drm/i915/gt/selftest_rc6.c? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
