Re: [PATCH] drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect maximum delay in usleep_range() introduces massive timer slack, defeating exponential backoff and causing extreme latency and spurious timeouts.
- [Medium] The commit message claims to simplify a `sleep_min` assignment, but no such change exists in the code.
- [Low] 64-bit division on 32-bit architectures causes link errors.
--

commit 76cc600b7d97fbc1edcd9a133df8168f9ad41977
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums

This patch adds overflow checks for delays in __xe_mmio_wait32() and
introduces caps to the maximum in-loop wait time. It also aims to pick
the correct sleep or delay helper based on the requested wait duration.

> drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
> 
> Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
> and pick the correct helper accordingly. Fix sign-overflow and avoid
> growing delays becoming intollerably large by capping the in-loop
> wait time. Simplify sleep_min assignment by removing redundant cast.

[Severity: Medium]
The commit message claims to simplify the sleep_min assignment by removing
a redundant cast, but the variable sleep_min does not exist in the modified
code. Could the commit message be updated to reflect the actual changes?

> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 7fa18dfcb5a28..cf2e05aaa4b2f 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c

[ ... ]

> @@ -349,11 +349,23 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
>  		if (ktime_after(ktime_add_us(cur, wait), end))
>  			wait = ktime_us_delta(end, cur);
>  
> -		if (atomic)
> -			udelay(wait);
> -		else
> -			usleep_range(wait, wait << 1);
> -		wait <<= 1;
> +#define __XE_MMIO_WAIT_MAX_INLOOP_TIME (100 * USEC_PER_MSEC)
> +		if (atomic) {
> +			if (wait <= MAX_UDELAY_MS * USEC_PER_MSEC)
> +				udelay(wait);
> +			else
> +				mdelay(DIV_ROUND_UP(wait, USEC_PER_MSEC));

[Severity: Low]
Wait is defined as an s64 type. Does using DIV_ROUND_UP(wait, USEC_PER_MSEC)
here cause a 64-bit division? On 32-bit architectures, this could lead to an
undefined reference to __divdi3 and break the build.

> +		} else {
> +			usleep_range(wait, __XE_MMIO_WAIT_MAX_INLOOP_TIME);

[Severity: High]
Does hardcoding __XE_MMIO_WAIT_MAX_INLOOP_TIME (100ms) as the maximum bound
for usleep_range() introduce massive timer slack? For example, on the first
iteration where wait is 10us, this allows the scheduler to delay the wakeup
for up to 100ms, completely defeating the intended exponential backoff.
This could cause massive latency spikes for fast MMIO operations in non-atomic
calls to __xe_mmio_wait32().

> +		}
> +		/*
> +		 * As we keep doubling the wait time for every check that fails, cap the
> +		 * in-loop delay-or-sleep to less than 2x 100 miliseconds to prevent from
> +		 * expanding 'wait' into exponentially longer wait times per loop that
> +		 * end up delaying the next completion check way later than tolerable.
> +		 */
> +		wait = wait < __XE_MMIO_WAIT_MAX_INLOOP_TIME >> 1 ?
> +		       wait << 1 : __XE_MMIO_WAIT_MAX_INLOOP_TIME;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907235524.7807-2-alan.previn.teres.alexis@intel.com?part=1