Re: [PATCH v3 5/5] drm/xe/oa: Quiesce OA streams on driver unbind and PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe/oa: incomplete teardown when xe_oa_release() precedes xe_oa_quiesce()
- [High] drm/xe/oa: missing wait on stream->last_fence in xe_oa_quiesce_oa_unit()
- [Medium] drm/xe/oa: incomplete LRI batch drain on user exec_q in xe_oa_load_with_lri()
--

commit 3662190fcd57d2e4b3bb172dd837ebb9aa4e7e39
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/oa: Quiesce OA streams on driver unbind and PCI reset

This commit introduces xe_oa_quiesce() called from xe_oa_fini() to clean up
active OA streams during driver unbind or PCI reset while keeping the
stream structure alive until the file descriptor is closed. It also wraps
OA mmap and observation ioctls in drm_dev_enter() sections.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 7a151f338ae25..b4ba8874f17eb 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c

[ ... ]

> @@ -752,6 +775,20 @@ static int xe_oa_load_with_lri(struct xe_oa_stream *stream, struct xe_oa_reg *re
>  		goto free_bb;
>  	}
>  	xe_bb_free(bb, fence);
> +
> +	/*
> +	 * When quiescing the stream, GuC submission and the fence irqs are torn down right
> +	 * after, so wait for the batch. It may be queued behind other jobs of the exec queue,
> +	 * which complete or get reset within the job timeout.
> +	 */
> +	if (READ_ONCE(stream->dead)) {
> +		struct xe_exec_queue *q = stream->exec_q ?: stream->k_exec_q;
> +		long timeout = msecs_to_jiffies(q->sched_props.job_timeout_ms) + HZ;
> +
> +		if (!dma_fence_wait_timeout(fence, false, timeout))
> +			drm_err(&stream->oa->xe->drm,
> +				"Timed out waiting for OA LRI batch on quiesce\n");
> +	}
>  	dma_fence_put(fence);
>  
>  	return 0;

[Severity: Medium]
As reported by Sashiko on v3 [1], when a stream is attached to a user
stream->exec_q in drivers/gpu/drm/xe/xe_oa.c:xe_oa_load_with_lri(), is
waiting for q->sched_props.job_timeout_ms + HZ sufficient to guarantee the
LRI teardown batch completes before GuC submission and fence IRQs are torn
down?

When xe_device_remove() -> xe_oa_fini() -> xe_oa_quiesce() ->
xe_oa_quiesce_oa_unit() -> xe_oa_stream_hw_teardown() ->
xe_oa_disable_metric_set() -> xe_oa_configure_oa_context() ->
xe_oa_load_with_lri() submits to a user stream->exec_q, it looks like
dma_fence_wait_timeout() can time out in three situations:

1. If stream->exec_q belongs to a preempt-fence (LR mode) VM,
xe_bo_pci_dev_remove_all() evicts the VM's BOs after drm_dev_unplug() and
suspends stream->exec_q via __guc_exec_queue_process_msg_suspend(). If the
queue was idle when preempt_rebind_work_func() ran,
vm->preempt.rebind_deactivated is set and xe_oa_submit_bb() does not call
xe_vm_reactivate_rebind(), so submit_exec_queue() returns early on
exec_queue_suspended(q) without submitting the LRI batch to GuC while the
scheduler timeout is MAX_SCHEDULE_TIMEOUT.

2. If an earlier user job on stream->exec_q->entity is blocked on an
unsignaled input dependency in drm_sched_entity_pop_job(), the LRI job
queued behind it never enters sched->pending_list or starts its timeout
timer.

3. If a prior user job on stream->exec_q times out after drm_dev_unplug(),
guc_exec_queue_timedout_job() takes the !drm_dev_enter() path:

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_timedout_job() {
	if (!drm_dev_enter(&xe->drm, &idx)) {
		xe_sched_job_set_error(job, -ENODEV);
		return DRM_GPU_SCHED_STAT_NO_HANG;
	}
}

Because that only errors the single head job without disabling scheduling or
canceling subsequent jobs, the OA LRI job behind it requires another full
job_timeout_ms cycle.

