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
