Re: [PATCH 1/2] drm/xe: harden adjust_idledly() against divide-by-zero and overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning early when `maxcnt == 0` bypasses the workaround logic and fails to clear the `INHIBIT_SWITCH_UNTIL_PREEMPTED` bit if it was set in hardware.

Pre-existing issues:
- [High] Using `DIV_ROUND_CLOSEST_ULL` can round up, resulting in an `idledly` value that is greater than or equal to `maxcnt`, violating the workaround's core requirement and causing continuous kernel warnings.
--

commit 33df9c0262d14ae39bdd6d5eab48d83f8de9e00b
Author: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>

drm/xe: harden adjust_idledly() against divide-by-zero and overflow

This commit addresses corner-case issues in adjust_idledly() that could
cause kernel panics, wrap-arounds, or integer overflows. It adds early
returns for zero timestamp_base or IDLE_WAIT_TIME and upgrades
tick-to-ns math to 64-bit using DIV_ROUND_CLOSEST_ULL().

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 010499766fce2..6ffb0ece05612 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -594,18 +594,29 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>  	bool inhibit_switch = 0;
>  
>  	if (!IS_SRIOV_VF(gt_to_xe(hwe->gt)) && XE_GT_WA(gt, 16023105232)) {
> +		/* xe_gt_clock_init() zeroes timestamp_base on unknown crystal clock. */
> +		if (!idledly_units_ps) {
> +			xe_gt_warn(gt, "idledly WA skipped: timestamp_base is 0\n");
> +			return;
> +		}
> +
>  		idledly = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>  		maxcnt = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
>  
>  		inhibit_switch = idledly & INHIBIT_SWITCH_UNTIL_PREEMPTED;
>  		idledly = REG_FIELD_GET(IDLE_DELAY, idledly);
> -		idledly = DIV_ROUND_CLOSEST(idledly * idledly_units_ps, 1000);
> +		idledly = DIV_ROUND_CLOSEST_ULL((u64)idledly * idledly_units_ps, 1000);
>  		maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);
> +
> +		/* Return early rather than underflow maxcnt - 1 on a zero IDLE_WAIT_TIME. */
> +		if (!maxcnt)
> +			return;

[Severity: High]
Does returning early here bypass the workaround logic and fail to clear the
INHIBIT_SWITCH_UNTIL_PREEMPTED bit?

The commit message explicitly states that IDLE_WAIT_TIME can read back as 0.
If maxcnt evaluates to 0 and the inhibit_switch bit was set in hardware,
this early return skips the subsequent register write entirely, leaving the
context switch inhibit bit permanently active. The workaround should
gracefully clear the inhibit bit even if it cannot set a valid delay.

> +
>  		maxcnt *= maxcnt_units_ns;
>  
>  		if (xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
> -			idledly = DIV_ROUND_CLOSEST(((maxcnt - 1) * 1000),
> -						    idledly_units_ps);
> +			idledly = DIV_ROUND_CLOSEST_ULL((u64)(maxcnt - 1) * 1000,
> +							idledly_units_ps);

[Severity: High]
This is a pre-existing issue, but does using DIV_ROUND_CLOSEST_ULL here
cause the resulting idledly to round up and violate the maxcnt constraint?

Because DIV_ROUND_CLOSEST_ULL rounds to the nearest integer, the resulting
tick count can exceed maxcnt. For example, on a 12MHz crystal platform with
timestamp_base = 83333, idledly_units_ps is 666664 ps. If maxcnt is 1, maxcnt
in ns is 640. The calculation gives 639000 / 666664, which rounds to 1 tick.
When read back on the next invocation, 1 tick converts to 667 ns, and
667 >= 640 evaluates to true, triggering constant xe_gt_WARN_ON spam.

Should floor division be used instead to ensure idledly remains strictly
less than maxcnt?

>  			xe_mmio_write32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base), idledly);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909072540.99303-1-tilak.tirumalesh.tangudu@intel.com?part=1