Re: [PATCH v8 19/24] drm/xe: Add ULLS migration job support to migration layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/xe/migrate: ULLS exit worker fires during system suspend causing fatal hardware access
- [High] drm/xe/migrate: ULLS state not cleared on GT reset causes migration engine hang
--

commit ffd4d32bbca19023aed05a8d61a40704a5e6e6a4
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Add ULLS migration job support to migration layer

This commit adds a function to enter Ultra Low Latency Submission (ULLS)
mode for migration jobs, and a delayed worker to exit it for power savings.
It introduces several ULLS state flags and related job submission triggers.

> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c

[ ... ]

> +static struct dma_fence *xe_migrate_job_push(struct xe_migrate *m,
> +					     struct xe_sched_job *job)
> +{
> +	enum xe_ulls_state ulls = ULLS_NONE;
> +
> +	lockdep_assert_held(&m->job_mutex);
> +
> +	if (xe_migrate_ulls_enabled(m)) {
> +		ulls = ULLS_ACTIVE;

[Severity: High]
Does the software state stay out of sync if a GT reset occurs while ULLS is
active?

If m->ulls.enabled is never cleared by a GT reset hook, the next migration
job submitted post-reset will see ulls.enabled as true. It will then submit
as a chained job (ULLS_ACTIVE), bypassing the GuC doorbell (H2G
XE_GUC_ACTION_SCHED_CONTEXT).

Since the hardware was reset, the context needs a schedule doorbell to pick
up the new tail, potentially causing the migration engine to hang permanently.

> +		mod_delayed_work(system_percpu_wq, &m->ulls.exit_work,
> +				 ULLS_EXIT_JIFFIES);
> +	}
> +
> +	return __xe_migrate_job_push(m, job, ulls);
> +}

[ ... ]

> +static void xe_migrate_ulls_exit(struct work_struct *work)
> +{
> +	struct xe_migrate *m = container_of(work, struct xe_migrate,
> +					    ulls.exit_work.work);
> +	struct xe_device *xe = tile_to_xe(m->tile);
> +	struct xe_sched_job *job = NULL;
> +	struct dma_fence *fence = NULL;
> +	u64 batch_addr[2] = { 0, 0 };
> +	int idx;
> +
> +	xe_assert(xe, m->ulls.enabled);
> +
> +	if (!drm_dev_enter(&xe->drm, &idx))
> +		return;
> +
> +	/*
> +	 * Must be done outside job_mutex as that lock is tainted with
> +	 * reclaim and must be done holding a pm ref.
> +	 */
> +	job = xe_sched_job_create(m->q, batch_addr);

[Severity: Critical]
Could this worker fire during system suspend and attempt invalid hardware
access?

The system suspend path (xe_pm_suspend) does not appear to call
cancel_delayed_work_sync(&m->ulls.exit_work).

If ULLS is entered right before suspend (e.g., from an eviction page fault),
this exit worker might execute after the device has entered D3Cold. This
would lead to a machine check, PCI abort, or kernel panic when
xe_sched_job_create() pushes the job and the scheduler writes to GuC MMIO
or the GGTT ring buffer on a powered-down device.

> +	if (WARN_ON_ONCE(IS_ERR(job))) {
> +		drm_dev_exit(idx);
> +		mod_delayed_work(system_percpu_wq, &m->ulls.exit_work,
> +				 ULLS_EXIT_JIFFIES);
> +		return;		/* Not fatal */
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925201526.1355279-1-matthew.brost@intel.com?part=19