Re: [PATCH v4 4/7] drm/xe/oa: Skip OA HW teardown on a wedged device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/xe/oa: active hrtimer freed in xe_oa_destroy_locked() when wedged
--

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

drm/xe/oa: Skip OA HW teardown on a wedged device

This commit skips OA hardware disable and teardown operations when closing an
OA stream on a wedged device to avoid MMIO waits, batch submissions, and
forcewake hardware accesses on an unresponsive or disabled device.

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index fabbbcf16f4ef..7552ace821caf 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c

[ ... ]

> @@ -1705,7 +1712,8 @@ static void xe_oa_destroy_locked(struct xe_oa_stream *stream)
>  {
>  	xe_pm_runtime_get(gt_to_xe(stream->gt));
>  
> -	if (stream->enabled)
> +	/* A wedged device may be unresponsive, skip HW access */
> +	if (stream->enabled && !xe_wedged(gt_to_xe(stream->gt)))
>  		xe_oa_disable_locked(stream);

[Severity: Critical]
When an enabled sampling OA stream (where stream->sample is true) is closed
after the device becomes wedged, does skipping xe_oa_disable_locked(stream)
here leave stream->poll_check_timer armed while the stream and its OA buffer
are freed?

When a sampling stream is enabled via
xe_oa_enable_locked() -> xe_oa_stream_enable(), it starts
stream->poll_check_timer. The only place that cancels this timer is
xe_oa_stream_disable(), which is called from xe_oa_disable_locked():

drivers/gpu/drm/xe/xe_oa.c:xe_oa_stream_disable() {
	xe_oa_disable(stream);

	if (stream->sample)
		hrtimer_cancel(&stream->poll_check_timer);
	...
}

When xe_wedged() is true, xe_oa_destroy_locked() now skips
xe_oa_disable_locked() and proceeds directly to:

xe_oa_destroy_locked()
  -> xe_oa_stream_destroy()
    -> xe_oa_stream_free_resources()
      -> xe_oa_free_oa_buffer()
  -> kfree(stream)

This frees the active hrtimer and unpins and frees stream->oa_buffer.bo
while stream->poll_check_timer is still queued. When
xe_oa_poll_check_timer_cb() next fires, it will dereference the freed stream
and stream->oa_buffer.bo, read MMIO on the wedged device via
xe_oa_hw_tail_read(), and re-arm itself with HRTIMER_RESTART:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_poll_check_timer_cb() {
	struct xe_oa_stream *stream =
		container_of(hrtimer, typeof(*stream), poll_check_timer);

	if (xe_oa_buffer_check_unlocked(stream))
		wake_up(&stream->poll_wq);

	hrtimer_forward_now(hrtimer, ns_to_ktime(stream->poll_period_ns));

	return HRTIMER_RESTART;
}

Even with the subsequent patch in this series that handles device unplug in
xe_oa_release(), closing an OA stream fd on a wedged device that has not
been unplugged still enters the drm_dev_enter() branch in xe_oa_release()
and calls xe_oa_destroy_locked().

Should stream->poll_check_timer still be cancelled when stream->enabled and
stream->sample are true, even when the hardware disable in xe_oa_disable()
is skipped for a wedged device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105001559.3288915-9-umesh.nerlige.ramappa@intel.com?part=4