When dma_fence_wait_timeout() times out here, won't the LRI batch's
xe_hw_fence remain on gt->fence_irq[i].pending and trigger
XE_WARN_ON(!list_empty(&irq->pending)) in xe_hw_fence_irq_finish() during
xe_gt_fini()?

[ ... ]

> @@ -1691,6 +1731,12 @@ static long xe_oa_ioctl(struct file *file,
>  
>  static void xe_oa_destroy_locked(struct xe_oa_stream *stream)
>  {
> +	if (stream->dead) {
> +		mutex_destroy(&stream->stream_lock);
> +		kfree(stream);
> +		return;
> +	}
> +
>  	xe_pm_runtime_get(gt_to_xe(stream->gt));
>  
>  	if (stream->enabled)

[Severity: High]
What happens if userspace closes the OA stream file descriptor (or the
process exits) after xe_device_remove() or xe_wedge_declare() has started,
but before the devm callback xe_oa_fini() -> xe_oa_quiesce_gt() acquires
gt->oa.gt_lock?

In that window, xe_oa_release() -> xe_oa_destroy_locked() in
drivers/gpu/drm/xe/xe_oa.c sees stream->dead == false and runs the normal
teardown path:

1. In xe_oa_destroy_locked() -> xe_oa_stream_destroy() ->
xe_oa_stream_hw_teardown() -> xe_oa_disable_metric_set() ->
xe_oa_configure_oa_context() -> xe_oa_load_with_lri(), READ_ONCE(stream->dead)
is false, so xe_oa_load_with_lri() does not wait for the submitted LRI
batch fence. Because xe_oa_stream_destroy() also clears u->exclusive_stream
to NULL, xe_oa_quiesce_oa_unit() sees !stream and returns immediately,
allowing guc_submit_fini() and xe_gt_fini() -> xe_hw_fence_irq_finish() to
run while the LRI batch fence is still pending on gt->fence_irq[i].pending.

2. If the device is wedged (including after prepare_device_for_reset() has
called pci_disable_device()), xe_oa_destroy_locked() does not check
xe_wedged(xe) and still calls xe_oa_disable_locked() and
xe_oa_stream_hw_teardown(), performing MMIO accesses and LRI batch
submissions against a wedged or disabled device.

3. If xe_oa_destroy_locked() runs after xe_bo_pci_dev_remove_all(),
xe_oa_free_oa_buffer() calls xe_bo_unpin_map_no_vm(stream->oa_buffer.bo)
without calling xe_bo_dma_unmap_pinned(). Since xe_bo_unpin() removes the
BO from xe->pinned.early.kernel_bo_present, if an mmap VMA still holds a BO
reference or ttm_bo_release() defers cleanup to bdev->wq,
xe_bo_pinned_fini() will not unmap its DMA mapping before devres release
completes.

Should xe_oa_destroy_locked() also handle the unplugged and wedged cases
(unmapping the pinned OA BO DMA, skipping HW access when wedged, and
waiting for any in-flight LRI fences when unplugged)?

[ ... ]

> @@ -2575,6 +2643,92 @@ static void xe_oa_unregister(void *arg)
>  	oa->metrics_kobj = NULL;
>  }
>  
> +static void xe_oa_quiesce_oa_unit(struct xe_oa_unit *u, bool hw_accessible)
> +{
> +	struct xe_oa_stream *stream = u->exclusive_stream;
> +	unsigned long flags;
> +
> +	if (!stream)
> +		return;
> +
> +	mutex_lock(&stream->stream_lock);
> +	if (stream->enabled && hw_accessible)
> +		xe_oa_disable_locked(stream);
> +
> +	stream->enabled = false;
> +
> +	spin_lock_irqsave(&stream->oa_buffer.ptr_lock, flags);
> +	WRITE_ONCE(stream->dead, true);
> +	spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> +
> +	if (stream->sample)
> +		hrtimer_cancel(&stream->poll_check_timer);
> +
> +	wake_up_all(&stream->poll_wq);
> +
> +	/*
> +	 * Restore the HW state while the stream still holds its runtime PM ref and exec_q.
> +	 * The device is not reset on unbind, so this can't be left to a later reset.
> +	 */
> +	if (hw_accessible)
> +		xe_oa_stream_hw_teardown(stream);
> +
> +	/*
> +	 * drm_dev_unplug() has already zapped the userspace mappings of the OA buffer and
> +	 * refaults get a dummy page, so the OA buffer can go. Each vma holds a reference on
> +	 * the BO, keeping the BO object itself alive until munmap. The BO is then no longer
> +	 * pinned when xe_bo_pinned_fini() DMA-unmaps the other kernel BOs, so drop its DMA
> +	 * mapping here, while it is still pinned.
> +	 */
> +	xe_bo_dma_unmap_pinned(stream->oa_buffer.bo);
> +	xe_oa_stream_free_resources(stream);
> +	if (stream->exec_q) {
> +		xe_exec_queue_put(stream->exec_q);
> +		stream->exec_q = NULL;
> +	}
> +	drm_syncobj_put(stream->ufence_syncobj);
> +
> +	WRITE_ONCE(u->exclusive_stream, NULL);
> +	mutex_unlock(&stream->stream_lock);
> +}

[Severity: High]
Does xe_oa_quiesce_oa_unit() in drivers/gpu/drm/xe/xe_oa.c also need to
wait for or cancel stream->last_fence before tearing down hardware state
and freeing stream resources?

When userspace opens an OA stream (DRM_XE_OBSERVATION_OP_STREAM_OPEN) or
reconfigures it (DRM_XE_OBSERVATION_IOCTL_CONFIG) with output syncs
(num_signal > 0), xe_oa_emit_oa_config() submits oa_bo->bb via
xe_oa_submit_bb(stream, XE_OA_SUBMIT_ADD_DEPS, oa_bo->bb), records the job
fence in stream->last_fence via xe_oa_update_last_fence(), and returns
without waiting:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_emit_oa_config() {
	/* Update last fence too before adding callback */
	xe_oa_update_last_fence(stream, fence);
	...
	/* If nothing needs to be signaled we wait synchronously */
	if (!num_signal) {
		dma_fence_wait(&ofence->base, false);
		dma_fence_put(&ofence->base);
	}
}

If xe_oa_fini() -> xe_oa_quiesce() -> xe_oa_quiesce_oa_unit() runs while
stream->last_fence is still unsignaled:

1. For streams without stream->exec_q (where batches run on
stream->k_exec_q), streams on video engines where
xe_oa_configure_oa_context() is a no-op, or when hw_accessible is false,
xe_oa_load_with_lri() is never called. Neither xe_oa_quiesce_oa_unit() nor
xe_oa_free_configs() waits for stream->last_fence.

2. The in-flight oa_bo->bb LRI batch on stream->k_exec_q can execute on the
GPU during or after xe_oa_stream_hw_teardown(), overwriting OA/NOA
registers after xe_oa_disable_metric_set() has cleared them and after
xe_force_wake_put() has dropped stream->fw_ref.

3. When devres subsequently runs guc_submit_fini() and xe_gt_fini() ->
xe_hw_fence_irq_finish(), an in-flight config batch job on
sched->pending_list still has its xe_hw_fence on gt->fence_irq[i].pending,
triggering XE_WARN_ON(!list_empty(&irq->pending)). Moreover, if that config
batch job is still queued in q->entity waiting on an unsignaled input sync
dependency from stream->syncs, the job holds a reference on the queue (so
xe_exec_queue_put() in xe_oa_stream_free_resources() does not run
xe_exec_queue_destroy() / drm_sched_entity_fini()), while
sched->pending_list is empty so xe_guc_submit_pause_abort() does not run
TDR, leaking the scheduler job, suballocation, xe_exec_queue, and
drm_device reference.

[1] https://lore.kernel.org/r/20261030233839.2284694-8-umesh.nerlige.ramappa@intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104005221.3230003-7-umesh.nerlige.ramappa@intel.com?part=5