Re: [PATCH 03/10] drm/xe: Use picosecond as common unit in adjust_idledly()

[sashiko-bot@kernel.org]

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

commit 6274ddb08f811b772af3c2cc759b8be4b565872c
Author: Gustavo Sousa <gustavo.sousa@intel.com>

drm/xe: Use picosecond as common unit in adjust_idledly()

This commit changes adjust_idledly() to use picoseconds instead of
nanoseconds as the common unit of time to avoid precision loss and
eliminate extra integer divisions.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 6872d1ca656e2..dcc8a6d53c5d0 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -585,17 +585,19 @@ 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(u32 idledly_ns, u32 idledly_units_ps)
> +static u32 idledly_floor_ticks(u64 idledly_ps, u32 idledly_units_ps)
>  {
> -	return DIV_ROUND_DOWN_ULL((u64)idledly_ns * 1000, 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, idledly_hw, idledly_reg_val, maxcnt;
> +	u32 idledly_reg_val, maxcnt_reg_val;
>  	u32 idledly_units_ps = 8 * gt->info.timestamp_base;
> -	u32 maxcnt_units_ns = 640;
> +	u32 maxcnt_units_ps = 640000;
> +	u64 idledly_ps, idledly_hw_ps;
> +	u64 maxcnt_ps;
>  	bool inhibit_switch = false;
>  	bool wa_applied = false;
>  	bool clamped_below_maxcnt = false;
> @@ -604,29 +606,27 @@ static void adjust_idledly(struct xe_hw_engine *hwe)
>  		return;
>  
>  	if (XE_GT_WA(gt, 16023105232) || XE_GT_WA(gt, 14025941587)) {
> -		u32 mincnt_idledly_ns = 5000;
> +		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));
> -		maxcnt = xe_mmio_read32(&gt->mmio, RING_PWRCTX_MAXCNT(hwe->mmio_base));
> -
>  		inhibit_switch = idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> -		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
> -		idledly = DIV_ROUND_CLOSEST_ULL((u64)idledly * idledly_units_ps, 1000);
> -		idledly_hw = idledly;
> -		maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);
> -		maxcnt *= maxcnt_units_ns;
> +		idledly_ps = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val) * 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;

[Severity: High]
Can the multiplications computing idledly_ps and maxcnt_ps in
adjust_idledly() overflow 32-bit unsigned arithmetic before being widened
to u64?

In include/drm/intel/reg_bits.h, REG_FIELD_GET() returns a u32:

    #define REG_FIELD_GET(__mask, __val)	((u32)FIELD_GET(__mask, __val))

Because idledly_units_ps and maxcnt_units_ps are both declared as u32,
both multiplications are evaluated in 32-bit arithmetic before assignment
to u64 idledly_ps and u64 maxcnt_ps.

For idledly_ps, dropping the (u64) cast that was added in commit
d864065ea25e ("drm/xe: harden adjust_idledly() against divide-by-zero and
overflow") means IDLE_DELAY (a 21-bit field, REG_GENMASK(20, 0)) multiplied
by idledly_units_ps (up to 666,664 ps) wraps around U32_MAX whenever
IDLE_DELAY is >= 6443 ticks.

Similarly, for maxcnt_ps, scaling the unit multiplier from 640 ns to
640,000 ps in u32 means IDLE_WAIT_TIME (a 20-bit field, REG_GENMASK(19, 0))
multiplied by maxcnt_units_ps wraps around U32_MAX whenever IDLE_WAIT_TIME
is >= 6711 ticks (~4.29 ms out of its ~671 ms range).

When either product wraps, xe_gt_WARN_ON(gt, idledly_hw_ps >= maxcnt_ps)
can trigger spuriously, and the Wa_14025941587 and Wa_16023105232 checks
can program a corrupted IDLE_DELAY tick count into RING_IDLEDLY.

Should idledly_units_ps and maxcnt_units_ps be declared as u64 (or the
operands cast to u64 before multiplying)?

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