Re: [RFC v2 04/21] drm/xe: Implement xe_access_counter_handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing guard for USM disabled state or uninitialized queue in `xe_access_counter_handler` causes division by zero, uninitialized spinlock usage, and NULL pointer dereference.
- [High] Use-after-free of `pagefault_wq` when queuing access counter work during device teardown or unbind.
- [Medium] Unconditional `drm_warn` when the access counter queue is full causes kernel log flooding.
--

commit 4068a8bc69a082a892fb80bc8c9a526e108c2eb2
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

drm/xe: Implement xe_access_counter_handler

This commit implements the access counter handler to process G2H
notifications from the GuC. It pushes access counter events into a queue
and schedules a worker on the pagefault workqueue to process them.

> diff --git a/drivers/gpu/drm/xe/xe_access_counter.c b/drivers/gpu/drm/xe/xe_access_counter.c
> index ae939576b72da..6920e52cf1543 100644
> --- a/drivers/gpu/drm/xe/xe_access_counter.c
> +++ b/drivers/gpu/drm/xe/xe_access_counter.c

[ ... ]

> @@ -90,6 +96,22 @@ int xe_access_counter_init(struct xe_device *xe)
>   */
>  int xe_access_counter_handler(struct xe_device *xe, struct xe_access_counter *ac)
>  {
> -	/* Stub implementation - to be filled in */
> -	return 0;
> +	struct xe_usm_queue *ac_queue = &xe->usm.ac_queue;
> +	int idx;
> +	unsigned long flags;
> +	bool full;
> +
> +	spin_lock_irqsave(&ac_queue->lock, flags);

[Severity: High]
What happens in xe_access_counter_handler() if the USM subsystem is disabled?

If xe->info.has_usm is false, xe_access_counter_init() will exit early without
allocating ac_queue->data, setting ac_queue->size, or initializing the
spinlock.

If the hardware or GuC firmware sends a G2H notification anyway (due to an
unexpected state or buggy firmware), will this unconditionally acquire an
uninitialized lock?

Furthermore, xe_usm_queue_full() will evaluate to false due to underflow,
causing xe_usm_queue_push() to perform a modulo by zero on q->size and a
memcpy() to a NULL ac_queue->data pointer. Is an early return guard needed
here to verify USM is fully initialized?

> +	idx = xe_access_counter_work_index(xe);
> +	full = xe_usm_queue_full(ac_queue);
> +	if (!full) {
> +		xe_usm_queue_push(ac_queue, ac);
> +		queue_work(xe->usm.pagefault_wq, &xe->usm.ac_workers[idx].work);

[Severity: High]
During device unbind or driver removal, can xe->usm.pagefault_wq be used
after it is freed?

Because devres executes cleanup actions in reverse order of registration,
xe_pagefault_fini() destroys pagefault_wq before the GuC is stopped and
IRQs are uninstalled (which were registered earlier in probe).

If an access counter notification arrives during this teardown window,
could we queue work to a destroyed workqueue?

> +	} else {
> +		drm_warn(&xe->drm,
> +			 "AccessCounter Queue full, shouldn't be possible\n");

[Severity: Medium]
Could this unconditional warning flood the kernel log?

If the consumer worker is delayed, or if the hardware generates access
counter notifications at a high rate, the queue will fill up.

Once full, this handler will unconditionally print the warning for every
subsequent event, which bypasses rate-limiting. Would drm_warn_ratelimited()
be safer to use here?

> +	}
> +	spin_unlock_irqrestore(&ac_queue->lock, flags);
> +
> +	return full ? -ENOSPC : 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909124457.1610890-23-himal.prasad.ghimiray@intel.com?part=4