Re: [PATCH] drm/xe: Add wa_14025941587 to xe2, xe3 and xe3p platforms

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Potential division by zero in DIV_ROUND_CLOSEST if the GT crystal clock frequency is invalid or unrecognized.
- [High] 32-bit integer overflow during nanosecond unit conversions for `idledly` causes incorrect delay calculations and erroneous hardware timer programming.
- [High] Integer underflow when maxcnt is zero leads to a massive delay calculation that subsequently overflows.
--

commit 36b72781bea702adafa74030ffeb7bc9ba2efaf8
Author: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>

    drm/xe: Add wa_14025941587 to xe2, xe3 and xe3p platforms
    
    Avoid IDLEDLY timer programming lessthan 5 micro secs.
    Apply wa_14025941587 to Graphics Versions 20.01 to 35.11
    and Media Versions 13.01 to 35.03

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 010499766fce2..5a87f26d7dd4a 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -588,25 +588,57 @@ static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
>  static void adjust_idledly(struct xe_hw_engine *hwe)
>  {
[ ... ]
> -		inhibit_switch = idledly & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> -		idledly = REG_FIELD_GET(IDLE_DELAY, idledly);
> +		inhibit_switch = idledly_reg_val & INHIBIT_SWITCH_UNTIL_PREEMPTED;
> +		idledly = REG_FIELD_GET(IDLE_DELAY, idledly_reg_val);
>  		idledly = DIV_ROUND_CLOSEST(idledly * idledly_units_ps, 1000);

[Severity: High]
This is a pre-existing issue, but does this multiplication overflow 32 bits?

With idledly capable of being up to 21-bits and idledly_units_ps typically
around 666,664, the multiplication can reach ~1.4 trillion, which
overflows the 32-bit limit before DIV_ROUND_CLOSEST is evaluated. This wrap
around could cause an incorrect delay calculation.

>  		maxcnt = REG_FIELD_GET(IDLE_WAIT_TIME, maxcnt);
>  		maxcnt *= maxcnt_units_ns;
[ ... ]
> +		if (XE_GT_WA(gt, 16023105232) &&
> +		    xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
> +			idledly = (maxcnt - 1);

[Severity: High]
This is a pre-existing issue, but does this subtraction underflow if maxcnt
evaluates to 0?

If the hardware initializes with 0 in the IDLE_WAIT_TIME field, maxcnt will
be 0. Because idledly is unsigned, idledly >= maxcnt will be true, causing
idledly to underflow to U32_MAX.

> +			wa_applied = true;
> +			/* inhibit_switch override to keep
> +			 * semantics of Wa_16023105232
> +			 */
> +			if (inhibit_switch)
> +				idledly_reg_val &= ~INHIBIT_SWITCH_UNTIL_PREEMPTED;
> +		}
> +
> +		if (wa_applied) {
> +			idledly_reg_val &= ~IDLE_DELAY;
> +			idledly_reg_val |= REG_FIELD_PREP(IDLE_DELAY,
> +						 DIV_ROUND_CLOSEST(idledly * 1000,
> +								   idledly_units_ps));

[Severity: High]
This is a pre-existing issue, but does this code suffer from a division by
zero panic?

If the hardware register RPM_CONFIG0 contains an unrecognized crystal clock
frequency, xe_gt_clock_init() sets gt->info.timestamp_base to 0. This makes
idledly_units_ps evaluate to 0, which triggers a panic here.

Also, does the idledly * 1000 multiplication overflow the 32-bit limit?

If maxcnt was 0 and idledly underflowed to U32_MAX earlier, this
multiplication reaches ~4.29 trillion, causing another silent overflow
before the division and programming an incorrect delay into the hardware.

> +			xe_gt_dbg(gt, "idledly %s: set %u max=%u inh=%u ts=%u\n",
> +				  hwe->name, idledly, maxcnt,
> +				  !!inhibit_switch, gt->info.timestamp_base);
> +			xe_mmio_write32(&gt->mmio,
> +					RING_IDLEDLY(hwe->mmio_base),
> +					idledly_reg_val);
>  		}
>  	}
>  }

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