Re: [PATCH v8 17/20] drm/xe/vsec: Support late bind fw information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition causing devres leak and zombie auxiliary device during unbind.
- [High] Concurrent access to `set_telem_region` without `xe->pmt.lock` causes hardware state corruption.
- [Medium] Uninterruptible sleep for 20 seconds in debugfs read path causes Denial of Service.
--

commit 4c5a8d1261728742c4507e3c8f19ebb935da6c19
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support late bind fw information

This commit adds support for CRI firmware late binding. It updates the XE
PMT probe and access paths to check for late bind devices, verify the
firmware state, and wait for readiness before proceeding.

diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
index cb3d9142346cf..c2f527e7894af 100644
--- a/drivers/gpu/drm/xe/xe_vsec.c
+++ b/drivers/gpu/drm/xe/xe_vsec.c
@@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
[ ... ]
> +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 uninterruptible sleep risk blocking hot-unplugs for up to 20
seconds?

This msleep() puts the process into an uninterruptible sleep state. When
called from user-space via xe_pmt_telem_read(), the thread has already
called drm_dev_enter(), meaning it holds the SRCU read lock for the entire
wait period. If a hot-unplug event occurs, drm_dev_unplug() will be blocked
waiting for the SRCU grace period to complete, effectively causing a denial
of service on the device removal path.

> +	}
> +
> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> +
> +	return false;
> +}
[ ... ]
@@ -471,12 +541,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
[ ... ]
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
> +	struct intel_vsec_platform_info *info;
> +	struct device *dev = xe->drm.dev;
> +	enum xe_vsec platform;
> +
> +	platform = get_platform_info(xe);
> +	if (platform != XE_VSEC_CRI)
> +		return;
> +
> +	info = &xe_vsec_info[platform];
> +	if (!info->headers)
> +		return;
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);

[Severity: High]
Is xe->pmt.lock needed here to protect the set_telem_region hardware
register?

In other code paths, the lock is correctly acquired before updating this
hardware state:

xe_pmt_telem_read() {
    ...
    mutex_lock(&xe->pmt.lock);
    xe->soc_remapper.set_telem_region(xe, mem_region);
    ...
}

Since cri_late_bind_probe() accesses it asynchronously from the worker
thread without acquiring the lock, concurrent debugfs reads could cause the
PMT hardware to read from the wrong region or return garbage telemetry
data.

> +
> +	intel_vsec_register(dev, info);

[Severity: High]
Does this sequence race with device unbind, leading to orphaned devres
entries?

When the device unbinds, devres_release_all() splices the parent device's
resource list into a local list and begins executing actions, including
vsec_disable_late_bind_work().

If vsec_disable_late_bind_work() blocks below on disable_delayed_work_sync(),
the worker may currently be executing this call to intel_vsec_register().

intel_vsec_register() will allocate new devres entries (such as auxiliary
devices) on the parent device. Because the parent's devres head is now
empty (having been moved to the local list for teardown), these new entries
are appended to the main list but will never be freed since the teardown is
already in progress. This leaves zombie auxiliary devices and causes memory
leaks.

> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If the queued work is canceled, the runtime reference needs to be
> +	 * released here.
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
> +		xe_pm_runtime_put(xe);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911201148.1610547-22-michael.j.ruhl@intel.com?part=17