Re: [PATCH v10 21/27] drm/xe/eudebug: Add read/count/compare helper for eu attention

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unaligned memory access and endianness bugs due to incorrect type punning in `xe_eu_attentions_count`.
- [High] Unbounded CPU stall / soft lockup due to 1.5-second `udelay()` busy-waiting in `xe_gt_eu_attentions_read`.
- [Medium] Silent failure when reading hardware attentions causes the debugger to process zeroed memory as valid EU threads.
- [Medium] Meaningful code and commit message mismatch: The patch claims to add an EU thread compare function, but the implementation is completely missing.
--

commit a0b2cd07131e7090249ddf2ff617e3c4283b5549
Author: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

drm/xe/eudebug: Add read/count/compare helper for eu attention

This commit adds the xe_eu_attentions structure to capture and store EU
attention bits. It also introduces functions to read and count the number of
EU threads that have triggered attentions.

>     drm/xe/eudebug: Add read/count/compare helper for eu attention
>     
>     Add xe_eu_attentions structure to capture and store eu attention bits.
>     Add a function to count the number of eu threads that have turned on from
>     eu attentions, and add a function to count the number of eu threads that
>     have changed on a state between eu attentions.

[Severity: Medium]
This isn't a bug, but the commit message states it adds a compare helper "to
count the number of eu threads that have changed on a state between eu
attentions." Does the patch actually include this function? It appears to be
missing from the implementation.

> diff --git a/drivers/gpu/drm/xe/xe_gt_debug.c b/drivers/gpu/drm/xe/xe_gt_debug.c
> index b20e42e66c5bc..bff78c0b232a8 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debug.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debug.c
[ ... ]
> @@ -199,3 +202,50 @@ int xe_gt_eu_threads_needing_attention(struct xe_gt *gt)
>  
>  	return err < 0 ? 0 : err;
>  }
> +
> +static inline unsigned int
> +xe_eu_attentions_count(const struct xe_eu_attentions *a)
> +{
> +	return bitmap_weight((void *)a->att, a->size * BITS_PER_BYTE);
> +}

[Severity: High]
Does casting this u8 array to an unsigned long pointer cause unaligned memory
accesses?

In drivers/gpu/drm/xe/xe_gt_debug_types.h, the xe_eu_attentions struct
declares att as a u8 array, meaning the struct has at most a 4-byte
alignment (dictated by unsigned int size). 

When xe_eu_attentions_count() in drivers/gpu/drm/xe/xe_gt_debug.c casts this
array to void * and passes it to bitmap_weight(), which expects an unsigned
long *, it can cause undefined behavior. Dereferencing a 4-byte aligned
address as an 8-byte unsigned long can cause faults on architectures with
strict alignment requirements, or incorrect bit counting on big-endian systems.

> +
> +void xe_gt_eu_attentions_read(struct xe_gt *gt,
> +			      struct xe_eu_attentions *a,
> +			      const unsigned int settle_time_ms)
> +{
> +	unsigned int attn, prev = 0;
> +	ktime_t end, now;
> +
> +	now = ktime_get_raw();
> +	/* Initially wait longer for attentions to appear */
> +	end = ktime_add_ms(now, settle_time_ms * 10);
> +
> +	a->size = min_t(int,
> +			xe_gt_eu_attention_bitmap_size(gt),
> +			sizeof(a->att));
> +
> +	do {
> +		if (xe_gt_eu_attention_bitmap(gt, a->att, a->size) < 0)
> +			return;

[Severity: Medium]
Will this early return swallow the error code and corrupt the debugger's
data flow?

If xe_gt_eu_attention_bitmap() fails in xe_gt_eu_attentions_read(), it
returns early but leaves a->size set to a positive value. Since the function
signature is void, the caller has no way of knowing a failure occurred.

Could the caller then process the structure's att array (which contains
zeroes) and falsely report that no threads triggered an attention?

> +
> +		if (!settle_time_ms)
> +			break;
> +
> +		attn = xe_eu_attentions_count(a);
> +
> +		now = ktime_get_raw();
> +
> +		if (attn > prev) {
> +			prev = attn;
> +			end = ktime_add_ms(now, settle_time_ms);
> +		}
> +
> +		udelay(15);
> +
> +		/*
> +		 * XXX We are gathering data for production SIP to find
> +		 * the upper limit of settle time. For now, we wait full
> +		 * timeout value regardless.
> +		 */
> +	} while ((attn < a->size * 8) && ktime_before(now, end));
> +}

[Severity: High]
Can this loop cause an unbounded CPU stall or soft lockup?

The loop in xe_gt_eu_attentions_read() waits for a timeout of
settle_time_ms * 10. When called from a pagefault handler with
XE_GT_ATTENTION_TIMEOUT_MS (150ms), the timeout is 1.5 seconds. Inside the
loop, it busy-waits using udelay(15) instead of a sleeping delay.

Additionally, if any DSS in the middle of the topology is fused off, attn
will never reach a->size * 8 because a->size overestimates the valid bits.
Does this guarantee the loop will always busy-wait for the full 1.5 seconds
per call?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903145952.848051-1-mika.kuoppala@linux.intel.com?part=21