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 2 potential issue(s) to consider:
- [Low] drm/xe: commit message describes dropped design changes
- [Low] drm/xe: preprocessor macro defined inside function body
--

commit 7206a8140e2444dbf990387d48807b42ec0fbee5
Author: Alan Previn <alan.previn.teres.alexis@intel.com>

drm/xe/mmio: Fix xe_mmio_wait32() to honor delay/sleep maximums
    
This patch bounds the maximum in-loop wait time in __xe_mmio_wait32 to
100ms to avoid excessively large delays. It also caps udelay usage to
MAX_UDELAY_MS for atomic contexts.

>     Check for overflow in udelay/usleep_range use in __xe_mmio_wait32
>     and pick the correct helper according to the wait time and atomic.
>     Implement a similar helper to replace DIV_ROUND_UP for 32-bit CPUs.

[Severity: Low]
Does the commit message need an update here? The text claims to pick the
correct helper according to the wait time and implement a similar helper
to replace DIV_ROUND_UP, but the actual diff only modifies the sleeping
logic and caps the wait times.

It seems the DIV_ROUND_UP redesign was dropped but the description was not
fully updated to reflect this.

> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 7fa18dfcb5a28..5a03698729ce5 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -350,10 +350,11 @@ static int __xe_mmio_wait32(struct xe_mmio *mmio, struct xe_reg reg, u32 mask, u
>  			wait = ktime_us_delta(end, cur);
>  
>  		if (atomic)
> -			udelay(wait);
> +			udelay(min_t(s64, wait, (MAX_UDELAY_MS * 1000)));
>  		else
>  			usleep_range(wait, wait << 1);

[Severity: Medium]
This isn't a bug introduced by this patch, but __xe_mmio_wait32 is called
with wait values that can reach 100,000 microseconds (100ms) due to the
backoff limit. According to the kernel timer guidelines, usleep_range
should only be used for delays under 20ms, while msleep is recommended
for larger delays to avoid using high-overhead hrtimers. Could we switch
to msleep for the longer iterations?

> -		wait <<= 1;
> +#define __XE_MMIO_WAIT_MAX_BACKOFF_100MS (100 * USEC_PER_MSEC)

[Severity: Low]
Is it intended to define this preprocessor macro inside the function body?
Macros do not have block scope in C, so placing it here can be stylistically
misleading and will likely trigger checkpatch warnings. Could this be moved
outside the function body?

> +		wait = min_t(s64, wait << 1, __XE_MMIO_WAIT_MAX_BACKOFF_100MS);
>  	}
>  
>  	if (ret != 0) {

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