Re: [PATCH v5 3/4] drm/xe/forcewake: flush delayed release at power boundaries

On 8/12/2026 5:23 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] PMU events hold long-lived forcewake references that outlive device teardown and suspend boundaries, defeating the patch's attempt to quiesce forcewake accesses before power-off.
> --
>
> --- Patch [3]: [PATCH v5 3/4] drm/xe/forcewake: flush delayed release at power boundaries ---
> commit 137d6a969747b4828385eae977e1c1ba6eb0b98d
> Author: Zongyao Bai <[email protected]>
>
> drm/xe/forcewake: flush delayed release at power boundaries
>      
> Add a synchronous forcewake flush that cancels per-domain timers and
> settles pending sleep acknowledgments. Provide a scoped cleanup class
> so the final put is always followed by the flush before the caller
> continues toward power-off.
>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 3f5013c7424c8..ef43899e1f7d3 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
>> @@ -391,6 +391,55 @@ void xe_force_wake_put_delay(struct xe_force_wake *fw, unsigned int fw_ref)
>>   	__xe_force_wake_put(fw, fw_ref, true);
>>   }
>>   
>> +/**
>> + * xe_force_wake_flush - Flush delayed forcewake releases
>> + * @fw: Pointer to the force wake structure
>> + *
>> + * Synchronously cancels delayed-release timers and completes pending sleep
>> + * requests and ACK waits. This must be called after the final forcewake put
>> + * and before power-off because the timer callback accesses MMIO.
>> + *
>> + * The caller must ensure no forcewake reference outlives this call and
>> + * serialize against new delayed releases or final puts that consume a
>> + * recorded delayed-release request. Timers are canceled before taking
>> + * fw->lock because their callbacks acquire the same lock.
>> + */
>> +void xe_force_wake_flush(struct xe_force_wake *fw)
>> +{
>> +	struct xe_gt *gt = fw->gt;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp, sleep, ack_fail = 0;
>> +	unsigned long flags;
>> +
>> +	for_each_fw_domain(domain, fw, tmp)
>> +		hrtimer_cancel(&domain->sleep_timer);
>> +
>> +	spin_lock_irqsave(&fw->lock, flags);
>> +	sleep = fw->sleep_ack_pending_domains;
>> +	for_each_fw_domain(domain, fw, tmp) {
>> +		assert_domain_state(fw, domain);
>> +		domain->delayed_release_requested = false;
>> +		if ((fw->delayed_release_domains & BIT(domain->id)) &&
>> +		    !domain->ref) {
> [Severity: High]
> This is a pre-existing issue, but does this flush mechanism fail to quiesce
> active PMU references before power-off?
>
> If a user opens a PMU event (e.g., engine-active-ticks) and acquires a
> forcewake reference, it can outlive device teardown or suspend. When
> xe_force_wake_flush() runs, it skips the domain because domain->ref > 0.
>
> If the device is unplugged or suspended while the PMU event is open, the domain
> is not put to sleep due to the active reference, and the hardware loses power.
> On resume, software may incorrectly believe the domain is awake, failing to send
> wake requests and causing hardware timeouts or hangs on subsequent accesses.
>
> Furthermore, on unplug, mmio_fini sets regs = NULL. When the user finally
> closes the PMU event fd, xe_pmu_event_destroy() releases forcewake:
>
> drivers/gpu/drm/xe/xe_pmu.c:xe_pmu_event_destroy() {
>      ...
>      xe_force_wake_put(gt_to_fw(gt), *fw_ref);
>      ...
> }
>
> Will this cause a use-after-unmap logic hitting a WARN_ON_ONCE in
> mmio_available when accessing the unmapped MMIO?


Agreed that this is pre-existing, and it's independent of this series.
PMU acquires its forcewake reference with the plain 
xe_force_wake_get()/xe_force_wake_put() path,
not the delayed release added here, and this series does not touch 
xe_pmu.c.
xe_force_wake_flush() skips a domain with ref > 0 for the same reason 
the ordinary put() does
so the behavior is identical with or without delayed release.

And this issue scenario should be addressed in the PMU/unplug teardown path.
It out of the scope of this patch. I prefer follow up with another patch.

Zongyao


>
>> +			fw->delayed_release_domains &= ~BIT(domain->id);
>> +			sleep |= BIT(domain->id);
>> +			domain_sleep(gt, domain);
>> +		}
>> +		assert_domain_state(fw, domain);
>> +	}
>> +	for_each_fw_domain_masked(domain, sleep, fw, tmp) {
>> +		fw->sleep_ack_pending_domains &= ~BIT(domain->id);
>> +		if (domain_sleep_wait(gt, domain) == 0)
>> +			fw->awake_domains &= ~BIT(domain->id);
>> +		else
>> +			ack_fail |= BIT(domain->id);
>> +	}
>> +	spin_unlock_irqrestore(&fw->lock, flags);
>> +
>> +	xe_gt_WARN(gt, ack_fail, "Forcewake domain%s %#x failed to acknowledge sleep request\n",
>> +		   str_plural(hweight_long(ack_fail)), ack_fail);
>> +}