Re: [PATCH v5 1/3] drm/xe: Capture devcoredump on TLB invalidation timeout

[sashiko-bot@kernel.org]

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 <talesam@gmail.com>

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/20260921182121.308217-1-talesam@gmail.com?part=1