Re: [PATCH] drm/i915/selftests: read the 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[] read after retire in perf_mi_bb_start()
--

commit 50caf80a061d2ff30673d4eea9e27e211c297b1d
Author: Wentao Liang <vulab@iscas.ac.cn>

drm/i915/selftests: read the seqno before dropping the request

This commit moves the read of rq->hwsp_seqno[] in perf_mi_bb_start()
before i915_request_put(rq) so the request reference is held while
sampling the cycle counts.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> index 5ffa5e30f4192..1846f1c7e77ee 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c
> @@ -196,11 +196,17 @@ static int perf_mi_bb_start(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[3] - rq->hwsp_seqno[2];

[Severity: Medium]
Does holding a reference on rq prevent rq->hwsp_seqno from being
redirected away from the timeline HWSP when i915_request_retire() runs?

In the scenario described by the commit message, where
i915_request_retire() runs before or concurrently with
i915_request_wait() returning, i915_request_retire() calls
i915_request_mark_complete() before signaling rq->fence and dropping the
initial request reference:

drivers/gpu/drm/i915/i915_request.c:i915_request_retire() {
	...
	trace_i915_request_retire(rq);
	i915_request_mark_complete(rq);
	...
	if (!i915_request_signaled(rq)) {
		spin_lock_irq(&rq->lock);
		dma_fence_signal_locked(&rq->fence);
		spin_unlock_irq(&rq->lock);
	}
	...
	i915_request_put(rq);
}

And i915_request_mark_complete() rewrites rq->hwsp_seqno to point at the
64-bit rq->fence.seqno field inside struct dma_fence:

drivers/gpu/drm/i915/i915_request.h:
static inline void i915_request_mark_complete(struct i915_request *rq)
{
	WRITE_ONCE(rq->hwsp_seqno, /* decouple from HWSP */
		   (u32 *)&rq->fence.seqno);
}

Once i915_request_mark_complete() has executed, indexing
rq->hwsp_seqno[2] and rq->hwsp_seqno[3] in perf_mi_bb_start() reads out
of bounds past rq->fence.seqno into adjacent fields of struct dma_fence
(rq->fence.flags and rq->fence.refcount) rather than reading the
timestamps written by write_timestamp(rq, 2) and write_timestamp(rq, 3)
in the timeline HWSP.

Should perf_mi_bb_start() save const u32 *hwsp = rq->hwsp_seqno before
calling i915_request_add(rq) (similar to __live_rc6_ctx() in
drivers/gpu/drm/i915/gt/selftest_rc6.c), or read from
ce->timeline->hwsp_seqno directly?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104025345.1527509-1-vulab@iscas.ac.cn?part=1