Re: [PATCH v4 1/3] drm/xe: Capture devcoredump on TLB invalidation timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/xe: data race in xe_tlb_inval_fence_timeout() reading seqno_recv
--

commit ef794f6bbaafa679257b9fec9405245af4942e68
Author: Tales A. Mendonça <talesam@gmail.com>

drm/xe: Capture devcoredump on TLB invalidation timeout

This commit adds xe_devcoredump_gt(), a variant of xe_devcoredump() for hangs
that are not tied to an exec queue or job. It hooks this new capture mechanism
into the TLB invalidation timeout path to capture the GuC log and CT state.

Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8678

> diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c b/drivers/gpu/drm/xe/xe_tlb_inval.c
> index 7c3bf32d9258b..7a0c04fac60aa 100644
> --- a/drivers/gpu/drm/xe/xe_tlb_inval.c
> +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c
[ ... ]
> @@ -97,6 +107,16 @@ static void xe_tlb_inval_fence_timeout(struct work_struct *work)
>  		queue_delayed_work(tlb_inval->timeout_wq, &tlb_inval->fence_tdr,
>  				   timeout_delay);
>  	spin_unlock_irq(&tlb_inval->pending_lock);
> +
> +	/*
> +	 * Capture the GuC log and CT state so the firmware side of the hang
> +	 * can be inspected; there is no queue or job to blame here. Must be
> +	 * outside pending_lock as the capture takes sleeping locks.
> +	 */
> +	if (timedout_seqno)
> +		xe_devcoredump_gt(tlb_inval_to_gt(tlb_inval),
> +				  "TLB invalidation fence timeout, seqno=%d recv=%d",
> +				  timedout_seqno, tlb_inval->seqno_recv);

[Severity: Low]
Does this code introduce a data race when reading tlb_inval->seqno_recv?

In xe_tlb_inval_fence_timeout() in drivers/gpu/drm/xe/xe_tlb_inval.c, the
pending_lock spinlock is dropped just before this block. Because seqno_recv
is read locklessly here, and concurrent updates (such as those in
xe_tlb_inval_done_handler()) use WRITE_ONCE(), reading it directly could
result in a data race.

Could this lead to KCSAN warnings or logging a corrupted value in the
devcoredump reason string? Would it be safer to read this using READ_ONCE()
or cache the value while the lock is held?

>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917163553.1742580-1-talesam@gmail.com?part=1