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

["Belgaumkar, Vinay" <vinay.belgaumkar@intel.com>]

On 9/11/2026 10:44 AM, Tangudu, Tilak Tirumalesh wrote:
>
>> -----Original Message-----
>> From: Belgaumkar, Vinay <vinay.belgaumkar@intel.com>
>> Sent: 11 September 2026 23:09
>> To: Tangudu, Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>;
>> Upadhyay, Tejas <tejas.upadhyay@intel.com>; Sousa, Gustavo
>> <gustavo.sousa@intel.com>; intel-xe@lists.freedesktop.org
>> Subject: Re: [PATCH 1/2] drm/xe: harden adjust_idledly() against divide-by-
>> zero and overflow
>>
>>
>> On 9/9/2026 9:18 AM, tilak.tirumalesh.tangudu@intel.com wrote:
>>> From: Tangudu Tilak Tirumalesh <tilak.tirumalesh.tangudu@intel.com>
>>>
>>> adjust_idledly() has several corner-case issues flagged during review:
>>>
>>>     1. If xe_gt_clock_init() failed to recognise the crystal clock,
>>>        gt->info.timestamp_base is 0, which makes idledly_units_ps also 0.
>>>        The subsequent DIV_ROUND_CLOSEST(..., idledly_units_ps) is then a
>>>        divide-by-zero and panics the kernel.
>>>
>>>     2. The tick-to-ns conversions are done in u32:
>>>           idledly * idledly_units_ps, (maxcnt - 1) * 1000
>>>        Both overflow u32 before DIV_ROUND_CLOSEST() sees them.
>>>
>>>     3. If IDLE_WAIT_TIME reads back as 0, maxcnt evaluates to 0 and
>>>        the maxcnt - 1 clamp wraps to 0xFFFFFFFF in u32.
>>>
>>>     4. The register only stores whole ticks, so the clamped ns value has
>>>        to be converted to ticks and back. DIV_ROUND_CLOSEST() can round
>>>        that conversion up past maxcnt:
>>>
>>>          maxcnt = 640 ns, one tick = 666664 ps
>>>
>>>          clamp:        maxcnt - 1        = 639 ns
>>>          ns -> ticks:  639000 / 666664   = 0.958 -> rounds to 1 tick
>>>          tick -> ns:    1 * 666664 / 1000 = 667 ns
>>>
>>>        667 ns is programmed into RING_IDLEDLY, but 667 >= maxcnt (640),
>>>        so xe_gt_WARN_ON() fires again on every subsequent init.
>>>
>>> Warn and return early if timestamp_base is 0 (the unknown-crystal path).
>>> Do the conversions in u64 via the *_ULL() helpers so they cannot wrap.
>>> Clamp with a floor (DIV_ROUND_DOWN_ULL) so the programmed delay
>> stays
>>> strictly below maxcnt, and guard the maxcnt == 0 case with a zero
>>> delay while still writing RING_IDLEDLY so
>>> INHIBIT_SWITCH_UNTIL_PREEMPTED is cleared.
>>>
>>> Fixes: d2de4410a88f ("drm/xe: Apply Wa_16023105232")
>>> Cc: stable@vger.kernel.org
>>> Assisted-by: GitHub_Copilot:claude-opus-4.8
>>> Signed-off-by: Tangudu Tilak Tirumalesh
>>> <tilak.tirumalesh.tangudu@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_hw_engine.c | 14 +++++++++++---
>>>    1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
>>> b/drivers/gpu/drm/xe/xe_hw_engine.c
>>> index 010499766fce..c86890e09b55 100644
>>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>>> @@ -594,18 +594,26 @@ 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");
>> There is already a xe_gt_warn in xe_gt_clock.c when timestamp_base is set to
>> 0 in xe_gt_clock.c, so this is redundant. CI failures will happen if this is the
>> case anyway.
> I will make it silent and remove warn but "return" should stay right?
>>> +			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);
>>>    		maxcnt *= maxcnt_units_ns;
>>>
>>>    		if (xe_gt_WARN_ON(gt, idledly >= maxcnt || inhibit_switch)) {
>>> -			idledly = DIV_ROUND_CLOSEST(((maxcnt - 1) * 1000),
>>> -						    idledly_units_ps);
>>> +			/* Floor below maxcnt; write 0 to still clear the inhibit
>> bit. */
>>> +			idledly = maxcnt ?
>> shouldn't this be idledly >= maxcnt ?
>   idledly >= maxcnt  this is already in if guard
> the above check is for maxcnt zero case

ok, makes sense. LGTM.

Reviewed-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

>
>> Thanks,
>>
>> Vinay.
>>
>>> +				DIV_ROUND_DOWN_ULL((u64)(maxcnt - 1) *
>> 1000,
>>> +						   idledly_units_ps) : 0;
>>>    			xe_mmio_write32(&gt->mmio, RING_IDLEDLY(hwe-
>>> mmio_base), idledly);
>>>    		}
>>>    	}