Re: [PATCH v7 10/24] drm/xe: Update GuC submission backend to run PT jobs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] xe: memory leak of pt_job_ops when PT job is aborted before execution
- [High] xe: PT jobs swallow cancellation error statuses and signal false success
--

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

drm/xe: Update GuC submission backend to run PT jobs

PT jobs bypass GPU execution for the final step of a bind job, using the
CPU to program the required page tables. Teach the GuC submission backend
how to execute these jobs.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 4bd1ead57efa4..9faddb7c6407f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -1238,21 +1240,44 @@ static void submit_exec_queue(struct xe_exec_queue *q, struct xe_sched_job *job)
>  static struct dma_fence *
>  guc_exec_queue_run_job(struct drm_sched_job *drm_job)
>  {
>  	struct xe_sched_job *job = to_xe_sched_job(drm_job);
>  	struct xe_exec_queue *q = job->q;
>  	struct xe_guc *guc = exec_queue_to_guc(q);
> -	bool killed_or_banned_or_wedged =
> -		exec_queue_killed_or_banned_or_wedged(q);
> +	bool killed_or_banned_or_wedged_or_error  =
> +		exec_queue_killed_or_banned_or_wedged(q) ||
> +		xe_sched_job_is_error(job);

[Severity: High]
Can this condition fail to detect errors for PT jobs?

PT jobs are initialized with a global stub fence. When the queue is
killed or banned, xe_sched_job_set_error() attempts to set an error on this
fence but fails because the stub fence is already permanently signaled. As a
result, xe_sched_job_is_error() will always be false for PT jobs.

>  	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
>  		     exec_queue_banned(q) || exec_queue_suspended(q));
>  
>  	trace_xe_sched_job_run(job);
>  
> -	if (!killed_or_banned_or_wedged && !xe_sched_job_is_error(job)) {
> +	if (is_pt_job(job)) {
> +		xe_gt_assert(guc_to_gt(guc), !exec_queue_registered(q));
> +		run_pt_job(job, killed_or_banned_or_wedged_or_error);
> +		xe_pt_job_ops_put(job->pt_update[0].pt_job_ops);

[Severity: High]
Does this leak the pt_job_ops when a PT job is aborted before execution?

If the DRM scheduler is stopped, the entity killed, or the execution queue
banned before run_job is invoked, the scheduler calls free_job directly
without running it. The free_job callback invokes xe_sched_job_destroy(),
which then calls xe_sched_job_free_fences(). That function explicitly skips
PT jobs via an early return:

xe_sched_job_free_fences() {
	if (job->is_pt_job)
		return;
	...
}

Since xe_pt_job_ops_put() is only called here in guc_exec_queue_run_job(),
which is completely bypassed on this cancellation path, does the refcount
leak?

> +		dma_fence_put(job->fence);	/* Drop ref from xe_sched_job_arm */
> +
> +		return NULL;

[Severity: High]
Does this swallow cancellation error statuses and signal false success for
PT jobs?

Even when killed_or_banned_or_wedged_or_error is true, this path
unconditionally returns NULL. The DRM scheduler treats a NULL return from
run_job as an immediate successful completion, signaling the software
finished fence with 0 instead of an error (like -ECANCELED).

Userspace waiting on a failed or cancelled bind will observe false success
and might proceed to access memory that hasn't been properly mapped.

> +	} else if (!killed_or_banned_or_wedged_or_error) {
>  		if (xe_exec_queue_is_multi_queue_secondary(q)) {
>  			struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);

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