Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unprotected MMIO access in `xe_tlb_inval_kick` can cause a crash if the device suspends concurrently. --
--- Patch [3]: [RFC PATCH 3/3] drm/xe: Kick GuC while TLB invalidation acks are overdue --- commit c7a516f0e42fcf29db699b7dce1432a3b42378cd Author: Tales A. Mendonça <[email protected]> drm/xe: Kick GuC while TLB invalidation acks are overdue This commit adds a delayed work to periodically poke the GuC when a TLB invalidation ack is delayed. This provides a workaround for a firmware stall issue by re-reading the GuC status, flushing the CT fast-path, and ringing the GuC doorbell until the ack arrives. 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 9dd04d5bc4cd2..16c32d5669f36 100644 > --- a/drivers/gpu/drm/xe/xe_tlb_inval.c > +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c [ ... ] > @@ -268,6 +285,55 @@ static bool xe_tlb_inval_seqno_past(struct xe_tlb_inval > *tlb_inval, int seqno) > return seqno_recv >= seqno; > } > > +static void xe_tlb_inval_kick(struct work_struct *work) > +{ > + struct xe_tlb_inval *tlb_inval = container_of(work, struct xe_tlb_inval, > + kick_work.work); > + struct xe_gt *gt = tlb_inval_to_gt(tlb_inval); > + struct xe_tlb_inval_fence *fence; > + ktime_t inval_time = 0; > + int seqno = 0; > + > + spin_lock_irq(&tlb_inval->pending_lock); > + fence = list_first_entry_or_null(&tlb_inval->pending_fences, > + struct xe_tlb_inval_fence, link); > + if (fence) { > + seqno = fence->seqno; > + inval_time = fence->inval_time; > + } > + spin_unlock_irq(&tlb_inval->pending_lock); > + > + if (!seqno) > + return; > + > + /* > + * Poke the GuC: read its status register, flush the CT fast-path and > + * ring the doorbell. On ARL with GuC 70.53.0 the ack for a pending > + * invalidation sometimes only arrives seconds after the request even > + * though the H2G was consumed immediately; a doorbell ring while the > + * ack is overdue usually unsticks it within ~250ms (see Link in the > + * commit message). Keep kicking every interval until the ack shows > + * up; the TDR bounds how long this can go on. > + */ > + if (gt->gtidle.idle_residency) > + xe_guc_pc_c_status(>->uc.guc.pc); > + tlb_inval->ops->flush(tlb_inval); > + xe_guc_notify(>->uc.guc); [Severity: High] Could this lead to an unprotected MMIO access if the device suspends concurrently? When pending_lock is dropped above, the device is kept awake by the runtime PM reference tied to the pending fence. If the GuC ack arrives concurrently on another CPU, xe_tlb_inval_done_handler() will acquire the lock, signal the fence, and call xe_pm_runtime_put() via xe_tlb_inval_fence_fini(). Since cancel_delayed_work() does not block if the worker is already running, is it possible for the last PM reference to be dropped before we reach these MMIO accesses in xe_guc_pc_c_status() and xe_guc_notify()? If the device suspends, accessing these registers could trigger a PCIe timeout or master abort and crash the system. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
