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

[sashiko-bot@kernel.org]

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 <michael.j.ruhl@intel.com>

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?

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().

[ ... ]
> @@ -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.

[ ... ]
> +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
the hardware active indefinitely. The pending async idle/suspend work can
be cancelled by pm_runtime_disable() during the device removal path.

[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:

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-michael.j.ruhl@intel.com?part=9