On Mon, Sep 21, 2026 at 05:28:22PM -0300, Tales A. Mendonça wrote: > 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. >
This is correct. Matt > 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
