RE: [PATCH v2 09/10] drm/xe/vsec: Support late bind fw information

>-----Original Message-----
>From: [email protected] <[email protected]>
>Sent: Wednesday, August 12, 2026 3:54 PM
>To: Ruhl, Michael J <[email protected]>
>Cc: [email protected]
>Subject: Re: [PATCH v2 09/10] drm/xe/vsec: Support late bind fw information
>
>Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to
>consider:
>- [High] Double `xe_pm_runtime_put(xe)` in the success path of
>`xe_pmt_write_reg`.
>- [High] The late bind delayed work is cancelled via a `devm` action, causing a
>race during teardown that registers child devices on a deleted parent.
>- [Medium] Using asynchronous `xe_pm_runtime_put()` during device
>teardown can leave hardware active indefinitely.
>- [Medium] `wait_for_fw` places the calling process in uninterruptible sleep (`D`
>state) for up to 20 seconds.
>--
>
>--- Patch [9]: [PATCH v2 09/10] drm/xe/vsec: Support late bind fw information
>---
>commit 5901be98f83d9c07e1385a372c7e330c405c0188
>Author: Michael J. Ruhl <[email protected]>
>
>drm/xe/vsec: Support late bind fw information
>
>CRI FW is loaded on power on.  Because of this, access to
>the FW cannot be done until it is running.
>
>Update the XE PMT probe and access to check for late bind
>devices, verify, and wait for the appropriate FW state
>before probe or access.
>
>> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
>> --- a/drivers/gpu/drm/xe/xe_vsec.c
>> +++ b/drivers/gpu/drm/xe/xe_vsec.c
>[ ... ]
>> +static bool wait_for_fw(struct xe_device *xe)
>> +{
>> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
>> +
>> +	if (xe->info.platform != XE_CRESCENTISLAND)
>> +		return true;
>> +
>> +	while (retries--) {
>> +		if (xe_is_oobmsm_fw_ready(xe))
>> +			return true;
>> +
>> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
>
>[Severity: Medium]
>Does this msleep() place the calling process in an uninterruptible sleep
>for up to 20 seconds?

Yes.  There is no way to provide a non-blocking path  to this data, and the potential
time is necessary to verify that the FW is not going to be loaded successfully.

>If the firmware is unresponsive, userspace processes reading or writing the
>PMT sysfs interface might become unkillable and hang while calling
>xe_pmt_telem_read(), xe_pmt_read_reg(), or xe_pmt_write_reg().

Agreed.  Not sure how to provide a better answer.

>[ ... ]
>> @@ -416,6 +488,10 @@ static int xe_pmt_write_reg(struct device *dev, u32
>guid, u32 reg, u32 offset)
>>  	disc_addr += CRI_DISCOVERY_OFFSET + inst + offset;
>>
>>  	xe_pm_runtime_get(xe);
>> +	if (!wait_for_fw(xe)) {
>> +		ret = -ENODATA;
>> +		goto runtime_exit;
>> +	}
>>  	mutex_lock(&xe->pmt.lock);
>>
>>  	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
>> @@ -425,6 +501,9 @@ static int xe_pmt_write_reg(struct device *dev, u32
>guid, u32 reg, u32 offset)
>>  	mutex_unlock(&xe->pmt.lock);
>>  	xe_pm_runtime_put(xe);
>>
>> +runtime_exit:
>> +	xe_pm_runtime_put(xe);
>> +
>>  dev_exit:
>>  	drm_dev_exit(idx);
>
>[Severity: High]
>Will the success path fall through here and execute the newly added
>runtime_exit cleanup path?
>
>It appears that if wait_for_fw() succeeds, the function calls
>xe_pm_runtime_put() once right after mutex_unlock(), and then immediately
>falls through to the runtime_exit label, resulting in two sequential
>xe_pm_runtime_put() calls.
>
>This could drop the PM usage counter below its baseline, potentially
>powering off the device prematurely while it is still in use.

Agreed this is a bug. and is fixed.

>[ ... ]
>> +static void vsec_disable_late_bind_work(void *arg)
>> +{
>> +	struct xe_device *xe = arg;
>> +
>> +	/*
>> +	 * If was work was cancelled while it was still pending, we need to
>> +	 * take care of releasing the runtime reference
>> +	 */
>> +	if (disable_delayed_work_sync(&xe->pmt.work))
>> +		xe_pm_runtime_put(xe);
>
>[Severity: Medium]
>Should this use a synchronous put instead of an asynchronous one?
>
>Using an asynchronous xe_pm_runtime_put() during device teardown can
>leave

What is the synchronous version of this call?  This is the only runtime_put call in the API.

>the hardware active indefinitely. The pending async idle/suspend work can
>be cancelled by pm_runtime_disable() during the device removal path.

This is called in the device remove path, so the power states should be correct.

>[Severity: High]
>Can this devm action race with cri_late_bind_probe_work() during teardown?
>
>If the device is unbound during the 20-second firmware initialization
>window, the driver core removes all children via device_del() before
>starting devres release. The disable_delayed_work_sync() here blocks until
>the work completes.
>
>Concurrently, if cri_late_bind_probe_work() executes:

If the disable succeeds the work will execute.

If it fails, the work will do the runtime_put.

I don't see the race condition.

>static void cri_late_bind_probe_work(struct work_struct *work)
>{
>	...
>	if (xe_is_oobmsm_fw_ready(xe)) {
>		cri_late_bind_probe(xe);
>		...
>
>It could proceed to call intel_vsec_register().
>
>Would this register a child device on a parent that has already been
>deleted, causing devres list corruption or a sysfs use-after-free?
>
>
>--
>Sashiko AI review ·
>https://sashiko.dev/#/patchset/20260812193742.2170188-12-
>[email protected]?part=9