Re: [PATCH v5 2/4] drm/xe/forcewake: add delayed-release state machine

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Use-After-Free in delayed-release forcewake due to devm/drmm teardown asymmetry and timer re-arming
--

--- Patch [2]: [PATCH v5 2/4] drm/xe/forcewake: add delayed-release state machine ---
commit 7a90eb7b65ee38383fb6f1973ddea842f2c9c391
Author: Zongyao Bai <[email protected]>

drm/xe/forcewake: add delayed-release state machine

Add an opt-in forcewake release path that keeps an idle domain awake
for a short hold interval. A subsequent get can reuse the domain
without issuing another wake request or waiting for its ACK.

> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> index 197e2197bd0a3..3f5013c7424c8 100644
> --- a/drivers/gpu/drm/xe/xe_force_wake.c
> +++ b/drivers/gpu/drm/xe/xe_force_wake.c

[ ... ]

> +static void xe_force_wake_fini(void *arg)
> +{
> +	struct xe_force_wake *fw = arg;
> +	struct xe_force_wake_domain *domain;
> +	unsigned int tmp;
> +
> +	for_each_fw_domain(domain, fw, tmp)
> +		hrtimer_cancel(&domain->sleep_timer);
> +}
> +
> -void xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
> +int xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>  {

[ ... ]

>  	}
> +
> +	return devm_add_action_or_reset(xe->drm.dev, xe_force_wake_fini, fw);
>  }

[Severity: High]
Does registering this teardown via devm create a use-after-free window?

Since devm teardown (PCI unbind) happens before drmm teardown, background
tasks like gt->ordered_wq (which are destroyed during drmm teardown) can
still be running after xe_force_wake_fini() has executed.

Also, if a timer was pending here, canceling it without putting the domain
to sleep could leave the hardware permanently awake. Is there a need to
clear the state or issue domain_sleep() during teardown?

[ ... ]

> @@ -242,12 +330,24 @@ void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>  
>  	spin_lock_irqsave(&fw->lock, flags);
>  	for_each_fw_domain_masked(domain, fw_ref, fw, tmp) {
> +		assert_domain_state(fw, domain);
>  		xe_gt_assert(gt, domain->ref);
>  
>  		if (!--domain->ref) {
> -			sleep |= BIT(domain->id);
> -			domain_sleep(gt, domain);
> +			if (delayed_release || domain->delayed_release_requested) {
> +				domain->delayed_release_requested = false;
> +				fw->delayed_release_domains |= BIT(domain->id);
> +				hrtimer_start(&domain->sleep_timer,
> +					      us_to_ktime(xe->forcewake_hold_delay_us),
> +					      HRTIMER_MODE_REL);

[Severity: High]
If a background task drops the last reference here after xe_force_wake_fini()
has already run, could this unconditionally re-arm the timer?

If the timer is re-armed during this devm/drmm teardown window, the timer
callback might fire after the struct xe_gt memory has been completely freed
by drmm, resulting in a use-after-free.

Would it be safer to use drmm_add_action_or_reset() for teardown, or add a
mechanism to ensure we don't start timers when the device is being destroyed?

> +			} else {
> +				sleep |= BIT(domain->id);
> +				domain_sleep(gt, domain);
> +			}
> +		} else if (delayed_release) {
> +			domain->delayed_release_requested = true;
>  		}
> +		assert_domain_state(fw, domain);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2