Re: [RFC v2 01/21] drm/xe: Add xe_usm_queue generic USM circular buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] CIRC_SPACE macro misuse and missing power-of-two enforcement for queue size.
- [Low] Expensive integer division used in IRQ hot path instead of bitwise AND.
- [Low] Unused header inclusion for <linux/workqueue.h>.
--

commit 9fcff35c519c0e3d9b6f11658ca9f7a63e42fc15
Author: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

drm/xe: Add xe_usm_queue generic USM circular buffer

This commit introduces struct xe_usm_queue, a generic lock-protected
circular FIFO used by USM consumers to receive fixed-size entries from IRQ
context. It provides inline helpers to push and pop entries, and to check if
the queue is full.

> diff --git a/drivers/gpu/drm/xe/xe_usm_queue.h b/drivers/gpu/drm/xe/xe_usm_queue.h
> new file mode 100644
> index 0000000000000..64f708a5f835a
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_usm_queue.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2026 Intel Corporation
> + */
> +
> +#ifndef _XE_USM_QUEUE_H_
> +#define _XE_USM_QUEUE_H_
> +
> +#include <linux/circ_buf.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>

[Severity: Low]
Since the embedded work_struct worker was removed from the queue in v2 of
this patch, is this header inclusion now unused?

> +
> +/**
> + * struct xe_usm_queue - Generic USM circular FIFO queue
> + *
> + * A lock-protected circular buffer used by the access counter
> + * consumer. Producers push fixed-size entries from IRQ context;
> + * workers process them asynchronously.
> + */
> +struct xe_usm_queue {
> +	/**
> +	 * @data: Raw byte buffer backing the queue, protected by @lock
> +	 */
> +	void *data;
> +	/** @size: Total size of @data in bytes */
> +	u32 size;

[Severity: Medium]
Does the size field need to enforce or document that it must be a power of
two?

If the queue is initialized with a non-power-of-two size, the CIRC_SPACE
macro used in xe_usm_queue_full() will compute incorrect free space values,
potentially leading to buffer overflows or premature 'queue full' rejections.

> +	/** @head: Write cursor in bytes, moved by producer, protected by @lock */
> +	u32 head;
> +	/** @tail: Read cursor in bytes, moved by consumer, protected by @lock */
> +	u32 tail;

[ ... ]

> +static inline bool xe_usm_queue_pop(struct xe_usm_queue *q, void *out)
> +{
> +	bool found = false;
> +
> +	spin_lock_irq(&q->lock);
> +	if (q->tail != q->head) {
> +		memcpy(out, q->data + q->tail, q->data_size);
> +		q->tail = (q->tail + q->entry_size) % q->size;

[Severity: Low]
Could this expensive integer division be replaced with a bitwise AND?

Since q->size must be a power of two for CIRC_SPACE() to function correctly,
optimizing this modulo operation to & (q->size - 1) would remove an integer
division operation.

> +		found = true;
> +	}
> +	spin_unlock_irq(&q->lock);
> +
> +	return found;
> +}
> +
> +/**
> + * xe_usm_queue_peek - Check whether the queue has at least one entry

[ ... ]

> +static inline void xe_usm_queue_push(struct xe_usm_queue *q, const void *in)
> +{
> +	lockdep_assert_held(&q->lock);
> +
> +	memcpy(q->data + q->head, in, q->data_size);
> +	q->head = (q->head + q->entry_size) % q->size;

[Severity: Low]
Similarly, could this integer division be optimized to a bitwise AND to avoid
a performance penalty in the hardware interrupt hot path?

> +}
> +#endif

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