Good catch - that read is genuinely racy, and it is my patch that
introduced it. seqno_recv is written with WRITE_ONCE() from
xe_tlb_inval_done_handler(), and I deliberately moved the devcoredump
call outside pending_lock (the capture takes sleeping locks), which
left the read unsynchronised.
Fixed locally by sampling it while the lock is still held:
seqno_recv = tlb_inval->seqno_recv;
spin_unlock_irq(&tlb_inval->pending_lock);
...
if (timedout_seqno)
xe_devcoredump_gt(tlb_inval_to_gt(tlb_inval),
"TLB invalidation fence timeout,
seqno=%d recv=%d",
timedout_seqno, seqno_recv);
This will be in v5, together with any other review feedback.
Em qui., 17 de set. de 2026 às 13:50, <[email protected]> escreveu:
>
> 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 <[email protected]>
>
> 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/[email protected]?part=1
--
Com os cumprimentos,
Tales A. Mendonça
talesam.org
communitybig.org
