Re: [PATCH v9 17/20] 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] devres: memory leak and dangling devices due to worker racing with devres_release_all
- [High] pm: non-freezable delayed work accesses MMIO during system suspend
- [Medium] pm: asynchronous pm_runtime_put() during device removal causes incorrect power state
- [Medium] pm: uninterruptible msleep in wait_for_fw() blocks task freezer and system suspend
--

commit 2311d189c57553ad608d675783c5e664a010f6ef
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
> index d1427ec507285..f905ef51df656 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)
>  	return -ENODEV;
>  }
>  
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_sysctrl_is_oobmsm_fw_ready(xe)) {
> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	xe->pmt.retry_count++;
> +
> +	/* wait up to 20 seconds */
> +	if (xe->pmt.retry_count == VSEC_LATE_BIND_RETRY) {
> +		drm_warn(&xe->drm, "PMT probe: Late Binding failed to complete\n");
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
> +
> +	if (!schedule_delayed_work(&xe->pmt.work, msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC)))
> +		xe_pm_runtime_put(xe);
> +}

[Severity: High]
Does scheduling this delayed work on the default non-freezable workqueue risk
MMIO access during system suspend?

If the system enters suspend (S3) while the worker is in its polling loop,
the PCI device is put into the D3 state. Because the worker is on a
non-freezable workqueue, it continues running and calls
xe_sysctrl_is_oobmsm_fw_ready(), which performs MMIO reads on powered-down
hardware.

> +
> +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_sysctrl_is_oobmsm_fw_ready(xe))
> +			return true;
> +
> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
> +	}

[Severity: Medium]
Could this uninterruptible sleep delay task freezing for up to 20 seconds
during system suspend?

If userspace initiates a read on the PMT telemetry sysfs file, calling
xe_pmt_telem_read(), it enters this wait_for_fw() loop. If a system suspend is
triggered concurrently, the uninterruptible msleep() loop prevents the task
from handling the freeze signal before the 20-second freezer timeout expires.

[ ... ]

> @@ -476,12 +544,46 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> +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;
> +
> +	guard(mutex)(&xe->pmt.lock);
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);
> +}
> +
> +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);
> +}

[Severity: Medium]
Is it safe to use an asynchronous runtime put here during the device removal
path?

When device removal triggers this function via devres cleanup, the pending
async idle/suspend work can be cancelled by pm_runtime_disable(), leaving
the hardware in an active power state indefinitely. Should this be a
synchronous put (xe_pm_runtime_put_sync()) instead?

[ ... ]

> @@ -489,11 +591,11 @@ void xe_vsec_init(struct xe_device *xe)
[ ... ]
>  	case XE_VSEC_CRI:
>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_sysctrl_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}
> +

[Severity: High]
Can this worker thread race with devres_release_all() during device unbind?

If the driver is unbound while the worker is in its polling loop,
devres_release_all() splices the devres list and executes
vsec_disable_late_bind_work(), which waits for the worker to finish.

However, the worker calls intel_vsec_register() in cri_late_bind_probe(),
which adds new devres items to the device. Since devres_release_all() only
splices the list once before executing it, any devres items added by the
worker are permanently leaked.

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