Re: [PATCH 04/10] drm/xe: Factor out {min,max}_idledly_ps 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 032b173104de210ca0228dd31e057951f3cde20d
Author: Gustavo Sousa <gustavo.sousa@intel.com>

drm/xe: Factor out {min,max}_idledly_ps in adjust_idledly()

This commit reorganizes adjust_idledly() so that workaround-specific logic
computes min_idledly_ps and max_idledly_ps before applying and rounding
those limits to the RING_IDLEDLY register field.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index dcc8a6d53c5d0..2d2a80cc2da35 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -585,99 +585,81 @@ static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  	xe_reg_whitelist_process_engine(hwe);
>  }
>  
> -static u32 idledly_floor_ticks(u64 idledly_ps, u32 idledly_units_ps)
> -{
> -	return DIV_ROUND_DOWN_ULL(idledly_ps, idledly_units_ps);
> -}
> -
>  static void adjust_idledly(struct xe_hw_engine *hwe)
>  {
>  	struct xe_gt *gt = hwe->gt;
> -	u32 idledly_reg_val, maxcnt_reg_val;
> +	u32 idledly_reg_val;
> +	u32 idledly;
>  	u32 idledly_units_ps = 8 * gt->info.timestamp_base;
> -	u32 maxcnt_units_ps = 640000;
>  	u64 idledly_ps, idledly_hw_ps;
> -	u64 maxcnt_ps;
> +	u64 min_idledly_ps = 0;
> +	u64 max_idledly_ps = -1;
>  	bool inhibit_switch = false;
>  	bool wa_applied = false;
> -	bool clamped_below_maxcnt = false;
>  
>  	if (IS_SRIOV_VF(gt_to_xe(gt)))
>  		return;
>  
>  	if (XE_GT_WA(gt, 16023105232) || XE_GT_WA(gt, 14025941587)) {
> -		u32 mincnt_idledly_ps = 5000000;
> -
>  		/* xe_gt_clock_init() warns and zeroes timestamp_base on unknown crystal clock. */
>  		if (!idledly_units_ps)
>  			return;
>  
>  		idledly_reg_val = xe_mmio_read32(&gt->mmio, RING_IDLEDLY(hwe->mmio_base));
>  		inhibit_switch = idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> -		idledly_ps = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val) * idledly_units_ps;
> +		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
> +		idledly_ps = idledly * idledly_units_ps;
>  		idledly_hw_ps = idledly_ps;
>  
> -		maxcnt_reg_val = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
> -		maxcnt_ps = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt_reg_val) * maxcnt_units_ps;
> -
> -		/*
> -		 * Wa_14025941587 is applied before Wa_16023105232, which takes
> -		 * priority if the two ever conflict (not expected in practice).
> -		 */
> -		if (XE_GT_WA(gt, 14025941587) &&
> -		    idledly_ps < mincnt_idledly_ps) {
> -			idledly_ps = mincnt_idledly_ps;
> -			wa_applied = true;
> -		}
> +		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));
> +
> +			max_idledly_ps = min_t(u64,
> +					       max_idledly_ps,
> +					       REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps);
> +
>  			/* Clear the inhibit switch without disturbing a valid delay. */
>  			if (inhibit_switch) {
>  				idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
>  				wa_applied = true;
>  			}
> +		}
>  
> -			/* Warn only on the value read from hardware. */
> -			xe_gt_WARN_ON(gt, idledly_hw_ps >= maxcnt_ps);
> +		/*
> +		 * 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;
> +			wa_applied = true;
> +		}
>  
> -			if (idledly_ps >= maxcnt_ps) {
> -				/* maxcnt may be 0 if IDLE_WAIT_TIME is unprogrammed. */
> -				idledly_ps = maxcnt_ps ? maxcnt_ps - 1 : 0;
> -				clamped_below_maxcnt = true;
> -				wa_applied = true;
> +		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 these tick-to-picosecond multiplications in adjust_idledly() in
drivers/gpu/drm/xe/xe_hw_engine.c overflow 32 bits before being widened to
u64?

When hw_engine_init() calls adjust_idledly() on non-VF platforms where
Wa_16023105232 or Wa_14025941587 is active, idledly, idledly_units_ps (up to
8 * 83333 = 666,664 ps), maxcnt_units_ps (640,000 ps), and the return value
of REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) are all u32.

In the three assignments:

	idledly_ps = idledly * idledly_units_ps;

both operands are u32. Since IDLE_DELAY is a 21-bit field (REG_GENMASK(20, 0),
up to 2,097,151 ticks), the product can reach ~1.398e12 ps and wraps modulo
2^32 for idledly >= 6443 ticks before assignment to u64 idledly_ps.

Similarly, in:

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

REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt) * maxcnt_units_ps is evaluated in 32-bit
unsigned arithmetic before min_t(u64, ...) casts the result to u64. Because
IDLE_WAIT_TIME is a 20-bit field (REG_GENMASK(19, 0), up to 1,048,575 ticks),
the product can reach ~6.71e11 ps and wraps 32 bits for IDLE_WAIT_TIME >= 6711
ticks, which can misprogram RING_IDLEDLY or trigger false
xe_gt_WARN_ON(gt, idledly_hw_ps >= max_idledly_ps) warnings.

Commit d864065ea25e ("drm/xe: harden adjust_idledly() against divide-by-zero
and overflow") previously added explicit (u64) casts to prevent 32-bit
truncation when converting ticks to time units. Should idledly_units_ps and
maxcnt_units_ps (or the multiplication operands) be promoted to u64 here as
well?

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