Re: [PATCH] drm/i915/selftests: read the noop seqno before dropping the request

[sashiko-bot@kernel.org]

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 <vulab@iscas.ac.cn>

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/20261104025344.1527492-1-vulab@iscas.ac.cn?part=1