Re: [PATCH 06/10] drm/xe: Drop early and redundant workaround checks in adjust_idledly()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: 32-bit multiplication overflow in adjust_idledly()
--

commit 1632f9f2c26646be902e4438963fff43c54e34fd
Author: Gustavo Sousa <gustavo.sousa@intel.com>

drm/xe: Drop early and redundant workaround checks in adjust_idledly()

This commit removes the outer workaround check in adjust_idledly() and
unindents the body now that each workaround check is scoped to its own
conditional block and the function returns early when no workaround
applies.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index e201fa96ee867..00af7344067d3 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -600,77 +600,75 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>  	if (IS_SRIOV_VF(gt_to_xe(gt)))
>  		return;
>  
> -	if (XE_GT_WA(gt, 16023105232) || XE_GT_WA(gt, 14025941587)) {
> -		/* xe_gt_clock_init() warns and zeroes timestamp_base on unknown crystal clock. */
> -		if (!idledly_units_ps)
> -			return;
> +	/* xe_gt_clock_init() warns and zeroes timestamp_base on unknown crystal clock. */
> +	if (!idledly_units_ps)
> +		return;
>  
> -		if (XE_GT_WA(gt, 14025941587))
> -			min_idledly_ps = max(min_idledly_ps, 5000000);
> +	if (XE_GT_WA(gt, 14025941587))
> +		min_idledly_ps = max(min_idledly_ps, 5000000);
>  
> -		if (XE_GT_WA(gt, 16023105232)) {
> -			u32 maxcnt_units_ps = 640000;
> -			u32 maxcnt = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
> +	if (XE_GT_WA(gt, 16023105232)) {
> +		u32 maxcnt_units_ps = 640000;
> +		u32 maxcnt = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
>  
> -			max_idledly_ps = min_t(u64,
> -					       max_idledly_ps,
> -					       REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps);
> +		max_idledly_ps = min_t(u64,
> +				       max_idledly_ps,
> +				       REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps);
>  
> -			clear_inhibit_switch = true;
> -		}
> +		clear_inhibit_switch = true;
> +	}
>  
> -		/* No applicable workaround found. */
> -		if (min_idledly_ps == 0 && max_idledly_ps == -1 && !clear_inhibit_switch)
> -			return;
> +	/* No applicable workaround found. */
> +	if (min_idledly_ps == 0 && max_idledly_ps == -1 && !clear_inhibit_switch)
> +		return;
>  
> -		idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
> +	idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>  
> -		if (clear_inhibit_switch && idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED) {
> -			idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
> -			wa_applied = true;
> -		}
> +	if (clear_inhibit_switch && idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED) {
> +		idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
> +		wa_applied = true;
> +	}
> +
> +	idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
> +	idledly_ps = idledly * idledly_units_ps;
> +	idledly_hw_ps = idledly_ps;
>  
> -		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
> +	/*
> +	 * min_idledly_ps is applied before max_idledly_ps, which takes
> +	 * priority if the two ever conflict (not expected in practice).
> +	 */
> +	if (idledly_ps < min_idledly_ps) {
> +		idledly = DIV_ROUND_UP_ULL(min_idledly_ps, idledly_units_ps);
>  		idledly_ps = idledly * idledly_units_ps;
> -		idledly_hw_ps = idledly_ps;
> +		wa_applied = true;
> +	}
>  
> -		/*
> -		 * min_idledly_ps is applied before max_idledly_ps, which takes
> -		 * priority if the two ever conflict (not expected in practice).
> -		 */
> -		if (idledly_ps < min_idledly_ps) {
> -			idledly = DIV_ROUND_UP_ULL(min_idledly_ps, idledly_units_ps);
> +	if (idledly_ps >= max_idledly_ps) {
> +		if (max_idledly_ps) {
> +			idledly = DIV_ROUND_DOWN_ULL(max_idledly_ps - 1, idledly_units_ps);
>  			idledly_ps = idledly * idledly_units_ps;

[Severity: Medium]
Can the picosecond multiplications in adjust_idledly() overflow 32 bits?

In the Wa_16023105232 check:

	max_idledly_ps = min_t(u64,
			       max_idledly_ps,
			       REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps);

both REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) and maxcnt_units_ps (640000)
have type u32. The multiplication is evaluated in 32-bit unsigned
arithmetic before min_t(u64, ...) widens the result to u64. Since
IDLE_WAIT_TIME is REG_GENMASK(19, 0), any register value of 6711 ticks or
greater (~4.295 ms out of the ~671 ms range) exceeds U32_MAX when
multiplied by 640000 and wraps around.

Similarly, idledly (extracted from the 21-bit IDLE_DELAY field) and
idledly_units_ps (8 * gt->info.timestamp_base) are both u32:

	idledly_ps = idledly * idledly_units_ps;

At all three assignments of idledly_ps in adjust_idledly(), the product is
computed in 32-bit arithmetic before being assigned to the u64 idledly_ps
variable, which overflows whenever the delay exceeds ~4.295 ms.

When reached during GT initialization via:

  xe_gt_init()
    -> gt_init_with_all_forcewake()
      -> xe_hw_engines_init()
        -> hw_engine_init()
          -> adjust_idledly()

this truncation can trigger a spurious xe_gt_WARN_ON(gt, idledly_hw_ps >=
max_idledly_ps) warning and program an improperly clamped IDLE_DELAY value
into RING_IDLEDLY.

Should maxcnt_units_ps and idledly_units_ps be declared as u64 (or the
multiplications use mul_u32_u32() / a u64 cast) so the products are
evaluated in 64-bit arithmetic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-wa_16031259911-v1-0-b0453f79c4e1@intel.com?part=6