On Mon, Sep 21, 2026 at 03:33:00PM -0300, [email protected] wrote:
> [High] drm/xe: MMIO access without PM reference in devcoredump on TLB timeout
Correct on both counts, and it is my patch that introduces it.
Tracing it: every pending invalidation fence holds a runtime PM
reference, taken by xe_pm_runtime_get_noresume() in
xe_tlb_inval_fence_init(). xe_tlb_inval_fence_signal() calls
xe_tlb_inval_fence_fini(), which drops it. The timeout loop signals all
the expired fences and only then, after dropping pending_lock, calls
xe_devcoredump_gt(). devcoredump_snapshot() acquires forcewake without
taking a PM reference of its own - it has always relied on the caller
holding one, which the existing job-timeout caller does. The new
GT-level caller may have just released the last one, so the device can
begin autosuspending before the snapshot touches the hardware.
Fixed for v6 by holding a reference across the capture, taken while the
pending fences still guarantee the device is awake:
if (!timedout_seqno) {
/*
* Hold a PM reference across the capture below. Every
* pending fence holds one, so the device is awake
* here, but signalling them may drop the last
* reference and let it autosuspend before the
* snapshot touches the hardware.
*/
xe_pm_runtime_get_noresume(xe);
}
and released after xe_devcoredump_gt() returns.
Taking it inside pending_lock is safe: xe_pm_runtime_get_noresume() goes
through pm_runtime_get_if_in_use(), and its "Missing outer runtime PM
protection" warning cannot fire here, because the fence reference is
still held at that point.
Tales
Em seg., 21 de set. de 2026 às 15:33, <[email protected]> escreveu:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] drm/xe: MMIO access without PM reference in devcoredump on TLB
> timeout
> --
>
> commit a463de937c804fe9be7ed86bfbfa297e4f35c466
> Author: Tales A. Mendonça <[email protected]>
>
> drm/xe: Capture devcoredump on TLB invalidation timeout
>
> This commit adds xe_devcoredump_gt() to capture the GuC log and CT state
> for GT-level hangs that are not tied to a specific queue or job. It then
> hooks this capture mechanism into the TLB invalidation timeout path. This
> helps in diagnosing firmware stalls where the invalidation request was
> consumed but the ack was delayed.
>
> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/work_items/8678
>
> > diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c
> > b/drivers/gpu/drm/xe/xe_devcoredump.c
> > --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> > +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> [ ... ]
> > @@ -323,40 +318,44 @@ static void devcoredump_snapshot(struct
> > xe_devcoredump *coredump,
> > struct xe_gt *gt,
> > struct xe_exec_queue *q,
> > struct xe_sched_job *job)
> > {
> [ ... ]
> > /* keep going if fw fails as we still want to save the memory and SW
> > data */
> > - CLASS(xe_force_wake, fw_ref)(gt_to_fw(q->gt), XE_FORCEWAKE_ALL);
> > + CLASS(xe_force_wake, fw_ref)(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>
> [Severity: High]
> If this capture path is reached without holding a PM runtime reference, does
> acquiring the forcewake here attempt MMIO accesses while the device is in a
> low-power state? Accessing MMIO registers when the device has entered
> autosuspend (D3cold) typically causes unhandled PCIe errors or system
> crashes.
>
> [ ... ]
> > diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c
> > b/drivers/gpu/drm/xe/xe_tlb_inval.c
> > --- a/drivers/gpu/drm/xe/xe_tlb_inval.c
> > +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c
> [ ... ]
> > @@ -90,13 +98,27 @@ static void xe_tlb_inval_fence_timeout(struct
> > work_struct *work)
> > "TLB invalidation fence timeout, seqno=%d recv=%d",
> > fence->seqno, tlb_inval->seqno_recv);
> >
> > + timedout_seqno = fence->seqno;
> > +
> > fence->base.error = -ETIME;
> > xe_tlb_inval_fence_signal(fence);
>
> [Severity: High]
> Can this sequence drop the last PM runtime reference before the devcoredump
> is captured?
>
> Looking at xe_tlb_inval_fence_signal(), it calls xe_tlb_inval_fence_fini()
> which drops the PM runtime reference (xe_pm_runtime_put). If this was the
> last active reference, the device could begin autosuspending.
>
> > }
> > if (!list_empty(&tlb_inval->pending_fences))
> > queue_delayed_work(tlb_inval->timeout_wq,
> > &tlb_inval->fence_tdr,
> > timeout_delay);
> > + seqno_recv = tlb_inval->seqno_recv;
> > 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, hence
> > + * @seqno_recv is sampled above while the lock is still held.
> > + */
> > + if (timedout_seqno)
> > + xe_devcoredump_gt(tlb_inval_to_gt(tlb_inval),
> > + "TLB invalidation fence timeout, seqno=%d
> > recv=%d",
> > + timedout_seqno, seqno_recv);
>
> Since the PM reference was dropped earlier in the loop, would this call to
> xe_devcoredump_gt() proceed without holding one? If the thread gets
> preempted long enough for the autosuspend delay to expire, the forcewake
> operation inside devcoredump_snapshot() might hit the hardware while it is
> unpowered.
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=1
--
Com os cumprimentos,
Tales A. Mendonça
talesam.org
communitybig.org
